Merge branch 'dblessing-expiring-oauth-tokens' into 'master'

Add Expiring OAuth Access Tokens

See merge request gitlab-org/gitlab!69514

app/controllers/admin/applications_controller.rb

@@ -18,7 +18,10 @@ class Admin::ApplicationsController < Admin::ApplicationController
   end
 
   def new
-    @application = Doorkeeper::Application.new
+    # Default access tokens to expire. This preserves backward compatibility
+    # with existing applications. This will be removed in 15.0.
+    # Removal issue: https://gitlab.com/gitlab-org/gitlab/-/issues/340848
+    @application = Doorkeeper::Application.new(expire_access_tokens: true)
   end
 
   def edit
@@ -55,10 +58,13 @@ class Admin::ApplicationsController < Admin::ApplicationController
     @application = ApplicationsFinder.new(id: params[:id]).execute
   end
 
-  # Only allow a trusted parameter "white list" through.
+  def permitted_params
+    super << :trusted
+  end
+
   def application_params
-    params
-      .require(:doorkeeper_application)
-      .permit(:name, :redirect_uri, :trusted, :scopes, :confidential)
+    super.tap do |params|
+      params[:owner] = nil
+    end
   end
 end

app/controllers/concerns/oauth_applications.rb

@@ -18,4 +18,14 @@ module OauthApplications
   def load_scopes
     @scopes ||= Doorkeeper.configuration.scopes
   end
+
+  def permitted_params
+    %i{name redirect_uri scopes confidential expire_access_tokens}
+  end
+
+  def application_params
+    params
+      .require(:doorkeeper_application)
+      .permit(*permitted_params)
+  end
 end

app/controllers/groups/settings/applications_controller.rb

@@ -54,8 +54,10 @@ module Groups
         # https://gitlab.com/gitlab-org/gitlab/-/issues/324187
         @applications = @group.oauth_applications.limit(100)
 
-        # Don't overwrite a value possibly set by `create`
-        @application ||= Doorkeeper::Application.new
+        # Default access tokens to expire. This preserves backward compatibility
+        # with existing applications. This will be removed in 15.0.
+        # Removal issue: https://gitlab.com/gitlab-org/gitlab/-/issues/340848
+        @application ||= Doorkeeper::Application.new(expire_access_tokens: true)
       end
 
       def set_application
@@ -63,10 +65,7 @@ module Groups
       end
 
       def application_params
-        params
-          .require(:doorkeeper_application)
-          .permit(:name, :redirect_uri, :scopes, :confidential)
-          .tap do |params|
+        super.tap do |params|
           params[:owner] = @group
         end
       end

app/controllers/oauth/applications_controller.rb

@@ -25,7 +25,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
   end
 
   def create
-    @application = Applications::CreateService.new(current_user, create_application_params).execute(request)
+    @application = Applications::CreateService.new(current_user, application_params).execute(request)
 
     if @application.persisted?
       flash[:notice] = I18n.t(:notice, scope: [:doorkeeper, :flash, :applications, :create])
@@ -51,8 +51,10 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
     @authorized_anonymous_tokens = @authorized_tokens.reject(&:application)
     @authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?)
 
-    # Don't overwrite a value possibly set by `create`
-    @application ||= Doorkeeper::Application.new
+    # Default access tokens to expire. This preserves backward compatibility
+    # with existing applications. This will be removed in 15.0.
+    # Removal issue: https://gitlab.com/gitlab-org/gitlab/-/issues/340848
+    @application ||= Doorkeeper::Application.new(expire_access_tokens: true)
   end
 
   # Override Doorkeeper to scope to the current user
@@ -64,8 +66,8 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController
     render "errors/not_found", layout: "errors", status: :not_found
   end
 
-  def create_application_params
-    application_params.tap do |params|
+  def application_params
+    super.tap do |params|
       params[:owner] = current_user
     end
   end

app/views/admin/applications/_form.html.haml

@@ -33,6 +33,14 @@
       %span.form-text.text-muted
         = _('The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.')
 
+  = content_tag :div, class: 'form-group row' do
+    .col-sm-2.col-form-label.pt-0
+      = f.label :expire_access_tokens
+    .col-sm-10
+      = f.check_box :expire_access_tokens
+      %span.form-text.text-muted
+        = _('Access tokens expire after 2 hours. A refresh token may be used at any time to generate a new access token. Non-expiring access tokens are deprecated. Clear this setting to enable backward compatibility.')
+
   .form-group.row
     .col-sm-2.col-form-label.pt-0
       = f.label :scopes

app/views/shared/doorkeeper/applications/_form.html.haml

