From 07a353259424cd225368ca4af59883eebf10066d Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 21 May 2019 18:04:09 +0300 Subject: Geo: Remove Gitlab::LfsToken::LegacyRedisDeviseToken implementation We kept it for smooth update only --- ...disdevisetoken-implementation-and-usage-geo.yml | 5 ++ lib/gitlab/lfs_token.rb | 42 +--------- spec/lib/gitlab/lfs_token_spec.rb | 98 +++++----------------- 3 files changed, 28 insertions(+), 117 deletions(-) create mode 100644 changelogs/unreleased/8723-geo-remove-gitlab-lfstoken-legacyredisdevisetoken-implementation-and-usage-geo.yml diff --git a/changelogs/unreleased/8723-geo-remove-gitlab-lfstoken-legacyredisdevisetoken-implementation-and-usage-geo.yml b/changelogs/unreleased/8723-geo-remove-gitlab-lfstoken-legacyredisdevisetoken-implementation-and-usage-geo.yml new file mode 100644 index 00000000000..173c7d9383e --- /dev/null +++ b/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 diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb index 31e6fc9d8c7..124e34562c1 100644 --- a/lib/gitlab/lfs_token.rb +++ b/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) || - LegacyRedisDeviseToken.new(actor).token_valid?(token_to_check) + HMACToken.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 diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb index 8961ecc4be0..701ed1f3a1b 100644 --- a/spec/lib/gitlab/lfs_token_spec.rb +++ b/spec/lib/gitlab/lfs_token_spec.rb @@ -77,96 +77,42 @@ 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 - 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 + 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 'where the token is valid' do - it 'returns true' do - expect(lfs_token.token_valid?(lfs_token.token)).to be_truthy + 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 - 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 + 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 + 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 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 + expect(lfs_token.token_valid?(lfs_token.token)).to be_truthy end end end -- cgit v1.2.1