Revert "LfsToken uses JSONWebToken::HMACToken by default"

This reverts commit 22954f22

lib/gitlab/auth.rb

@@ -199,7 +199,7 @@ module Gitlab
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
-      def lfs_token_check(login, encoded_token, project)
+      def lfs_token_check(login, password, project)
         deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/)
 
         actor =
@@ -222,7 +222,7 @@ module Gitlab
             read_authentication_abilities
           end
 
-        if token_handler.token_valid?(encoded_token)
+        if Devise.secure_compare(token_handler.token, password)
           Gitlab::Auth::Result.new(actor, nil, token_handler.type, authentication_abilities)
         end
       end

lib/gitlab/lfs_token.rb

@@ -2,22 +2,11 @@
 
 module Gitlab
   class LfsToken
-    module LfsTokenHelper
-      def user?
-        actor.is_a?(User)
-      end
-
-      def actor_name
-        user? ? actor.username : "lfs+deploy-key-#{actor.id}"
-      end
-    end
-
-    include LfsTokenHelper
-
-    DEFAULT_EXPIRE_TIME = 1800
-
     attr_accessor :actor
 
+    TOKEN_LENGTH = 50
+    EXPIRY_TIME = 1800
+
     def initialize(actor)
       @actor =
         case actor
@@ -30,108 +19,36 @@ module Gitlab
         end
     end
 
-    def token(expire_time: DEFAULT_EXPIRE_TIME)
-      HMACToken.new(actor).token(expire_time)
-    end
+    def token
+      Gitlab::Redis::SharedState.with do |redis|
+        token = redis.get(redis_shared_state_key)
+        token ||= Devise.friendly_token(TOKEN_LENGTH)
+        redis.set(redis_shared_state_key, token, ex: EXPIRY_TIME)
 
-    def token_valid?(token_to_check)
-      HMACToken.new(actor).token_valid?(token_to_check) ||
-        LegacyRedisDeviseToken.new(actor).token_valid?(token_to_check)
+        token
+      end
     end
 
     def deploy_key_pushable?(project)
       actor.is_a?(DeployKey) && actor.can_push_to?(project)
     end
 
-    def type
-      user? ? :lfs_token : :lfs_deploy_token
-    end
-
-    private  # rubocop:disable Lint/UselessAccessModifier
-
-    class HMACToken
-      include LfsTokenHelper
-
-      def initialize(actor)
-        @actor = actor
-      end
-
-      def token(expire_time)
-        hmac_token = JSONWebToken::HMACToken.new(secret)
-        hmac_token.expire_time = Time.now + expire_time
-        hmac_token[:data] = { actor: actor_name }
-        hmac_token.encoded
-      end
-
-      def token_valid?(token_to_check)
-        decoded_token = JSONWebToken::HMACToken.decode(token_to_check, secret).first
-        decoded_token.dig('data', 'actor') == actor_name
-      rescue JWT::DecodeError
-        false
-      end
-
-      private
-
-      attr_reader :actor
-
-      def secret
-        salt + key
-      end
-
-      def salt
-        case actor
-        when DeployKey, Key
-          actor.fingerprint.delete(':').first(16)
-        when User
-          # Take the last 16 characters as they're more unique than the first 16
-          actor.id.to_s + actor.encrypted_password.last(16)
-        end
-      end
-
-      def key
-        # Take 16 characters of attr_encrypted_db_key_base, as that's what the
-        # cipher needs exactly
-        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)
+    def user?
+      actor.is_a?(User)
     end
 
-      def stored_token
-        Gitlab::Redis::SharedState.with { |redis| redis.get(state_key) }
+    def type
+      user? ? :lfs_token : :lfs_deploy_token
     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
+    def actor_name
+      actor.is_a?(User) ? actor.username : "lfs+deploy-key-#{actor.id}"
     end
 
     private
 
-      attr_reader :actor
-
-      def state_key
-        "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}"
-      end
+    def redis_shared_state_key
+      "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
     end
   end
 end

spec/lib/gitlab/lfs_token_spec.rb

-# frozen_string_literal: true
-
 require 'spec_helper'
 
-describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
+describe Gitlab::LfsToken do
   describe '#token' do
     shared_examples 'an LFS token generator' do
-      it 'returns a computed token' do
-        expect(Settings).to receive(:attr_encrypted_db_key_base).and_return('fbnbv6hdjweo53qka7kza8v8swxc413c05pb51qgtfte0bygh1p2e508468hfsn5ntmjcyiz7h1d92ashpet5pkdyejg7g8or3yryhuso4h8o5c73h429d9c3r6bjnet').twice
-
-        token = lfs_token.token
+      it 'returns a randomly generated token' do
+        token = handler.token
 
         expect(token).not_to be_nil
         expect(token).to be_a String