@@ -18,6 +18,12 @@
     %span.form-text.text-muted
       = _('The application will be used where the client secret can be kept confidential. Native mobile apps and Single Page Apps are considered non-confidential.')
 
+  .form-group.form-check
+    = f.check_box :expire_access_tokens, class: 'form-check-input'
+    = f.label :expire_access_tokens, class: 'label-bold form-check-label'
+    %span.form-text.text-muted
+      = _('Access tokens expire after 2 hours. A refresh token may be used at any time to generate a new access token. Non-expiring access tokens are deprecated. Clear this setting to enable backward compatibility.')
+
   .form-group
     = f.label :scopes, class: 'label-bold'
     = render 'shared/tokens/scopes_form', prefix: 'doorkeeper_application', token: @application, scopes: @scopes

config/initializers/doorkeeper.rb

@@ -38,8 +38,11 @@ Doorkeeper.configure do
   # authorization_code_expires_in 10.minutes
 
   # Access token expiration time (default 2 hours).
-  # If you want to disable expiration, set this to nil.
-  access_token_expires_in nil
+  # Until 15.0, applications can opt-out of expiring tokens.
+  # Removal issue: https://gitlab.com/gitlab-org/gitlab/-/issues/340848
+  custom_access_token_expires_in do |context|
+    context.client&.expire_access_tokens ? 2.hours : Float::INFINITY
+  end
 
   # Reuse access token for the same resource owner within an application (disabled by default)
   # Rationale: https://github.com/doorkeeper-gem/doorkeeper/issues/383

db/migrate/20210902184334_add_expire_access_tokens_to_doorkeeper_application.rb

+# frozen_string_literal: true
+
+class AddExpireAccessTokensToDoorkeeperApplication < Gitlab::Database::Migration[1.0]
+  def change
+    add_column :oauth_applications, :expire_access_tokens, :boolean, default: false, null: false
+  end
+end

db/schema_migrations/20210902184334

+508b8d4608d28b2a12cf429262c3dd336e130013a41e51ff6c95027ece1540e5
\ No newline at end of file

db/structure.sql

@@ -16386,7 +16386,8 @@ CREATE TABLE oauth_applications (
     owner_id integer,
     owner_type character varying,
     trusted boolean DEFAULT false NOT NULL,
-    confidential boolean DEFAULT true NOT NULL
+    confidential boolean DEFAULT true NOT NULL,
+    expire_access_tokens boolean DEFAULT false NOT NULL
 );
 
 CREATE SEQUENCE oauth_applications_id_seq

locale/gitlab.pot

@@ -1741,6 +1741,9 @@ msgstr ""
 msgid "Access to '%{classification_label}' not allowed"
 msgstr ""
 
+msgid "Access tokens expire after 2 hours. A refresh token may be used at any time to generate a new access token. Non-expiring access tokens are deprecated. Clear this setting to enable backward compatibility."
+msgstr ""
+
 msgid "AccessDropdown|Deploy Keys"
 msgstr ""
 

spec/requests/oauth_tokens_spec.rb

@@ -55,5 +55,29 @@ RSpec.describe 'OAuth Tokens requests' do
 
       expect(json_response['access_token']).not_to be_nil
     end
+
+    context 'when the application is configured to use expiring tokens' do
+      before do
+        application.update!(expire_access_tokens: true)
+      end
+
+      it 'generates an access token with an expiration' do
+        request_access_token(user)
+
+        expect(json_response['expires_in']).not_to be_nil
+      end
+    end
+
+    context 'when the application is configured not to use expiring tokens' do
+      before do
+        application.update!(expire_access_tokens: false)
+      end
+
+      it 'generates an access token without an expiration' do
+        request_access_token(user)
+
+        expect(json_response.key?('expires_in')).to eq(false)
+      end
+    end
   end
 end

spec/support/shared_examples/features/manage_applications_shared_examples.rb

@@ -9,9 +9,11 @@ RSpec.shared_examples 'manage applications' do
     visit new_application_path
 
     expect(page).to have_content 'Add new application'
+    expect(find('#doorkeeper_application_expire_access_tokens')).to be_checked
 
     fill_in :doorkeeper_application_name, with: application_name
     fill_in :doorkeeper_application_redirect_uri, with: application_redirect_uri
+    uncheck :doorkeeper_application_expire_access_tokens
     check :doorkeeper_application_scopes_read_user
     click_on 'Save application'
 
@@ -22,6 +24,8 @@ RSpec.shared_examples 'manage applications' do
 
     click_on 'Edit'
 
+    expect(find('#doorkeeper_application_expire_access_tokens')).not_to be_checked
+
     application_name_changed = "#{application_name} changed"
 
     fill_in :doorkeeper_application_name, with: application_name_changed