diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5b4ca6b5..5b49951d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -9,6 +9,7 @@ def current_user = nil # TODO: this is a temp hack to fix partials until /backen helper_method :detected_country_alpha2 + prepend_before_action :set_current_identity_session before_action :invalidate_v1_sessions, :authenticate_identity!, :set_honeybadger_context before_action :set_paper_trail_whodunnit @@ -36,6 +37,11 @@ def authenticate_identity! unless identity_signed_in? return if controller_name == "onboardings" + # The active account can expire while its siblings are still live. We never + # promote one silently, so send the user to pick instead of showing a login + # screen that makes it look like the whole browser was signed out. + return redirect_to browser_accounts_path if other_live_accounts_in_browser? + if request.xhr? redirect_to welcome_path else @@ -110,5 +116,23 @@ def current_onboarding_step private - def touch_session_last_seen_at = current_session&.touch_last_seen_at + def set_current_identity_session + # ||= so an explicit account selection made by an earlier prepended callback + # survives — see OidcAccountSelection. That selection, not the browser's + # active account, is what auth_time/amr/acr must describe. + Current.identity_session ||= current_session + end + + # Gated on the same flag check /accounts itself uses, so with the flag off both + # answer false and there is nothing to bounce between. + def other_live_accounts_in_browser? + return false unless account_chooser_available? + + current_browser_session&.live_identity_sessions&.exists? || false + end + + def touch_session_last_seen_at + current_session&.touch_last_seen_at + current_browser_session&.touch_last_seen_at + end end diff --git a/app/controllers/browser_accounts_controller.rb b/app/controllers/browser_accounts_controller.rb new file mode 100644 index 00000000..cc197774 --- /dev/null +++ b/app/controllers/browser_accounts_controller.rb @@ -0,0 +1,161 @@ +class BrowserAccountsController < ApplicationController + layout "logged_out" + + FEATURE_FLAG = :multi_account_sessions_2026_07_24 + + # Reachable with no active account: signing one account out leaves the browser + # session alive with the others, and we never auto-promote a sibling. + skip_before_action :authenticate_identity! + + before_action :require_browser_session + before_action :require_feature_flag + + def index + @accounts = accounts + @pending_token = params[:pending] + @preselect_public_id = params[:preselect] + @at_account_limit = current_browser_session.at_account_limit? + end + + def switch + target = find_account(params[:id]) + return account_not_found if target.nil? + + switch_account!(target) + remember_pending_selection(target) + + resume_or(root_path) + end + + # Leaves mid-flow to authenticate a different account, then comes back. The + # parked request travels as an opaque handle in return_to, so the login flow + # itself needs no knowledge of any of this. + def add + if current_browser_session.at_account_limit? + flash[:error] = t("accounts.limit_error", max: BrowserSession::MAX_ACCOUNTS) + return redirect_to browser_accounts_path(pending: params[:pending]) + end + + # Lets LoginsController#ensure_no_user! through for this flow only. + session[:adding_account] = true + + return_to = params[:pending].present? ? resume_browser_account_path(pending: params[:pending]) : root_path + redirect_to login_path(return_to: return_to) + end + + def destroy + target = find_account(params[:id]) + return account_not_found if target.nil? + + result = remove_account!(target) + return account_not_found if result.nil? + + flash[:info] = "Signed out of #{target.identity.primary_email}." + + if result == :signed_out + redirect_to welcome_path + else + redirect_to browser_accounts_path(pending: params[:pending]) + end + end + + def resume + pending = PendingAuthorization.consume!( + token: params[:pending], + browser_session: current_browser_session + ) + + if pending.nil? + flash[:error] = "That sign-in request expired. Start again from the app you were signing into." + return redirect_to root_path + end + + session.delete(:adding_account) + + # Authenticating an account through "use another account" *is* the choice. + # Without recording it the replayed request resolves as ambiguous again and + # bounces the user straight back to the chooser they just came from. + remember_selection_for(pending, current_session) + + redirect_to rebuilt_request_path(pending), allow_other_host: false + end + + private + + def accounts + current_browser_session + .live_identity_sessions + .includes(:identity, :login_attempt) + end + + def find_account(public_id) + return nil if public_id.blank? + + accounts.find { |session| session.identity.public_id == public_id } + end + + def account_not_found + flash[:error] = "That account isn't signed in on this browser." + redirect_to browser_accounts_path + end + + def require_browser_session + return if current_browser_session&.live_identity_sessions&.exists? + + redirect_to welcome_path + end + + def require_feature_flag + return if Flipper.enabled?(FEATURE_FLAG, current_identity) + + redirect_to root_path + end + + # Recording the choice before resuming means the replayed request resolves to + # this account without needing to carry it in the URL. + def remember_pending_selection(identity_session) + pending = PendingAuthorization.active.find_by(token: params[:pending]) + return if pending.nil? || pending.browser_session_id != current_browser_session.id + + remember_selection_for(pending, identity_session) + end + + def remember_selection_for(pending, identity_session) + return if identity_session.nil? + + kind, ref = client_reference_for(pending) + return if ref.blank? + + current_browser_session.remember_selection!(kind: kind, ref: ref, identity: identity_session.identity) + end + + def client_reference_for(pending) + case pending.kind + when "oidc" + [ "oidc", pending.payload.dig("params", "client_id") ] + when "saml" + [ "saml", pending.payload["entity_id"] ] + end + end + + def resume_or(fallback) + return redirect_to(resume_browser_account_path(pending: params[:pending])) if params[:pending].present? + + redirect_to fallback + end + + # Only ever rebuilds a request we parked ourselves, and only to the two + # endpoints that can park one. + def rebuilt_request_path(pending) + case pending.kind + when "oidc" + authorize_params = pending.payload["params"] || {} + "/oauth/authorize?#{authorize_params.to_query}" + when "saml" + url = pending.payload["url"].to_s + url.start_with?("/saml/auth") ? url : root_path + else + root_path + end + end +end diff --git a/app/controllers/concerns/oidc_account_selection.rb b/app/controllers/concerns/oidc_account_selection.rb new file mode 100644 index 00000000..dfb8b5b6 --- /dev/null +++ b/app/controllers/concerns/oidc_account_selection.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +# Account selection for the OIDC authorize endpoint. +# +# doorkeeper-openid_connect handles prompt=login and max_age for us (once +# auth_time comes from the right session), but it has no id_token_hint support, +# no way to return account_selection_required, and its select_account hook is +# unconfigured — which currently makes prompt=select_account a hard error. +# +# This runs ahead of the gem's authenticate_resource_owner! chain: if it +# redirects, the gem never sees the request. +module OidcAccountSelection + extend ActiveSupport::Concern + + CLIENT_KIND = "oidc" + + # Everything the authorize endpoint (Doorkeeper plus openid_connect) + # understands. Parking an allowlist rather than the raw request keeps Rails + # form cruft — authenticity_token, commit, _method — out of the replayed URL. + PARKED_AUTHORIZE_PARAMS = %w[ + client_id redirect_uri response_type response_mode scope state nonce + prompt max_age login_hint id_token_hint acr_values claims claims_locales + display ui_locales code_challenge code_challenge_method + ].freeze + + included do + prepend_before_action :resolve_oidc_account_selection, only: [ :new, :create ] + end + + # Consulted by Doorkeeper's resource_owner_authenticator so the grant is issued + # to the selected account rather than whichever one happens to be active. + def oidc_selected_identity + @oidc_selected_identity + end + + private + + def resolve_oidc_account_selection + return if params[:client_id].blank? + + hint = IdTokenHintVerifier.call(params[:id_token_hint], audience: params[:client_id]) + return render_oidc_selection_error(:invalid_request) if hint.invalid? + + decision = AccountSelectionResolver.new( + browser_session: current_browser_session, + client_kind: CLIENT_KIND, + client_ref: params[:client_id], + prompt: params[:prompt].to_s.split(/ +/), + login_hint: params[:login_hint], + id_token_hint_subject: hint.subject + ).call + + case decision.action + when :login_required + # Nothing signed in here: fall through to the normal unauthenticated path + # (resource_owner_authenticator redirects to /oauth/welcome). + nil + when :error + render_oidc_selection_error(decision.error) + when :chooser + return select_oidc_account(decision) if account_chooser_available? + + # Chooser disabled: behave exactly as before, using the active account. + apply_oidc_selection(current_session) + when :proceed + # A stale tab must not approve consent for an account other than the one + # whose data it displayed. + if request.post? && params[:selected_account].present? && + params[:selected_account] != decision.identity_session.identity.public_id + return select_oidc_account(decision) + end + + apply_oidc_selection(decision.identity_session) + end + end + + def apply_oidc_selection(identity_session) + return if identity_session.nil? + + @oidc_selected_identity = identity_session.identity + Current.identity_session = identity_session + + # Step-up happens on a new request, where request-local selection is gone. + # Make the account that actually needs reauthentication active before the + # OIDC hook redirects there, otherwise step-up would verify a sibling. + activate_oidc_session_for_reauthentication(identity_session) if oidc_reauthentication_required?(identity_session) + end + + def select_oidc_account(decision) + park_oidc_request_and_choose(preselect: decision.preselect_identity) + end + + # Also the backstop for doorkeeper-openid_connect's select_account hook, which + # is reached only on paths this concern doesn't resolve itself. + def park_oidc_request_and_choose(preselect: nil) + pending = PendingAuthorization.park!( + browser_session: current_browser_session, + kind: CLIENT_KIND, + payload: oidc_pending_payload + ) + + redirect_to browser_accounts_path( + pending: pending.token, + preselect: preselect&.public_id + ) + end + + def oidc_pending_payload + authorization_params = request.request_parameters + .merge(request.query_parameters) + .slice(*PARKED_AUTHORIZE_PARAMS) + + prompt_values = authorization_params["prompt"].to_s.split(/ +/).reject { |value| value == "select_account" } + if prompt_values.empty? + authorization_params.delete("prompt") + else + authorization_params["prompt"] = prompt_values.join(" ") + end + + { "params" => authorization_params } + end + + # OIDC error responses belong on the client's redirect_uri, not on an HTML + # error page — the RP has to be able to see them. + def render_oidc_selection_error(name) + error_response = + if name == :invalid_request + Doorkeeper::OAuth::InvalidRequestResponse.new( + name: name, + state: params[:state], + redirect_uri: params[:redirect_uri] + ) + else + Doorkeeper::OAuth::ErrorResponse.new( + name: name, + state: params[:state], + redirect_uri: params[:redirect_uri] + ) + end + + response.headers.merge!(error_response.headers) + + if oidc_error_redirect_uri_valid? + redirect_to error_response.redirect_uri, allow_other_host: true + else + render json: { error: name }, status: :bad_request + end + end + + def oidc_error_redirect_uri_valid? + application = Doorkeeper.config.application_model.find_by(uid: params[:client_id]) + return false if application.nil? || params[:redirect_uri].blank? + + Doorkeeper::OAuth::Helpers::URIChecker.valid_for_authorization?( + params[:redirect_uri], + application.redirect_uri + ) + end + + def oidc_reauthentication_required?(identity_session) + return true if params[:prompt].to_s.split(/ +/).include?("login") + + max_age = params[:max_age].to_s + max_age_seconds = max_age.to_i + return false unless max_age == "0" || max_age_seconds.positive? + + auth_time = [ identity_session.created_at, identity_session.last_step_up_at ].compact.max + auth_time.nil? || (Time.zone.now - auth_time) > [ 1, max_age_seconds ].max + end + + # Goes through switch_account! rather than activate! so an RP-triggered change + # of active account rotates the cookie and lands in the audit log exactly like + # a user-initiated switch — it is the same observable change. + def activate_oidc_session_for_reauthentication(identity_session) + browser_session = current_browser_session + return if browser_session.nil? || browser_session.active_identity_session_id == identity_session.id + + switch_account!(identity_session) + end +end diff --git a/app/controllers/concerns/saml_account_selection.rb b/app/controllers/concerns/saml_account_selection.rb new file mode 100644 index 00000000..2ebd9c5c --- /dev/null +++ b/app/controllers/concerns/saml_account_selection.rb @@ -0,0 +1,129 @@ +# frozen_string_literal: true + +# Account selection for the SAML IdP. +# +# SAML has no equivalent of prompt=select_account, so policy stands in for +# protocol: the first SSO to a service provider from a browser holding several +# accounts asks, and the answer is remembered per entity ID. `?select_account=1` +# forces the question again. +# +# IdP-initiated flows always ask, because there is no SP request to correlate +# against and nothing trustworthy to disambiguate with. +module SAMLAccountSelection + extend ActiveSupport::Concern + + CLIENT_KIND = "saml" + + included do + helper_method :saml_identity + end + + # The account this SSO is for. Falls back to the active account when selection + # doesn't apply (single account, or the chooser is off). + def saml_identity + @saml_selected_identity || current_identity + end + + private + + # Returns false when it has redirected or rendered, matching the `return unless` + # style the rest of SAMLController uses. + def resolve_saml_account!(entity_id:, force_chooser: false) + return true unless account_chooser_available? + + decision = AccountSelectionResolver.new( + browser_session: current_browser_session, + client_kind: CLIENT_KIND, + client_ref: entity_id, + force_chooser: force_chooser || params[:select_account].present? + ).call + + case decision.action + when :proceed + @saml_selected_identity = decision.identity_session.identity + true + when :chooser + park_saml_request_and_choose(entity_id: entity_id, decision: decision) + false + else + # :login_required is handled by SAMLController's own current_identity check. + true + end + end + + # The chooser records the choice and activates the account. The one-shot + # chooser trigger is removed so replay can continue with that selection. + def park_saml_request_and_choose(entity_id:, decision:) + pending = PendingAuthorization.park!( + browser_session: current_browser_session, + kind: CLIENT_KIND, + payload: { "url" => saml_replay_url, "entity_id" => entity_id } + ) + + redirect_to browser_accounts_path( + pending: pending.token, + preselect: decision.preselect_identity&.public_id + ) + end + + def saml_replay_url + replay_params = request.query_parameters.except("select_account") + query = replay_params.to_query + query.present? ? "#{request.path}?#{query}" : request.path + end + + # IdP-initiated can't be parked and replayed — there's no GET to come back to — + # so the chooser is rendered inline and posts straight back to this endpoint. + def render_saml_inline_chooser(entity_id:, accounts: nil) + decision = AccountSelectionResolver.new( + browser_session: current_browser_session, + client_kind: CLIENT_KIND, + client_ref: entity_id, + force_chooser: true + ).call + + @accounts = accounts || eligible_saml_accounts + @preselect_public_id = decision.preselect_identity&.public_id + + # Same layout as the standalone chooser: this is an interstitial in a sign-in + # flow, not a page of the app. The default layout would wrap it in the + # signed-in chrome, whose sidebar has no business rendering here. + render "saml/choose_account", layout: "logged_out" + end + + def resolve_saml_account_from_params!(entity_id:) + return true if params[:selected_account].blank? + + session_for_selection = eligible_saml_accounts.find do |ident_session| + ident_session.identity.public_id == params[:selected_account] + end + + if session_for_selection.nil? + @error = "That account isn't signed in on this browser." + render :error, status: :bad_request + return false + end + + @saml_selected_identity = session_for_selection.identity + current_browser_session&.remember_selection!( + kind: CLIENT_KIND, ref: entity_id, identity: session_for_selection.identity + ) + true + end + + # Offering an account the SP will reject is worse than not offering it. Filtered + # against the same allowed_emails list the SSO endpoints enforce. + def eligible_saml_accounts + sessions = current_browser_session&.live_identity_sessions&.includes(:identity, :login_attempt) || [] + allowed = @sp_config&.dig(:allowed_emails) + return sessions.to_a if allowed.blank? + + sessions.select { |ident_session| allowed.include?(ident_session.identity.primary_email) } + end + + def saml_account_selection_needed? + return false unless account_chooser_available? + + eligible_saml_accounts.size > 1 + end +end diff --git a/app/controllers/logins_controller.rb b/app/controllers/logins_controller.rb index ae6b699f..0f4d7ec7 100644 --- a/app/controllers/logins_controller.rb +++ b/app/controllers/logins_controller.rb @@ -292,6 +292,12 @@ def send_v2_login_code(identity, attempt = nil) end def handle_post_verification_redirect + # /accounts/add checks the cap before sending anyone to the login form, but + # that check and this one are separate requests. Check again here, and + # rescue below for the case where a concurrent login takes the last slot + # in between — sign_in raises rather than evicting anyone. + return account_limit_reached if browser_account_limit_reached? + # Only create session if authentication requirements are met LoginAttempt.transaction do @attempt.lock! @@ -336,6 +342,24 @@ def handle_post_verification_redirect redirect_to root_path end end + rescue SessionsHelper::AccountLimitError + # Lost a race with another tab after the pre-check above. + account_limit_reached + end + + # Reauthenticating an account that is already here replaces its session rather + # than adding one, so it is never blocked by the cap. + def browser_account_limit_reached? + browser_session = current_browser_session + return false if browser_session.nil? || browser_session.expired? + return false if browser_session.identity_session_for(@identity).present? + + browser_session.at_account_limit? + end + + def account_limit_reached + flash[:error] = I18n.t("accounts.limit_error", max: BrowserSession::MAX_ACCOUNTS) + redirect_to browser_accounts_path end def provision_slack_on_first_login(scenario) diff --git a/app/controllers/saml_controller.rb b/app/controllers/saml_controller.rb index ff161139..2c58bd20 100644 --- a/app/controllers/saml_controller.rb +++ b/app/controllers/saml_controller.rb @@ -1,5 +1,6 @@ class SAMLController < ApplicationController include SAMLHelper + include SAMLAccountSelection layout "logged_out", only: [ :welcome ] @@ -20,7 +21,6 @@ def idp_initiated end return unless ensure_sp_configured!(slug: params[:slug]) - return unless check_allowed_emails! unless @sp_config[:allow_idp_initiated] @error = "This SP is not configured for IdP-initiated authentication" @@ -31,7 +31,23 @@ def idp_initiated redirect_to saml_welcome_path(return_to: request.fullpath) and return end - if params[:slug] == "slack" && current_identity.disallow_slack + # IdP-initiated has no SP request to correlate against, so it always asks when + # this browser holds more than one eligible account. + return unless resolve_saml_account_from_params!(entity_id: @sp_config[:entity_id]) + + if @saml_selected_identity.nil? + eligible_accounts = eligible_saml_accounts + + if eligible_accounts.one? + @saml_selected_identity = eligible_accounts.first.identity + elsif eligible_accounts.many? + render_saml_inline_chooser(entity_id: @sp_config[:entity_id], accounts: eligible_accounts) and return + end + end + + return unless check_allowed_emails! + + if params[:slug] == "slack" && saml_identity.disallow_slack @error = "Unable to log in right now" render :error, status: :forbidden and return end @@ -41,16 +57,16 @@ def idp_initiated # Try to assign to Slack workspace if not yet done if params[:slug] == "slack" provision_slack_via_scim_if_needed - try_assign_to_slack_workspace unless current_identity.is_in_workspace + try_assign_to_slack_workspace unless saml_identity.is_in_workspace end response = build_saml_response( - identity: current_identity, + identity: saml_identity, sp_config: @sp_config, in_response_to: nil ) - render_saml_response(saml_response: response, sp_config: @sp_config) + render_saml_response(saml_response: response, sp_config: @sp_config, identity: saml_identity) end def sp_initiated_get @@ -63,9 +79,13 @@ def sp_initiated_get redirect_to saml_welcome_path(return_to: request.fullpath) and return end + # Selection runs before check_replay! — the request may be parked and replayed + # after the chooser, and marking it as seen first would break that. + return unless resolve_saml_account!(entity_id: @sp_config[:entity_id]) + return unless check_allowed_emails! - if @sp_config[:slug] == "slack" && current_identity.disallow_slack + if @sp_config[:slug] == "slack" && saml_identity.disallow_slack @error = "Unable to log in right now" render :error, status: :forbidden and return end @@ -76,13 +96,19 @@ def sp_initiated_get # back to this same URL after login return unless check_replay! + current_browser_session&.remember_selection!( + kind: SAMLAccountSelection::CLIENT_KIND, + ref: @sp_config[:entity_id], + identity: saml_identity + ) + response = build_saml_response( - identity: current_identity, + identity: saml_identity, sp_config: @sp_config, in_response_to: @authn_request ) - render_saml_response(saml_response: response, sp_config: @sp_config) + render_saml_response(saml_response: response, sp_config: @sp_config, identity: saml_identity) rescue SAML2::MissingMessage # hotfix for zach email @@ -116,26 +142,26 @@ def welcome private def provision_slack_via_scim_if_needed - return if current_identity.slack_id.present? + return if saml_identity.slack_id.present? - scenario = current_identity.onboarding_scenario_instance + scenario = saml_identity.onboarding_scenario_instance slack_result = SCIMService.find_or_create_user( - identity: current_identity, + identity: saml_identity, scenario: scenario ) if slack_result[:success] - current_identity.update(slack_id: slack_result[:slack_id]) - Rails.logger.info "Slack provisioning successful via SCIM for #{current_identity.id}: #{slack_result[:message]}" + saml_identity.update(slack_id: slack_result[:slack_id]) + Rails.logger.info "Slack provisioning successful via SCIM for #{saml_identity.id}: #{slack_result[:message]}" else - Rails.logger.error "Slack provisioning failed via SCIM for #{current_identity.id}: #{slack_result[:error]}" + Rails.logger.error "Slack provisioning failed via SCIM for #{saml_identity.id}: #{slack_result[:error]}" Sentry.capture_message( "Slack provisioning failed via SCIM", level: :error, extra: { - identity_public_id: current_identity.public_id, - identity_email: current_identity.primary_email, + identity_public_id: saml_identity.public_id, + identity_email: saml_identity.primary_email, slack_error: slack_result[:error] } ) @@ -144,20 +170,20 @@ def provision_slack_via_scim_if_needed end def try_assign_to_slack_workspace - return unless current_identity.slack_id.present? + return unless saml_identity.slack_id.present? - case SlackService.user_workspace_status(user_id: current_identity.slack_id) + case SlackService.user_workspace_status(user_id: saml_identity.slack_id) when :in_workspace - current_identity.update(is_in_workspace: true) unless current_identity.is_in_workspace + saml_identity.update(is_in_workspace: true) unless saml_identity.is_in_workspace when :not_in_workspace - scenario = current_identity.onboarding_scenario_instance + scenario = saml_identity.onboarding_scenario_instance return unless scenario.slack_channels.any? AssignSlackWorkspaceJob.perform_later( - slack_id: current_identity.slack_id, + slack_id: saml_identity.slack_id, user_type: scenario.slack_user_type, channel_ids: scenario.slack_channels, - identity_id: current_identity.id + identity_id: saml_identity.id ) end end @@ -296,9 +322,9 @@ def ensure_sp_configured!(entity_id: nil, slug: nil) def check_allowed_emails! return true unless @sp_config[:allowed_emails].present? - return true unless current_identity + return true unless saml_identity - unless @sp_config[:allowed_emails].include?(current_identity.primary_email) + unless @sp_config[:allowed_emails].include?(saml_identity.primary_email) @error = "You are not authorized to access this service" render :error, status: :forbidden and return false end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 0f818c04..fcf4aeb9 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,7 +1,27 @@ class SessionsController < ApplicationController + # Signing out with several accounts in the browser leaves the browser session + # alive but with no active account, so this has to stay reachable. + skip_before_action :authenticate_identity!, only: [ :logout_all ] + def logout - flash[:info] = "You've been logged out. Nice seeing you!" - sign_out + # Read the flag before signing out — afterwards there's no identity to gate on. + per_account = Flipper.enabled?(BrowserAccountsController::FEATURE_FLAG, current_identity) + + result = per_account ? sign_out : sign_out_all_accounts + + if result == :accounts_remaining + flash[:info] = "Signed out of that account." + redirect_to browser_accounts_path + else + flash[:info] = "You've been logged out. Nice seeing you!" + redirect_to welcome_path + end + end + + def logout_all + sign_out_all_accounts + + flash[:info] = "You've been signed out of every account in this browser." redirect_to welcome_path end end diff --git a/app/helpers/account_monogram_helper.rb b/app/helpers/account_monogram_helper.rb new file mode 100644 index 00000000..08baf26c --- /dev/null +++ b/app/helpers/account_monogram_helper.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +# Identities have no avatar, and fetching one from a third party would leak an +# email hash on every chooser render. A deterministic monogram is enough to make +# rows visually distinct without adding an avatar pipeline or a privacy problem. +module AccountMonogramHelper + HACK_CLUB_EMAIL_DOMAIN = "hackclub.com" + + # Picked for adequate contrast against white monogram text in both themes. + MONOGRAM_COLORS = %w[ + #2f6f4f + #1f5f8b + #5b3f9d + #8b2f5f + #8b4a1f + #3f5f2f + #1f6f6f + #6b2f2f + ].freeze + + def account_monogram_initials(identity) + initials = [ identity.first_name, identity.last_name ] + .compact_blank + .map { |name| name.to_s.strip.first } + .join + .upcase + + initials.presence || identity.primary_email.to_s.first.to_s.upcase.presence || "?" + end + + def account_monogram_color(identity) + seed = Digest::SHA256.hexdigest(identity.public_id.to_s).to_i(16) + MONOGRAM_COLORS[seed % MONOGRAM_COLORS.size] + end + + def hack_club_account?(identity) + identity.primary_email.to_s.downcase.end_with?("@#{HACK_CLUB_EMAIL_DOMAIN}") + end + + # Distinguishing a work identity from a personal one is the whole point of the + # chooser, so it gets a text label rather than colour alone. + def account_kind_label(identity) + hack_club_account?(identity) ? t("accounts.kind.hack_club") : t("accounts.kind.personal") + end +end diff --git a/app/helpers/saml_helper.rb b/app/helpers/saml_helper.rb index b60fe780..e9b7c96e 100644 --- a/app/helpers/saml_helper.rb +++ b/app/helpers/saml_helper.rb @@ -27,7 +27,10 @@ def build_saml_response(identity:, sp_config:, in_response_to: nil) saml_response end - def render_saml_response(saml_response:, sp_config:) + # `identity` is the account the assertion was built for, which is not always the + # browser's active account once several are signed in. + def render_saml_response(saml_response:, sp_config:, identity: current_identity) + @saml_identity = identity signed_xml = SAMLService::Signing.sign_response(saml_response) @saml_response = Base64.strict_encode64(signed_xml.to_s) @saml_acs_url = sp_config[:entity].service_providers.first.assertion_consumer_services.default.location @@ -37,8 +40,8 @@ def render_saml_response(saml_response:, sp_config:) render :error, status: :bad_request and return end - if current_identity - current_identity.create_activity :saml_login, owner: current_identity, recipient: current_identity, + if identity + identity.create_activity :saml_login, owner: identity, recipient: identity, parameters: { service_provider: sp_config[:slug], name: sp_config[:friendly_name] } end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 1c4025c5..01541c60 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -2,55 +2,119 @@ module SessionsHelper class AccountLockedError < StandardError; end + class AccountLimitError < StandardError; end - def sign_in(identity:, fingerprint_info: {}, impersonate: false) + SESSION_DURATION = 1.month + COOKIE_NAME = :session_token + + # Signs `identity` in. By default the account joins whatever browser session + # this request already has, so a browser can hold several accounts at once. + # Pass `browser_session: nil` to deliberately start a fresh one. + # + # Signing in account B never inherits account A's authentication — B gets its + # own IdentitySession, with its own expiry, step-up state and login factors. + def sign_in(identity:, fingerprint_info: {}, browser_session: :current) raise(AccountLockedError, "Your HCB account has been locked.") if identity.locked? # Preserve fingerprint info from session if not passed fingerprint_info = session[:fingerprint_info] if fingerprint_info.blank? && session[:fingerprint_info].present? + fingerprint_info = (fingerprint_info || {}).with_indifferent_access # Preserve flow data before resetting session return_to = session[:return_to] + target = browser_session == :current ? current_browser_session : browser_session + reset_session # Restore flow data after session reset session[:return_to] = return_to if return_to.present? - session_token = SecureRandom.urlsafe_base64 - session_duration = 1.month - expires_at = session_duration.seconds.from_now - cookies.encrypted[:session_token] = { - value: session_token, - expires: expires_at, - httponly: true, - secure: Rails.env.production?, - same_site: :lax - } - cookies.encrypted[:signed_user] = { - value: identity.signed_id(expires_in: 2.months, purpose: :remember_me), - expires: 2.months.from_now, - httponly: true, - secure: Rails.env.production?, - same_site: :lax - } - ident_session = identity.sessions.build( - session_token:, + expires_at = SESSION_DURATION.from_now + session_attributes = { + session_token: BrowserSession.generate_token, fingerprint: fingerprint_info[:fingerprint], device_info: fingerprint_info[:device_info], os_info: fingerprint_info[:os_info], timezone: fingerprint_info[:timezone], ip: fingerprint_info[:ip], - expires_at: - ) + expires_at: expires_at + } + + ident_session = nil + added_to_existing = false + + BrowserSession.transaction do + if target&.persisted? && !target.expired? + existing = target.identity_session_for(identity) + + if existing + # This was a real authentication, not an account switch. Replace the + # old session so expiry, auth_time and the LoginAttempt assurance + # binding all describe the authentication that just completed. + existing.revoke!(reason: "reauthenticated") + ident_session = identity.sessions.create!(browser_session: target, **session_attributes) + else + raise AccountLimitError if target.at_account_limit? + + added_to_existing = target.live_identity_sessions.exists? + ident_session = identity.sessions.create!(browser_session: target, **session_attributes) + end - ident_session.save! + target.activate!(ident_session) + # Rotate on every change to the account set (session fixation). + target.rotate_token! + target.extend_expiry!(expires_at) + else + target = BrowserSession.start!(expires_at: expires_at) + ident_session = identity.sessions.create!(browser_session: target, **session_attributes) + target.activate!(ident_session) + end + end + + write_browser_session_cookie(target) + + if added_to_existing + # Deliberately does not name the other accounts in this browser: audit log + # entries are visible to the account they belong to. + ident_session.create_activity :account_added, owner: identity, recipient: identity + end + + @current_browser_session = target + @current_session = ident_session self.current_identity = identity ident_session end + # Makes an account that's already signed into this browser the active one. + def switch_account!(identity_session) + browser_session = current_browser_session + return nil if browser_session.nil? + return nil unless identity_session&.live? + return nil unless identity_session.browser_session_id == browser_session.id + + browser_session.activate!(identity_session) + browser_session.rotate_token! + write_browser_session_cookie(browser_session) + + identity_session.create_activity :account_switched, + owner: identity_session.identity, recipient: identity_session.identity + + @current_session = identity_session + self.current_identity = identity_session.identity + + identity_session + end + + # Set while a signed-in user is deliberately authenticating an additional + # account, which is the one case where reaching the login form while already + # signed in is intended. + def adding_account? = session[:adding_account].present? + def ensure_no_user! + return if adding_account? + if identity_signed_in? flash[:info] = "you're already logged in, silly!" redirect_to root_path @@ -67,38 +131,169 @@ def current_identity @current_identity ||= current_session&.identity end + def current_browser_session + return @current_browser_session if defined?(@current_browser_session) + + token = cookies.encrypted[COOKIE_NAME] + return @current_browser_session = nil if token.blank? + + resolution = SessionResolver.resolve(token) + + @current_browser_session = + if resolution.nil? + nil + elsif resolution.legacy? + SessionResolver.adopt_legacy!(resolution.identity_session, token: token) + else + resolution.browser_session + end + end + + # The account session this request is acting as. Nil when the active account + # has expired even though siblings are still live — we never silently promote + # another account, because that would change `sub` mid-session. def current_session return @current_session if defined?(@current_session) - session_token = cookies.encrypted[:session_token] + @current_session = current_browser_session&.active_session + end - return nil if session_token.nil? + # The account a relying-party request is being authorized for. Set by + # OidcAccountSelection when the browser holds several accounts; otherwise the + # active one. Defined here rather than as a controller helper_method so views + # rendered outside that concern degrade sensibly instead of raising. + def authorizing_identity + @oidc_selected_identity || current_identity + end - # Find a valid session (not expired) using the session token - @current_session = IdentitySession.not_expired.find_by(session_token:) + def account_chooser_available? + Flipper.enabled?(BrowserAccountsController::FEATURE_FLAG, current_identity) end - def sign_out - session = current_identity - &.sessions - &.find_by(session_token: cookies.encrypted[:session_token]) + def other_account_sessions + browser_session = current_browser_session + return IdentitySession.none if browser_session.nil? - if session - session.update(signed_out_at: Time.now, expires_at: Time.now) - session.create_activity :sign_out, owner: current_identity, recipient: current_identity + browser_session.live_identity_sessions.where.not(id: current_session&.id) + end + + def multiple_accounts_signed_in? + (current_browser_session&.account_count || 0) > 1 + end + + # Signs out one account, not the browser. Returns :accounts_remaining when + # other accounts are still signed in here, otherwise :signed_out. + def sign_out(identity_session: current_session, reason: "user_signout") + browser_session = current_browser_session + target = identity_session + + if target + target.revoke!(reason: reason) + target.create_activity :sign_out, owner: target.identity, recipient: target.identity + end + + if browser_session + browser_session.reload + + if browser_session.live_identity_sessions.exists? + if browser_session.active_identity_session_id == target&.id + browser_session.update!(active_identity_session: nil) + end + browser_session.rotate_token! + + # The Rails session belongs to the account that just left — fingerprint + # info, return_to and the rest must not follow the next account around. + # Only the browser session cookie survives, and it is rewritten below. + reset_session + write_browser_session_cookie(browser_session) + + @current_browser_session = browser_session + @current_session = nil + self.current_identity = nil + + return :accounts_remaining + end + + browser_session.destroy! + end + + forget_browser_session_cookie + @current_browser_session = nil + @current_session = nil + self.current_identity = nil + + reset_session + + :signed_out + end + + # Signs out every account in this browser and discards the browser session. + def sign_out_all_accounts(reason: "user_signout_all") + browser_session = current_browser_session + + if browser_session + browser_session.live_identity_sessions.each do |ident_session| + ident_session.revoke!(reason: reason) + ident_session.create_activity :sign_out, + owner: ident_session.identity, recipient: ident_session.identity + end + browser_session.destroy! end - cookies.delete(:session_token) + forget_browser_session_cookie + @current_browser_session = nil + @current_session = nil self.current_identity = nil reset_session + + :signed_out + end + + # Removes an account from this browser without touching the active one. + def remove_account!(identity_session, reason: "user_removed") + browser_session = current_browser_session + return nil if browser_session.nil? + return nil unless identity_session&.browser_session_id == browser_session.id + + return sign_out(identity_session: identity_session, reason: reason) if identity_session.id == current_session&.id + + identity_session.revoke!(reason: reason) + identity_session.create_activity :account_removed, + owner: identity_session.identity, recipient: identity_session.identity + + browser_session.rotate_token! + write_browser_session_cookie(browser_session) + + :removed end + # Every session for this identity on other devices. Distinct from the accounts + # held by this browser — don't conflate the two in UI. def sign_out_of_all_sessions(identity = current_identity) # Destroy all the sessions except the current session identity &.sessions &.where&.not(id: current_session&.id) - &.update_all(signed_out_at: Time.now, expires_at: Time.now) + &.update_all(signed_out_at: Time.now, expires_at: Time.now, revoked_reason: "user_signout_other_devices") + end + + private + + def write_browser_session_cookie(browser_session) + cookies.encrypted[COOKIE_NAME] = { + value: browser_session.token, + expires: browser_session.expires_at, + httponly: true, + secure: Rails.env.production?, + same_site: :lax + } + end + + def forget_browser_session_cookie + cookies.delete(COOKIE_NAME) + # Written by an earlier implementation, never read, and previously never + # cleaned up on sign-out. + cookies.delete(:signed_user) end end diff --git a/app/models/browser_session.rb b/app/models/browser_session.rb new file mode 100644 index 00000000..20a6170f --- /dev/null +++ b/app/models/browser_session.rb @@ -0,0 +1,155 @@ +# A browser session is the thing the cookie points at. It owns one or more +# IdentitySessions — one per signed-in account — and a pointer to whichever one +# is currently active. +# +# Authentication assurance is deliberately NOT stored here. Expiry, step-up and +# login factors all live on the individual IdentitySession, so signing into a +# second account never inherits the first account's 2FA. +class BrowserSession < ApplicationRecord + # A person with a work and a personal account needs two. Anything past this is + # not a use case we're supporting, and an unbounded list is a footgun. + MAX_ACCOUNTS = 5 + + LAST_SEEN_AT_COOLDOWN = 5.minutes + + # Skipping :token alone isn't enough — the columns PaperTrail actually sees are + # the ciphertext and the blind index, and the blind index is deterministic, so + # versioning it would let anyone with the versions table correlate cookies. + has_paper_trail skip: [ :token, :token_ciphertext, :token_bidx ] + has_encrypted :token + blind_index :token + + has_many :identity_sessions, dependent: :nullify + belongs_to :active_identity_session, class_name: "IdentitySession", optional: true + has_many :client_selections, class_name: "BrowserSession::ClientSelection", dependent: :destroy + has_many :pending_authorizations, dependent: :destroy + + validates :token, presence: true + + validate :active_session_belongs_to_this_browser_session + + scope :expired, -> { where("expires_at <= ?", Time.now) } + scope :not_expired, -> { where("expires_at > ?", Time.now) } + + def self.generate_token = SecureRandom.urlsafe_base64 + + def self.start!(expires_at:) + create!(token: generate_token, expires_at: expires_at) + end + + def expired? = expires_at <= Time.now + + # Every account currently usable in this browser. Oldest first so the chooser + # order is stable as accounts come and go. + def live_identity_sessions + identity_sessions.not_expired.where(signed_out_at: nil).order(:created_at) + end + + def live_identities + Identity.where(id: live_identity_sessions.select(:identity_id)) + end + + def identity_session_for(identity) + live_identity_sessions.find_by(identity_id: identity.id) + end + + def account_count = live_identity_sessions.count + + def at_account_limit? = account_count >= MAX_ACCOUNTS + + # The active session may have expired while other accounts are still live. We + # never silently promote a sibling — that would change `sub` mid-session — so + # this returns nil and the caller sends the user to the chooser. + def active_session + session = active_identity_session + return nil if session.nil? + return nil if session.expired? || session.signed_out_at.present? + + session + end + + def active_identity = active_session&.identity + + def activate!(identity_session) + unless identity_session.browser_session_id == id + raise ArgumentError, "identity session does not belong to this browser session" + end + + update!(active_identity_session: identity_session) + end + + # Session fixation defence: the cookie value changes whenever the set of + # accounts in this browser changes, without disturbing the accounts themselves. + def rotate_token! + update!(token: self.class.generate_token) + token + end + + def extend_expiry!(new_expires_at) + return if expires_at.present? && expires_at >= new_expires_at + + update!(expires_at: new_expires_at) + end + + def touch_last_seen_at + return if last_seen&.after?(LAST_SEEN_AT_COOLDOWN.ago) + + update_column(:last_seen, Time.current) + end + + def selection_for(kind:, ref:) + client_selections.find_by(client_kind: kind.to_s, client_ref: ref.to_s) + end + + # Upsert rather than find-then-save: concurrent tabs authorizing the same client + # would otherwise race the unique index, and a constraint violation raised + # inside a surrounding transaction poisons it — no rescue can recover there. + def remember_selection!(kind:, ref:, identity:) + kind = kind.to_s + ref = ref.to_s + raise ArgumentError, "unknown client kind #{kind}" unless ClientSelection::KINDS.include?(kind) + + now = Time.current + ClientSelection.upsert_all( + [ { + browser_session_id: id, + client_kind: kind, + client_ref: ref, + identity_id: identity.id, + last_used_at: now, + created_at: now, + updated_at: now + } ], + unique_by: :index_client_selections_on_browser_session_and_client, + # updated_at is appended by Rails; naming it here too is a syntax error. + update_only: [ :identity_id, :last_used_at ] + ) + + client_selections.reset + selection_for(kind: kind, ref: ref) + end + + # Sticky selections point at identities, so a remembered account may no longer + # have a live session here. Callers treat that as "no selection" but can still + # preselect it in the chooser. + def remembered_identity_session(kind:, ref:) + identity_id = selection_for(kind: kind, ref: ref)&.identity_id + return nil unless identity_id + + live_identity_sessions.find_by(identity_id: identity_id) + end + + def revoke_all!(reason:) + live_identity_sessions.each { |s| s.revoke!(reason: reason) } + update!(active_identity_session: nil) + end + + private + + def active_session_belongs_to_this_browser_session + return if active_identity_session_id.nil? + return if active_identity_session&.browser_session_id == id + + errors.add(:active_identity_session, "must belong to this browser session") + end +end diff --git a/app/models/browser_session/client_selection.rb b/app/models/browser_session/client_selection.rb new file mode 100644 index 00000000..27d6241f --- /dev/null +++ b/app/models/browser_session/client_selection.rb @@ -0,0 +1,17 @@ +# Remembers which account a browser last used for a given relying party, so +# returning to a tool doesn't re-prompt. Keyed by client_id for OIDC and by SP +# entity ID for SAML. +# +# This is a convenience only. It never overrides prompt=select_account, an +# id_token_hint, or a login_hint naming a different account. +class BrowserSession::ClientSelection < ApplicationRecord + self.table_name = "browser_session_client_selections" + + KINDS = %w[oidc saml].freeze + + belongs_to :browser_session + belongs_to :identity + + validates :client_kind, presence: true, inclusion: { in: KINDS } + validates :client_ref, presence: true +end diff --git a/app/models/current.rb b/app/models/current.rb new file mode 100644 index 00000000..5873e111 --- /dev/null +++ b/app/models/current.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Current < ActiveSupport::CurrentAttributes + attribute :identity_session +end diff --git a/app/models/identity_session.rb b/app/models/identity_session.rb index 470b41e2..7e106b21 100644 --- a/app/models/identity_session.rb +++ b/app/models/identity_session.rb @@ -1,11 +1,16 @@ class IdentitySession < ApplicationRecord LAST_SEEN_AT_COOLDOWN = 5.minutes - has_paper_trail skip: [ :session_token ] + # :session_token alone doesn't match the real columns, and the blind index is + # deterministic — see BrowserSession for the same fix. + has_paper_trail skip: [ :session_token, :session_token_ciphertext, :session_token_bidx ] has_encrypted :session_token blind_index :session_token belongs_to :identity + # Nullable while legacy pre-multi-account sessions are still alive; they get + # adopted into a BrowserSession on their next request. + belongs_to :browser_session, optional: true has_one :login_attempt, foreign_key: :session_id include PublicActivity::Model @@ -31,6 +36,56 @@ class IdentitySession < ApplicationRecord def expired? = expires_at <= Time.now + def live? = !expired? && signed_out_at.nil? + + def revoke!(reason: nil) + now = Time.now + update!(signed_out_at: now, expires_at: now, revoked_reason: reason) + end + + # Authentication assurance, derived from this session's own login factors. + # Never from the browser session, and never from a sibling account. + # + # RFC 8176 has no value for an emailed login link/code, so it maps to `otp` + # alongside TOTP. Lossy, but every alternative is either a lie or a + # non-standard value relying parties won't recognise. + AMR_BY_FACTOR = { + "email" => "otp", + "legacy_email" => "otp", + "totp" => "otp", + "backup_code" => "otp", + "webauthn" => "hwk" + }.freeze + + ACR_SINGLE_FACTOR = "urn:hackclub:auth:1" + ACR_MULTI_FACTOR = "urn:hackclub:auth:2" + + def completed_authentication_factors + factors = login_attempt&.authentication_factors + return [] if factors.blank? + + factors.select { |_name, satisfied| satisfied }.keys + end + + def multi_factor? = completed_authentication_factors.size >= 2 + + # nil rather than a guess when there's no factor record — legacy sessions + # predate LoginAttempt binding, and omitting the claim is honest. + def amr_values + factors = completed_authentication_factors + return nil if factors.empty? + + values = factors.filter_map { |factor| AMR_BY_FACTOR[factor] }.uniq + values << "mfa" if multi_factor? + values.presence + end + + def acr_value + return nil if completed_authentication_factors.empty? + + multi_factor? ? ACR_MULTI_FACTOR : ACR_SINGLE_FACTOR + end + def clear_metadata! update!( device_info: nil, diff --git a/app/models/pending_authorization.rb b/app/models/pending_authorization.rb new file mode 100644 index 00000000..5307e713 --- /dev/null +++ b/app/models/pending_authorization.rb @@ -0,0 +1,60 @@ +# A parked authorization request, so "use another account" can leave the middle +# of an OIDC or SAML flow, run a full login, and come back with every parameter +# intact. +# +# The URL only ever carries the opaque handle. The request itself is encrypted at +# rest because the payload can include a login_hint. +class PendingAuthorization < ApplicationRecord + EXPIRATION = 15.minutes + KINDS = %w[oidc saml].freeze + + has_encrypted :token + blind_index :token + has_encrypted :payload, type: :json + + belongs_to :browser_session + + validates :kind, presence: true, inclusion: { in: KINDS } + + scope :active, -> { where(consumed_at: nil).where("expires_at > ?", Time.now) } + + def self.generate_token = SecureRandom.urlsafe_base64 + + def self.park!(browser_session:, kind:, payload:) + # Handles live for 15 minutes and are never read again afterwards. Reaping the + # browser session's own dead ones here keeps the table bounded without needing + # a scheduled job. + browser_session.pending_authorizations.where("expires_at <= ?", Time.now).delete_all + + create!( + browser_session: browser_session, + kind: kind.to_s, + payload: payload, + token: generate_token, + expires_at: EXPIRATION.from_now + ) + end + + # Single use, and only by the browser session that parked it. Both checks + # happen under a row lock so two tabs can't both resume the same request. + def self.consume!(token:, browser_session:, kind: nil) + return nil if token.blank? || browser_session.nil? + + record = active.find_by(token: token) + return nil if record.nil? + + record.with_lock do + return nil if record.consumed_at.present? + return nil if record.browser_session_id != browser_session.id + return nil if kind.present? && record.kind != kind.to_s + + record.update!(consumed_at: Time.current) + end + + record + end + + def expired? = expires_at <= Time.now + + def consumed? = consumed_at.present? +end diff --git a/app/services/account_selection_resolver.rb b/app/services/account_selection_resolver.rb new file mode 100644 index 00000000..93356251 --- /dev/null +++ b/app/services/account_selection_resolver.rb @@ -0,0 +1,112 @@ +# Decides which account a relying party request should use, given what's signed +# into this browser plus whatever the request asked for. +# +# Deliberately free of HTTP so the whole decision table is unit-testable. Shared +# by the OIDC authorize endpoint and the SAML SSO endpoint — SAML has no +# `prompt`, so it simply passes none. +class AccountSelectionResolver + # :proceed — use decision.identity_session, no UI + # :chooser — ask the user; preselect_identity is a suggestion, not a decision + # :login_required — nothing usable is signed in here + # :error — resolve was impossible without UI (prompt=none) + Decision = Data.define(:action, :identity_session, :preselect_identity, :login_hint, :error) do + def proceed? = action == :proceed + def chooser? = action == :chooser + def login_required? = action == :login_required + def error? = action == :error + end + + def initialize(browser_session:, client_kind:, client_ref:, prompt: [], login_hint: nil, + id_token_hint_subject: nil, force_chooser: false) + @browser_session = browser_session + @client_kind = client_kind + @client_ref = client_ref + @prompt = Array(prompt).map(&:to_s) + @login_hint = login_hint.presence + @id_token_hint_subject = id_token_hint_subject.presence + @force_chooser = force_chooser + end + + def call + return decide(:login_required) if candidates.empty? + + # id_token_hint is an instruction, not a preference: if the named subject + # isn't here we must not substitute another account. + if @id_token_hint_subject.present? + return decide(:proceed, identity_session: hinted_session) if hinted_session + return account_selection_required if silent? + + return decide(:chooser) + end + + return chooser_decision if @force_chooser || @prompt.include?("select_account") + + # A login_hint naming an account that isn't signed in here is still a + # statement about who the RP expects. Consenting as whoever happens to be + # signed in would hand over the wrong account without ever saying so, even + # when that's the only account — so ask (or, when we can't ask, say why). + if @login_hint.present? + return decide(:proceed, identity_session: hinted_session) if hinted_session + return account_selection_required if silent? + + return decide(:chooser) + end + + if silent? + return decide(:proceed, identity_session: remembered_session) if remembered_session + return decide(:proceed, identity_session: candidates.first) if candidates.one? + + return account_selection_required + end + + return decide(:proceed, identity_session: candidates.first) if candidates.one? + return decide(:proceed, identity_session: remembered_session) if remembered_session + + chooser_decision + end + + private + + def silent? = @prompt.include?("none") + + def candidates + @candidates ||= @browser_session ? @browser_session.live_identity_sessions.to_a : [] + end + + # A login_hint that doesn't match anything here is a hint for the login form, + # not grounds for picking someone. + def hinted_session + return @hinted_session if defined?(@hinted_session) + + @hinted_session = + if @id_token_hint_subject.present? + candidates.find { |session| session.identity.public_id == @id_token_hint_subject } + elsif @login_hint.present? + normalized = @login_hint.to_s.strip.downcase + candidates.find { |session| session.identity.primary_email.to_s.downcase == normalized } + end + end + + def remembered_session + return @remembered_session if defined?(@remembered_session) + + @remembered_session = + @browser_session&.remembered_identity_session(kind: @client_kind, ref: @client_ref) + end + + def chooser_decision + decide(:chooser, preselect_identity: (hinted_session || remembered_session)&.identity) + end + + def account_selection_required = decide(:error, error: :account_selection_required) + + def decide(action, identity_session: nil, preselect_identity: nil, error: nil) + Decision.new( + action: action, + identity_session: identity_session, + preselect_identity: preselect_identity, + login_hint: @login_hint, + error: error + ) + end +end diff --git a/app/services/id_token_hint_verifier.rb b/app/services/id_token_hint_verifier.rb new file mode 100644 index 00000000..e52d0d30 --- /dev/null +++ b/app/services/id_token_hint_verifier.rb @@ -0,0 +1,62 @@ +# Verifies an `id_token_hint` we issued and extracts its subject. +# +# Signature and issuer are checked; expiry deliberately is not — OIDC Core +# requires that an expired ID token still be accepted as a hint, which is the +# normal case (the RP is telling us who the user was last time). +class IdTokenHintVerifier + Result = Data.define(:subject, :error) do + def present? = subject.present? + def invalid? = error.present? + end + + class << self + def call(hint, audience: nil) + return Result.new(subject: nil, error: nil) if hint.blank? + + payload = decode(hint) + return Result.new(subject: nil, error: :invalid_request) if payload.nil? + return Result.new(subject: nil, error: :invalid_request) unless issuer_matches?(payload) + return Result.new(subject: nil, error: :invalid_request) unless audience_matches?(payload, audience) + + subject = payload["sub"].presence + return Result.new(subject: nil, error: :invalid_request) if subject.nil? + + Result.new(subject: subject, error: nil) + end + + private + + def decode(hint) + key = Doorkeeper::OpenidConnect.signing_key + return nil if key.nil? + + JWT.decode( + hint, + key.keypair.public_key, + true, + algorithm: Doorkeeper::OpenidConnect.signing_algorithm.to_s.upcase, + verify_expiration: false, + verify_iat: false + ).first + rescue JWT::DecodeError, NoMethodError + nil + end + + def issuer_matches?(payload) + payload["iss"].to_s == expected_issuer + end + + # An RP handing us another client's ID token has no business steering our + # account selection. + def audience_matches?(payload, audience) + return true if audience.blank? + + Array(payload["aud"]).include?(audience.to_s) + end + + def expected_issuer + configured = Doorkeeper::OpenidConnect.configuration.issuer + configured.respond_to?(:call) ? configured.call(nil, nil).to_s : configured.to_s + end + end +end diff --git a/app/services/session_resolver.rb b/app/services/session_resolver.rb new file mode 100644 index 00000000..425684d8 --- /dev/null +++ b/app/services/session_resolver.rb @@ -0,0 +1,61 @@ +# Turns the value of the session cookie into a browser session and the account +# session it currently points at. +# +# Shared by SessionsHelper and the SuperAdminConstraint in routes.rb so there is +# exactly one definition of "who is this cookie". `resolve` never writes; +# adopting a legacy session into a BrowserSession is a separate, explicit step. +class SessionResolver + Resolution = Data.define(:browser_session, :identity_session, :legacy) do + def legacy? = legacy + end + + class << self + def resolve(token) + return nil if token.blank? + + browser_session = BrowserSession.not_expired.find_by(token: token) + if browser_session + return Resolution.new( + browser_session: browser_session, + identity_session: browser_session.active_session, + legacy: false + ) + end + + legacy_session = legacy_identity_session(token) + return nil if legacy_session.nil? + + Resolution.new(browser_session: nil, identity_session: legacy_session, legacy: true) + end + + # Read-only convenience for callers that only want the account (e.g. routing + # constraints, which must not write). + def identity_session(token) = resolve(token)&.identity_session + + # Cookies minted before browser sessions existed hold an IdentitySession + # token directly. They keep working, and are adopted on next request. + def legacy_identity_session(token) + IdentitySession + .not_expired + .where(browser_session_id: nil, signed_out_at: nil) + .find_by(session_token: token) + end + + # Adopts a legacy session, reusing the cookie value as the browser session + # token so nobody is logged out and no Set-Cookie is needed. + def adopt_legacy!(identity_session, token:) + BrowserSession.transaction do + browser_session = BrowserSession.create!( + token: token, + expires_at: identity_session.expires_at || SessionsHelper::SESSION_DURATION.from_now + ) + identity_session.update!(browser_session: browser_session) + browser_session.activate!(identity_session) + browser_session + end + rescue ActiveRecord::RecordNotUnique + # A concurrent request in another tab adopted it first. Theirs is fine. + BrowserSession.not_expired.find_by(token: token) + end + end +end diff --git a/app/views/browser_accounts/_account_list.html.erb b/app/views/browser_accounts/_account_list.html.erb new file mode 100644 index 00000000..7a299ed9 --- /dev/null +++ b/app/views/browser_accounts/_account_list.html.erb @@ -0,0 +1,17 @@ +<%# + Shared by the standalone chooser and the SAML inline chooser. + Locals: accounts, submit_url, hidden_params, removable, preselect_public_id +%> +
+ <% accounts.each do |account| %> +
+ <%= render "browser_accounts/account_row", + account: account, + submit_url: submit_url, + hidden_params: local_assigns[:hidden_params] || {}, + removable: local_assigns.fetch(:removable, false), + field_name: local_assigns.fetch(:field_name, :id), + preselect_public_id: local_assigns[:preselect_public_id] %> +
+ <% end %> +
diff --git a/app/views/browser_accounts/_account_row.html.erb b/app/views/browser_accounts/_account_row.html.erb new file mode 100644 index 00000000..c3f81538 --- /dev/null +++ b/app/views/browser_accounts/_account_row.html.erb @@ -0,0 +1,60 @@ +<%# + Every field here is load-bearing: two accounts belonging to the same person are + the hardest thing to tell apart, and the full email is the only reliable + discriminator. Do not truncate it. + + Locals: account, submit_url, hidden_params (hash), removable (bool), + field_name (param the chosen account's public id is submitted as) +%> +<% identity = account.identity %> +<% is_active = current_session&.id == account.id %> +<% is_preselected = local_assigns[:preselect_public_id].present? && local_assigns[:preselect_public_id] == identity.public_id %> + +<%= form_tag submit_url, method: :post, class: "account-row-form" do %> + <% (local_assigns[:hidden_params] || {}).each do |name, value| %> + <%= hidden_field_tag name, value, id: nil %> + <% end %> + <%= hidden_field_tag local_assigns.fetch(:field_name, :id), identity.public_id, id: nil %> + + +<% end %> + +<% if local_assigns.fetch(:removable, false) %> + <%= button_to t("accounts.remove"), + browser_account_path(id: identity.public_id), + method: :delete, + params: { pending: (local_assigns[:hidden_params] || {})[:pending] }.compact, + class: "secondary delete account-row-remove", + form: { data: { turbo_confirm: t("accounts.remove_confirm", email: identity.primary_email) } } %> +<% end %> diff --git a/app/views/browser_accounts/index.html.erb b/app/views/browser_accounts/index.html.erb new file mode 100644 index 00000000..9bb42e93 --- /dev/null +++ b/app/views/browser_accounts/index.html.erb @@ -0,0 +1,36 @@ +<% content_for :title, t("accounts.title") %> +
+
+ <%= vite_image_tag "images/hc-square.png", alt: "Hack Club logo", class: "brand-logo" %> + <%= t("brand") %> +
+ +
+
+

