Merge branch 'fj-bump-lfs-token-default-expiration-time' into 'master'

Increase LFS token default time to 2 hours

See merge request gitlab-org/gitlab!33140

changelogs/unreleased/fj-bump-lfs-token-default-expiration-time.yml

+---
+title: Increase LFS token default time to 2 hours
+merge_request: 33140
+author:
+type: changed

lib/gitlab/lfs_token.rb

@@ -14,7 +14,7 @@ module Gitlab
 
     include LfsTokenHelper
 
-    DEFAULT_EXPIRE_TIME = 1800
+    DEFAULT_EXPIRE_TIME = 7200 # Default value 2 hours
 
     attr_accessor :actor
 

spec/lib/gitlab/lfs_token_spec.rb

@@ -3,6 +3,13 @@
 require 'spec_helper'
 
 describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
+  let_it_be(:user)       { create(:user) }
+  let_it_be(:project)    { create(:project) }
+  let_it_be(:deploy_key) { create(:deploy_key) }
+
+  let(:actor)     { user }
+  let(:lfs_token) { described_class.new(actor) }
+
   describe '#token' do
     shared_examples 'a valid LFS token' do
       it 'returns a computed token' do
@@ -10,14 +17,11 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
 
         expect(token).not_to be_nil
         expect(token).to be_a String
-        expect(described_class.new(actor).token_valid?(token)).to be_truthy
+        expect(described_class.new(actor).token_valid?(token)).to be true
       end
     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_behaves_like 'a valid LFS token'
 
       it 'returns the correct username' do
@@ -30,9 +34,7 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
     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) }
+      let_it_be(:actor) { create(:key, user: user) }
 
       it_behaves_like 'a valid LFS token'
 
@@ -46,10 +48,8 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
     end
 
     context 'when the actor is a deploy key' do
+      let(:actor) { deploy_key }
       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)
@@ -74,45 +74,45 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
   end
 
   describe '#token_valid?' do
-    let(:actor) { create(:user, username: 'test_user_lfs_1') }
-    let(:lfs_token) { described_class.new(actor) }
-
     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
+          expect(lfs_token.token_valid?('junk')).to be false
         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
+
+          expect(lfs_token.token_valid?(fiddled_token)).to be false
         end
       end
 
-      context "because it was generated with a different secret" do
+      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
+
+          expect(lfs_token.token_valid?(different_secret_token)).to be false
         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
+
+          # Needs to be at least LfsToken::DEFAULT_EXPIRE_TIME + 60 seconds
+          # in order to check whether it is valid 1 minute after it has expired
+          Timecop.freeze(Time.now + described_class::DEFAULT_EXPIRE_TIME + 60) do
+            expect(lfs_token.token_valid?(expired_token)).to be false
           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
+          expect(lfs_token.token_valid?(lfs_token.token)).to be true
         end
       end
 
@@ -121,7 +121,7 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
           let(:actor) { create(:user, :blocked) }
 
           it 'returns false' do
-            expect(lfs_token.token_valid?(lfs_token.token)).to be_falsey
+            expect(lfs_token.token_valid?(lfs_token.token)).to be false
           end
         end
 
@@ -129,7 +129,7 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
           let(:actor) { create(:user, password_expires_at: 1.minute.ago) }
 
           it 'returns false' do
-            expect(lfs_token.token_valid?(lfs_token.token)).to be_falsey
+            expect(lfs_token.token_valid?(lfs_token.token)).to be false
           end
         end
       end
@@ -143,7 +143,7 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
           let(:actor) { create(:user, :blocked) }
 
           it 'returns false' do
-            expect(lfs_token.token_valid?(lfs_token.token)).to be_falsey
+            expect(lfs_token.token_valid?(lfs_token.token)).to be false
           end
         end
 
@@ -151,7 +151,7 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
           let(:actor) { create(:user, password_expires_at: 1.minute.ago) }
 
           it 'returns true' do
-            expect(lfs_token.token_valid?(lfs_token.token)).to be_truthy
+            expect(lfs_token.token_valid?(lfs_token.token)).to be true
           end
         end
       end
@@ -159,27 +159,21 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
   end
 
   describe '#deploy_key_pushable?' do
-    let(:lfs_token) { described_class.new(actor) }
-
     context 'when actor is not a DeployKey' do
-      let(:actor) { create(:user) }
-      let(:project) { create(:project) }
-
       it 'returns false' do
-        expect(lfs_token.deploy_key_pushable?(project)).to be_falsey
+        expect(lfs_token.deploy_key_pushable?(project)).to be false
       end
     end
 
     context 'when actor is a DeployKey' do
-      let(:deploy_keys_project) { create(:deploy_keys_project, can_push: can_push) }
-      let(:project) { deploy_keys_project.project }
+      let(:deploy_keys_project) { create(:deploy_keys_project, project: project, can_push: can_push) }
       let(:actor) { deploy_keys_project.deploy_key }
 
       context 'but the DeployKey cannot push to the project' do
         let(:can_push) { false }
 
         it 'returns false' do
-          expect(lfs_token.deploy_key_pushable?(project)).to be_falsey
+          expect(lfs_token.deploy_key_pushable?(project)).to be false
         end
       end
 
@@ -187,27 +181,23 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
         let(:can_push) { true }
 
         it 'returns true' do
-          expect(lfs_token.deploy_key_pushable?(project)).to be_truthy
+          expect(lfs_token.deploy_key_pushable?(project)).to be true
         end
       end
     end
   end
 
   describe '#type' do
-    let(:lfs_token) { described_class.new(actor) }
-
     context 'when actor is not a User' do
-      let(:actor) { create(:deploy_key) }
+      let(:actor) { deploy_key }
 
-      it 'returns false' do
+      it 'returns :lfs_deploy_token type' do
         expect(lfs_token.type).to eq(:lfs_deploy_token)
       end
     end
 
     context 'when actor is a User' do
-      let(:actor) { create(:user) }
-
-      it 'returns false' do
+      it 'returns :lfs_token type' do
         expect(lfs_token.type).to eq(:lfs_token)
       end
     end
@@ -215,8 +205,6 @@ describe Gitlab::LfsToken, :clean_gitlab_redis_shared_state do
 
   describe '#authentication_payload' do
     it 'returns a Hash designed for gitlab-shell' do
-      actor = create(:user)
-      lfs_token = described_class.new(actor)
       repo_http_path = 'http://localhost/user/repo.git'
       authentication_payload = lfs_token.authentication_payload(repo_http_path)