DEV: Do not require session confirmation for new users (#24799)

When making sensitive changes to an account (adding 2FA or passkeys), we
require users to confirm their password. This is to prevent an attacker
from adding 2FA to an account they have access to.

However, on newly created accounts, we should not require this, it's an
extra step and it doesn't provide extra security (since the account was
just created). This commit makes it so that we don't require session
confirmation for accounts created less than 5 minutes ago.
This commit is contained in:
Penar Musaraj 2024-02-15 12:29:16 -05:00 committed by GitHub
parent 292685d3de
commit 974b3a2a6f
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 40 additions and 4 deletions

View File

@ -1525,7 +1525,7 @@ class UsersController < ApplicationController
end
def trusted_session
render json: secure_session_confirmed? ? success_json : failed_json
render json: secure_session_confirmed? || user_just_created ? success_json : failed_json
end
def list_second_factors
@ -1746,12 +1746,17 @@ class UsersController < ApplicationController
render json: success_json
end
def user_just_created
current_user.created_at > 5.minutes.ago
end
def check_confirmed_session
if SiteSetting.enable_discourse_connect || !SiteSetting.enable_local_logins
raise Discourse::NotFound
end
raise Discourse::InvalidAccess.new unless current_user && secure_session_confirmed?
raise Discourse::InvalidAccess.new if !current_user
raise Discourse::InvalidAccess.new unless user_just_created || secure_session_confirmed?
end
def revoke_account

View File

@ -4,7 +4,9 @@ require "rotp"
RSpec.describe UsersController do
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:user1) { Fabricate(:user, username: "someusername", refresh_auto_groups: true) }
fab!(:user1) do
Fabricate(:user, username: "someusername", refresh_auto_groups: true, created_at: 6.minutes.ago)
end
fab!(:another_user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:invitee) { Fabricate(:user) }
fab!(:inviter) { Fabricate(:user) }
@ -5486,6 +5488,19 @@ RSpec.describe UsersController do
expect(response_body["key"]).to be_present
expect(response_body["qr"]).to be_present
end
it "raises an error for a user created > 5 mins ago without a confirmed session" do
post "/users/create_second_factor_totp.json"
expect(response.status).to eq(403)
end
it "does not require confirming session for a user created < 5 mins ago" do
user1.update(created_at: Time.now.utc - 4.minutes)
post "/users/create_second_factor_totp.json"
expect(response.status).to eq(200)
end
end
end
end
@ -6498,7 +6513,7 @@ RSpec.describe UsersController do
expect(response.status).to eq(403)
end
it "resopnds with a 'failed' result by default" do
it "responds with a 'failed' result by default" do
sign_in(user1)
get "/u/trusted-session.json"
@ -6506,6 +6521,15 @@ RSpec.describe UsersController do
expect(response.parsed_body["failed"]).to eq("FAILED")
end
it "responds with a 'success' result if user was recently created" do
sign_in(user1)
user1.update(created_at: Time.now.utc - 4.minutes)
get "/u/trusted-session.json"
expect(response.status).to eq(200)
expect(response.parsed_body["success"]).to eq("OK")
end
it "response with 'success' on a confirmed session" do
user2 = Fabricate(:user, password: "8555039dd212cc66ec68")
sign_in(user2)

View File

@ -66,6 +66,10 @@ describe "Changing email", type: :system do
end
it "works when user has webauthn 2fa" do
# enforced 2FA flow needs a user created > 5 minutes ago
user.created_at = 6.minutes.ago
user.save!
sign_in user
DiscourseWebauthn.stubs(:origin).returns(current_host + ":" + Capybara.server_port.to_s)

View File

@ -9,6 +9,9 @@ describe "User preferences | Security", type: :system do
before do
user.activate
# testing the enforced 2FA flow requires a user that was created > 5 minutes ago
user.created_at = 6.minutes.ago
user.save!
sign_in(user)
# system specs run on their own host + port