From b64d01bc1030ef54ee96d205d30718781f2d59ae Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 24 Jul 2024 17:19:58 +1000 Subject: [PATCH] FIX: store information about the login method in the database. (#28054) Previously in these 2 PRs, we introduced a new site setting `SiteSetting.enforce_second_factor_on_external_auth`. https://github.com/discourse/discourse/pull/27547 https://github.com/discourse/discourse/pull/27674 When disabled, it should enforce 2FA for local login with username and password and skip the requirement when authenticating with oauth2. We stored information about the login method in a secure session but it is not reliable. Therefore, information about the login method is moved to the database. --- app/controllers/application_controller.rb | 2 +- app/controllers/session_controller.rb | 1 - .../users/omniauth_callbacks_controller.rb | 3 +-- app/models/user.rb | 3 +++ app/models/user_auth_token.rb | 27 ++++++++++--------- ...32_add_login_method_to_user_auth_tokens.rb | 7 +++++ lib/auth/default_current_user_provider.rb | 2 ++ spec/requests/application_controller_spec.rb | 4 +-- .../omniauth_callbacks_controller_spec.rb | 6 +---- spec/requests/session_controller_spec.rb | 1 + 10 files changed, 33 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20240724021732_add_login_method_to_user_auth_tokens.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0b2813f461c..e50a212bcac 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -609,7 +609,7 @@ class ApplicationController < ActionController::Base def login_method return if current_user.anonymous? - secure_session["oauth"] == "true" ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL + current_user.authenticated_with_oauth ? Auth::LOGIN_METHOD_OAUTH : Auth::LOGIN_METHOD_LOCAL end private diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 07cbdf93e0e..15928ec2bad 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -370,7 +370,6 @@ class SessionController < ApplicationController return render(json: @second_factor_failure_payload) if !second_factor_auth_result.ok if user.active && user.email_confirmed? - secure_session["oauth"] = false if !SiteSetting.persistent_sessions login(user, second_factor_auth_result) else not_activated(user) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index a4f1bd60c69..90d18a388ce 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -86,7 +86,6 @@ class Users::OmniauthCallbacksController < ApplicationController cookies["_bypass_cache"] = true cookies[:authentication_data] = { value: client_hash.to_json, path: Discourse.base_path("/") } - secure_session.set("oauth", true, expires: SiteSetting.maximum_session_age.hours) redirect_to @origin end @@ -183,7 +182,7 @@ class Users::OmniauthCallbacksController < ApplicationController return end - log_on_user(user) + log_on_user(user, { authenticated_with_oauth: true }) Invite.invalidate_for_email(user.email) # invite link can't be used to log in anymore session[:authentication] = nil # don't carry around old auth info, perhaps move elsewhere @auth_result.authenticated = true diff --git a/app/models/user.rb b/app/models/user.rb index cd38dde9993..9fdd3fcdb93 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -253,6 +253,9 @@ class User < ActiveRecord::Base # Cache for user custom fields. Currently it is used to display quick search results attr_accessor :custom_data + # Information if user was authenticated with OAuth + attr_accessor :authenticated_with_oauth + scope :with_email, ->(email) { joins(:user_emails).where("lower(user_emails.email) IN (?)", email) } diff --git a/app/models/user_auth_token.rb b/app/models/user_auth_token.rb index 14be316e2a9..c8e76a9d2d7 100644 --- a/app/models/user_auth_token.rb +++ b/app/models/user_auth_token.rb @@ -78,7 +78,8 @@ class UserAuthToken < ActiveRecord::Base client_ip: nil, path: nil, staff: nil, - impersonate: false + impersonate: false, + authenticated_with_oauth: false ) token = SecureRandom.hex(16) hashed_token = hash_token(token) @@ -90,6 +91,7 @@ class UserAuthToken < ActiveRecord::Base auth_token: hashed_token, prev_auth_token: hashed_token, rotated_at: Time.zone.now, + authenticated_with_oauth: !!authenticated_with_oauth, ) user_auth_token.unhashed_auth_token = token @@ -278,17 +280,18 @@ end # # Table name: user_auth_tokens # -# id :integer not null, primary key -# user_id :integer not null -# auth_token :string not null -# prev_auth_token :string not null -# user_agent :string -# auth_token_seen :boolean default(FALSE), not null -# client_ip :inet -# rotated_at :datetime not null -# created_at :datetime not null -# updated_at :datetime not null -# seen_at :datetime +# id :integer not null, primary key +# user_id :integer not null +# auth_token :string not null +# prev_auth_token :string not null +# user_agent :string +# auth_token_seen :boolean default(FALSE), not null +# client_ip :inet +# rotated_at :datetime not null +# created_at :datetime not null +# updated_at :datetime not null +# seen_at :datetime +# authenticated_with_oauth :boolean default(FALSE) # # Indexes # diff --git a/db/migrate/20240724021732_add_login_method_to_user_auth_tokens.rb b/db/migrate/20240724021732_add_login_method_to_user_auth_tokens.rb new file mode 100644 index 00000000000..4372451f4b8 --- /dev/null +++ b/db/migrate/20240724021732_add_login_method_to_user_auth_tokens.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddLoginMethodToUserAuthTokens < ActiveRecord::Migration[7.1] + def change + add_column :user_auth_tokens, :authenticated_with_oauth, :boolean, default: false + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 0800286ea9d..e7b0a256d48 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -140,6 +140,7 @@ class Auth::DefaultCurrentUserProvider end current_user = @user_token.try(:user) + current_user.authenticated_with_oauth = @user_token.authenticated_with_oauth if current_user end if !current_user @@ -267,6 +268,7 @@ class Auth::DefaultCurrentUserProvider client_ip: @request.ip, staff: user.staff?, impersonate: opts[:impersonate], + authenticated_with_oauth: opts[:authenticated_with_oauth], ) set_auth_cookie!(@user_token.unhashed_auth_token, user, cookie_jar) diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 65e86683057..539e8728825 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -152,8 +152,8 @@ RSpec.describe ApplicationController do it "should redirect users when enforce_second_factor is 'all' and authenticated via oauth" do SiteSetting.enforce_second_factor = "all" - write_secure_session("oauth", true) sign_in(user) + user.user_auth_tokens.last.update(authenticated_with_oauth: true) get "/" expect(response).to redirect_to("/u/#{user.username}/preferences/second-factor") @@ -162,8 +162,8 @@ RSpec.describe ApplicationController do it "should not redirect users when enforce_second_factor is 'all', authenticated via oauth but enforce_second_factor_on_external_auth is false" do SiteSetting.enforce_second_factor = "all" SiteSetting.enforce_second_factor_on_external_auth = false - write_secure_session("oauth", true) sign_in(user) + user.user_auth_tokens.last.update(authenticated_with_oauth: true) get "/" expect(response.status).to eq(200) diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index 2a412225603..1e27b1b9fb3 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -236,11 +236,6 @@ RSpec.describe Users::OmniauthCallbacksController do expect(data["email_valid"]).to eq(true) expect(data["can_edit_username"]).to eq(true) expect(data["destination_url"]).to eq(destination_url) - expect(read_secure_session["oauth"]).to eq("true") - expect(Discourse.redis.ttl("#{session[:secure_session_id]}oauth")).to be_between( - SiteSetting.maximum_session_age.hours.seconds - 10, - SiteSetting.maximum_session_age.hours.seconds, - ) end it "should return the right response for staged users" do @@ -402,6 +397,7 @@ RSpec.describe Users::OmniauthCallbacksController do user.reload expect(user.email_confirmed?).to eq(true) + expect(user.user_auth_tokens.last.authenticated_with_oauth).to be true end it "should return the authenticated response with the correct path for subfolders" do diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 25ad4e17853..a4e255bdf53 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -2010,6 +2010,7 @@ RSpec.describe SessionController do expect(session[:current_user_id]).to eq(user.id) expect(user.user_auth_tokens.count).to eq(1) + expect(user.user_auth_tokens.last.authenticated_with_oauth).to be false unhashed_token = decrypt_auth_cookie(cookies[:_t])[:token] expect(UserAuthToken.hash_token(unhashed_token)).to eq( user.user_auth_tokens.first.auth_token,