<%= t("accounts.title") %>

+ <%= t("accounts.subtitle") %> +
+ + <%= render "browser_accounts/account_list", + accounts: @accounts, + submit_url: switch_browser_account_path, + hidden_params: { pending: @pending_token }.compact_blank, + removable: true, + preselect_public_id: @preselect_public_id %> + + <% if @at_account_limit %> + <%= render Components::Banner.new(kind: :warning) do %> + <%= t("accounts.limit_reached", max: BrowserSession::MAX_ACCOUNTS) %> + <% end %> + <% else %> + <%= button_to t("accounts.use_another"), + add_browser_account_path(pending: @pending_token), + method: :post, + class: "secondary" %> + <% end %> + +
+ <%= button_to t("accounts.sign_out_all"), logout_all_path, method: :delete, class: "secondary delete" %> +
+
+
diff --git a/app/views/doorkeeper/authorizations/new.html.erb b/app/views/doorkeeper/authorizations/new.html.erb index 1bcca8ed..5dabc579 100644 --- a/app/views/doorkeeper/authorizations/new.html.erb +++ b/app/views/doorkeeper/authorizations/new.html.erb @@ -1,7 +1,11 @@ <% application = Program.find_by(uid: @pre_auth.client.uid) scenario_class = application&.onboarding_scenario_class - scenario = scenario_class&.new(current_identity) + # authorizing_identity, not current_identity: with several accounts in this + # browser they can differ, and everything below must describe the account that + # is about to be handed over. + consent_identity = authorizing_identity + scenario = scenario_class&.new(consent_identity) custom_logo_path = scenario&.logo_path %> <% content_for :title, "Authorize #{@pre_auth.client.name}" %> @@ -28,11 +32,32 @@ <% end %> + <% if consent_identity %> +
+ + + <%= t('accounts.signing_in_as') %> + <%= consent_identity.primary_email %> + + + <% if account_chooser_available? %> + <%= link_to t('accounts.switch'), + oauth_authorization_path(request.query_parameters.merge(prompt: 'select_account')), + class: "secondary" %> + <% end %> +
+ <% end %> + <% if @pre_auth.scopes.count > 0 %> <% scopes = @pre_auth.scopes.map(&:to_s) not_set = t('.data.not_set') - scope_data = OAuthScope.consent_data_by_scope(scopes, current_identity) + scope_data = OAuthScope.consent_data_by_scope(scopes, consent_identity) unknown_scopes = scopes.reject { |s| OAuthScope.known?(s) } %> @@ -93,6 +118,16 @@ <%= hidden_field_tag :nonce, @pre_auth.nonce, id: nil %> <%= hidden_field_tag :code_challenge, @pre_auth.code_challenge, id: nil %> <%= hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method, id: nil %> + <%# Hints have to survive the round-trip or max_age/id_token_hint silently + stop applying once the user clicks. `prompt` is deliberately NOT + re-posted: re-sending select_account or consent would loop forever. %> + <%= hidden_field_tag :max_age, params[:max_age], id: nil if params[:max_age].present? %> + <%= hidden_field_tag :login_hint, params[:login_hint], id: nil if params[:login_hint].present? %> + <%= hidden_field_tag :id_token_hint, params[:id_token_hint], id: nil if params[:id_token_hint].present? %> + <%= hidden_field_tag :acr_values, params[:acr_values], id: nil if params[:acr_values].present? %> + <%# Binds this consent to the account whose data is displayed above, so a + stale tab can't approve for a different account. %> + <%= hidden_field_tag :selected_account, consent_identity&.public_id, id: nil %> <%= submit_tag t('.deny'), class: "secondary delete" %> <% end %> @@ -106,6 +141,16 @@ <%= hidden_field_tag :nonce, @pre_auth.nonce, id: nil %> <%= hidden_field_tag :code_challenge, @pre_auth.code_challenge, id: nil %> <%= hidden_field_tag :code_challenge_method, @pre_auth.code_challenge_method, id: nil %> + <%# Hints have to survive the round-trip or max_age/id_token_hint silently + stop applying once the user clicks. `prompt` is deliberately NOT + re-posted: re-sending select_account or consent would loop forever. %> + <%= hidden_field_tag :max_age, params[:max_age], id: nil if params[:max_age].present? %> + <%= hidden_field_tag :login_hint, params[:login_hint], id: nil if params[:login_hint].present? %> + <%= hidden_field_tag :id_token_hint, params[:id_token_hint], id: nil if params[:id_token_hint].present? %> + <%= hidden_field_tag :acr_values, params[:acr_values], id: nil if params[:acr_values].present? %> + <%# Binds this consent to the account whose data is displayed above, so a + stale tab can't approve for a different account. %> + <%= hidden_field_tag :selected_account, consent_identity&.public_id, id: nil %> <%= submit_tag auth_label, "x-bind:disabled": "!ready", "x-bind:value": "ready ? authLabel : `Wait ${countdown}s...`", class: "approve" %> <% end %> diff --git a/app/views/public_activity/identity_session/_account_added.html.erb b/app/views/public_activity/identity_session/_account_added.html.erb new file mode 100644 index 00000000..74b07a1f --- /dev/null +++ b/app/views/public_activity/identity_session/_account_added.html.erb @@ -0,0 +1,15 @@ +<%# + Deliberately says nothing about the other accounts in the browser. This log is + visible to the account it belongs to, so naming a sibling identity would leak + the existence and address of a separate account. +%> +<%= render Components::PublicActivity::Snippet.new(activity, owner: activity.trackable&.identity) do %> + <% + session = activity.trackable + device_parts = [] + device_parts << session&.device_info if session&.device_info.present? + device_parts << session&.os_info if session&.os_info.present? + device_str = device_parts.any? ? " on #{device_parts.join(", ")}" : "" + %> + was added to an existing browser session<%= device_str %>. +<% end %> diff --git a/app/views/public_activity/identity_session/_account_removed.html.erb b/app/views/public_activity/identity_session/_account_removed.html.erb new file mode 100644 index 00000000..2e96773f --- /dev/null +++ b/app/views/public_activity/identity_session/_account_removed.html.erb @@ -0,0 +1,4 @@ +<%# Never names the other accounts in the browser — see _account_added. %> +<%= render Components::PublicActivity::Snippet.new(activity, owner: activity.trackable&.identity) do %> + was signed out of a browser session. +<% end %> diff --git a/app/views/public_activity/identity_session/_account_switched.html.erb b/app/views/public_activity/identity_session/_account_switched.html.erb new file mode 100644 index 00000000..c022b396 --- /dev/null +++ b/app/views/public_activity/identity_session/_account_switched.html.erb @@ -0,0 +1,4 @@ +<%# Never names the account switched away from — see _account_added. %> +<%= render Components::PublicActivity::Snippet.new(activity, owner: activity.trackable&.identity) do %> + became the active account in this browser. +<% end %> diff --git a/app/views/saml/choose_account.html.erb b/app/views/saml/choose_account.html.erb new file mode 100644 index 00000000..4093f64c --- /dev/null +++ b/app/views/saml/choose_account.html.erb @@ -0,0 +1,26 @@ +<% content_for :title, t("accounts.saml_title", service: @sp_config[:friendly_name]) %> +
+
+ <%= vite_image_tag "images/hc-square.png", alt: "Hack Club logo", class: "brand-logo" %> + <%= t("brand") %> +
+ +
+
+

<%= t("accounts.saml_title", service: @sp_config[:friendly_name]) %>

+ <%= t("accounts.saml_subtitle") %> +
+ + <%# Posts straight back to this endpoint: an IdP-initiated flow has no GET to + park and replay, so the choice travels with the resubmission. %> + <%= render "browser_accounts/account_list", + accounts: @accounts, + submit_url: idp_initiated_saml_path(slug: params[:slug]), + field_name: :selected_account, + preselect_public_id: @preselect_public_id %> + +

+ <%= t("accounts.saml_eligibility_note") %> +

+
+
diff --git a/app/views/saml/http_post.html.erb b/app/views/saml/http_post.html.erb index fdca6c0b..a348e760 100644 --- a/app/views/saml/http_post.html.erb +++ b/app/views/saml/http_post.html.erb @@ -1,5 +1,6 @@ <% content_for :title, t(".title") %> -<% unless current_identity.saml_debug? %> +<% saml_debug = (@saml_identity || current_identity)&.saml_debug? %> +<% unless saml_debug %> <% content_for :head do %>