diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 449aa90b0e6674a9a763ba0563006fa4730c174d..ce7d64336c86197ade874da05367a85d505322d2 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -18,7 +18,10 @@ def show 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 @@ def set_application @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 diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index d97e22df47217f8c554a3c31ef17a7df8d752a70..d2c746db12d769bef2341baefffa6e5a55e7615e 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -18,4 +18,14 @@ def prepare_scopes 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 diff --git a/app/controllers/groups/settings/applications_controller.rb b/app/controllers/groups/settings/applications_controller.rb index cefb54258671199ae37a4b8b627f31d3187b6b09..f05a96d7810f2386aa01fa84ec05d2841d72af03 100644 --- a/app/controllers/groups/settings/applications_controller.rb +++ b/app/controllers/groups/settings/applications_controller.rb @@ -54,8 +54,10 @@ def set_index_vars # 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,12 +65,9 @@ def set_application end def application_params - params - .require(:doorkeeper_application) - .permit(:name, :redirect_uri, :scopes, :confidential) - .tap do |params| - params[:owner] = @group - end + super.tap do |params| + params[:owner] = @group + end end end end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index 8158db282fb059c85a9dbfbad8827873fc055408..81f188256bace0da54a2e6acb03ef077c1ef4445 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -25,7 +25,7 @@ def index 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 @@ def set_index_vars @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 @@ def set_application 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 diff --git a/app/views/admin/applications/_form.html.haml b/app/views/admin/applications/_form.html.haml index 74eda21d5bd695f5c7172fea7d343a6129b89bf9..a1990ad57501fd636b9fdfae804c7fc387c1cf28 100644 --- a/app/views/admin/applications/_form.html.haml +++ b/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 diff --git a/app/views/shared/doorkeeper/applications/_form.html.haml b/app/views/shared/doorkeeper/applications/_form.html.haml index 91a32b55542b5cb8e27a2c9b6a7d6abc1a7dde1b..180c658dbdcc09ead7c4e093ce26a75c384f8969 100644 --- a/app/views/shared/doorkeeper/applications/_form.html.haml +++ b/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 diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 4533779339a849a38f2c4c93a114f26695f523f1..477d419576a2839528fc05bb54a3497ef290a3e0 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -38,8 +38,11 @@ # 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 diff --git a/db/migrate/20210902184334_add_expire_access_tokens_to_doorkeeper_application.rb b/db/migrate/20210902184334_add_expire_access_tokens_to_doorkeeper_application.rb new file mode 100644 index 0000000000000000000000000000000000000000..4638637331d82c3236a615d2e577c22020998e9d --- /dev/null +++ b/db/migrate/20210902184334_add_expire_access_tokens_to_doorkeeper_application.rb @@ -0,0 +1,7 @@ +# 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 diff --git a/db/schema_migrations/20210902184334 b/db/schema_migrations/20210902184334 new file mode 100644 index 0000000000000000000000000000000000000000..f35699b357ed357000d66e81428629db05d666ba --- /dev/null +++ b/db/schema_migrations/20210902184334 @@ -0,0 +1 @@ +508b8d4608d28b2a12cf429262c3dd336e130013a41e51ff6c95027ece1540e5 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 3f604828eed0b8aea8ef0f651b7ffd29fb264097..8ae34bb6b65c73d49e92b040a51a9a98be36eaa3 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -16354,7 +16354,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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 4f4e2d4122e289d4c81b90a04e441047740852cb..b16fd81bafe95e46bf1ab752fd921738773a5a8e 100644 --- a/locale/gitlab.pot +++ b/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 "" diff --git a/spec/requests/oauth_tokens_spec.rb b/spec/requests/oauth_tokens_spec.rb index 6d944bbc783e96cfe1a3368454b4aa5626ab780e..fdcc76f42cc2fb00cb52cd9cf0c9bfd310da442d 100644 --- a/spec/requests/oauth_tokens_spec.rb +++ b/spec/requests/oauth_tokens_spec.rb @@ -55,5 +55,29 @@ def generate_access_grant(user) 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 diff --git a/spec/support/shared_examples/features/manage_applications_shared_examples.rb b/spec/support/shared_examples/features/manage_applications_shared_examples.rb index 38bb87eaed24e8a727a9acf4d534a7def57d7438..0161899cb76a2c95e721dd5c8ef2e0cbe73091e0 100644 --- a/spec/support/shared_examples/features/manage_applications_shared_examples.rb +++ b/spec/support/shared_examples/features/manage_applications_shared_examples.rb @@ -9,9 +9,11 @@ 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 @@ 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