From db6139804a81099abd9bfeddbbe9fa610b983ea9 Mon Sep 17 00:00:00 2001 From: Leo Wilkin Date: Mon, 19 Oct 2026 03:39:45 +0100 Subject: [PATCH 1/2] link persona inquiries staff create by hand in the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit for account recovery, staff make an inquiry in the persona dashboard (reference id = the identity's public id) and send the user the hosted link directly. until now the webhooks for those inquiries matched nothing and were dropped. inquiry.created now carries the creator through: HCA's own inquiries say "API" and are ignored; anything else is matched against active super admins / manual document verifiers, and the inquiry becomes a draft PersonaVerification on the identity so the usual webhooks drive it. these never auto-approve — persona passing a document doesn't show the person owns the account — so they always land in manual review. --- .../webhooks/persona_controller.rb | 20 +++++ app/jobs/persona/link_staff_inquiry_job.rb | 77 +++++++++++++++++++ app/jobs/persona/verification_pipeline_job.rb | 5 +- .../verification/persona_verification.rb | 9 ++- .../persona/link_staff_inquiry_job_spec.rb | 76 ++++++++++++++++++ .../persona/verification_pipeline_job_spec.rb | 35 +++++++++ spec/requests/webhooks/persona_spec.rb | 48 ++++++++++++ 7 files changed, 268 insertions(+), 2 deletions(-) create mode 100644 app/jobs/persona/link_staff_inquiry_job.rb create mode 100644 spec/jobs/persona/link_staff_inquiry_job_spec.rb create mode 100644 spec/jobs/persona/verification_pipeline_job_spec.rb diff --git a/app/controllers/webhooks/persona_controller.rb b/app/controllers/webhooks/persona_controller.rb index 12ba48d9..71d9c634 100644 --- a/app/controllers/webhooks/persona_controller.rb +++ b/app/controllers/webhooks/persona_controller.rb @@ -16,6 +16,7 @@ def create inquiry_id = parsed_body.dig(:data, :attributes, :payload, :data, :id) return head(:bad_request) if event_name.blank? || inquiry_id.blank? + return link_staff_inquiry(inquiry_id) if event_name == "inquiry.created" return head(:ok) unless HANDLED_EVENTS.include?(event_name) Persona::ProcessInquiryEventJob.perform_later( @@ -28,6 +29,25 @@ def create private + # inquiries we create through the API have creator "API"; anything else is + # the email of whoever made it by hand in the persona dashboard — see + # LinkStaffInquiryJob + def link_staff_inquiry(inquiry_id) + attributes = parsed_body.dig(:data, :attributes, :payload, :data, :attributes) || {} + creator = attributes[:creator] + reference_id = attributes[:"reference-id"] + + if creator.present? && creator != "API" && reference_id.present? + Persona::LinkStaffInquiryJob.perform_later( + inquiry_id: inquiry_id, + creator: creator, + reference_id: reference_id + ) + end + + head :ok + end + def verify_signature! signature_header = request.headers["Persona-Signature"] return head(:unauthorized) if signature_header.blank? diff --git a/app/jobs/persona/link_staff_inquiry_job.rb b/app/jobs/persona/link_staff_inquiry_job.rb new file mode 100644 index 00000000..afdfeb54 --- /dev/null +++ b/app/jobs/persona/link_staff_inquiry_job.rb @@ -0,0 +1,77 @@ +# links an inquiry that staff created by hand in the persona dashboard to +# the identity named in its reference id. +# +# this is the account recovery path: the user can't sign in or read their +# email, so staff make an inquiry with reference id = the identity's public +# id and send them persona's hosted link directly. persona's inquiry.created +# webhook carries the dashboard user's email as `creator` — only active +# reviewers count. once linked, the inquiry is an ordinary PersonaVerification +# and the usual completed/approved webhooks drive it, except it never +# auto-approves (see PersonaVerification::STAFF_CREATED_ISSUE). +class Persona::LinkStaffInquiryJob < ApplicationJob + queue_as :default + + PERSONA_TYPES = %w[Verification::PersonaVerification Verification::PersonaStudentIdVerification].freeze + + def perform(inquiry_id:, creator:, reference_id:) + return if Verification.exists?(persona_inquiry_id: inquiry_id) + return if VerificationCase.exists?(persona_inquiry_id: inquiry_id) + + Sentry.set_tags(component: "persona", event: "inquiry.created") + Sentry.set_extras(inquiry_id: inquiry_id, reference_id: reference_id) + + staff = staff_user_for(creator) + unless staff + Sentry.capture_message("[Persona] dashboard inquiry created by someone who isn't an active reviewer", level: :warning) + return + end + + identity = Identity.find_by_public_id(reference_id) + unless identity + Sentry.capture_message("[Persona] dashboard inquiry reference id doesn't match an identity", level: :warning) + return + end + + stale_inquiry_ids = [] + verification = identity.with_lock do + # something already under review — don't stack a second submission on it + if identity.verifications.where(type: PERSONA_TYPES, status: :pending).exists? + Sentry.capture_message("[Persona] dashboard inquiry for an identity with a pending verification", level: :warning) + nil + else + # a draft would shadow this one when the user next signs in, and its + # inquiry is stale anyway — same cleanup the verification flow does + identity.verifications.where(type: PERSONA_TYPES, status: :draft).find_each do |draft| + stale_inquiry_ids << draft.persona_inquiry_id if draft.persona_inquiry_id.present? + draft.destroy! + end + + Verification::PersonaVerification.create!( + identity: identity, + persona_inquiry_id: inquiry_id, + issues: [ Verification::PersonaVerification::STAFF_CREATED_ISSUE ] + ) + end + end + return unless verification + + stale_inquiry_ids.each do |stale_id| + Persona.instance.expire_inquiry(stale_id) + rescue Persona::APIError => e + Sentry.capture_exception(e) + end + + verification.create_activity(:persona_inquiry_linked_by_staff, owner: staff, recipient: identity, + parameters: { inquiry_id: inquiry_id, expired_inquiry_ids: stale_inquiry_ids }) + end + + private + + def staff_user_for(creator) + Backend::User.joins(:identity) + .where(active: true) + .where("lower(identities.primary_email) = ?", creator.to_s.strip.downcase) + .where("backend_users.super_admin OR backend_users.manual_document_verifier") + .first + end +end diff --git a/app/jobs/persona/verification_pipeline_job.rb b/app/jobs/persona/verification_pipeline_job.rb index 007a5eb4..b10110bd 100644 --- a/app/jobs/persona/verification_pipeline_job.rb +++ b/app/jobs/persona/verification_pipeline_job.rb @@ -31,7 +31,10 @@ def run_decisioning case verdict when :approved if !@verification.auto_approvable? - @verification.update(issues: @verification.issues + [ "Student ID — requires manual review" ]) + # staff-created inquiries already carry their own issue + unless @verification.try(:staff_created_inquiry?) + @verification.update(issues: @verification.issues + [ "Student ID — requires manual review" ]) + end Slack::NotifyReviewQueueJob.perform_later(@verification) elsif @record&.birthdate && Identity.calculate_age(@record.birthdate) < 13 @verification.update(issues: @verification.issues + [ "Under 13 — requires manual review" ]) diff --git a/app/models/verification/persona_verification.rb b/app/models/verification/persona_verification.rb index 84f85426..29b42598 100644 --- a/app/models/verification/persona_verification.rb +++ b/app/models/verification/persona_verification.rb @@ -106,9 +106,16 @@ def relevant_record = persona_record def needs_break_glass? = true def auto_break_glass_reason = pending? ? "to review submission" : nil def nukeable? = draft? || pending? - def auto_approvable? = true + def auto_approvable? = !staff_created_inquiry? def status_pending_partial = "verifications/status/pending_persona" + # set by Persona::LinkStaffInquiryJob. a hand-made inquiry is an account + # recovery: persona passing the document says nothing about whether the + # person owns this account, so a human always makes the call. + STAFF_CREATED_ISSUE = "Inquiry created by staff in the Persona dashboard — requires manual review" + + def staff_created_inquiry? = issues.include?(STAFF_CREATED_ISSUE) + def inquiry_unlinked? return false unless persona_inquiry_id.present? && persona_record_id.present? persona_record&.inquiry_id != persona_inquiry_id diff --git a/spec/jobs/persona/link_staff_inquiry_job_spec.rb b/spec/jobs/persona/link_staff_inquiry_job_spec.rb new file mode 100644 index 00000000..1910ec2c --- /dev/null +++ b/spec/jobs/persona/link_staff_inquiry_job_spec.rb @@ -0,0 +1,76 @@ +require "rails_helper" + +RSpec.describe Persona::LinkStaffInquiryJob, type: :job do + let(:identity) { create(:identity) } + let(:reviewer_identity) { create(:identity, primary_email: "reviewer@hackclub.com") } + let!(:reviewer) { create(:backend_user, identity: reviewer_identity, manual_document_verifier: true) } + let(:inquiry_id) { "inq_dashboard123" } + let(:mock_service) { instance_double(Persona::APIService) } + + before { allow(Persona).to receive(:instance).and_return(mock_service) } + + def perform(creator: "reviewer@hackclub.com", reference_id: identity.public_id) + described_class.perform_now(inquiry_id: inquiry_id, creator: creator, reference_id: reference_id) + end + + it "creates a draft persona verification flagged for manual review" do + expect { perform }.to change(Verification::PersonaVerification, :count).by(1) + + verification = identity.verifications.last + expect(verification).to be_draft + expect(verification.persona_inquiry_id).to eq(inquiry_id) + expect(verification).to be_staff_created_inquiry + expect(verification).not_to be_auto_approvable + end + + it "records which reviewer linked it" do + perform + + activity = PublicActivity::Activity.where("key LIKE ?", "%persona_inquiry_linked_by_staff").sole + expect(activity.owner).to eq(reviewer) + expect(activity.parameters[:inquiry_id]).to eq(inquiry_id) + end + + it "matches the creator email case-insensitively" do + expect { perform(creator: "Reviewer@HackClub.com") }.to change(Verification::PersonaVerification, :count).by(1) + end + + it "ignores creators who aren't backend users" do + expect { perform(creator: "someone@example.com") }.not_to change(Verification, :count) + end + + it "ignores backend users without review rights" do + reviewer.update!(manual_document_verifier: false) + expect { perform }.not_to change(Verification, :count) + end + + it "ignores deactivated backend users" do + reviewer.deactivate! + expect { perform }.not_to change(Verification, :count) + end + + it "ignores a reference id that isn't an identity" do + expect { perform(reference_id: "ident!nope") }.not_to change(Verification, :count) + end + + it "is a no-op when the inquiry is already linked" do + create(:persona_verification, identity: identity, persona_inquiry_id: inquiry_id) + expect { perform }.not_to change(Verification, :count) + end + + it "leaves a pending verification alone" do + record = create(:identity_persona_record, identity: identity) + create(:persona_verification, :pending, identity: identity, persona_record: record, persona_inquiry_id: record.inquiry_id) + expect { perform }.not_to change(Verification, :count) + end + + it "replaces a stale draft and expires its inquiry" do + stale = create(:persona_verification, identity: identity) + expect(mock_service).to receive(:expire_inquiry).with(stale.persona_inquiry_id) + + perform + + expect(Verification.exists?(stale.id)).to be(false) + expect(identity.verifications.sole.persona_inquiry_id).to eq(inquiry_id) + end +end diff --git a/spec/jobs/persona/verification_pipeline_job_spec.rb b/spec/jobs/persona/verification_pipeline_job_spec.rb new file mode 100644 index 00000000..cf7c4e2b --- /dev/null +++ b/spec/jobs/persona/verification_pipeline_job_spec.rb @@ -0,0 +1,35 @@ +require "rails_helper" + +RSpec.describe Persona::VerificationPipelineJob, type: :job do + let(:identity) { create(:identity) } + let(:record) { create(:identity_persona_record, identity: identity) } + + before { allow(ResemblanceNoticerEngine).to receive(:run) } + + def pending_verification(**attrs) + create(:persona_verification, :pending, identity: identity, persona_record: record, persona_inquiry_id: record.inquiry_id, **attrs) + end + + context "with a staff-created inquiry" do + let(:verification) { pending_verification(issues: [ Verification::PersonaVerification::STAFF_CREATED_ISSUE ]) } + + it "sends it to manual review even when decisioning approves" do + expect { + described_class.perform_now(verification) + }.to have_enqueued_job(Slack::NotifyReviewQueueJob) + + verification.reload + expect(verification).to be_pending + expect(verification.issues).to eq([ Verification::PersonaVerification::STAFF_CREATED_ISSUE ]) + end + end + + context "with an ordinary inquiry" do + let(:verification) { pending_verification } + + it "still auto-approves" do + described_class.perform_now(verification) + expect(verification.reload).to be_approved + end + end +end diff --git a/spec/requests/webhooks/persona_spec.rb b/spec/requests/webhooks/persona_spec.rb index b22e05e5..65a5ce6f 100644 --- a/spec/requests/webhooks/persona_spec.rb +++ b/spec/requests/webhooks/persona_spec.rb @@ -237,6 +237,54 @@ def sign_payload(body, secret: webhook_secret, timestamp: Time.now.to_i) end end + context "with inquiry.created" do + def created_payload(creator:) + { + data: { + attributes: { + name: "inquiry.created", + payload: { + data: { + id: "inq_dashboard123", + type: "inquiry", + attributes: { status: "created", "reference-id": identity.public_id, creator: creator } + } + } + } + } + }.to_json + end + + def post_signed(body) + post "/webhooks/persona", + params: body, + headers: { "Content-Type" => "application/json", "Persona-Signature" => sign_payload(body) } + end + + it "enqueues LinkStaffInquiryJob when a dashboard user created the inquiry" do + body = created_payload(creator: "reviewer@hackclub.com") + + expect { post_signed(body) }.to have_enqueued_job(Persona::LinkStaffInquiryJob).with( + inquiry_id: "inq_dashboard123", creator: "reviewer@hackclub.com", reference_id: identity.public_id + ) + expect(response).to have_http_status(:ok) + end + + it "ignores API-created inquiries" do + body = created_payload(creator: "API") + + expect { post_signed(body) }.not_to have_enqueued_job + expect(response).to have_http_status(:ok) + end + + it "ignores inquiries without a creator" do + body = created_payload(creator: nil) + + expect { post_signed(body) }.not_to have_enqueued_job + expect(response).to have_http_status(:ok) + end + end + context "with multiple signatures (key rotation)" do it "accepts if any v1 signature is valid" do valid_sig = sign_payload(inquiry_completed_payload) From 4113fb68368dacadf1579a8ec57c3510526cdc04 Mon Sep 17 00:00:00 2001 From: Leo Wilkin Date: Mon, 19 Oct 2026 03:46:08 +0100 Subject: [PATCH 2/2] get lint and brakeman green again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit both were already failing on main: - rubocop: missing spaces inside the array brackets in backup_code.rb - brakeman: bin/brakeman runs --ensure-latest, so 8.1.0 coming out failed every PR. bumped to 8.1.0, which raised two new warnings: - a weak XSS on the email typo banner. it was already escaped, but switching to an _html translation key lets rails do the escaping and drops the manual html_escape + html_safe - rails 8.0 EOL on 2026-11-07. ignored with a note — the 8.1 upgrade needs its own PR before then --- Gemfile.lock | 2 +- app/models/identity/backup_code.rb | 2 +- app/views/identities/new.html.erb | 2 +- config/brakeman.ignore | 19 +++++++++++++++++++ config/locales/en.yml | 2 +- 5 files changed, 23 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index eb316d54..9e8a35a5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -147,7 +147,7 @@ GEM block_cipher_kit (0.0.4) bootsnap (1.23.0) msgpack (~> 1.2) - brakeman (8.0.6) + brakeman (8.1.0) racc browser (6.2.0) builder (3.3.0) diff --git a/app/models/identity/backup_code.rb b/app/models/identity/backup_code.rb index d8c1b6ba..1d4e4fc3 100644 --- a/app/models/identity/backup_code.rb +++ b/app/models/identity/backup_code.rb @@ -27,7 +27,7 @@ class Identity::BackupCode < ApplicationRecord end end event :mark_discarded do - transitions from: [:active, :previewed], to: :discarded + transitions from: [ :active, :previewed ], to: :discarded end end diff --git a/app/views/identities/new.html.erb b/app/views/identities/new.html.erb index 34b18b28..14465ea9 100644 --- a/app/views/identities/new.html.erb +++ b/app/views/identities/new.html.erb @@ -34,7 +34,7 @@ - <%= t(".email_typo", suggested: ERB::Util.html_escape(@identity.suggested_email)).html_safe %> + <%= t(".email_typo_html", suggested: @identity.suggested_email) %>