diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 189500f59..392793200 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -30,7 +30,7 @@ ENCRYPTION_DETERMINISTIC_KEY=32characterrandomstring12345678902 ENCRYPTION_KEY_DERIVATION_SALT=16charssalt1234 ``` -Visit , log in with an email address, then enable Developer Mode in HCA settings. After that, navigate to the "Developers' Corner" and "app yourself up", specifying a callback URL of `http://localhost:3000/auth/hca/callback` and minimum scopes of `email`, `slack_id`, and `verification_status`. +Visit , log in with an email address, then enable Developer Mode in HCA settings. After that, navigate to the "Developers' Corner" and "app yourself up", specifying a callback URL of `http://localhost:3000/auth/hca/callback` and minimum scopes of `openid`, `email`, and `slack_id`. Then, fill out the following fields in your `.env` file: @@ -38,6 +38,8 @@ Then, fill out the following fields in your `.env` file: # Hack Club Account HCA_CLIENT_ID= HCA_CLIENT_SECRET= +# Optional when PUBLIC_URL is set; must exactly match the callback registered in HCA. +HCA_REDIRECT_URI=http://localhost:3000/auth/hca/callback ``` Start the containers: diff --git a/Gemfile b/Gemfile index 3c4a203e6..0985c5f4b 100644 --- a/Gemfile +++ b/Gemfile @@ -57,7 +57,9 @@ gem "ruby-vips", "~> 2.3", require: false gem "dotenv-rails" # Authentication -# gem "oauth2" +gem "omniauth", "~> 2.1" +gem "omniauth_openid_connect", "~> 0.8" +gem "omniauth-rails_csrf_protection", "~> 1.0" # Added from the code block gem "http" diff --git a/Gemfile.lock b/Gemfile.lock index a0fca668e..d02e36cb7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -79,7 +79,9 @@ GEM uri (>= 0.13.1) addressable (2.9.0) public_suffix (>= 2.0.2, < 8.0) + aes_key_wrap (1.1.0) ast (2.4.3) + attr_required (1.0.2) autotuner (1.1.0) aws-eventstream (1.4.0) aws-partitions (1.1274.0) @@ -110,6 +112,7 @@ GEM parser (>= 2.4) smart_properties bigdecimal (4.1.2) + bindata (3.0.0) bindex (0.8.1) bootsnap (1.24.6) msgpack (~> 1.2) @@ -175,6 +178,14 @@ GEM tzinfo faker (3.8.0) i18n (>= 1.8.11, < 2) + faraday (2.14.3) + faraday-net_http (>= 2.0, < 3.5) + json + logger + faraday-follow_redirects (0.5.0) + faraday (>= 1, < 3) + faraday-net_http (3.4.4) + net-http (~> 0.5) ffi (1.17.4-aarch64-linux-gnu) ffi (1.17.4-aarch64-linux-musl) ffi (1.17.4-arm-linux-gnu) @@ -211,6 +222,8 @@ GEM railties (>= 6.1.0) thor (>= 1.0.0) hashdiff (1.2.1) + hashie (5.1.0) + logger htmlentities (4.4.2) http (6.0.4) http-cookie (~> 1.0) @@ -241,6 +254,13 @@ GEM js_from_routes (4.0.2) railties (>= 5.1, < 9) json (2.21.1) + json-jwt (1.17.2) + activesupport (>= 4.2) + aes_key_wrap + base64 + bindata + faraday (~> 2.0) + faraday-follow_redirects json-schema (6.2.0) addressable (~> 2.8) bigdecimal (>= 3.1, < 5) @@ -291,6 +311,8 @@ GEM prism (~> 1.5) msgpack (1.8.4) mutex_m (0.3.0) + net-http (0.9.1) + uri (>= 0.11.1) net-imap (0.6.6) date net-protocol @@ -318,6 +340,29 @@ GEM oj (3.17.4) bigdecimal (>= 3.0) ostruct (>= 0.2) + omniauth (2.1.4) + hashie (>= 3.4.6) + logger + rack (>= 2.2.3) + rack-protection + omniauth-rails_csrf_protection (1.0.2) + actionpack (>= 4.2) + omniauth (~> 2.0) + omniauth_openid_connect (0.8.0) + omniauth (>= 1.9, < 3) + openid_connect (~> 2.2) + openid_connect (2.5.0) + activemodel + attr_required (>= 1.0.0) + faraday (~> 2.0) + faraday-follow_redirects + json-jwt (>= 1.16) + mail + rack-oauth2 (~> 2.2) + swd (~> 2.0) + tzinfo + validate_url + webfinger (~> 2.0) ostruct (0.6.3) paper_trail (17.0.0) activerecord (>= 7.1) @@ -364,6 +409,13 @@ GEM rack (>= 3.0.14) rack-mini-profiler (4.0.1) rack (>= 1.2.0) + rack-oauth2 (2.3.0) + activesupport + attr_required + faraday (~> 2.0) + faraday-follow_redirects + json-jwt (>= 1.11.0) + rack (>= 2.1.0) rack-protection (4.2.1) base64 (>= 0.1.0) logger (>= 1.6.0) @@ -515,6 +567,11 @@ GEM stackprof (0.2.28) stimulus-rails (1.3.4) railties (>= 6.0.0) + swd (2.0.3) + activesupport (>= 3) + attr_required (>= 0.0.5) + faraday (~> 2.0) + faraday-follow_redirects thor (1.5.0) thruster (0.1.23) thruster (0.1.23-aarch64-linux) @@ -534,6 +591,9 @@ GEM uniform_notifier (1.18.0) uri (1.1.1) useragent (0.16.11) + validate_url (1.0.15) + activemodel (>= 3.0.0) + public_suffix vite_rails (3.11.1) railties (>= 5.1, < 9) vite_ruby (~> 3.0, >= 3.2.2) @@ -547,6 +607,10 @@ GEM actionview (>= 8.0.0) bindex (>= 0.4.0) railties (>= 8.0.0) + webfinger (2.1.3) + activesupport + faraday (~> 2.0) + faraday-follow_redirects webmock (3.26.2) addressable (>= 2.8.0) crack (>= 0.3.2) @@ -609,6 +673,9 @@ DEPENDENCIES maxminddb memory_profiler oj + omniauth (~> 2.1) + omniauth-rails_csrf_protection (~> 1.0) + omniauth_openid_connect (~> 0.8) paper_trail pg premailer-rails diff --git a/app/controllers/api/hackatime/v1/hackatime_controller.rb b/app/controllers/api/hackatime/v1/hackatime_controller.rb index 17a0a87d2..35c19a4cc 100644 --- a/app/controllers/api/hackatime/v1/hackatime_controller.rb +++ b/app/controllers/api/hackatime/v1/hackatime_controller.rb @@ -198,7 +198,7 @@ def set_user return render_unauthorized unless valid_key.present? @user = valid_key.user - render_unauthorized unless @user + render_unauthorized unless @user&.authentication_allowed? end # allow either heartbeat or heartbeats diff --git a/app/controllers/api/v1/authenticated/application_controller.rb b/app/controllers/api/v1/authenticated/application_controller.rb index ce9bddfff..3a0da8dea 100644 --- a/app/controllers/api/v1/authenticated/application_controller.rb +++ b/app/controllers/api/v1/authenticated/application_controller.rb @@ -16,8 +16,10 @@ def ensure_api_access_allowed render json: { error: "Unauthorized" }, status: :unauthorized if current_user&.api_access_restricted? end - def ensure_no_pending_deletion - render json: { error: "Unauthorized" }, status: :unauthorized if current_user&.pending_deletion? + def ensure_profile_access_allowed + return if current_user&.authentication_allowed? && !current_user.pending_deletion? + + render json: { error: "Unauthorized" }, status: :unauthorized end end end diff --git a/app/controllers/api/v1/authenticated/me_controller.rb b/app/controllers/api/v1/authenticated/me_controller.rb index 79337404b..d8f2ee857 100644 --- a/app/controllers/api/v1/authenticated/me_controller.rb +++ b/app/controllers/api/v1/authenticated/me_controller.rb @@ -3,7 +3,7 @@ module V1 module Authenticated class MeController < ApplicationController skip_before_action :ensure_api_access_allowed, only: :index - before_action :ensure_no_pending_deletion, only: :index + before_action :ensure_profile_access_allowed, only: :index def index app = doorkeeper_token&.application diff --git a/app/controllers/api/v1/my/heartbeats_controller.rb b/app/controllers/api/v1/my/heartbeats_controller.rb index b413d1edb..14fea150b 100644 --- a/app/controllers/api/v1/my/heartbeats_controller.rb +++ b/app/controllers/api/v1/my/heartbeats_controller.rb @@ -52,7 +52,7 @@ def ensure_authenticated! return render_unauthorized unless valid_key.present? @current_user = valid_key.user - render_unauthorized unless @current_user + render_unauthorized unless @current_user&.authentication_allowed? end def current_user = @current_user diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3e832da3a..8c7b1f162 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -42,7 +42,20 @@ def try_rack_mini_profiler_enable end def current_user - @current_user ||= User.find_by(id: session[:user_id]) if session[:user_id] + return @current_user if defined?(@current_user) + + @current_user = resolve_session_user + end + + def resolve_session_user + return unless session[:user_id] + + user = User.find_by(id: session[:user_id]) + session_version = session[:authentication_version] || 0 + return user if user&.authentication_allowed? && user.authentication_version == session_version.to_i + + reset_session + nil end def user_signed_in? @@ -62,8 +75,14 @@ def persist_theme_cookie def safe_return_url(url) return nil if url.blank? - return nil unless url.start_with?("/") && !url.start_with?("//") + return nil if url.match?(/[\\\x00-\x1F\x7F]/) + + uri = URI.parse(url) + return nil if uri.scheme || uri.host || !uri.path.start_with?("/") || uri.path.start_with?("//") + url + rescue URI::InvalidURIError + nil end # Build a return_data hash from a continue URL, extracting known query diff --git a/app/controllers/concerns/admin_api_key_authentication.rb b/app/controllers/concerns/admin_api_key_authentication.rb index e88c76093..5926efc18 100644 --- a/app/controllers/concerns/admin_api_key_authentication.rb +++ b/app/controllers/concerns/admin_api_key_authentication.rb @@ -19,5 +19,5 @@ def auth_admin_api_key(token) true end - def admin_api_user?(user) = user&.admin_level.in?(ADMIN_API_LEVELS) + def admin_api_user?(user) = user&.authentication_allowed? && user.admin_level.in?(ADMIN_API_LEVELS) end diff --git a/app/controllers/deletion_requests_controller.rb b/app/controllers/deletion_requests_controller.rb index f3f2b5aa1..bae69e3c1 100644 --- a/app/controllers/deletion_requests_controller.rb +++ b/app/controllers/deletion_requests_controller.rb @@ -21,11 +21,10 @@ def create def cancel @deletion_request = current_user.active_deletion_request - if @deletion_request&.can_be_cancelled? - @deletion_request.cancel! + if @deletion_request&.cancel! redirect_to my_settings_path, notice: "Your deletion request has been cancelled!" else - redirect_to deletion_path + redirect_to my_settings_path, alert: "Your deletion request could not be cancelled." end end diff --git a/app/controllers/dev_controller.rb b/app/controllers/dev_controller.rb index 64e0d6bbf..f17f7d895 100644 --- a/app/controllers/dev_controller.rb +++ b/app/controllers/dev_controller.rb @@ -1,5 +1,5 @@ class DevController < ApplicationController - before_action :ensure_development_environment + before_action :ensure_local_environment def index render plain: <<~TEXT @@ -13,9 +13,14 @@ def log_me_in email_address = EmailAddress.find_by(email: params[:email].downcase) return render plain: "No local user has that email address.\n", status: :not_found unless email_address - reset_session - session[:user_id] = email_address.user_id - redirect_to root_path, notice: "Signed in as #{email_address.email}." + establish_local_session(email_address.user, label: email_address.email) + end + + def log_me_in_user + user = User.find_by(id: params[:id]) + return render plain: "No local user has that ID.\n", status: :not_found unless user + + establish_local_session(user, label: "user ##{user.id}") end def log_me_out @@ -25,7 +30,18 @@ def log_me_out private - def ensure_development_environment - raise ActionController::RoutingError, "Not Found" unless Rails.env.development? + def establish_local_session(user, label:) + return render plain: "That local user cannot sign in.\n", status: :forbidden unless user.authentication_allowed? + + reset_session + session[:user_id] = user.id + session[:authentication_version] = user.authentication_version + session[:auth_provider] = "development" + session[:authenticated_at] = Time.current.to_i + redirect_to root_path, notice: "Signed in as #{label}." + end + + def ensure_local_environment + raise ActionController::RoutingError, "Not Found" unless Rails.env.local? end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5c8c9d6b7..5fad72131 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,48 +1,63 @@ class SessionsController < ApplicationController - def hca_new - session[:return_data] = build_return_data(params[:continue]) if params[:continue].present? - Rails.logger.info("Sessions return data: #{session[:return_data]}") - redirect_uri = url_for(action: :hca_create, only_path: false) - - redirect_to User.hca_authorize_url(redirect_uri), - host: "https://auth.hackclub.com", - allow_other_host: "https://auth.hackclub.com" - end + HCA_AUTHENTICATION_MAX_AGE = 30.minutes + + rescue_from OauthAuthentication::HcaIdentityConflictError, with: :handle_hca_identity_conflict + + def hca_new = failure def hca_create - return if handle_oauth_error("HCA", redirect_path: root_path, alert_label: "Hack Club Auth") + auth = request.env["omniauth.auth"] + info = auth&.dig("info") || {} + claims = auth&.dig("extra", "raw_info") || {} + subject = auth&.dig("uid").to_s + email = (claims["email"] || info["email"]).to_s.strip.downcase + verified = claims["email_verified"] == true || info["email_verified"] == true + continuation = safe_return_url(request.env.dig("omniauth.params", "continue")) + return failure unless auth&.dig("provider") == "hca" && verified && subject.match?(/\Aident![A-Za-z0-9_-]+\z/) && email.present? + + identity = { "subject" => subject, "email" => email, "slack_uid" => claims["slack_id"].presence, "created_at" => Time.current.to_i, "continue" => continuation }.compact + user = User.from_hca_identity(subject:, email:, slack_uid: identity["slack_uid"]) + return establish_hca_session(user, continuation:) if user + + session[:pending_hca] = identity + redirect_to signin_path, notice: "Choose how to finish setting up your Hackatime account." + end - redirect_uri = url_for(action: :hca_create, only_path: false) - @user = User.from_hca_token(params[:code], redirect_uri, client_ip) + def hca_account + pending = pending_hca_identity + return failure unless pending - if @user&.persisted? - preserved_return_data = session[:return_data] - reset_session - session[:user_id] = @user.id - session[:return_data] = preserved_return_data if preserved_return_data - notice = "Successfully signed in with Hack Club Auth! Welcome!" - - if @user.previously_new_record? - redirect_to setup_path, notice: notice - elsif session[:return_data]&.dig("url").present? - redirect_to session[:return_data].delete("url"), notice: notice - else - redirect_to root_path, notice: notice - end - else - redirect_to root_path, alert: "Failed to authenticate with Hack Club Auth!" + user = User.create_from_hca_identity!(subject: pending["subject"], email: pending["email"], slack_uid: pending["slack_uid"], country_code: User.country_code_from_ip(client_ip)) + establish_hca_session(user, continuation: pending["continue"], onboarding: true) + end + + def hca_recovery + pending = pending_hca_identity + if pending && (email_address = EmailAddress.find_by("LOWER(email) = ?", params[:email].to_s.strip.downcase)) && !email_address.source_preserved_for_deletion? && email_address.user.authentication_allowed? && email_address.user.hca_id.blank? && email_address.user.admin_level == "default" && !email_address.user.pending_deletion? + token = email_address.user.sign_in_tokens.create!(auth_type: :hca_recovery, return_data: { "hca_subject" => pending["subject"] }) + LoopsMailer.hca_recovery_email(email_address.email, token.token).deliver_later end + redirect_to signin_path, notice: "If that account is eligible, we've sent recovery instructions." + end + + def hca_cancel + session.delete(:pending_hca) + redirect_to signin_path end def slack_new + pending = pending_hca_identity + purpose = pending ? "recovery" : "integration" + return redirect_to(signin_path, alert: "Sign in with Hack Club Account again first.") if purpose == "integration" && !recent_hca_authentication? + redirect_uri = url_for(action: :slack_create, only_path: false) oauth_nonce = SecureRandom.hex(24) - session[:slack_oauth_state_nonce] = oauth_nonce state_payload = { token: oauth_nonce, - close_window: params[:close_window].present?, - continue: params[:continue] + purpose: purpose, + user_id: (current_user.id if purpose == "integration") }.to_json + session[:slack_oauth_state] = state_payload Rails.logger.info "Starting Slack OAuth flow with redirect URI: #{redirect_uri}" redirect_to User.slack_authorize_url(redirect_uri, state: state_payload), @@ -54,36 +69,32 @@ def slack_create return if handle_oauth_error("Slack", redirect_path: root_path, alert_label: "Slack") redirect_uri = url_for(action: :slack_create, only_path: false) - slack_state = parse_slack_state(params[:state]) - unless valid_oauth_state?(provider: "Slack", session_key: :slack_oauth_state_nonce, received_nonce: slack_state&.dig("token")) + unless valid_oauth_state?(provider: "Slack", session_key: :slack_oauth_state, received_nonce: params[:state]) return redirect_to(root_path, alert: "Failed to authenticate with Slack") end + slack_state = parse_slack_state(params[:state]) + return failure unless slack_state - @user = User.from_slack_token(params[:code], redirect_uri, client_ip) + identity = User.exchange_slack_code(params[:code], redirect_uri) + return failure unless identity - if @user&.persisted? - reset_session - session[:user_id] = @user.id - notice = "Successfully signed in with Slack! Welcome!" - - continue_url = safe_return_url(slack_state&.dig("continue").presence) - - if slack_state&.dig("close_window") - redirect_to close_window_path - elsif @user.previously_new_record? - session[:return_data] = build_return_data(continue_url) - redirect_to setup_path, notice: notice - elsif continue_url.present? - redirect_to continue_url, notice: notice # codeql[rb/url-redirection] - else - redirect_to root_path, notice: notice - end + if slack_state["purpose"] == "integration" + return failure unless current_user&.id == slack_state["user_id"].to_i && recent_hca_authentication? + return failure unless User.connect_slack_identity!(current_user, identity) + redirect_to my_settings_path, notice: "Successfully re-authorised Slack." else - report_message("Failed to create/update user from Slack data") - redirect_to root_path, alert: "Failed to sign in with Slack" + pending = pending_hca_identity + user = User.find_by(slack_uid: identity[:uid]) + return failure unless pending && user + bind_recovered_hca!(user, pending, proven_slack_uid: identity[:uid]) + establish_hca_session(user, continuation: pending["continue"]) end end + def failure + redirect_to signin_path, alert: "Authentication failed. Please try again." + end + def close_window = render(:close_window, layout: false) def github_new @@ -130,19 +141,10 @@ def github_unlink end def email - email = params[:email].downcase - continue_param = params[:continue] - - if Rails.env.production? - HandleEmailSigninJob.perform_later(email, continue_param, client_ip) - else - token = HandleEmailSigninJob.perform_now(email, continue_param, client_ip) - public_url = ENV["PUBLIC_URL"].presence || root_url - session[:dev_magic_link] = URI.join(public_url, auth_token_path(token)).to_s - end - - redirect_path = params[:redirect_to] == "signin" ? signin_path(sign_in_email: true) : root_path(sign_in_email: true) - redirect_to redirect_path, notice: "Check your email for a sign-in link!" + redirect_to signin_path( + login_hint: params[:email].to_s.strip.downcase.presence, + continue: safe_return_url(params[:continue]) + ), notice: "Email sign-in has moved to Hack Club Account." end def add_email @@ -231,20 +233,13 @@ def token valid_token = SignInToken.where(token: params[:token], used_at: nil) .where("expires_at > ?", Time.current).first - if valid_token - valid_token.mark_used! - reset_session - session[:user_id] = valid_token.user_id - continue_url = safe_return_url(valid_token.continue_param) - session[:return_data] = (valid_token.return_data || {}).merge(build_return_data(continue_url)) - if continue_url.present? - redirect_to continue_url, notice: "Successfully signed in!" # codeql[rb/url-redirection] - else - redirect_to root_path, notice: "Successfully signed in!" - end - else - redirect_to root_path, alert: "Invalid or expired link" - end + return redirect_to(root_path, alert: "Invalid or expired link") unless valid_token&.hca_recovery? + + pending = pending_hca_identity + return redirect_to(signin_path, alert: "Restart recovery in this browser.") unless pending && valid_token.return_data&.dig("hca_subject") == pending["subject"] + return redirect_to(signin_path, alert: "This recovery link has expired or was already used.") unless consume_hca_recovery_token!(valid_token, pending) + + establish_hca_session(valid_token.user, continuation: pending["continue"]) end def impersonate @@ -262,13 +257,17 @@ def impersonate return redirect_to(root_path, alert: "nice try, you cant do that") if blocked session[:impersonater_user_id] ||= current_user.id + session[:impersonater_authentication_version] ||= current_user.authentication_version session[:user_id] = user.id + session[:authentication_version] = user.authentication_version redirect_to root_path, notice: "Impersonating #{user.display_name}" end def stop_impersonating session[:user_id] = session[:impersonater_user_id] + session[:authentication_version] = session[:impersonater_authentication_version] session[:impersonater_user_id] = nil + session[:impersonater_authentication_version] = nil redirect_to root_path, notice: "Stopped impersonating" end @@ -281,6 +280,73 @@ def destroy def client_ip = request.headers["CF-Connecting-IP"].presence || request.remote_ip + def pending_hca_identity + pending = session[:pending_hca] + unless pending.is_a?(Hash) && pending["created_at"].to_i > 10.minutes.ago.to_i + session.delete(:pending_hca) + return + end + + pending + end + + def recent_hca_authentication? + current_user.present? && + session[:auth_provider] == "hca" && + session[:authenticated_at].to_i > HCA_AUTHENTICATION_MAX_AGE.ago.to_i + end + + def consume_hca_recovery_token!(token, pending) + SignInToken.transaction do + token.lock! + return false if token.used_at? || token.expires_at <= Time.current || token.return_data&.dig("hca_subject") != pending["subject"] + + bind_recovered_hca!(token.user, pending) + token.mark_used! + end + true + end + + def establish_hca_session(user, continuation: nil, onboarding: false) + return failure unless user&.authentication_allowed? + + return_data = build_return_data(continuation) + reset_session + session[:user_id] = user.id + session[:authentication_version] = user.authentication_version + session[:auth_provider] = "hca" + session[:authenticated_at] = Time.current.to_i + session[:return_data] = return_data if return_data.present? + return redirect_to(setup_path, notice: "Account created successfully.") if onboarding + return redirect_to(continuation, notice: "Successfully signed in!") if safe_return_url(continuation) # codeql[rb/url-redirection] + + redirect_to root_path, notice: "Successfully signed in!" + end + + def bind_recovered_hca!(user, pending, proven_slack_uid: nil) + User.transaction do + user.lock! + subject_owner = User.lock.find_by(hca_id: pending["subject"]) + email_owner = EmailAddress.lock.find_by("LOWER(email) = ?", pending["email"])&.user + claim_slack_owner = User.lock.find_by(slack_uid: pending["slack_uid"]) if pending["slack_uid"].present? + conflicting_claim = [ subject_owner, email_owner, claim_slack_owner ].compact.any? { |owner| owner != user } + slack_proof_mismatch = proven_slack_uid.present? && pending["slack_uid"].present? && proven_slack_uid != pending["slack_uid"] + if !user.authentication_allowed? || user.hca_id.present? && user.hca_id != pending["subject"] || user.admin_level != "default" || user.pending_deletion? || conflicting_claim || slack_proof_mismatch + User.raise_hca_conflict!(subject: pending["subject"], reason: "recovery_conflict", email_user: email_owner, slack_user: claim_slack_owner) + end + + attributes = { hca_id: pending["subject"], hca_access_token: nil, hca_scopes: [] } + attributes[:slack_uid] = proven_slack_uid if proven_slack_uid.present? && User.where(slack_uid: proven_slack_uid).where.not(id: user.id).none? + user.update!(attributes) + User.attach_hca_email!(user, pending["email"]) + end + end + + def handle_hca_identity_conflict(error) + HCAIdentityConflict.record!(error) + redirect_to signin_path, alert: "We couldn't safely link this Hack Club Account. Please contact support." + end + def parse_slack_state(raw_state) JSON.parse(raw_state) rescue JSON::ParserError, TypeError diff --git a/app/controllers/settings/slack_github_controller.rb b/app/controllers/settings/slack_github_controller.rb index 97eb685db..537a0a493 100644 --- a/app/controllers/settings/slack_github_controller.rb +++ b/app/controllers/settings/slack_github_controller.rb @@ -31,6 +31,7 @@ def section_props channel_ids = enabled_sailors_logs.pluck(:slack_channel_id) { + csrf_token: form_authenticity_token, user: user_props(keys: %i[uses_slack_status]), slack: { can_enable_status: can_enable_slack_status, diff --git a/app/controllers/static_pages_controller.rb b/app/controllers/static_pages_controller.rb index 86621c8d0..4f1171e56 100644 --- a/app/controllers/static_pages_controller.rb +++ b/app/controllers/static_pages_controller.rb @@ -27,12 +27,17 @@ def index def signin return redirect_to root_path if current_user + pending = session[:pending_hca] + pending = nil unless pending.is_a?(Hash) && pending["created_at"].to_i > 10.minutes.ago.to_i + session.delete(:pending_hca) unless pending render inertia: "Auth/SignIn", props: { sign_in_email: params[:sign_in_email].present?, show_dev_tool: Rails.env.development?, dev_magic_link: (Rails.env.development? ? session.delete(:dev_magic_link) : nil), csrf_token: form_authenticity_token, - continue_param: params[:continue].presence + continue_param: params[:continue].presence, + login_hint: params[:login_hint].presence, + pending_hca: pending && { email: pending["email"] } } end diff --git a/app/javascript/layouts/app/Sidebar.svelte b/app/javascript/layouts/app/Sidebar.svelte index 5117f0481..8eeb54e9e 100644 --- a/app/javascript/layouts/app/Sidebar.svelte +++ b/app/javascript/layouts/app/Sidebar.svelte @@ -20,7 +20,7 @@ onLogout: () => void; } = $props(); - const loginPath = sessions.slackNew.path(); + const loginPath = "/signin"; const isBrowser = typeof window !== "undefined"; const handleNavLinkClick = () => { diff --git a/app/javascript/pages/Auth/SignIn.svelte b/app/javascript/pages/Auth/SignIn.svelte index 46b5045c6..fe1335c66 100644 --- a/app/javascript/pages/Auth/SignIn.svelte +++ b/app/javascript/pages/Auth/SignIn.svelte @@ -13,12 +13,16 @@ dev_magic_link, csrf_token, continue_param, + login_hint, + pending_hca, }: { sign_in_email: boolean; show_dev_tool: boolean; dev_magic_link?: string | null; csrf_token: string; continue_param?: string | null; + login_hint?: string | null; + pending_hca?: { email: string } | null; } = $props(); @@ -47,8 +51,9 @@ {show_dev_tool} {dev_magic_link} {csrf_token} - redirect_to="signin" {continue_param} + {login_hint} + {pending_hca} />
diff --git a/app/javascript/pages/Home/signedOut/AuthForm.svelte b/app/javascript/pages/Home/signedOut/AuthForm.svelte index 186be5ee0..1d591d1de 100644 --- a/app/javascript/pages/Home/signedOut/AuthForm.svelte +++ b/app/javascript/pages/Home/signedOut/AuthForm.svelte @@ -2,22 +2,23 @@ import Button from "../../../components/Button.svelte"; import HackClubLogo from "../../../components/HackClubLogo.svelte"; import { sessions } from "../../../api"; - import Slack from "hcicons-svelte/slack"; let { sign_in_email, show_dev_tool, dev_magic_link, csrf_token, - redirect_to, continue_param, + login_hint, + pending_hca, }: { sign_in_email: boolean; show_dev_tool: boolean; dev_magic_link?: string | null; csrf_token: string; - redirect_to?: string; continue_param?: string | null; + login_hint?: string | null; + pending_hca?: { email: string } | null; } = $props(); const query = $derived( @@ -26,16 +27,57 @@ const hcaAuthPath = $derived( query ? sessions.hcaNew.path(query) : sessions.hcaNew.path(), ); - const slackAuthPath = $derived( - query ? sessions.slackNew.path(query) : sessions.slackNew.path(), - ); - const emailAuthPath = sessions.email.path(); - let isSigningIn = $state(false);
- {#if sign_in_email} + {#if pending_hca} +
+
+

Continue as {pending_hca.email}

+

+ Create a new account or prove ownership of an older Hackatime account. +

+
+
+ + +
+
+ + +
+
+ + + + +
+
+ + + +
+
+ {:else if sign_in_email}
@@ -54,71 +96,46 @@ {/if}
{:else} - (isSigningIn = true)} - class="w-full flex items-center justify-center gap-3 px-6 py-3.5 rounded-xl bg-primary text-on-primary font-medium hover:opacity-90 transition-all" +
(isSigningIn = true)} + class="space-y-3" > - {#if isSigningIn} - - - - - {:else} - - {/if} - Sign in with Hack Club - - - - - Sign in with Slack - - -
-
- or -
-
- - - {#if redirect_to} - - {/if} - {#if continue_param} - - {/if} -
- - -
+ +

+ We'll hand this email to Hack Club Account. Hackatime does not send a + sign-in link. +

+
{/if}
diff --git a/app/javascript/pages/Users/Settings/SlackGithub.svelte b/app/javascript/pages/Users/Settings/SlackGithub.svelte index 5750c215b..815a26043 100644 --- a/app/javascript/pages/Users/Settings/SlackGithub.svelte +++ b/app/javascript/pages/Users/Settings/SlackGithub.svelte @@ -18,6 +18,7 @@ slack, github, errors, + csrf_token, }: SlackGithubPageProps = $props(); let unlinkGithubModalOpen = $state(false); @@ -35,12 +36,12 @@ >
{#if !slack.can_enable_status} - - Re-authorize with Slack - +
+ + +
{/if}
; slack: SlackProps; github: GithubProps; diff --git a/app/jobs/handle_email_signin_job.rb b/app/jobs/handle_email_signin_job.rb index a4c2e9a57..2dcd168a4 100644 --- a/app/jobs/handle_email_signin_job.rb +++ b/app/jobs/handle_email_signin_job.rb @@ -1,16 +1,6 @@ class HandleEmailSigninJob < ApplicationJob queue_as :latency_critical - def perform(email, continue_param = nil, ip_address = nil) - email_address = ActiveRecord::Base.transaction do - EmailAddress.find_by(email: email) || begin - user = User.create!(country_code: User.country_code_from_ip(ip_address)) - user.email_addresses.create!(email: email, source: :signing_in) - end - end - - token = email_address.user.create_email_signin_token(continue_param: continue_param).token - LoopsMailer.sign_in_email(email_address.email, token).deliver_now - token - end + # Keep the class loadable while jobs queued before the HCA cutover drain. + def perform(*) = nil end diff --git a/app/jobs/process_account_deletions_job.rb b/app/jobs/process_account_deletions_job.rb index d7e2481bc..cfb793b99 100644 --- a/app/jobs/process_account_deletions_job.rb +++ b/app/jobs/process_account_deletions_job.rb @@ -6,8 +6,15 @@ def perform Rails.logger.info "kerblamming ##{deletion_request.user_id}" begin - AnonymizeUserService.call(deletion_request.user) - deletion_request.complete! + completed = deletion_request.with_lock do + deletion_request.reload + next false unless deletion_request.approved? && deletion_request.scheduled_deletion_at <= Time.current + + AnonymizeUserService.call(deletion_request.user) + deletion_request.complete! + true + end + next unless completed Rails.logger.info "kerblamed account ##{deletion_request.user_id}" rescue StandardError => e diff --git a/app/jobs/slack_profile_sync_job.rb b/app/jobs/slack_profile_sync_job.rb index c94b135f1..fb52a20e8 100644 --- a/app/jobs/slack_profile_sync_job.rb +++ b/app/jobs/slack_profile_sync_job.rb @@ -11,10 +11,19 @@ class SlackProfileSyncJob < ApplicationJob def perform(user_id) user = User.find_by(id: user_id) - return unless user&.slack_uid.present? + return unless user&.authentication_allowed? && !user.pending_deletion? && user.slack_uid.present? - user.update_from_slack - user.save! if user.changed? + slack_uid = user.slack_uid + user_data = user.raw_slack_user_info + return unless user_data.present? + + user.with_lock do + return unless user.authentication_allowed? && !user.pending_deletion? && user.slack_uid == slack_uid + + user.apply_slack_profile_attributes(user_data) + user.slack_synced_at = Time.current + user.save! + end rescue SlackIntegration::RateLimitedError => e raise if executions >= 5 diff --git a/app/mailers/loops_mailer.rb b/app/mailers/loops_mailer.rb index 9605d57f6..9e3866e9c 100644 --- a/app/mailers/loops_mailer.rb +++ b/app/mailers/loops_mailer.rb @@ -9,4 +9,9 @@ def sign_in_email(email, token) subject: "Your Hackatime sign-in link" ) end + + def hca_recovery_email(email, token) + @recovery_url = auth_token_url(token) + mail(to: email, subject: "Recover your Hackatime account") + end end diff --git a/app/models/concerns/oauth_authentication.rb b/app/models/concerns/oauth_authentication.rb index 4d85ed248..d1a4597e7 100644 --- a/app/models/concerns/oauth_authentication.rb +++ b/app/models/concerns/oauth_authentication.rb @@ -2,17 +2,20 @@ module OauthAuthentication extend ActiveSupport::Concern include ErrorReporting - class_methods do - include ErrorReporting + class HcaIdentityConflictError < StandardError + attr_reader :hca_id, :reason, :email_user_id, :slack_user_id - def hca_authorize_url(redirect_uri) - URI.parse("#{HCAService.host}/oauth/authorize?#{{ - redirect_uri:, - client_id: ENV["HCA_CLIENT_ID"], - response_type: "code", - scope: "email slack_id verification_status" - }.to_query}") + def initialize(hca_id:, reason:, email_user_id: nil, slack_user_id: nil) + @hca_id = hca_id + @reason = reason + @email_user_id = email_user_id + @slack_user_id = slack_user_id + super(reason) end + end + + class_methods do + include ErrorReporting def slack_authorize_url(redirect_uri, state: nil, close_window: false, continue_param: nil) state ||= { token: SecureRandom.hex(24), close_window: close_window, continue: continue_param }.to_json @@ -33,51 +36,137 @@ def github_authorize_url(redirect_uri, state: nil) }.to_query}") end - def from_hca_token(code, redirect_uri, ip_address = nil) - response = HTTP.post("#{HCAService.host}/oauth/token", form: { - client_id: ENV["HCA_CLIENT_ID"], client_secret: ENV["HCA_CLIENT_SECRET"], - redirect_uri: redirect_uri, code: code, grant_type: "authorization_code" - }) - access_token = JSON.parse(response.body.to_s)["access_token"] - return nil if access_token.nil? - - hca_data = ::HCAService.me(access_token) - identity = hca_data["identity"] - @user = User.find_by_hca_id(identity["id"]) if identity["id"].present? - @user ||= User.find_by_slack_uid(identity["slack_id"]) if identity["slack_id"].present? - @user ||= EmailAddress.find_by(email: identity["primary_email"])&.user if identity["primary_email"].present? - - if @user - attrs = { hca_scopes: hca_data["scopes"], hca_id: identity["id"], hca_access_token: access_token } - attrs[:country_code] = country_code_from_ip(ip_address) if @user.country_code.blank? - @user.update!(attrs) - - if @user.slack_uid.blank? && identity["slack_id"].present? - begin - @user.update!(slack_uid: identity["slack_id"]) - rescue ActiveRecord::RecordNotUnique - @user.reload - rescue ActiveRecord::RecordInvalid => e - raise unless e.record.errors.of_kind?(:slack_uid, :taken) - - @user.reload - end + def from_hca_identity(subject:, email:, slack_uid: nil, country_code: nil, retrying: false) + email = email.to_s.strip.downcase + slack_uid = slack_uid.presence + return nil if subject.blank? || email.blank? + + user = transaction do + subject_user = lock.find_by(hca_id: subject) + if subject_user + raise_hca_conflict!(subject:, reason: "anonymized", email_user: subject_user) unless subject_user.authentication_allowed? + + record_known_hca_conflict!(subject_user, subject:, email:, slack_uid:) + sync_known_hca_user!(subject_user, email:, slack_uid:, country_code:) + next subject_user end - else - ActiveRecord::Base.transaction do - @user = User.create!( - hca_id: identity["id"], slack_uid: identity["slack_id"], - hca_scopes: hca_data["scopes"], hca_access_token: access_token, - country_code: country_code_from_ip(ip_address) - ) - EmailAddress.create!(email: identity["primary_email"], user: @user) if identity["primary_email"].present? + + email_address = EmailAddress.lock.find_by("LOWER(email) = ?", email) + email_user = email_address&.user + slack_user = lock.find_by(slack_uid: slack_uid) if slack_uid + + if email_address&.source_preserved_for_deletion? || email_user&.anonymized? || slack_user&.anonymized? + raise_hca_conflict!(subject:, reason: "anonymized", email_user:, slack_user:) + end + + candidates = [ email_user, slack_user ].compact.uniq + if candidates.many? + raise_hca_conflict!(subject:, reason: "split_identity", email_user:, slack_user:) end + + candidate = candidates.first + next nil unless candidate + + # Email and Slack lookups can reach the same legacy user through + # different rows. Reload it under lock before deciding whether this + # subject may claim it so concurrent HCA callbacks cannot overwrite + # one another's subject. + candidate.lock! + + reason = + if !candidate.authentication_allowed? + "anonymized" + elsif candidate.hca_id.present? + "subject_already_linked" + elsif candidate.admin_level != "default" + "elevated_account" + elsif candidate.pending_deletion? + "pending_deletion" + elsif slack_uid && candidate.slack_uid.present? && candidate.slack_uid != slack_uid + "slack_identity_mismatch" + end + raise_hca_conflict!(subject:, reason:, email_user:, slack_user:) if reason + + candidate.update!( + hca_id: subject, + slack_uid: candidate.slack_uid || slack_uid, + country_code: candidate.country_code || country_code, + hca_access_token: nil, + hca_scopes: [] + ) + attach_hca_email!(candidate, email) + candidate + end + + SlackProfileSyncJob.perform_later(user.id) if user&.slack_uid.present? + user + rescue ActiveRecord::RecordNotUnique + raise if retrying + + from_hca_identity(subject:, email:, slack_uid:, country_code:, retrying: true) + end + + def create_from_hca_identity!(subject:, email:, slack_uid: nil, country_code: nil) + existing_user = from_hca_identity(subject:, email:, slack_uid:, country_code:) + return existing_user if existing_user + + user = transaction do + created_user = create!( + hca_id: subject, + slack_uid: slack_uid.presence, + country_code:, + hca_access_token: nil, + hca_scopes: [] + ) + created_user.email_addresses.create!(email:, source: :hca) + created_user end - SlackProfileSyncJob.perform_later(@user.id) if @user.slack_uid.present? - @user + SlackProfileSyncJob.perform_later(user.id) if user.slack_uid.present? + user + rescue ActiveRecord::RecordNotUnique + from_hca_identity(subject:, email:, slack_uid:, country_code:) || raise + end + + def sync_known_hca_user!(user, email:, slack_uid:, country_code:) + claimed_slack_user = lock.find_by(slack_uid: slack_uid) if slack_uid + attributes = { + country_code: user.country_code || country_code, + hca_access_token: nil, + hca_scopes: [] + } + attributes[:slack_uid] = slack_uid if user.slack_uid.blank? && claimed_slack_user.nil? + user.update!(attributes) + attach_hca_email!(user, email) + end + + def attach_hca_email!(user, email) + email_address = EmailAddress.find_by("LOWER(email) = ?", email) + user.email_addresses.create!(email:, source: :hca) unless email_address + end + + def record_known_hca_conflict!(user, subject:, email:, slack_uid:) + email_user = EmailAddress.find_by("LOWER(email) = ?", email)&.user + slack_user = find_by(slack_uid: slack_uid) if slack_uid + return unless [ email_user, slack_user ].compact.any? { |claim_user| claim_user != user } || (slack_uid && user.slack_uid.present? && user.slack_uid != slack_uid) + + HCAIdentityConflict.record!(HcaIdentityConflictError.new( + hca_id: subject, + reason: "known_subject_claim_drift", + email_user_id: email_user&.id, + slack_user_id: slack_user&.id + )) end - def from_slack_token(code, redirect_uri, ip_address = nil) + def raise_hca_conflict!(subject:, reason:, email_user: nil, slack_user: nil) + raise HcaIdentityConflictError.new( + hca_id: subject, + reason:, + email_user_id: email_user&.id, + slack_user_id: slack_user&.id + ) + end + + def exchange_slack_code(code, redirect_uri) response = HTTP.post("https://slack.com/api/oauth.v2.access", form: { client_id: ENV["SLACK_CLIENT_ID"], client_secret: ENV["SLACK_CLIENT_SECRET"], code: code, redirect_uri: redirect_uri @@ -90,28 +179,44 @@ def from_slack_token(code, redirect_uri, ip_address = nil) user_data = JSON.parse(user_response.body.to_s) return nil unless user_data["ok"] - slack_user = user_data["user"] || {} - email = (slack_user["profile"] || {})["email"]&.downcase - email_address = EmailAddress.find_or_initialize_by(email: email) - user = email_address.user || User.find_or_initialize_by(slack_uid: data.dig("authed_user", "id")).tap do |u| - u.email_addresses << email_address unless u.email_addresses.include?(email_address) + { + uid: data.dig("authed_user", "id"), + access_token: data.dig("authed_user", "access_token"), + scopes: data.dig("authed_user", "scope").to_s.split(/,\s*/), + profile: user_data["user"] || {} + } + rescue JSON::ParserError, HTTP::Error => e + report_error(e, message: "Slack OAuth exchange failed") + nil + end + + def connect_slack_identity!(user, identity) + slack_uid = identity&.dig(:uid) + return false unless user && slack_uid.present? + + user.with_lock do + return false unless user.authentication_allowed? + return false if user.pending_deletion? + return false if user.slack_uid.present? && user.slack_uid != slack_uid + return false if where(slack_uid:).where.not(id: user.id).exists? + + slack_user = identity[:profile] + user.slack_uid ||= slack_uid + user.apply_slack_profile_attributes(slack_user) + user.parse_and_set_timezone(slack_user["tz"]) if slack_user["tz"].present? + user.slack_access_token = identity[:access_token] + user.slack_scopes = identity[:scopes] + user.save! + true end + rescue ActiveRecord::RecordNotUnique + user.reload + false + rescue ActiveRecord::RecordInvalid => e + raise unless e.record == user && e.record.errors.of_kind?(:slack_uid, :taken) - user.email_addresses.source_slack.where.not(email: email).update_all(source: :signing_in) - email_address.source = :slack - email_address.save! if email_address.persisted? - - user.slack_uid = data.dig("authed_user", "id") - user.apply_slack_profile_attributes(slack_user) - user.parse_and_set_timezone(slack_user["tz"]) - user.slack_access_token = data["authed_user"]["access_token"] - user.slack_scopes = data["authed_user"]["scope"]&.split(/,\s*/) - user.country_code = country_code_from_ip(ip_address) if user.country_code.blank? - user.save! - user - rescue => e - report_error(e, message: "Error creating user from Slack data: #{e.message}") - nil + user.reload + false end def country_code_from_ip(ip_address) diff --git a/app/models/concerns/slack_integration.rb b/app/models/concerns/slack_integration.rb index 502c73b32..68b7e0f2b 100644 --- a/app/models/concerns/slack_integration.rb +++ b/app/models/concerns/slack_integration.rb @@ -53,7 +53,7 @@ def update_from_slack end # Assigns slack_username and slack_avatar_url from a Slack `user` payload. - # Shared by SlackIntegration#update_from_slack and OauthAuthentication.from_slack_token. + # Shared by SlackIntegration#update_from_slack and the Slack connection flow. def apply_slack_profile_attributes(slack_user) profile = slack_user["profile"] || {} self.slack_avatar_url = profile["image_192"] || profile["image_72"] diff --git a/app/models/deletion_request.rb b/app/models/deletion_request.rb index 425d20f8d..46a14d89c 100644 --- a/app/models/deletion_request.rb +++ b/app/models/deletion_request.rb @@ -21,8 +21,23 @@ def approve!(admin) scheduled_deletion_at: Time.current + 30.days) end - def cancel! = update!(status: :cancelled, cancelled_at: Time.current) - def complete! = update!(status: :completed, completed_at: Time.current) + def cancel! + with_lock do + return false unless pending? || approved? + + update!(status: :cancelled, cancelled_at: Time.current) + end + true + end + + def complete! + with_lock do + raise ActiveRecord::RecordInvalid.new(self) unless approved? + + update!(status: :completed, completed_at: Time.current) + end + end + def can_be_cancelled? = pending? || approved? def days_until_deletion diff --git a/app/models/email_address.rb b/app/models/email_address.rb index 85e5142ca..85d429e99 100644 --- a/app/models/email_address.rb +++ b/app/models/email_address.rb @@ -2,13 +2,13 @@ class EmailAddress < ApplicationRecord belongs_to :user has_paper_trail - validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP } + validates :email, presence: true, uniqueness: { case_sensitive: false }, format: { with: URI::MailTo::EMAIL_REGEXP } - enum :source, { signing_in: 0, github: 1, slack: 2, preserved_for_deletion: 3 }, prefix: true + enum :source, { signing_in: 0, github: 1, slack: 2, preserved_for_deletion: 3, hca: 4 }, prefix: true before_validation :downcase_email - def can_unlink? = !(source_github? || source_slack? || source_preserved_for_deletion?) + def can_unlink? = !(source_github? || source_slack? || source_preserved_for_deletion? || source_hca?) private diff --git a/app/models/hca_identity_conflict.rb b/app/models/hca_identity_conflict.rb new file mode 100644 index 000000000..c9c2661a2 --- /dev/null +++ b/app/models/hca_identity_conflict.rb @@ -0,0 +1,21 @@ +class HCAIdentityConflict < ApplicationRecord + belongs_to :email_user, class_name: "User", optional: true + belongs_to :slack_user, class_name: "User", optional: true + + validates :hca_id, :reason, :last_seen_at, presence: true + + def self.record!(error) + conflict = where(hca_id: error.hca_id, resolved_at: nil).first_or_initialize + conflict.assign_attributes( + reason: error.reason, + email_user_id: error.email_user_id, + slack_user_id: error.slack_user_id, + last_seen_at: Time.current, + occurrences: conflict.persisted? ? conflict.occurrences + 1 : 1 + ) + conflict.save! + conflict + rescue ActiveRecord::RecordNotUnique + retry + end +end diff --git a/app/models/sign_in_token.rb b/app/models/sign_in_token.rb index 8e2745daf..97ce94e95 100644 --- a/app/models/sign_in_token.rb +++ b/app/models/sign_in_token.rb @@ -1,7 +1,7 @@ class SignInToken < ApplicationRecord belongs_to :user - enum :auth_type, { email: 0, slack: 1, program_magic_link: 2 } + enum :auth_type, { email: 0, slack: 1, program_magic_link: 2, hca_recovery: 3 } validates :token, presence: true, uniqueness: true validates :auth_type, :expires_at, presence: true diff --git a/app/models/user.rb b/app/models/user.rb index d893762a5..276ba8b76 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -22,6 +22,7 @@ class User < ApplicationRecord before_validation :normalize_display_name_override encrypts :slack_access_token, :github_access_token, :hca_access_token + validates :hca_id, uniqueness: true, allow_nil: true validates :slack_uid, uniqueness: true, allow_nil: true validates :github_uid, uniqueness: { conditions: -> { where.not(github_access_token: nil) } }, allow_nil: true validates :timezone, inclusion: { in: TZInfo::Timezone.all_identifiers }, allow_nil: false @@ -258,7 +259,9 @@ def schedule_leaderboard_shadowban_expiration def streak_days = @streak_days ||= heartbeats.daily_streaks_for_users([ id ]).values.first def active_deletion_request = deletion_requests.active.order(created_at: :desc).first def pending_deletion? = active_deletion_request.present? - def api_access_restricted? = red? || pending_deletion? + def anonymized? = anonymized_at.present? + def authentication_allowed? = !anonymized? + def api_access_restricted? = red? || pending_deletion? || anonymized? def can_request_deletion? return false if pending_deletion? diff --git a/app/services/anonymize_user_service.rb b/app/services/anonymize_user_service.rb index 31a56a5b1..da5fecace 100644 --- a/app/services/anonymize_user_service.rb +++ b/app/services/anonymize_user_service.rb @@ -15,10 +15,14 @@ def initialize(user) def call ActiveRecord::Base.transaction do + user.lock! + first_anonymization = !user.anonymized? user.email_addresses.update_all(user_id: user.id, source: EmailAddress.sources[:preserved_for_deletion]) user.update!(ANONYMIZE_FIELDS.index_with { nil }.merge( slack_scopes: [], hca_scopes: [], - username: "deleted_user_#{user.id}", uses_slack_status: false + username: "deleted_user_#{user.id}", uses_slack_status: false, + anonymized_at: user.anonymized_at || Time.current, + authentication_version: user.authentication_version + (first_anonymization ? 1 : 0) )) destroy_associated_records end diff --git a/app/views/loops_mailer/hca_recovery_email.html.erb b/app/views/loops_mailer/hca_recovery_email.html.erb new file mode 100644 index 000000000..d42d4128c --- /dev/null +++ b/app/views/loops_mailer/hca_recovery_email.html.erb @@ -0,0 +1,6 @@ +

Recover your Hackatime account

+

+ Finish recovery in the same browser where you started signing in with your Hack Club Account. +

+<%= render "shared/mailer/button", url: @recovery_url, label: "Recover Hackatime account" %> +

This one-use link expires in 30 minutes. The link cannot sign in by itself.

diff --git a/app/views/loops_mailer/hca_recovery_email.text.erb b/app/views/loops_mailer/hca_recovery_email.text.erb new file mode 100644 index 000000000..c596a310c --- /dev/null +++ b/app/views/loops_mailer/hca_recovery_email.text.erb @@ -0,0 +1,6 @@ +Recover your Hackatime account + +Finish recovery in the same browser where you started signing in with your Hack Club Account: +<%= @recovery_url %> + +This one-use link expires in 30 minutes. The link cannot sign in by itself. diff --git a/config/application.rb b/config/application.rb index e3ae71d15..25156e8f9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -24,7 +24,7 @@ class Application < Rails::Application # Please, add to the `ignore` list any other `lib` subdirectories that do # not contain `.rb` files, or that should not be reloaded or eager loaded. # Common ones are `templates`, `generators`, or `middleware`, for example. - config.autoload_lib(ignore: %w[assets tasks]) + config.autoload_lib(ignore: %w[assets tasks omniauth]) config.eager_load_paths << Rails.root.join("test/mailers/previews").to_s # Configuration for the application, engines, and railties goes here. diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 013e047e9..2cbae6b3f 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -11,12 +11,9 @@ resource_owner_authenticator do if respond_to?(:current_user, true) user = send(:current_user) - client_id = request.params[:client_id] if user user - elsif client_id.present? && OauthApplication.find_by(uid: client_id)&.redirect_to_hca_login? - redirect_to(hca_auth_path(continue: request.fullpath)) else redirect_to(signin_path(continue: request.fullpath)) end diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb index a76d0afe8..44b268f59 100644 --- a/config/initializers/filter_parameter_logging.rb +++ b/config/initializers/filter_parameter_logging.rb @@ -4,7 +4,7 @@ # Use this to limit dissemination of sensitive information. # See the ActiveSupport::ParameterFilter documentation for supported notations and behaviors. Rails.application.config.filter_parameters += [ - :passw, :email, :secret, :token, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc, + :passw, :email, :login_hint, :secret, :token, :code, :assertion, :_key, :crypt, :salt, :certificate, :otp, :ssn, :cvv, :cvc, :_json, :hackatime, :heartbeat, :heartbeats, :ai_input_tokens, :ai_line_changes, :ai_model, :ai_output_tokens, :ai_prompt_length, :ai_session, :ai_subscription_plan, diff --git a/config/initializers/js_from_routes.rb b/config/initializers/js_from_routes.rb index e06cc53c1..26c598f6e 100644 --- a/config/initializers/js_from_routes.rb +++ b/config/initializers/js_from_routes.rb @@ -29,6 +29,9 @@ module JsFromRoutes signin signout hca_auth + hca_account + hca_recovery + hca_cancel slack_auth github_auth github_unlink diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb new file mode 100644 index 000000000..9c507d1c3 --- /dev/null +++ b/config/initializers/omniauth.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require Rails.root.join("lib/omniauth/strategies/hca") + +hca_redirect_uri = ENV["HCA_REDIRECT_URI"].presence || begin + public_url = ENV["PUBLIC_URL"].presence || "http://localhost:3000" + URI.join("#{public_url.delete_suffix('/')}/", "auth/hca/callback").to_s +end + +OmniAuth.config.logger = Rails.logger +OmniAuth.config.allowed_request_methods = [ :post ] + +Rails.application.config.middleware.use OmniAuth::Builder do + provider :hca, + issuer: HCAService.host, + discovery: true, + scope: %i[openid email slack_id], + response_type: :code, + uid_field: "sub", + send_state: true, + require_state: true, + send_nonce: true, + pkce: true, + client_signing_alg: :RS256, + client_auth_method: :basic, + client_options: { + identifier: ENV["HCA_CLIENT_ID"], + secret: ENV["HCA_CLIENT_SECRET"], + redirect_uri: hca_redirect_uri + } +end diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 2348ec5b8..52a795d49 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -57,7 +57,7 @@ def self.heartbeat_request?(req) end Rack::Attack.throttle("auth requests", limit: 5, period: 1.minute) do |req| - req.ip if req.path.in?([ "/login", "/signup", "/auth", "/sessions" ]) && req.post? + req.ip if req.post? && (req.path.start_with?("/auth/") || req.path.in?([ "/login", "/signup", "/auth", "/sessions" ])) end Rack::Attack.throttle("api requests", limit: 10000, period: 1.hour) do |req| diff --git a/config/routes.rb b/config/routes.rb index 75426d546..06cb244c9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -4,7 +4,8 @@ def initialize(*require) = @require = require.map(&:to_s) def matches?(request) return false unless request.session[:user_id] user = User.find_by(id: request.session[:user_id]) - user && @require.include?(user.admin_level) + session_version = request.session[:authentication_version] || 0 + user&.authentication_allowed? && user.authentication_version == session_version.to_i && @require.include?(user.admin_level) end end @@ -90,11 +91,12 @@ def matches?(request) get "/stop_impersonating", to: "sessions#stop_impersonating", as: :stop_impersonating - if Rails.env.development? - mount LetterOpenerWeb::Engine, at: "/letter_opener" + mount LetterOpenerWeb::Engine, at: "/letter_opener" if Rails.env.development? + if Rails.env.local? get "/__dev", to: "dev#index", as: :dev get "/__dev/log-me-in/:email", to: "dev#log_me_in", as: :dev_log_me_in, constraints: { email: /[^\/]+/ }, format: false + get "/__dev/log-me-in-user/:id", to: "dev#log_me_in_user", as: :test_log_me_in_user if Rails.env.test? get "/__dev/log-me-out", to: "dev#log_me_out", as: :dev_log_me_out end @@ -117,10 +119,14 @@ def matches?(request) get "/signin", to: "static_pages#signin", as: :signin # Auth routes - get "/auth/hca", to: "sessions#hca_new", as: :hca_auth + post "/auth/hca", to: "sessions#hca_new", as: :hca_auth get "/auth/hca/callback", to: "sessions#hca_create" - get "/auth/slack", to: "sessions#slack_new", as: :slack_auth + post "/auth/hca/account", to: "sessions#hca_account", as: :hca_account + post "/auth/hca/recovery", to: "sessions#hca_recovery", as: :hca_recovery + delete "/auth/hca/pending", to: "sessions#hca_cancel", as: :hca_cancel + post "/auth/slack", to: "sessions#slack_new", as: :slack_auth get "/auth/slack/callback", to: "sessions#slack_create" + get "/auth/failure", to: "sessions#failure", as: :auth_failure get "/auth/github", to: "sessions#github_new", as: :github_auth get "/auth/github/callback", to: "sessions#github_create" delete "/auth/github/unlink", to: "sessions#github_unlink", as: :github_unlink diff --git a/db/migrate/20260806152045_add_authentication_state_to_users.rb b/db/migrate/20260806152045_add_authentication_state_to_users.rb new file mode 100644 index 000000000..d87bd93bd --- /dev/null +++ b/db/migrate/20260806152045_add_authentication_state_to_users.rb @@ -0,0 +1,62 @@ +class AddAuthenticationStateToUsers < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + BATCH_SIZE = 1_000 + + def up + add_column :users, :anonymized_at, :datetime + # Starting at one intentionally invalidates browser sessions created by the + # pre-HCA release, which did not store an authentication version. + add_column :users, :authentication_version, :integer, default: 1, null: false + + backfill_completed_deletions + clear_hca_credentials + end + + def down + remove_column :users, :authentication_version + remove_column :users, :anonymized_at + end + + private + + def backfill_completed_deletions + loop do + updated = connection.update(<<~SQL.squish) + WITH completed_deletions AS ( + SELECT users.id, MAX(COALESCE(deletion_requests.completed_at, deletion_requests.updated_at, deletion_requests.created_at)) AS completed_at + FROM users + INNER JOIN deletion_requests ON deletion_requests.user_id = users.id + WHERE deletion_requests.status = 3 + AND users.anonymized_at IS NULL + GROUP BY users.id + ORDER BY users.id + LIMIT #{BATCH_SIZE} + ) + UPDATE users + SET anonymized_at = completed_deletions.completed_at + FROM completed_deletions + WHERE users.id = completed_deletions.id + SQL + break if updated.zero? + end + end + + def clear_hca_credentials + loop do + updated = connection.update(<<~SQL.squish) + UPDATE users + SET hca_access_token = NULL, + hca_scopes = '{}' + WHERE id IN ( + SELECT id + FROM users + WHERE hca_access_token IS NOT NULL OR CARDINALITY(hca_scopes) > 0 + ORDER BY id + LIMIT #{BATCH_SIZE} + ) + SQL + break if updated.zero? + end + end +end diff --git a/db/migrate/20260806152106_enforce_external_identity_uniqueness.rb b/db/migrate/20260806152106_enforce_external_identity_uniqueness.rb new file mode 100644 index 000000000..dca4b256a --- /dev/null +++ b/db/migrate/20260806152106_enforce_external_identity_uniqueness.rb @@ -0,0 +1,95 @@ +class EnforceExternalIdentityUniqueness < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + BATCH_SIZE = 1_000 + + def up + ensure_identity_data_is_unique! + + # A failed concurrent build leaves an invalid index behind. Removing any + # retry debris makes this non-transactional migration safe to rerun. + remove_index :users, name: :index_users_on_hca_id_unique, algorithm: :concurrently, if_exists: true + remove_index :email_addresses, name: :index_email_addresses_on_lower_email, algorithm: :concurrently, if_exists: true + + normalize_emails + + add_index :users, + :hca_id, + unique: true, + where: "hca_id IS NOT NULL", + name: :index_users_on_hca_id_unique, + algorithm: :concurrently + add_index :email_addresses, + "LOWER(email)", + unique: true, + name: :index_email_addresses_on_lower_email, + algorithm: :concurrently + + remove_index :users, + name: :index_users_on_hca_id, + algorithm: :concurrently, + if_exists: true + end + + def down + add_index :users, + :hca_id, + name: :index_users_on_hca_id, + algorithm: :concurrently, + if_not_exists: true + + remove_index :email_addresses, + name: :index_email_addresses_on_lower_email, + algorithm: :concurrently, + if_exists: true + remove_index :users, + name: :index_users_on_hca_id_unique, + algorithm: :concurrently, + if_exists: true + end + + private + + def normalize_emails + loop do + updated = connection.update(<<~SQL.squish) + UPDATE email_addresses + SET email = LOWER(email) + WHERE id IN ( + SELECT id + FROM email_addresses + WHERE email <> LOWER(email) + ORDER BY id + LIMIT #{BATCH_SIZE} + ) + SQL + break if updated.zero? + end + end + + def ensure_identity_data_is_unique! + duplicate_email_groups = select_value(<<~SQL.squish).to_i + SELECT COUNT(*) + FROM ( + SELECT LOWER(email) + FROM email_addresses + GROUP BY LOWER(email) + HAVING COUNT(*) > 1 + ) duplicates + SQL + duplicate_hca_groups = select_value(<<~SQL.squish).to_i + SELECT COUNT(*) + FROM ( + SELECT hca_id + FROM users + WHERE hca_id IS NOT NULL + GROUP BY hca_id + HAVING COUNT(*) > 1 + ) duplicates + SQL + return if duplicate_email_groups.zero? && duplicate_hca_groups.zero? + + raise ActiveRecord::MigrationError, + "External identities are not unique: #{duplicate_email_groups} case-insensitive email groups and #{duplicate_hca_groups} HCA ID groups require manual resolution" + end +end diff --git a/db/migrate/20260806152446_create_hca_identity_conflicts.rb b/db/migrate/20260806152446_create_hca_identity_conflicts.rb new file mode 100644 index 000000000..7ea802483 --- /dev/null +++ b/db/migrate/20260806152446_create_hca_identity_conflicts.rb @@ -0,0 +1,20 @@ +class CreateHCAIdentityConflicts < ActiveRecord::Migration[8.1] + def change + create_table :hca_identity_conflicts do |t| + t.string :hca_id, null: false + t.string :reason, null: false + t.references :email_user, foreign_key: { to_table: :users, on_delete: :nullify } + t.references :slack_user, foreign_key: { to_table: :users, on_delete: :nullify } + t.datetime :last_seen_at, null: false + t.integer :occurrences, default: 1, null: false + t.datetime :resolved_at + t.timestamps + end + + add_index :hca_identity_conflicts, + :hca_id, + unique: true, + where: "resolved_at IS NULL", + name: :index_active_hca_identity_conflicts_on_hca_id + end +end diff --git a/db/schema.rb b/db/schema.rb index b00e5b5bc..69cc32cd3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_04_184518) do +ActiveRecord::Schema[8.1].define(version: 2026_08_06_152446) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pg_stat_statements" @@ -135,6 +135,7 @@ t.integer "source" t.datetime "updated_at", null: false t.bigint "user_id", null: false + t.index "lower((email)::text)", name: "index_email_addresses_on_lower_email", unique: true t.index ["email"], name: "index_email_addresses_on_email", unique: true t.index ["email"], name: "index_email_addresses_on_email_trgm", opclass: :gin_trgm_ops, using: :gin t.index ["user_id"], name: "index_email_addresses_on_user_id" @@ -272,6 +273,21 @@ t.index ["scheduled_at"], name: "index_good_jobs_on_scheduled_at_unfinished_unperformed", where: "((finished_at IS NULL) AND (performed_at IS NULL))" end + create_table "hca_identity_conflicts", force: :cascade do |t| + t.datetime "created_at", null: false + t.bigint "email_user_id" + t.string "hca_id", null: false + t.datetime "last_seen_at", null: false + t.integer "occurrences", default: 1, null: false + t.string "reason", null: false + t.datetime "resolved_at" + t.bigint "slack_user_id" + t.datetime "updated_at", null: false + t.index ["email_user_id"], name: "index_hca_identity_conflicts_on_email_user_id" + t.index ["hca_id"], name: "index_active_hca_identity_conflicts_on_hca_id", unique: true, where: "(resolved_at IS NULL)" + t.index ["slack_user_id"], name: "index_hca_identity_conflicts_on_slack_user_id" + end + create_table "heartbeat_import_runs", force: :cascade do |t| t.datetime "created_at", null: false t.string "encrypted_api_key" @@ -673,6 +689,8 @@ create_table "users", force: :cascade do |t| t.integer "admin_level", default: 0, null: false t.boolean "allow_public_stats_lookup", default: true, null: false + t.datetime "anonymized_at" + t.integer "authentication_version", default: 1, null: false t.string "country_code" t.datetime "created_at", null: false t.boolean "default_timezone_leaderboard", default: true, null: false @@ -715,7 +733,7 @@ t.index ["github_uid", "github_access_token"], name: "index_users_on_github_uid_and_access_token" t.index ["github_uid"], name: "index_users_on_github_uid" t.index ["github_username"], name: "index_users_on_github_username_trgm", opclass: :gin_trgm_ops, using: :gin - t.index ["hca_id"], name: "index_users_on_hca_id" + t.index ["hca_id"], name: "index_users_on_hca_id_unique", unique: true, where: "(hca_id IS NOT NULL)" t.index ["leaderboard_shadowbanned"], name: "index_users_on_leaderboard_shadowbanned", where: "(leaderboard_shadowbanned = true)" t.index ["leaderboard_shadowbanned_by_id"], name: "index_users_on_leaderboard_shadowbanned_by_id" t.index ["slack_uid"], name: "index_users_on_slack_uid", unique: true @@ -766,6 +784,8 @@ add_foreign_key "email_addresses", "users" add_foreign_key "email_verification_requests", "users" add_foreign_key "goals", "users" + add_foreign_key "hca_identity_conflicts", "users", column: "email_user_id", on_delete: :nullify + add_foreign_key "hca_identity_conflicts", "users", column: "slack_user_id", on_delete: :nullify add_foreign_key "heartbeat_import_runs", "users" add_foreign_key "heartbeat_import_sources", "users" add_foreign_key "heartbeats", "ja4s", on_delete: :nullify diff --git a/db/seeds.rb b/db/seeds.rb index cd60be7f6..53b6dd442 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -30,18 +30,14 @@ key.token = 'dev-admin-api-key-12345' end - # Create a sign-in token that doesn't expire - token = test_user.sign_in_tokens.find_or_create_by(token: 'testing-token') do |t| - t.expires_at = 1.year.from_now - t.auth_type = :email - end + # Standalone sign-in tokens were retired in favour of HCA. + test_user.sign_in_tokens.where(token: 'testing-token').delete_all puts "Created test user:" puts " Username: #{test_user.display_name}" puts " Email: #{email.email}" puts " API Key: #{api_key.token}" puts " Admin API Key: #{admin_api_key.token}" - puts " Sign-in Token: #{token.token}" # Create sample heartbeats for last 7 days with variety of data if test_user.heartbeats.count < 50 diff --git a/docs/architecture.md b/docs/architecture.md index c2fc18203..089d55ca7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -55,11 +55,22 @@ example, request host), or for external links. API-only controllers may inherit ### Browser identity [`ApplicationController#current_user`](../app/controllers/application_controller.rb) -is exactly `User.find_by(id: session[:user_id])`. HCA, Slack, and single-use -email-link sign-in converge on a `User`; successful login resets the session -before assigning that ID. Slack and GitHub callback state is consumed and -compared with `secure_compare`; HCA currently does not use an OAuth state -nonce. Continuation URLs must be local paths (not `//...`). See +resolves `session[:user_id]` only while the stored authentication version still +matches the user and the account has not been anonymized. HCA OpenID Connect, +through OmniAuth, is the browser sign-in authority. Its authorization-code flow +requires state, nonce, PKCE, an RS256 ID token, a verified email and a stable +`ident!` subject. Successful login resets the session before assigning the user +and authentication version. The HCA cutover starts authentication versions at +one, so browser sessions created by older releases are rejected. HCA access, +refresh and ID tokens are not persisted. + +Slack OAuth is separate: after an unmatched HCA login it may prove ownership of +a legacy account, or after a recent HCA login it may refresh Slack integration +scopes for the same Slack identity. It cannot establish a standalone session or +switch users. Email links may likewise prove recovery only when tied to the +pending HCA subject and initiating browser; legacy standalone sign-in links are +rejected. GitHub remains a signed-in integration. Continuation URLs must be +local paths (not `//...`). See [`SessionsController`](../app/controllers/sessions_controller.rb). [`EmailAddress`](../app/models/email_address.rb) owns normalized, globally diff --git a/lib/omniauth/strategies/hca.rb b/lib/omniauth/strategies/hca.rb new file mode 100644 index 000000000..0f8c5d638 --- /dev/null +++ b/lib/omniauth/strategies/hca.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +module OmniAuth + module Strategies + class Hca < OpenIDConnect + option :name, "hca" + + def credentials = {} + + private + + def access_token + return @access_token if @access_token + + token_request_params = { + scope: (options.scope if options.send_scope_to_token_endpoint), + client_auth_method: options.client_auth_method + } + token_request_params[:code_verifier] = session.delete("omniauth.pkce.verifier") if options.pkce + + @access_token = client.access_token!(token_request_params) + verify_id_token!(@access_token.id_token) + @access_token + end + + def verify_id_token!(raw_id_token) + if raw_id_token.blank? + raise CallbackError.new(error: :missing_id_token, reason: "HCA did not return an ID token") + end + + id_token = decode_id_token(raw_id_token) + id_token.verify!( + issuer: options.issuer, + client_id: client_options.identifier, + nonce: stored_nonce + ) + validate_authorized_party!(id_token.raw_attributes) + validate_issued_at!(id_token.raw_attributes) + end + + def user_info + return @user_info if @user_info + + id_token_attributes = decode_id_token(access_token.id_token).raw_attributes + remote_user_info = access_token.userinfo!.raw_attributes + unless remote_user_info["sub"].present? && remote_user_info["sub"] == id_token_attributes["sub"] + raise CallbackError.new(error: :subject_mismatch, reason: "HCA UserInfo subject did not match the ID token") + end + + @user_info = ::OpenIDConnect::ResponseObject::UserInfo.new(remote_user_info.merge(id_token_attributes)) + end + + def decode_id_token(raw_id_token) + super + rescue JSON::JWK::Set::KidNotFound + raise if @refreshed_hca_jwks + + @refreshed_hca_jwks = true + @config = nil + @public_key = nil + retry + end + + def validate_authorized_party!(attributes) + audiences = Array(attributes["aud"]) + return if audiences.one? + return if attributes["azp"] == client_options.identifier + + raise CallbackError.new(error: :invalid_authorized_party, reason: "HCA ID token has an invalid authorized party") + end + + def validate_issued_at!(attributes) + issued_at = Time.at(Integer(attributes.fetch("iat"))) + return if issued_at <= 5.minutes.from_now + + raise CallbackError.new(error: :invalid_issued_at, reason: "HCA ID token was issued in the future") + rescue ArgumentError, KeyError, TypeError + raise CallbackError.new(error: :invalid_issued_at, reason: "HCA ID token has an invalid issued-at claim") + end + end + end +end diff --git a/test/controllers/api/admin/v1/oauth_admin_auth_test.rb b/test/controllers/api/admin/v1/oauth_admin_auth_test.rb index 542fc863a..2976d4eca 100644 --- a/test/controllers/api/admin/v1/oauth_admin_auth_test.rb +++ b/test/controllers/api/admin/v1/oauth_admin_auth_test.rb @@ -37,6 +37,15 @@ class Api::Admin::V1::OauthAdminAuthTest < ActionDispatch::IntegrationTest assert_response :unauthorized end + test "rejects oauth admin token for an anonymized user" do + token = oauth_token(@admin, "admin") + @admin.update!(anonymized_at: Time.current) + + get "/api/admin/v1/check", headers: bearer(token.token) + + assert_response :unauthorized + end + test "rejects oauth admin token after application is unverified" do t = oauth_token(@admin, "admin") @oauth.update!(verified: false) diff --git a/test/controllers/api/hackatime/v1/hackatime_controller_test.rb b/test/controllers/api/hackatime/v1/hackatime_controller_test.rb index 9a64df345..642b20587 100644 --- a/test/controllers/api/hackatime/v1/hackatime_controller_test.rb +++ b/test/controllers/api/hackatime/v1/hackatime_controller_test.rb @@ -2,6 +2,17 @@ require "stringio" class Api::Hackatime::V1::HackatimeControllerTest < ActionDispatch::IntegrationTest + test "anonymized user api key is rejected" do + user = User.create!(timezone: "UTC", anonymized_at: Time.current) + api_key = user.api_keys.create!(name: "primary") + + post "/api/hackatime/v1/users/current/heartbeats", + params: { entity: "src/main.rb", time: Time.current.to_f, type: "file" }.to_json, + headers: { "Authorization" => "Bearer #{api_key.token}", "CONTENT_TYPE" => "text/plain" } + + assert_response :unauthorized + end + test "single text plain heartbeat normalizes hash payloads" do user = User.create!(timezone: "UTC") api_key = user.api_keys.create!(name: "primary") diff --git a/test/controllers/api/v1/authenticated/me_controller_test.rb b/test/controllers/api/v1/authenticated/me_controller_test.rb index 43c1571d8..a3603d926 100644 --- a/test/controllers/api/v1/authenticated/me_controller_test.rb +++ b/test/controllers/api/v1/authenticated/me_controller_test.rb @@ -20,6 +20,16 @@ class Api::V1::Authenticated::MeControllerTest < ActionDispatch::IntegrationTest assert_response :unauthorized end + test "index rejects anonymized users with an existing OAuth token" do + user = User.create!(timezone: "UTC") + access_token = create_oauth_access_token(user) + user.update!(anonymized_at: Time.current, authentication_version: 1) + + get "/api/v1/authenticated/me", headers: { "Authorization" => "Bearer #{access_token.token}" } + + assert_response :unauthorized + end + private def create_oauth_access_token(user, scopes: "profile") diff --git a/test/controllers/api/v1/stats_controller_test.rb b/test/controllers/api/v1/stats_controller_test.rb index 7d16fe46f..39a9f1ef5 100644 --- a/test/controllers/api/v1/stats_controller_test.rb +++ b/test/controllers/api/v1/stats_controller_test.rb @@ -151,6 +151,16 @@ class Api::V1::StatsControllerTest < ActionDispatch::IntegrationTest assert_response :forbidden end + test "user_stats rejects anonymized owner OAuth token when public stats disabled" do + user = User.create!(username: "private_#{SecureRandom.hex(3)}", timezone: "UTC", allow_public_stats_lookup: false, + anonymized_at: Time.current) + access_token = create_oauth_access_token(user, scopes: "profile read") + + get "/api/v1/users/#{user.username}/stats", headers: { "Authorization" => "Bearer #{access_token.token}" } + + assert_response :forbidden + end + test "user_stats rejects owner OAuth token without read scope when public stats disabled" do user = User.create!(username: "private_#{SecureRandom.hex(3)}", timezone: "UTC", allow_public_stats_lookup: false) access_token = create_oauth_access_token(user, scopes: "profile") diff --git a/test/controllers/application_controller_test.rb b/test/controllers/application_controller_test.rb new file mode 100644 index 000000000..075913ccb --- /dev/null +++ b/test/controllers/application_controller_test.rb @@ -0,0 +1,48 @@ +require "test_helper" + +class ApplicationControllerTest < ActionController::TestCase + class CurrentUserController < ApplicationController + def show + render json: { user_id: current_user&.id } + end + end + + tests CurrentUserController + + setup do + @routes = ActionDispatch::Routing::RouteSet.new + @routes.draw { get "show" => "application_controller_test/current_user#show" } + end + + test "pre-HCA unversioned session is rejected after the cutover" do + user = User.create!(timezone: "UTC") + session[:user_id] = user.id + + get :show + + assert_nil response.parsed_body["user_id"] + assert_nil session[:user_id] + end + + test "authentication version mismatch rejects the user and clears the session" do + user = User.create!(timezone: "UTC", authentication_version: 1) + session[:user_id] = user.id + session[:authentication_version] = 0 + + get :show + + assert_nil response.parsed_body["user_id"] + assert_nil session[:user_id] + end + + test "anonymized user is rejected and the session is cleared" do + user = User.create!(timezone: "UTC", anonymized_at: Time.current, authentication_version: 1) + session[:user_id] = user.id + session[:authentication_version] = 1 + + get :show + + assert_nil response.parsed_body["user_id"] + assert_nil session[:user_id] + end +end diff --git a/test/controllers/custom_doorkeeper/authorizations_controller_test.rb b/test/controllers/custom_doorkeeper/authorizations_controller_test.rb index 803df3eb3..24887ef57 100644 --- a/test/controllers/custom_doorkeeper/authorizations_controller_test.rb +++ b/test/controllers/custom_doorkeeper/authorizations_controller_test.rb @@ -23,14 +23,14 @@ class CustomDoorkeeperAuthorizationsControllerTest < ActionDispatch::Integration assert_equal request.fullpath, Rack::Utils.parse_query(redirect_uri.query)["continue"] end - test "new redirects unauthenticated user to HCA sign in when application requires it" do + test "new redirects unauthenticated user to the HCA-backed sign-in screen when application requires it" do @oauth_app.update!(redirect_to_hca_login: true) get "/oauth/authorize", params: authorization_params assert_response :redirect redirect_uri = URI.parse(response.location) - assert_equal "/auth/hca", redirect_uri.path + assert_equal "/signin", redirect_uri.path assert_equal request.fullpath, Rack::Utils.parse_query(redirect_uri.query)["continue"] end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 10a523cc9..cc6e94a25 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -1,12 +1,17 @@ require "test_helper" require "uri" +require "webmock/minitest" class SessionsControllerTest < ActionDispatch::IntegrationTest setup do ActiveRecord::FixtureSet.reset_cache + OmniAuth.config.test_mode = true + OmniAuth.config.mock_auth[:hca] = hca_auth_hash end - # -- HCA: hca_new stores continue in session -- + teardown { OmniAuth.config.mock_auth[:hca] = nil } + + # -- HCA: request phase preserves a safe continuation -- test "hca_new stores continue path for oauth authorize" do continue_query = { @@ -17,36 +22,140 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest state: "a254695483383bd70ee41424b75d638a869e5d6769e11b50" } continue_path = "/oauth/authorize?#{Rack::Utils.build_query(continue_query)}" + user = User.create!(hca_id: "ident!test-user") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) - get hca_auth_path(continue: continue_path) + post hca_auth_path(continue: continue_path) + follow_redirect! - assert_equal continue_path, session.dig(:return_data, "url") - assert_response :redirect - assert_redirected_to %r{/oauth/authorize} + assert_redirected_to continue_path + assert_equal user.id, session[:user_id] + assert_equal "hca", session[:auth_provider] end test "hca_new rejects external continue URL" do - get hca_auth_path(continue: "https://evil.example.com/phish") + user = User.create!(hca_id: "ident!test-user") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) - assert_nil session.dig(:return_data, "url") - assert_response :redirect - assert_redirected_to %r{/oauth/authorize} + post hca_auth_path(continue: "https://evil.example.com/phish") + follow_redirect! + + assert_redirected_to root_path + assert_equal user.id, session[:user_id] end test "hca_new rejects javascript continue URL" do - get hca_auth_path(continue: "javascript:alert(1)") + user = User.create!(hca_id: "ident!test-user") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) - assert_nil session.dig(:return_data, "url") - assert_response :redirect - assert_redirected_to %r{/oauth/authorize} + post hca_auth_path(continue: "javascript:alert(1)") + follow_redirect! + + assert_redirected_to root_path + assert_equal user.id, session[:user_id] end test "hca_new rejects protocol-relative continue URL" do - get hca_auth_path(continue: "//evil.example.com/phish") + user = User.create!(hca_id: "ident!test-user") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) - assert_nil session.dig(:return_data, "url") - assert_response :redirect - assert_redirected_to %r{/oauth/authorize} + post hca_auth_path(continue: "//evil.example.com/phish") + follow_redirect! + + assert_redirected_to root_path + assert_equal user.id, session[:user_id] + end + + test "HCA callback rejects an unverified email" do + OmniAuth.config.mock_auth[:hca] = hca_auth_hash(email_verified: false) + + post hca_auth_path + follow_redirect! + + assert_redirected_to signin_path + assert_nil session[:user_id] + assert_nil session[:pending_hca] + end + + test "HCA callback rejects a malformed subject" do + OmniAuth.config.mock_auth[:hca] = hca_auth_hash(subject: "not-an-hca-subject") + + post hca_auth_path + follow_redirect! + + assert_redirected_to signin_path + assert_nil session[:user_id] + assert_nil session[:pending_hca] + end + + test "unknown HCA identity requires an explicit account choice" do + OmniAuth.config.mock_auth[:hca] = hca_auth_hash(subject: "ident!new-user", email: "new-user@example.com") + + post hca_auth_path + follow_redirect! + + assert_redirected_to signin_path + assert_nil session[:user_id] + assert_equal "ident!new-user", session.dig(:pending_hca, "subject") + + assert_difference -> { User.where(hca_id: "ident!new-user").count }, 1 do + post hca_account_path + end + + user = User.find_by!(hca_id: "ident!new-user") + assert_redirected_to setup_path + assert_equal user.id, session[:user_id] + assert_equal "hca", session[:auth_provider] + assert user.email_addresses.source_hca.exists?(email: "new-user@example.com") + end + + test "HCA email recovery links the pending subject only in the initiating browser" do + legacy_user = User.create! + legacy_user.email_addresses.create!(email: "legacy-recovery@example.com", source: :signing_in) + OmniAuth.config.mock_auth[:hca] = hca_auth_hash(subject: "ident!recovered", email: "new-hca@example.com") + + post hca_auth_path + follow_redirect! + post hca_recovery_path, params: { email: "legacy-recovery@example.com" } + token = legacy_user.sign_in_tokens.hca_recovery.last + + assert_not_nil token + assert_equal "ident!recovered", token.return_data["hca_subject"] + + other_browser = open_session + other_browser.get auth_token_path(token: token.token) + other_browser.assert_redirected_to signin_path + assert_nil other_browser.session[:user_id] + assert_nil token.reload.used_at + + get auth_token_path(token: token.token) + + assert_equal legacy_user.id, session[:user_id] + assert_equal "ident!recovered", legacy_user.reload.hca_id + assert token.reload.used_at.present? + end + + test "HCA callback records and denies split legacy identities" do + email_user = User.create! + email_user.email_addresses.create!(email: "split-controller@example.com", source: :signing_in) + slack_user = User.create!(slack_uid: "U_CONTROLLER_SPLIT") + OmniAuth.config.mock_auth[:hca] = hca_auth_hash( + subject: "ident!controller-split", + email: "split-controller@example.com", + slack_id: "U_CONTROLLER_SPLIT" + ) + + assert_difference -> { HCAIdentityConflict.count }, 1 do + post hca_auth_path + follow_redirect! + end + + assert_redirected_to signin_path + assert_nil session[:user_id] + conflict = HCAIdentityConflict.last + assert_equal "split_identity", conflict.reason + assert_equal email_user.id, conflict.email_user_id + assert_equal slack_user.id, conflict.slack_user_id end # -- Signin: preserves continue param -- @@ -69,132 +178,179 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_inertia_prop "continue_param", nil end - # -- Email auth: persists continue into sign-in token -- + # -- Legacy email entry points hand off to HCA -- - test "email auth stores continue param in sign-in token" do - user = User.create! + test "email auth redirects to HCA sign in with a login hint and safe continuation" do email = "continue-test-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email) - oauth_path = "/oauth/authorize?client_id=test&response_type=code" post email_auth_path, params: { email: email, continue: oauth_path } assert_response :redirect - - token = SignInToken.last - assert_not_nil token - assert_equal oauth_path, token.continue_param + assert_redirected_to signin_path(login_hint: email, continue: oauth_path) + assert_no_difference -> { SignInToken.count } do + follow_redirect! + end + assert_inertia_prop "login_hint", email + assert_inertia_prop "continue_param", oauth_path end - test "email auth uses the public URL for the development sign-in link" do - original_public_url = ENV["PUBLIC_URL"] - ENV["PUBLIC_URL"] = "https://hackatime.example.test/" - user = User.create! - email = "public-url-test-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email) - host! "3000-orb-id.e2b.app" + test "email auth normalizes the HCA login hint" do + post email_auth_path, params: { email: " Person@Example.COM " } - post email_auth_path, params: { email: email } + assert_redirected_to signin_path(login_hint: "person@example.com") + end - token = SignInToken.last - assert_equal "https://hackatime.example.test/auth/token/#{token.token}", session[:dev_magic_link] - ensure - ENV["PUBLIC_URL"] = original_public_url + test "email auth drops an unsafe continuation" do + post email_auth_path, params: { email: "person@example.com", continue: "https://evil.example/phish" } + + assert_redirected_to signin_path(login_hint: "person@example.com") end - test "email auth uses the request URL when the public URL is blank" do - original_public_url = ENV["PUBLIC_URL"] - ENV["PUBLIC_URL"] = "" + test "legacy standalone sign-in tokens cannot create a session" do user = User.create! - email = "blank-public-url-test-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email) - host! "hackatime.local" + %i[email slack program_magic_link].each do |auth_type| + sign_in_token = user.sign_in_tokens.create!(auth_type:) - post email_auth_path, params: { email: email } + get auth_token_path(token: sign_in_token.token) - token = SignInToken.last - assert_equal "http://hackatime.local/auth/token/#{token.token}", session[:dev_magic_link] - ensure - ENV["PUBLIC_URL"] = original_public_url + assert_redirected_to root_path + assert_nil session[:user_id] + assert_nil sign_in_token.reload.used_at + end end - test "email token redirects to continue param after sign in" do - user = User.create! - oauth_path = "/oauth/authorize?client_id=test&response_type=code" - sign_in_token = user.sign_in_tokens.create!( - auth_type: :email, - continue_param: oauth_path - ) + test "slack_new stores oauth nonce and embeds it in state" do + user = User.create!(hca_id: "ident!test-user", slack_uid: "U_TEST") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) + post hca_auth_path + follow_redirect! - get auth_token_path(token: sign_in_token.token) + post slack_auth_path assert_response :redirect - assert_redirected_to oauth_path - assert_equal user.id, session[:user_id] + assert_not_nil session[:slack_oauth_state] + + redirect_query = Rack::Utils.parse_nested_query(URI.parse(response.redirect_url).query) + state = JSON.parse(redirect_query["state"]) + + assert_equal session[:slack_oauth_state], redirect_query["state"] + assert_equal "integration", state["purpose"] + assert_equal user.id, state["user_id"] end - test "email token falls back to root when no continue param" do - user = User.create! - sign_in_token = user.sign_in_tokens.create!(auth_type: :email) + test "Slack cannot start a standalone login" do + post slack_auth_path - get auth_token_path(token: sign_in_token.token) + assert_redirected_to signin_path + assert_nil session[:slack_oauth_state] + assert_nil session[:user_id] + end - assert_response :redirect - assert_redirected_to root_path + test "Slack integration requires a recent HCA authentication" do + user = User.create!(hca_id: "ident!test-user", slack_uid: "U_TEST") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) + post hca_auth_path + follow_redirect! + + travel 31.minutes do + post slack_auth_path + end + + assert_redirected_to signin_path + assert_nil session[:slack_oauth_state] assert_equal user.id, session[:user_id] end - test "email token rejects external continue URL" do - user = User.create! - sign_in_token = user.sign_in_tokens.create!( - auth_type: :email, - continue_param: "https://evil.example.com/phish" - ) + test "Slack integration cannot switch the signed-in user" do + user = User.create!(hca_id: "ident!test-user", slack_uid: "U_EXPECTED") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) + post hca_auth_path + follow_redirect! + post slack_auth_path + state = session[:slack_oauth_state] + stub_slack_identity("U_OTHER") - get auth_token_path(token: sign_in_token.token) + get "/auth/slack/callback", params: { code: "oauth-code", state: state } - assert_response :redirect - assert_redirected_to root_path + assert_redirected_to signin_path assert_equal user.id, session[:user_id] + assert_equal "U_EXPECTED", user.reload.slack_uid + assert_nil user.slack_access_token end - test "email token rejects protocol-relative continue URL" do - user = User.create! - sign_in_token = user.sign_in_tokens.create!( - auth_type: :email, - continue_param: "//evil.example.com/phish" - ) + test "Slack integration can attach an unclaimed Slack identity to the signed-in HCA user" do + user = User.create!(hca_id: "ident!test-user") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) + post hca_auth_path + follow_redirect! + post slack_auth_path + state = session[:slack_oauth_state] + stub_slack_identity("U_NEW_INTEGRATION") - get auth_token_path(token: sign_in_token.token) + get "/auth/slack/callback", params: { code: "oauth-code", state: state } - assert_response :redirect - assert_redirected_to root_path + assert_redirected_to my_settings_path + assert_equal user.id, session[:user_id] + assert_equal "U_NEW_INTEGRATION", user.reload.slack_uid + assert_equal "slack-access-token", user.slack_access_token end - test "slack_new stores oauth nonce and embeds it in state" do - get slack_auth_path(close_window: true, continue: "/projects") + test "Slack recovery can link only after an unmatched HCA login" do + legacy_user = User.create!(slack_uid: "U_RECOVERY") + OmniAuth.config.mock_auth[:hca] = hca_auth_hash(subject: "ident!slack-recovered", email: "slack-recovered@example.com") + post hca_auth_path + follow_redirect! + assert_equal "ident!slack-recovered", session.dig(:pending_hca, "subject") - assert_response :redirect - assert_not_nil session[:slack_oauth_state_nonce] + post slack_auth_path + state = session[:slack_oauth_state] + stub_slack_identity("U_RECOVERY") - redirect_query = Rack::Utils.parse_nested_query(URI.parse(response.redirect_url).query) - state = JSON.parse(redirect_query["state"]) + get "/auth/slack/callback", params: { code: "oauth-code", state: state } + + assert_redirected_to root_path + assert_equal legacy_user.id, session[:user_id] + assert_equal "ident!slack-recovered", legacy_user.reload.hca_id + assert_equal "hca", session[:auth_provider] + end + + test "Slack recovery rejects a Slack account that differs from the HCA claim" do + legacy_user = User.create!(slack_uid: "U_RECOVERY_OTHER") + OmniAuth.config.mock_auth[:hca] = hca_auth_hash( + subject: "ident!slack-mismatch", + email: "slack-mismatch@example.com", + slack_id: "U_HCA_CLAIM" + ) + post hca_auth_path + follow_redirect! + post slack_auth_path + state = session[:slack_oauth_state] + stub_slack_identity("U_RECOVERY_OTHER") + + assert_difference -> { HCAIdentityConflict.count }, 1 do + get "/auth/slack/callback", params: { code: "oauth-code", state: state } + end - assert_equal session[:slack_oauth_state_nonce], state["token"] - assert_equal true, state["close_window"] - assert_equal "/projects", state["continue"] + assert_redirected_to signin_path + assert_nil session[:user_id] + assert_nil legacy_user.reload.hca_id + assert_equal "recovery_conflict", HCAIdentityConflict.last.reason end test "slack_create rejects oauth callback with mismatched state nonce" do - get slack_auth_path - expected_nonce = session[:slack_oauth_state_nonce] + user = User.create!(hca_id: "ident!test-user", slack_uid: "U_TEST") + user.email_addresses.create!(email: "hca-test@example.com", source: :hca) + post hca_auth_path + follow_redirect! + post slack_auth_path + expected_state = session[:slack_oauth_state] - get "/auth/slack/callback", params: { code: "oauth-code", state: { token: "wrong-#{expected_nonce}" }.to_json } + get "/auth/slack/callback", params: { code: "oauth-code", state: "wrong-#{expected_state}" } assert_response :redirect assert_redirected_to root_path - assert_nil session[:slack_oauth_state_nonce] + assert_nil session[:slack_oauth_state] end test "github_new stores oauth nonce and passes it in redirect state" do @@ -422,4 +578,32 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_equal admin.id, session[:user_id] assert_nil session[:impersonater_user_id] end + + private + + def stub_slack_identity(uid) + stub_request(:post, "https://slack.com/api/oauth.v2.access") + .to_return(body: { + ok: true, + authed_user: { id: uid, access_token: "slack-access-token", scope: "users.profile:read" } + }.to_json) + stub_request(:get, "https://slack.com/api/users.info?user=#{uid}") + .to_return(body: { ok: true, user: { id: uid, profile: {} } }.to_json) + end + + def hca_auth_hash(subject: "ident!test-user", email: "hca-test@example.com", slack_id: nil, email_verified: true) + OmniAuth::AuthHash.new( + provider: "hca", + uid: subject, + info: { email:, email_verified: }, + extra: { + raw_info: { + "sub" => subject, + "email" => email, + "email_verified" => email_verified, + "slack_id" => slack_id + }.compact + } + ) + end end diff --git a/test/integration/email_login_test.rb b/test/integration/email_login_test.rb index f8ddbf1a2..2064f54fe 100644 --- a/test/integration/email_login_test.rb +++ b/test/integration/email_login_test.rb @@ -1,75 +1,42 @@ require "test_helper" class EmailLoginTest < ActionDispatch::IntegrationTest - test "full email sign-in flow creates token and signs user in" do - user = User.create!(timezone: "UTC") + test "email sign-in entry point moves the user to HCA without creating a token" do email = "login-flow-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email, source: :signing_in) - assert_difference -> { SignInToken.count }, 1 do + assert_no_difference -> { SignInToken.count } do post email_auth_path, params: { email: email } end - assert_response :redirect - - token = SignInToken.last - assert_equal user.id, token.user_id - - get auth_token_path(token: token.token) - - assert_response :redirect - assert_redirected_to root_path - assert_equal user.id, session[:user_id] + assert_redirected_to signin_path(login_hint: email) + assert_nil session[:user_id] end - test "email sign-in is case-insensitive" do - user = User.create!(timezone: "UTC") - email = "case-test-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email, source: :signing_in) - - post email_auth_path, params: { email: email.upcase } - - assert_response :redirect + test "email sign-in passes a normalized email to HCA as a login hint" do + post email_auth_path, params: { email: " Legacy@Example.COM " } - token = SignInToken.last - assert_equal user.id, token.user_id + assert_redirected_to signin_path(login_hint: "legacy@example.com") end - test "email sign-in with continue param preserves redirect" do - user = User.create!(timezone: "UTC") - email = "continue-#{SecureRandom.hex(4)}@example.com" - user.email_addresses.create!(email: email, source: :signing_in) + test "email sign-in preserves a safe continuation for the HCA flow" do continue_path = "/oauth/authorize?client_id=test&response_type=code" - post email_auth_path, params: { email: email, continue: continue_path } - - token = SignInToken.last - assert_equal continue_path, token.continue_param + post email_auth_path, params: { email: "legacy@example.com", continue: continue_path } - get auth_token_path(token: token.token) - - assert_response :redirect - assert_redirected_to continue_path - assert_equal user.id, session[:user_id] + assert_redirected_to signin_path(login_hint: "legacy@example.com", continue: continue_path) end - test "email sign-in token can only be used once" do + test "legacy email sign-in token cannot create a session" do user = User.create!(timezone: "UTC") email = "once-#{SecureRandom.hex(4)}@example.com" user.email_addresses.create!(email: email, source: :signing_in) - post email_auth_path, params: { email: email } - token = SignInToken.last - - get auth_token_path(token: token.token) - assert_equal user.id, session[:user_id] - - delete signout_path - assert_nil session[:user_id] + token = user.sign_in_tokens.create!(auth_type: :email) get auth_token_path(token: token.token) assert_redirected_to root_path assert_nil session[:user_id] + assert_nil token.reload.used_at end test "expired email token does not sign user in" do diff --git a/test/jobs/process_account_deletions_job_test.rb b/test/jobs/process_account_deletions_job_test.rb new file mode 100644 index 000000000..184540d55 --- /dev/null +++ b/test/jobs/process_account_deletions_job_test.rb @@ -0,0 +1,37 @@ +require "test_helper" + +class ProcessAccountDeletionsJobTest < ActiveJob::TestCase + test "anonymizes and completes a due approved request atomically" do + user = User.create! + deletion_request = DeletionRequest.create_for_user!(user) + deletion_request.update!(status: :approved, scheduled_deletion_at: 1.minute.ago) + + ProcessAccountDeletionsJob.perform_now + + assert user.reload.anonymized? + assert deletion_request.reload.completed? + assert deletion_request.completed_at.present? + end + + test "does not process a request cancelled before its locked transition" do + user = User.create! + deletion_request = DeletionRequest.create_for_user!(user) + deletion_request.update!(status: :approved, scheduled_deletion_at: 1.minute.ago) + assert deletion_request.cancel! + + ProcessAccountDeletionsJob.perform_now + + assert_not user.reload.anonymized? + assert deletion_request.reload.cancelled? + end + + test "completed requests cannot later report a successful cancellation" do + user = User.create! + deletion_request = DeletionRequest.create_for_user!(user) + deletion_request.update!(status: :approved, scheduled_deletion_at: 1.minute.ago) + ProcessAccountDeletionsJob.perform_now + + assert_not deletion_request.reload.cancel! + assert deletion_request.completed? + end +end diff --git a/test/jobs/slack_profile_sync_job_test.rb b/test/jobs/slack_profile_sync_job_test.rb index 3e7309503..1cf0dc268 100644 --- a/test/jobs/slack_profile_sync_job_test.rb +++ b/test/jobs/slack_profile_sync_job_test.rb @@ -95,4 +95,29 @@ class SlackProfileSyncJobTest < ActiveJob::TestCase assert_not_requested :get, /slack\.com/ end + + test "does not restore Slack profile data when anonymization finishes during the request" do + user = User.create!(timezone: "UTC", slack_uid: "U_DELETION_RACE") + stub_request(:get, "https://slack.com/api/users.info?user=U_DELETION_RACE") + .to_return do + AnonymizeUserService.call(user) + { + body: { + ok: true, + user: { + name: "deleted-name", + profile: { image_192: "https://example.com/deleted-avatar.png" } + } + }.to_json + } + end + + SlackProfileSyncJob.perform_now(user.id) + + user.reload + assert user.anonymized? + assert_nil user.slack_uid + assert_nil user.slack_username + assert_nil user.slack_avatar_url + end end diff --git a/test/lib/omniauth/strategies/hca_test.rb b/test/lib/omniauth/strategies/hca_test.rb new file mode 100644 index 000000000..75ff74610 --- /dev/null +++ b/test/lib/omniauth/strategies/hca_test.rb @@ -0,0 +1,91 @@ +require "test_helper" + +class HCAStrategyTest < ActiveSupport::TestCase + setup do + @strategy = OmniAuth::Strategies::Hca.new( + ->(_env) { [ 200, {}, [] ] }, + issuer: "https://auth.hackclub.com", + pkce: true, + send_state: true, + send_nonce: true, + client_options: { + identifier: "client-id", + secret: "client-secret", + redirect_uri: "https://hackatime.example/auth/hca/callback" + } + ) + end + + test "does not expose provider tokens through OmniAuth credentials" do + assert_empty @strategy.credentials + end + + test "requires an ID token" do + error = assert_raises(OmniAuth::Strategies::OpenIDConnect::CallbackError) do + @strategy.send(:verify_id_token!, nil) + end + + assert_equal :missing_id_token, error.error + end + + test "requires UserInfo and ID token subjects to match" do + access_token = fake_access_token( + id_token: "raw-id-token", + user_info: { "sub" => "ident!different", "email" => "person@example.com" } + ) + @strategy.define_singleton_method(:access_token) { access_token } + @strategy.define_singleton_method(:decode_id_token) do |_raw_token| + Struct.new(:raw_attributes).new({ "sub" => "ident!expected" }) + end + + error = assert_raises(OmniAuth::Strategies::OpenIDConnect::CallbackError) do + @strategy.send(:user_info) + end + + assert_equal :subject_mismatch, error.error + end + + test "keeps nonce and PKCE verifier in the server session" do + client = Class.new do + attr_accessor :redirect_uri + attr_reader :authorization_options + + def authorization_uri(options) + @authorization_options = options + "https://auth.hackclub.com/oauth/authorize" + end + end.new + env = Rack::MockRequest.env_for("/?login_hint=person%40example.com") + env["rack.session"] = {} + @strategy.instance_variable_set(:@env, env) + @strategy.define_singleton_method(:client) { client } + + @strategy.send(:authorize_uri) + + options = client.authorization_options + assert_equal "person@example.com", options[:login_hint] + assert_equal "S256", options[:code_challenge_method] + assert options[:state].present? + assert options[:nonce].present? + assert env["rack.session"]["omniauth.pkce.verifier"].present? + assert_equal options[:state], env["rack.session"]["omniauth.state"] + assert_equal options[:nonce], env["rack.session"]["omniauth.nonce"] + end + + private + + def fake_access_token(id_token:, user_info:) + Class.new do + attr_reader :id_token + + define_method(:initialize) do |raw_id_token, attributes| + @id_token = raw_id_token + @attributes = attributes + end + + define_method(:userinfo!) do + Struct.new(:raw_attributes).new(@attributes) + end + end.new(id_token, user_info) + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c84e34da7..4ca56969d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -84,6 +84,14 @@ class UserTest < ActiveSupport::TestCase assert user.api_access_restricted? end + test "anonymized users cannot authenticate or access APIs" do + user = User.create!(timezone: "UTC", anonymized_at: Time.current) + + assert user.anonymized? + assert_not user.authentication_allowed? + assert user.api_access_restricted? + end + test "display name override takes precedence over synced provider names" do user = User.create!( timezone: "UTC", @@ -148,19 +156,15 @@ class UserTest < ActiveSupport::TestCase end test "HCA authentication fills a missing Slack ID on an existing account" do - user = User.create!(timezone: "UTC", hca_id: "hca-existing") - stub_request(:post, "https://hca.dinosaurbbq.org/oauth/token") - .to_return(body: { access_token: "hca-token" }.to_json) - stub_request(:get, "https://hca.dinosaurbbq.org/api/v1/me") - .with(headers: { "Authorization" => "Bearer hca-token" }) - .to_return(body: { - identity: { id: "hca-existing", slack_id: "U_FROM_HCA" }, - scopes: %w[email slack_id] - }.to_json) + user = User.create!(timezone: "UTC", hca_id: "ident!hca-existing") authenticated_user = nil assert_enqueued_with(job: SlackProfileSyncJob, args: [ user.id ]) do - authenticated_user = User.from_hca_token("code", "https://example.com/auth/hca/callback") + authenticated_user = User.from_hca_identity( + subject: "ident!hca-existing", + email: "hca-existing@example.com", + slack_uid: "U_FROM_HCA" + ) end assert_equal user, authenticated_user @@ -170,22 +174,18 @@ class UserTest < ActiveSupport::TestCase test "HCA authentication does not replace an existing Slack ID" do user = User.create!( timezone: "UTC", - hca_id: "hca-linked", + hca_id: "ident!hca-linked", slack_uid: "U_LINKED", slack_synced_at: 1.hour.ago ) - stub_request(:post, "https://hca.dinosaurbbq.org/oauth/token") - .to_return(body: { access_token: "hca-token" }.to_json) - stub_request(:get, "https://hca.dinosaurbbq.org/api/v1/me") - .with(headers: { "Authorization" => "Bearer hca-token" }) - .to_return(body: { - identity: { id: "hca-linked", slack_id: "U_DIFFERENT" }, - scopes: %w[email slack_id] - }.to_json) authenticated_user = nil assert_enqueued_with(job: SlackProfileSyncJob, args: [ user.id ]) do - authenticated_user = User.from_hca_token("code", "https://example.com/auth/hca/callback") + authenticated_user = User.from_hca_identity( + subject: "ident!hca-linked", + email: "hca-linked@example.com", + slack_uid: "U_DIFFERENT" + ) end assert_equal user, authenticated_user @@ -193,21 +193,145 @@ class UserTest < ActiveSupport::TestCase end test "HCA authentication does not claim a Slack ID linked to another account" do - user = User.create!(timezone: "UTC", hca_id: "hca-unlinked") + user = User.create!(timezone: "UTC", hca_id: "ident!hca-unlinked") User.create!(timezone: "UTC", slack_uid: "U_ALREADY_LINKED") - stub_request(:post, "https://hca.dinosaurbbq.org/oauth/token") - .to_return(body: { access_token: "hca-token" }.to_json) - stub_request(:get, "https://hca.dinosaurbbq.org/api/v1/me") - .with(headers: { "Authorization" => "Bearer hca-token" }) - .to_return(body: { - identity: { id: "hca-unlinked", slack_id: "U_ALREADY_LINKED" }, - scopes: %w[email slack_id] - }.to_json) - authenticated_user = User.from_hca_token("code", "https://example.com/auth/hca/callback") + authenticated_user = User.from_hca_identity( + subject: "ident!hca-unlinked", + email: "hca-unlinked@example.com", + slack_uid: "U_ALREADY_LINKED" + ) + + assert_equal user, authenticated_user + assert_nil user.reload.slack_uid + end + + test "known HCA subject remains authoritative when its current email belongs to another account" do + subject_user = User.create!(timezone: "UTC", hca_id: "ident!email-drift", slack_uid: "U_EMAIL_DRIFT") + email_user = User.create!(timezone: "UTC") + email_user.email_addresses.create!(email: "drifted@example.com", source: :signing_in) + + authenticated_user = nil + assert_difference -> { HCAIdentityConflict.count }, 1 do + authenticated_user = User.from_hca_identity( + subject: "ident!email-drift", + email: "drifted@example.com", + slack_uid: "U_EMAIL_DRIFT" + ) + end + + assert_equal subject_user, authenticated_user + assert_equal email_user, EmailAddress.find_by!(email: "drifted@example.com").user + assert_not subject_user.email_addresses.exists?(email: "drifted@example.com") + + conflict = HCAIdentityConflict.find_by!(hca_id: "ident!email-drift") + assert_equal "known_subject_claim_drift", conflict.reason + assert_equal email_user.id, conflict.email_user_id + assert_equal subject_user.id, conflict.slack_user_id + end + + test "HCA authentication links a Slack-matched legacy account when the HCA email differs" do + user = User.create!(timezone: "UTC", slack_uid: "U_MATCHED") + user.email_addresses.create!(email: "old-slack-email@example.com", source: :slack) + + authenticated_user = User.from_hca_identity( + subject: "ident!different-email", + email: "current-hca-email@example.com", + slack_uid: "U_MATCHED" + ) assert_equal user, authenticated_user + assert_equal "ident!different-email", user.reload.hca_id + assert user.email_addresses.exists?(email: "old-slack-email@example.com") + assert user.email_addresses.source_hca.exists?(email: "current-hca-email@example.com") + end + + test "HCA authentication rejects split email and Slack candidates" do + email_user = User.create!(timezone: "UTC") + email_user.email_addresses.create!(email: "split@example.com", source: :signing_in) + slack_user = User.create!(timezone: "UTC", slack_uid: "U_SPLIT") + + error = assert_raises(OauthAuthentication::HcaIdentityConflictError) do + User.from_hca_identity( + subject: "ident!split", + email: "split@example.com", + slack_uid: "U_SPLIT" + ) + end + + assert_equal "split_identity", error.reason + assert_equal email_user.id, error.email_user_id + assert_equal slack_user.id, error.slack_user_id + assert_nil email_user.reload.hca_id + assert_nil slack_user.reload.hca_id + end + + test "HCA authentication rejects a legacy candidate already linked to another subject" do + user = User.create!(timezone: "UTC", hca_id: "ident!existing") + user.email_addresses.create!(email: "already-linked@example.com", source: :hca) + + error = assert_raises(OauthAuthentication::HcaIdentityConflictError) do + User.from_hca_identity( + subject: "ident!different", + email: "already-linked@example.com" + ) + end + + assert_equal "subject_already_linked", error.reason + assert_equal "ident!existing", user.reload.hca_id + end + + test "HCA authentication returns nil when no legacy identity matches" do + assert_nil User.from_hca_identity( + subject: "ident!unmatched", + email: "unmatched@example.com", + slack_uid: "U_UNMATCHED" + ) + end + + test "HCA authentication rejects anonymized identity candidates" do + user = User.create!(timezone: "UTC", anonymized_at: Time.current) + user.email_addresses.create!(email: "preserved@example.com", source: :preserved_for_deletion) + + error = assert_raises(OauthAuthentication::HcaIdentityConflictError) do + User.from_hca_identity(subject: "ident!deleted", email: "preserved@example.com") + end + + assert_equal "anonymized", error.reason + assert_nil user.reload.hca_id + end + + test "Slack integration does not restore identity after a stale user is anonymized" do + user = User.create!(timezone: "UTC", hca_id: "ident!slack-race") + stale_user = User.find(user.id) + AnonymizeUserService.call(user) + + connected = User.connect_slack_identity!(stale_user, { + uid: "U_AFTER_DELETION", + access_token: "slack-access-token", + scopes: [ "users:read" ], + profile: { "name" => "restored" } + }) + + assert_not connected + assert_nil user.reload.slack_uid + assert_nil user.slack_access_token + end + + test "Slack integration rejects users with a pending deletion" do + user = User.create!(timezone: "UTC", hca_id: "ident!slack-pending-deletion") + DeletionRequest.create_for_user!(user) + + connected = User.connect_slack_identity!(user, { + uid: "U_PENDING_DELETION", + access_token: "slack-access-token", + scopes: [ "users:read" ], + profile: { "name" => "pending" } + }) + + assert_not connected assert_nil user.reload.slack_uid + assert_nil user.slack_access_token end test "creating a user with an email address queues a welcome email" do diff --git a/test/services/anonymize_user_service_test.rb b/test/services/anonymize_user_service_test.rb index 8c72c8a2a..6eb085aa4 100644 --- a/test/services/anonymize_user_service_test.rb +++ b/test/services/anonymize_user_service_test.rb @@ -1,6 +1,25 @@ require "test_helper" class AnonymizeUserServiceTest < ActiveSupport::TestCase + test "anonymization records its timestamp and increments authentication version once" do + user = User.create!(username: "versioned_#{SecureRandom.hex(4)}") + original_authentication_version = user.authentication_version + + AnonymizeUserService.call(user) + user.reload + original_anonymized_at = user.anonymized_at + + assert_not_nil original_anonymized_at + assert_equal original_authentication_version + 1, user.authentication_version + + travel 1.minute do + AnonymizeUserService.call(user) + end + + assert_equal original_anonymized_at, user.reload.anonymized_at + assert_equal original_authentication_version + 1, user.authentication_version + end + test "anonymization clears profile identity fields" do user = User.create!( username: "anon_#{SecureRandom.hex(4)}", diff --git a/test/test_helper.rb b/test/test_helper.rb index 75a9bc40a..d1d453efb 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -33,15 +33,13 @@ class TestCase module SystemTestAuthHelper def sign_in_as(user) - token = user.sign_in_tokens.create!(auth_type: :email) - visit auth_token_path(token: token.token) + visit test_log_me_in_user_path(id: user.id) end end module IntegrationTestAuthHelper def sign_in_as(user) - token = user.sign_in_tokens.create!(auth_type: :email) - get auth_token_path(token: token.token) + get test_log_me_in_user_path(id: user.id) assert_equal user.id, session[:user_id] end end