Merge branch '8723-geo-remove-gitlab-lfstoken-legacyredisdevisetoken-implementation-and-usage-geo' into 'master'

Geo: Remove Gitlab::LfsToken::LegacyRedisDeviseToken implementation

Closes gitlab-ee#8723

See merge request gitlab-org/gitlab-ce!28546

changelogs/unreleased/8723-geo-remove-gitlab-lfstoken-legacyredisdevisetoken-implementation-and-usage-geo.yml

@@ -0,0 +1,5 @@
+---
+title: 'Geo: Remove Gitlab::LfsToken::LegacyRedisDeviseToken implementation and usage'
+merge_request: 28546
+author:
+type: changed

lib/gitlab/lfs_token.rb

@@ -35,8 +35,7 @@ module Gitlab
     end
 
     def token_valid?(token_to_check)
-      HMACToken.new(actor).token_valid?(token_to_check) ||
+      HMACToken.new(actor).token_valid?(token_to_check)
-        LegacyRedisDeviseToken.new(actor).token_valid?(token_to_check)
     end
 
     def deploy_key_pushable?(project)
@@ -103,44 +102,5 @@ module Gitlab
         Settings.attr_encrypted_db_key_base.first(16)
       end
     end
-
-    # TODO: LegacyRedisDeviseToken and references need to be removed after
-    # next released milestone
-    #
-    class LegacyRedisDeviseToken
-      TOKEN_LENGTH = 50
-      DEFAULT_EXPIRY_TIME = 1800 * 1000 # 30 mins
-
-      def initialize(actor)
-        @actor = actor
-      end
-
-      def token_valid?(token_to_check)
-        Devise.secure_compare(stored_token, token_to_check)
-      end
-
-      def stored_token
-        Gitlab::Redis::SharedState.with { |redis| redis.get(state_key) }
-      end
-
-      # This method exists purely to facilitate legacy testing to ensure the
-      # same redis key is used.
-      #
-      def store_new_token(expiry_time_in_ms = DEFAULT_EXPIRY_TIME)
-        Gitlab::Redis::SharedState.with do |redis|
-          new_token = Devise.friendly_token(TOKEN_LENGTH)
-          redis.set(state_key, new_token, px: expiry_time_in_ms)
-          new_token
-        end
-      end
-
-      private
-
-      attr_reader :actor
-
-      def state_key
-        "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}"
-      end
-    end
   end
 end

spec/lib/gitlab/lfs_token_spec.rb

@@ -77,42 +77,35 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
     let(:actor) { create(:user, username: 'test_user_lfs_1') }
     let(:lfs_token) { described_class.new(actor) }
 
-    context 'for an HMAC token' do
+    context 'where the token is invalid' do
-      before do
+      context "because it's junk" do
-        # We're not interested in testing LegacyRedisDeviseToken here
+        it 'returns false' do
-        allow(Gitlab::LfsToken::LegacyRedisDeviseToken).to receive_message_chain(:new, :token_valid?).and_return(false)
+          expect(lfs_token.token_valid?('junk')).to be_falsey
+        end
       end
 
-      context 'where the token is invalid' do
+      context "because it's been fiddled with" do
-        context "because it's junk" do
+        it 'returns false' do
-          it 'returns false' do
+          fiddled_token = lfs_token.token.tap { |token| token[0] = 'E' }
-            expect(lfs_token.token_valid?('junk')).to be_falsey
+          expect(lfs_token.token_valid?(fiddled_token)).to be_falsey
-          end
         end
+      end
 
-        context "because it's been fiddled with" do
+      context "because it was generated with a different secret" do
-          it 'returns false' do
+        it 'returns false' do
-            fiddled_token = lfs_token.token.tap { |token| token[0] = 'E' }
+          different_actor = create(:user, username: 'test_user_lfs_2')
-            expect(lfs_token.token_valid?(fiddled_token)).to be_falsey
+          different_secret_token = described_class.new(different_actor).token
-          end
+          expect(lfs_token.token_valid?(different_secret_token)).to be_falsey
         end
+      end
 
-        context "because it was generated with a different secret" do
+      context "because it's expired" do
-          it 'returns false' do
+        it 'returns false' do
-            different_actor = create(:user, username: 'test_user_lfs_2')
+          expired_token = lfs_token.token
-            different_secret_token = described_class.new(different_actor).token
+          # Needs to be at least 1860 seconds, because the default expiry is
-            expect(lfs_token.token_valid?(different_secret_token)).to be_falsey
+          # 1800 seconds with an additional 60 second leeway.
-          end
+          Timecop.freeze(Time.now + 1865) do
-        end
+            expect(lfs_token.token_valid?(expired_token)).to be_falsey
-
-        context "because it's expired" do
-          it 'returns false' do
-            expired_token = lfs_token.token
-            # Needs to be at least 1860 seconds, because the default expiry is
-            # 1800 seconds with an additional 60 second leeway.
-            Timecop.freeze(Time.now + 1865) do
-              expect(lfs_token.token_valid?(expired_token)).to be_falsey
-            end
           end
         end
       end
@@ -123,53 +116,6 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
         end
       end
     end
-
-    context 'for a LegacyRedisDevise token' do
-      before do
-        # We're not interested in testing HMACToken here
-        allow_any_instance_of(Gitlab::LfsToken::HMACToken).to receive(:token_valid?).and_return(false)
-      end
-
-      context 'where the token is invalid' do
-        context "because it's junk" do
-          it 'returns false' do
-            expect(lfs_token.token_valid?('junk')).to be_falsey
-          end
-        end
-
-        context "because it's been fiddled with" do
-          it 'returns false' do
-            generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token
-            fiddled_token = generated_token.tap { |token| token[0] = 'E' }
-            expect(lfs_token.token_valid?(fiddled_token)).to be_falsey
-          end
-        end
-
-        context "because it was generated with a different secret" do
-          it 'returns false' do
-            different_actor = create(:user, username: 'test_user_lfs_2')
-            different_secret_token = described_class.new(different_actor).token
-            expect(lfs_token.token_valid?(different_secret_token)).to be_falsey
-          end
-        end
-
-        context "because it's expired" do
-          it 'returns false' do
-            generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token(1)
-            # We need a real sleep here because we need to wait for redis to expire the key.
-            sleep(0.01)
-            expect(lfs_token.token_valid?(generated_token)).to be_falsey
-          end
-        end
-      end
-
-      context 'where the token is valid' do
-        it 'returns true' do
-          generated_token = Gitlab::LfsToken::LegacyRedisDeviseToken.new(actor).store_new_token
-          expect(lfs_token.token_valid?(generated_token)).to be_truthy
-        end
-      end
-    end
   end
 
   describe '#deploy_key_pushable?' do