[Mohamad-Mortada]

Summary of the problem

We want to know who controls what Doorkeeper app inorder to contact them for stuff like migration.

Describe your changes

Added a column you must enter while creating the oauth app

[Luke-Oldenburg]

@claude review

[claude]

Claude finished @Luke-Oldenburg's task in 34s —— View job

---

Review of #15520: assign a POC to Doorkeeper OAuth applications

• Read the diff and surrounding code
• Post the review

I only read the code. I didn't run the specs or the app.

Overall: The approach is sound. It uses Doorkeeper's built-in enable_application_owner confirmation: true, a custom controller, and an email lookup in the form. There are a few things to fix or confirm before merging.

Issues

1. Existing apps have no owner (db/migrate/20261102120000_add_owner_to_oauth_applications.rb).

   • The migration adds nullable owner_id and owner_type columns with no backfill. With confirmation: true, every existing app now fails validation.
   • Editing an old app, rotating its secret, or updating it from the admin UI will fail until someone sets a POC. The edit form handles this, since it requires the email.
   • Any code or console script that saves an existing app, such as app.update(...) or app.renew_secret, will fail too.
   • Please confirm this is intended, or backfill existing apps in a data migration or maintenance task. That gives you an owner for the migration outreach this PR is meant to enable.
   • Also check that no other code paths create or save Doorkeeper::Application records, for example seeds, tasks, or lib.

2. The email lookup is silent and hard to debug (app/controllers/oauth_applications_controller.rb:8).

   • User.find_by(email: ...) returns nil for a typo or unknown email, so owner is nil. The user then sees a generic "can't be blank" error on the POC field with no hint that the email wasn't found.
   • Consider an explicit error such as "No HCB user with that email".
   • Also consider normalizing the input with .to_s.strip.downcase. Check whether User.email is citext or has its own normalization, and match that.
   • application_params also runs on update. If an edit omits owner_email, owner is merged as nil and wipes the owner. This is fine for the form, which prefills the email, but it's fragile. You could merge owner only when owner_email is present.

3. Extra database lookup (oauth_applications_controller.rb:7-8). application_params is called more than once per request in Doorkeeper's create and update flow, so User.find_by may run several times. Memoizing it is cheap.

4. Consider a plain belongs_to instead of a polymorphic owner.

   • The migration uses polymorphic: true. That matches Doorkeeper's defaults, so it's fine if you want to stay on the stock behavior.
   • If this should always be a User, you can get a real foreign key with a User-typed reference. That is a design choice, not a bug.
   • If you keep it polymorphic, Doorkeeper::Application#owner may need owner_type set to "User". Your spec asserts owner == poc, which covers this for the happy path.

Smaller notes

• skip_after_action :verify_authorized: This is needed because controller_path is oauth_applications, which doesn't match the doorkeeper/ prefix exclusion in ApplicationController:70. It's fine, but add a short comment on why. Access control relies entirely on admin_authenticator, which is unchanged.
• Copied view templates: _form, index, and show are full copies of Doorkeeper's views. They will drift on gem upgrades. Only the POC field and column differ, so this is acceptable.
• Test coverage: The request spec covers create, a missing POC, and the index. Add a case for update, to pin down the owner-wiping behavior in Gsuite #2, and one for the show page.
• Spec factory changes: The changes to the other specs (donations, user, locked user, impersonation, revoke task) just add owners. That's correct.
• Branch hygiene: My diff against the PR's earlier history was noisy (Ruby/Node bumps, Gemfile changes, Herb config, JS popover changes). I couldn't get a clean origin/main...HEAD diff because there is no merge base in this checkout. Please confirm the merge from main in d3f9ccf didn't pull in unrelated changes. The 16-file list in the PR looks right.
• Migration: disable_ddl_transaction! plus a concurrent index is the right pattern for strong_migrations.

Suggested next steps

1. Decide on a backfill for existing apps (WIP Credit cards #1).
2. Add a clearer error for an unknown email, and don't clobber the owner on updates (Gsuite #2).
3. Add an update spec.

[Mohamad-Mortada]

@claude review

[claude]

Claude finished @Mohamad-Mortada's task in 14s —— View job

---

Review of #15520: assign a POC to Doorkeeper OAuth applications

I only read the code. I didn't run the specs or the app. I also couldn't get a clean origin/main...HEAD diff because this checkout has no merge base. This review covers the current state of the branch files, including the latest commit (8f28156).

Overall: The latest commit addressed most of my earlier feedback. This looks close to mergeable.

Addressed since the last review

• Owner wiping on update: application_params now returns super when owner_email is absent (app/controllers/oauth_applications_controller.rb:9). A spec pins this down.
• Repeated lookup: The owner is memoized in @owner.
• Unknown email: The spec expects the message "Must be the email of an existing user."
• Test coverage: There are now specs for update, show, index, create, and the missing POC.
• Comment on skip_after_action: Added.

Remaining items

1. Existing apps have no owner (db/migrate/20261102120000_add_owner_to_oauth_applications.rb).
   • The columns are nullable and nothing is backfilled. With confirmation: true, any existing app fails validation on save. That includes secret rotation and console updates.
   • Please confirm this is intended, or add a maintenance task to assign owners. The PR's stated goal is contacting owners for migration, so you will need that data for old apps anyway.
   • Also check that seeds, rake tasks and lib don't create or save Doorkeeper::Application records without an owner.
2. Edge case in application_params. If owner_email is submitted blank, find_by(email: "") returns nil and the app gets the "must be an existing user" error. That is the right behavior. If emails are stored in a different case, though, find_by(email: ...) is an exact match, so a differently cased entry would be rejected. Consider .to_s.strip.downcase, or match whatever User normalizes to.
3. Polymorphic owner. polymorphic: true matches Doorkeeper's default, so it's fine. If the owner should always be a User, a plain belongs_to-style foreign key would be stricter. This is a design choice, not a bug.

Smaller notes

• The copied _form, index and show views will drift from the gem on upgrades. Only the POC field and column differ, so this is acceptable.
• The spec changes in the donations, user, locked user, impersonation and revoke task specs only add owners. That's correct.
• disable_ddl_transaction! plus a concurrent index is the right pattern for strong_migrations.
• Branch hygiene: Please confirm the merge from main (d3f9ccf) didn't pull in unrelated changes. The 16-file list looks right.

Suggested next steps

1. Decide on a backfill for existing apps (WIP Credit cards #1).
2. Optionally normalize the email lookup (Gsuite #2).