-        expect(described_class.new(actor).token_valid?(token)).to be_truthy
-      end
+        expect(token.length).to eq 50
       end
 
-    context 'when the actor is a user' do
-      let(:actor) { create(:user, username: 'test_user_lfs_1') }
-      let(:lfs_token) { described_class.new(actor) }
+      it 'returns the correct token based on the key' do
+        token = handler.token
 
-      before do
-        allow(actor).to receive(:encrypted_password).and_return('$2a$04$ETfzVS5spE9Hexn9wh6NUenCHG1LyZ2YdciOYxieV1WLSa8DHqOFO')
+        expect(handler.token).to eq(token)
       end
-
-      it_behaves_like 'an LFS token generator'
-
-      it 'returns the correct username' do
-        expect(lfs_token.actor_name).to eq(actor.username)
     end
 
-      it 'returns the correct token type' do
-        expect(lfs_token.type).to eq(:lfs_token)
-      end
-    end
-
-    context 'when the actor is a key' do
-      let(:user) { create(:user, username: 'test_user_lfs_2') }
-      let(:actor) { create(:key, user: user) }
-      let(:lfs_token) { described_class.new(actor) }
-
-      before do
-        allow(user).to receive(:encrypted_password).and_return('$2a$04$C1GAMKsOKouEbhKy2JQoe./3LwOfQAZc.hC8zW2u/wt8xgukvnlV.')
-      end
+    context 'when the actor is a user' do
+      let(:actor) { create(:user) }
+      let(:handler) { described_class.new(actor) }
 
       it_behaves_like 'an LFS token generator'
 
       it 'returns the correct username' do
-        expect(lfs_token.actor_name).to eq(user.username)
+        expect(handler.actor_name).to eq(actor.username)
       end
 
       it 'returns the correct token type' do
-        expect(lfs_token.type).to eq(:lfs_token)
+        expect(handler.type).to eq(:lfs_token)
       end
     end
 
     context 'when the actor is a deploy key' do
-      let(:actor_id) { 1 }
       let(:actor) { create(:deploy_key) }
-      let(:project) { create(:project) }
-      let(:lfs_token) { described_class.new(actor) }
-
-      before do
-        allow(actor).to receive(:id).and_return(actor_id)
-      end
+      let(:handler) { described_class.new(actor) }
 
       it_behaves_like 'an LFS token generator'
 
       it 'returns the correct username' do
-        expect(lfs_token.actor_name).to eq("lfs+deploy-key-#{actor_id}")
+        expect(handler.actor_name).to eq("lfs+deploy-key-#{actor.id}")
       end
 
       it 'returns the correct token type' do
-        expect(lfs_token.type).to eq(:lfs_deploy_token)
-      end
-    end
-
-    context 'when the actor is invalid' do
-      it 'raises an exception' do
-        expect { described_class.new('invalid') }.to raise_error('Bad Actor')
-      end
-    end
-  end
-
-  describe '#token_valid?' do
-    let(:actor) { create(:user, username: 'test_user_lfs_1') }
-    let(:lfs_token) { described_class.new(actor) }
-
-    before do
-      allow(actor).to receive(:encrypted_password).and_return('$2a$04$ETfzVS5spE9Hexn9wh6NUenCHG1LyZ2YdciOYxieV1WLSa8DHqOFO')
-    end
-
-    context 'for an HMAC token' do
-      before do
-        # We're not interested in testing LegacyRedisDeviseToken here
-        allow(Gitlab::LfsToken::LegacyRedisDeviseToken).to receive_message_chain(:new, :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
-            fiddled_token = lfs_token.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
-            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
-
-      context 'where the token is valid' do
-        it 'returns true' do
-          expect(lfs_token.token_valid?(lfs_token.token)).to be_truthy
-        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
+        expect(handler.type).to eq(:lfs_deploy_token)
       end
     end
   end

spec/requests/api/internal_spec.rb

@@ -156,8 +156,9 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq(user.username)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
+
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
-        expect(Gitlab::LfsToken.new(key).token_valid?(json_response['lfs_token'])).to be_truthy
       end
 
       it 'returns the correct information about the user' do
@@ -165,8 +166,9 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq(user.username)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).token)
+
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
-        expect(Gitlab::LfsToken.new(user).token_valid?(json_response['lfs_token'])).to be_truthy
       end
 
       it 'returns a 404 when no key or user is provided' do
@@ -196,8 +198,8 @@ describe API::Internal do
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).token)
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
-        expect(Gitlab::LfsToken.new(key).token_valid?(json_response['lfs_token'])).to be_truthy
       end
     end
   end