diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index deb60f69bc7..00ea6ecc0b0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -93,6 +93,8 @@ class ApplicationController < ActionController::Base def after_sign_out_path_for(_resource_or_scope) if ENV['OMNIAUTH_ONLY'] == 'true' && Rails.configuration.x.omniauth.oidc_enabled? '/auth/auth/openid_connect/logout' + elsif truthy_param?(:continue_to_stored_location) + stored_location_for(:user) || new_user_session_path else new_user_session_path end diff --git a/app/controllers/auth/registrations_controller.rb b/app/controllers/auth/registrations_controller.rb index 0616c1d6224..f8b18aac3ff 100644 --- a/app/controllers/auth/registrations_controller.rb +++ b/app/controllers/auth/registrations_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Auth::RegistrationsController < Devise::RegistrationsController + include ReauthConcern include RegistrationHelper include Auth::RegistrationSpamConcern @@ -33,8 +34,12 @@ class Auth::RegistrationsController < Devise::RegistrationsController extend_csp_for_oauth! if session[:registration_app_id] super do |resource| - # If created through an app, mark it as such - session[:created_by_app_id] = session.delete(:registration_app_id) if resource.persisted? && session[:registration_app_id] + if resource.persisted? + # If created through an app, mark it as such + session[:created_by_app_id] = session.delete(:registration_app_id) if session[:registration_app_id] + + fulfil_reauth_request + end end rescue ActiveRecord::MultiparameterAssignmentErrors => e handle_multiparameter_assignment_error(e.errors) diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index 3bc8b8143be..05017255c4e 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Auth::SessionsController < Devise::SessionsController + include ReauthConcern include Redisable MAX_2FA_ATTEMPTS_PER_HOUR = 10 @@ -12,8 +13,6 @@ class Auth::SessionsController < Devise::SessionsController skip_before_action :require_functional! skip_before_action :update_user_sign_in - around_action :preserve_stored_location, only: :destroy, if: :continue_after? - prepend_before_action :check_suspicious!, only: [:create] include Auth::TwoFactorAuthenticationConcern @@ -39,6 +38,15 @@ class Auth::SessionsController < Devise::SessionsController flash.delete(:notice) end + # By default, sign_out wipes the session cookie, but we want to keep the redirect location + def sign_out + return super unless continue_after? + + preserve_stored_location do + super + end + end + protected def find_user @@ -106,7 +114,7 @@ class Auth::SessionsController < Devise::SessionsController end def continue_after? - truthy_param?(:continue) + truthy_param?(:continue) || truthy_param?(:continue_to_stored_location) end def restart_session @@ -140,6 +148,8 @@ class Auth::SessionsController < Devise::SessionsController def on_authentication_success(user, security_measure) @on_authentication_success_called = true + fulfil_reauth_request + clear_2fa_attempt_from_user(user) clear_attempt_from_session diff --git a/app/controllers/concerns/reauth_concern.rb b/app/controllers/concerns/reauth_concern.rb new file mode 100644 index 00000000000..02afe1891b6 --- /dev/null +++ b/app/controllers/concerns/reauth_concern.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module ReauthConcern + extend ActiveSupport::Concern + + def fulfil_reauth_request + if session[:require_reauth_for] + session[:reauthed_for] = session.delete(:require_reauth_for) + else + session.delete(:reauthed_for) + end + end +end diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index f02b51a8555..833aeabf31a 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -3,6 +3,7 @@ class OAuth::AuthorizationsController < Doorkeeper::AuthorizationsController include Redisable + prepend_before_action :require_reauth!, only: :new prepend_before_action :store_current_location layout 'modal' @@ -13,6 +14,11 @@ class OAuth::AuthorizationsController < Doorkeeper::AuthorizationsController include Localized + # Make the action explicit so that rubocop does not complain + def new + super + end + private def store_current_location @@ -41,6 +47,20 @@ class OAuth::AuthorizationsController < Doorkeeper::AuthorizationsController super end + def require_reauth! + return unless %w(login create).include?(params['prompt']) + + if current_user + if session.delete(:reauthed_for) != "#{params[:client_id]}:#{params[:state]}" + session[:require_reauth_for] = "#{params[:client_id]}:#{params[:state]}" + + render :require_reauth + end + else + session[:require_reauth_for] = "#{params[:client_id]}:#{params[:state]}" + end + end + # This is used to record with which application the user was created def after_successful_authorization(context) redis.set("track_created_by_application_id:#{current_user.id}", context.auth.pre_auth.client.id, ex: context.auth.issued_token.expires_in || 15.minutes) if session.delete(:created_by_app_id) == context.auth.pre_auth.client.id && current_user.created_by_application_id.nil? diff --git a/app/presenters/oauth_metadata_presenter.rb b/app/presenters/oauth_metadata_presenter.rb index 674d541d150..a099fc16826 100644 --- a/app/presenters/oauth_metadata_presenter.rb +++ b/app/presenters/oauth_metadata_presenter.rb @@ -70,7 +70,7 @@ class OAuthMetadataPresenter < ActiveModelSerializers::Model end def prompt_values_supported - ['create'] + %w(login create) end private diff --git a/app/views/oauth/authorizations/require_reauth.html.haml b/app/views/oauth/authorizations/require_reauth.html.haml new file mode 100644 index 00000000000..7afe4b9d9e6 --- /dev/null +++ b/app/views/oauth/authorizations/require_reauth.html.haml @@ -0,0 +1,11 @@ +- content_for :page_title do + = t('doorkeeper.authorizations.require_reauth.title') + +.form-container.simple_form + .oauth-prompt + %h3= t('doorkeeper.authorizations.require_reauth.title') + + %p= t('doorkeeper.authorizations.require_reauth.prompt_html', handle: current_account.local_username_and_domain) + + .actions + = link_to t('doorkeeper.authorizations.require_reauth.log_out_and_continue'), destroy_user_session_path(continue_to_stored_location: true), method: :delete, class: 'button' diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index f0a47651cb4..74a643ade42 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -63,6 +63,10 @@ en: prompt_html: "%{client_name} would like permission to access your account. Only approve this request if you recognize and trust this source." review_permissions: Review permissions title: Authorization required + require_reauth: + log_out_and_continue: Log out and continue + prompt_html: You are currently logged in as %{handle}, but to continue, you will have to log out. + title: Re-authentication required show: title: Copy this authorization code and paste it to the application. authorized_applications: diff --git a/spec/system/oauth_spec.rb b/spec/system/oauth_spec.rb index d002e137100..d6532b58e0a 100644 --- a/spec/system/oauth_spec.rb +++ b/spec/system/oauth_spec.rb @@ -217,6 +217,59 @@ RSpec.describe 'Using OAuth from an external app' do expect { post '/oauth/token', params: { grant_type: 'authorization_code', client_id: client_app.uid, code: grant.token, client_secret: client_app.secret, redirect_uri: client_app.redirect_uri } } .to change { user.reload.created_by_application_id }.to(client_app.id) end + + context 'when the user is already logged in' do + before do + user = Fabricate(:user) + + visit new_user_session_path + fill_in_auth_details(user.email, user.password) + end + + it 'allows creating a new user' do + subject + + # It shows an interstitial to log out + expect(page) + .to have_title(I18n.t('doorkeeper.authorizations.require_reauth.title')) + + click_on I18n.t('doorkeeper.authorizations.require_reauth.log_out_and_continue') + + # Shows rules + expect(page) + .to have_title(I18n.t('auth.register')) + .and have_text(rule.text) + + click_on I18n.t('auth.rules.accept') + + # It presents the user with a sign-up page + expect(page) + .to have_text(I18n.t('auth.register')) + + # Avoid the registration spam check + travel_to 10.seconds.from_now + + fill_in 'user_account_attributes_username', with: 'alice' + fill_in 'user_email', with: 'test@example.com' + fill_in 'user_password', with: 'Test.123.Pass' + fill_in 'user_password_confirmation', with: 'Test.123.Pass' + check 'user_agreement' + + click_on(I18n.t('auth.register')) + + # Directly authorizes the app + user = User.find_by(email: 'test@example.com') + grant = Doorkeeper::AccessGrant.find_by(application: client_app, resource_owner_id: user.id) + expect(grant).to be_present + + # Redirects to the app's callback URL + expect(page).to redirect_to_callback_url + + # Claiming the token marks the user as created by the app + expect { post '/oauth/token', params: { grant_type: 'authorization_code', client_id: client_app.uid, code: grant.token, client_secret: client_app.secret, redirect_uri: client_app.redirect_uri } } + .to change { user.reload.created_by_application_id }.to(client_app.id) + end + end end context 'when the user is not already logged in' do