diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 189500f59..85f88e01a 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 callback URLs of `http://localhost:3000/auth/hca/callback` and `http://localhost:3000/deletion/hca/callback`, with minimum scopes of `email`, `slack_id`, and `verification_status`. Then, fill out the following fields in your `.env` file: diff --git a/app/controllers/deletion_requests_controller.rb b/app/controllers/deletion_requests_controller.rb index f3f2b5aa1..746333992 100644 --- a/app/controllers/deletion_requests_controller.rb +++ b/app/controllers/deletion_requests_controller.rb @@ -12,11 +12,45 @@ def show end def create - @deletion_request = DeletionRequest.create_for_user!(current_user, **deletion_request_params) - redirect_to deletion_path - rescue ActiveRecord::RecordInvalid => e - report_error(e, message: "Deletion request creation failed") - redirect_to my_settings_path + return begin_hca_step_up if current_user.hca_id.present? + + create_deletion_request(deletion_request_params) + end + + def hca_callback + pending_request = session.delete(:pending_deletion_request) + + unless valid_hca_state?(pending_request&.dig("state")) + report_message("HCA deletion step-up state was invalid") + return redirect_to(my_settings_path, alert: "Hack Club Auth verification failed. Please try again.") + end + + if params[:error].present? + report_message("HCA deletion step-up error: #{params[:error]}") unless params[:error] == "access_denied" + return redirect_to(my_settings_path, alert: "Hack Club Auth verification was cancelled.") + end + + unless pending_request["user_id"] == current_user.id && + pending_request["hca_id"] == current_user.hca_id && params[:code].present? + report_message("HCA deletion step-up context was invalid") + return redirect_to(my_settings_path, alert: "Hack Club Auth verification failed. Please try again.") + end + + redirect_uri = hca_deletion_callback_url + hca_id = User.hca_id_from_token(params[:code], redirect_uri) + unless hca_id.present? && hca_id == pending_request["hca_id"] + report_message("HCA deletion step-up identity did not match User ##{current_user.id}") + return redirect_to(my_settings_path, alert: "Please verify with the Hack Club Account linked to Hackatime.") + end + + unless current_user.can_request_deletion? + return redirect_to(my_settings_path, alert: "You can't request deletion right now.") + end + + create_deletion_request(pending_request.fetch("attributes").symbolize_keys) + rescue HTTP::Error, JSON::ParserError => e + report_error(e, message: "HCA deletion step-up failed") + redirect_to my_settings_path, alert: "Hack Club Auth verification failed. Please try again." end def cancel @@ -45,6 +79,42 @@ def check_can_request end end + def begin_hca_step_up + attributes = deletion_request_params + if attributes[:reason].to_s.length > DeletionRequest::MAX_REASON_LENGTH || + attributes[:reason_details].to_s.length > DeletionRequest::MAX_REASON_DETAILS_LENGTH + return redirect_to(my_settings_path, alert: "Deletion details are too long.") + end + + state = SecureRandom.hex(24) + session[:pending_deletion_request] = { + "state" => state, + "user_id" => current_user.id, + "hca_id" => current_user.hca_id, + "attributes" => attributes.stringify_keys + } + + redirect_to User.hca_authorize_url( + hca_deletion_callback_url, + state:, + prompt: "login", + scope: "openid email slack_id verification_status" + ), host: HCAService.host, allow_other_host: HCAService.host + end + + def valid_hca_state?(expected_state) + expected_state.present? && params[:state].present? && + ActiveSupport::SecurityUtils.secure_compare(params[:state].to_s, expected_state.to_s) + end + + def create_deletion_request(attributes) + @deletion_request = DeletionRequest.create_for_user!(current_user, **attributes) + redirect_to deletion_path + rescue ActiveRecord::RecordInvalid => e + report_error(e, message: "Deletion request creation failed") + redirect_to my_settings_path + end + def deletion_request_params params.fetch(:deletion_request, {}).permit(:reason, :reason_details).to_h.symbolize_keys end diff --git a/app/controllers/settings/privacy_controller.rb b/app/controllers/settings/privacy_controller.rb index bf1668e92..f5b342969 100644 --- a/app/controllers/settings/privacy_controller.rb +++ b/app/controllers/settings/privacy_controller.rb @@ -25,7 +25,8 @@ def render_privacy(status: :ok) = render_settings_page(active_section: "privacy" def section_props { user: user_props(keys: %i[allow_public_stats_lookup can_request_deletion]), - rotated_api_key: flash[:rotated_api_key] } + rotated_api_key: flash[:rotated_api_key], + deletion_reason_details_max_length: DeletionRequest::MAX_REASON_DETAILS_LENGTH } end def privacy_params = params.require(:user).permit(:allow_public_stats_lookup) diff --git a/app/javascript/pages/Users/Settings/Privacy.svelte b/app/javascript/pages/Users/Settings/Privacy.svelte index 375d6ac2d..af7ccaf70 100644 --- a/app/javascript/pages/Users/Settings/Privacy.svelte +++ b/app/javascript/pages/Users/Settings/Privacy.svelte @@ -17,6 +17,7 @@ user, rotated_api_key = "", errors, + deletion_reason_details_max_length, }: PrivacyPageProps = $props(); const deletionReasons = [ @@ -231,6 +232,7 @@ name="deletion_request[reason_details]" rows="4" bind:value={deletionReasonDetails} + maxlength={deletion_reason_details_max_length} required class="block w-full rounded-lg border border-surface-300 bg-darker px-3 py-2 text-sm text-surface-content placeholder:text-muted focus:border-primary focus:outline-none focus:ring-2 focus:ring-primary/30" placeholder="Tell us anything else we should know."> diff --git a/app/javascript/pages/Users/Settings/types.ts b/app/javascript/pages/Users/Settings/types.ts index 7d05253cf..f5a19d3b6 100644 --- a/app/javascript/pages/Users/Settings/types.ts +++ b/app/javascript/pages/Users/Settings/types.ts @@ -270,6 +270,7 @@ export type NotificationsPageProps = SettingsCommonProps & { export type PrivacyPageProps = SettingsCommonProps & { user: Pick; rotated_api_key?: string | null; + deletion_reason_details_max_length: number; }; export type GoalsPageProps = SettingsCommonProps & { diff --git a/app/models/concerns/oauth_authentication.rb b/app/models/concerns/oauth_authentication.rb index 4d85ed248..89b1b4e95 100644 --- a/app/models/concerns/oauth_authentication.rb +++ b/app/models/concerns/oauth_authentication.rb @@ -5,13 +5,28 @@ module OauthAuthentication class_methods do include ErrorReporting - def hca_authorize_url(redirect_uri) + def hca_authorize_url(redirect_uri, state: nil, prompt: nil, scope: "email slack_id verification_status") 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}") + scope:, + state:, + prompt: + }.compact.to_query}") + end + + def hca_id_from_token(code, redirect_uri) + response = HTTP.post("#{HCAService.host}/oauth/token", form: { + client_id: ENV["HCA_CLIENT_ID"], client_secret: ENV["HCA_CLIENT_SECRET"], + redirect_uri:, code:, grant_type: "authorization_code" + }) + token_data = JSON.parse(response.body.to_s) + access_token = token_data["access_token"] if token_data.is_a?(Hash) + return if access_token.nil? + + hca_data = HCAService.me(access_token) + hca_data.dig("identity", "id") if hca_data.is_a?(Hash) end def slack_authorize_url(redirect_uri, state: nil, close_window: false, continue_param: nil) diff --git a/app/models/deletion_request.rb b/app/models/deletion_request.rb index 425d20f8d..d6016c069 100644 --- a/app/models/deletion_request.rb +++ b/app/models/deletion_request.rb @@ -1,4 +1,7 @@ class DeletionRequest < ApplicationRecord + MAX_REASON_LENGTH = 100 + MAX_REASON_DETAILS_LENGTH = 1_000 + belongs_to :user belongs_to :admin_approved_by, class_name: "User", optional: true @@ -6,7 +9,9 @@ class DeletionRequest < ApplicationRecord validates :requested_at, presence: true validates :reason, presence: true, if: -> { reason_details.present? } + validates :reason, length: { maximum: MAX_REASON_LENGTH } validates :reason_details, presence: true, if: -> { reason.present? } + validates :reason_details, length: { maximum: MAX_REASON_DETAILS_LENGTH } validate :user_not_banned_from_deletion, on: :create scope :active, -> { where(status: [ :pending, :approved ]) } diff --git a/config/routes.rb b/config/routes.rb index cc31b8611..9f4b044d2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -223,6 +223,7 @@ def matches?(request) get "deletion", to: "deletion_requests#show", as: :deletion post "deletion", to: "deletion_requests#create", as: :create_deletion delete "deletion", to: "deletion_requests#cancel", as: :cancel_deletion + get "deletion/hca/callback", to: "deletion_requests#hca_callback", as: :hca_deletion_callback get "setup", to: "users#setup", as: :setup get "my/wakatime_setup", to: redirect("/setup") diff --git a/test/controllers/deletion_requests_controller_test.rb b/test/controllers/deletion_requests_controller_test.rb new file mode 100644 index 000000000..98e6089d4 --- /dev/null +++ b/test/controllers/deletion_requests_controller_test.rb @@ -0,0 +1,187 @@ +require "test_helper" +require "webmock/minitest" + +class DeletionRequestsControllerTest < ActionDispatch::IntegrationTest + setup do + @user = User.create!(timezone: "UTC") + sign_in_as(@user) + end + + test "create immediately creates a request when HCA is not linked" do + assert_difference "DeletionRequest.count", 1 do + post create_deletion_path, params: deletion_params + end + + assert_redirected_to deletion_path + end + + test "create requires HCA step-up when HCA is linked" do + @user.update!(hca_id: "hca-linked") + + assert_no_difference "DeletionRequest.count" do + post create_deletion_path, params: deletion_params + end + + authorize_uri = URI.parse(response.location) + authorize_query = Rack::Utils.parse_query(authorize_uri.query) + assert_equal "#{HCAService.host}/oauth/authorize", "#{authorize_uri.scheme}://#{authorize_uri.host}#{authorize_uri.path}" + assert_equal "login", authorize_query["prompt"] + assert_includes authorize_query["scope"].split, "openid" + assert_equal session.dig(:pending_deletion_request, "state"), authorize_query["state"] + assert_equal @user.id, session.dig(:pending_deletion_request, "user_id") + assert_equal "hca-linked", session.dig(:pending_deletion_request, "hca_id") + assert_equal deletion_params[:deletion_request][:reason], session.dig(:pending_deletion_request, "attributes", "reason") + end + + test "create rejects deletion details that cannot fit in the session" do + @user.update!(hca_id: "hca-linked") + + post create_deletion_path, params: { + deletion_request: deletion_params[:deletion_request].merge(reason_details: "a" * 10_000) + } + + assert_redirected_to my_settings_path + assert_equal "Deletion details are too long.", flash[:alert] + assert_nil session[:pending_deletion_request] + end + + test "HCA callback creates the pending request for the linked identity" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + stub_hca_identity("hca-linked") + + assert_difference "DeletionRequest.count", 1 do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to deletion_path + assert_equal deletion_params[:deletion_request][:reason], DeletionRequest.last.reason + assert_nil session[:pending_deletion_request] + end + + test "HCA callback rejects a different identity" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + stub_hca_identity("hca-someone-else") + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + end + + test "HCA callback handles an invalid response" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + stub_request(:post, "#{HCAService.host}/oauth/token").to_return(body: "not json") + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + assert_equal "Hack Club Auth verification failed. Please try again.", flash[:alert] + end + + test "HCA callback handles a connection failure" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + stub_request(:post, "#{HCAService.host}/oauth/token") + .to_raise(HTTP::ConnectionError.new("HCA unavailable")) + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + assert_equal "Hack Club Auth verification failed. Please try again.", flash[:alert] + end + + test "HCA callback rejects an invalid state" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: "wrong-state" } + end + + assert_redirected_to my_settings_path + assert_nil session[:pending_deletion_request] + end + + test "HCA callback cannot be replayed" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + stub_hca_identity("hca-linked") + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + end + + test "HCA callback rejects a changed linked identity" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + @user.update!(hca_id: "hca-changed") + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + end + + test "HCA callback rechecks deletion eligibility" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + state = session.dig(:pending_deletion_request, "state") + DeletionRequest.create_for_user!(@user) + stub_hca_identity("hca-linked") + + assert_no_difference "DeletionRequest.count" do + get hca_deletion_callback_path, params: { code: "step-up-code", state: state } + end + + assert_redirected_to my_settings_path + end + + test "HCA error must include the pending state" do + @user.update!(hca_id: "hca-linked") + post create_deletion_path, params: deletion_params + + get hca_deletion_callback_path, params: { error: "access_denied", state: "wrong-state" } + + assert_redirected_to my_settings_path + assert_equal "Hack Club Auth verification failed. Please try again.", flash[:alert] + end + + private + + def deletion_params + { + deletion_request: { + reason: "Something else", + reason_details: "I no longer need my account" + } + } + end + + def stub_hca_identity(hca_id) + stub_request(:post, "#{HCAService.host}/oauth/token") + .with(body: hash_including("code" => "step-up-code", "redirect_uri" => hca_deletion_callback_url)) + .to_return(body: { access_token: "step-up-token" }.to_json) + stub_request(:get, "#{HCAService.host}/api/v1/me") + .with(headers: { "Authorization" => "Bearer step-up-token" }) + .to_return(body: { identity: { id: hca_id } }.to_json) + end +end