chore(auth): remove dead code related to /certificate/sign#20207
chore(auth): remove dead code related to /certificate/sign#20207
Conversation
| // For desktop, the 'service' parameter for this event gets | ||
| // special-cased to 'sync' so that it matches its pre-oauth | ||
| // `/certificate/sign` event. | ||
| // special-cased to 'sync' for historical continuity with the pre-oauth flow. |
There was a problem hiding this comment.
Is this still true? Or maybe we need added context here? It's almost more confusing to me not to leave the certificate/sign comment there since historically that is what we were trying to match parity with (I think).
There was a problem hiding this comment.
@LZoog maybe other team members have more context?
I just approached logically -- if service is special-cased to match a pre-oauth, /certificate/sign event -- and then we remove the /certificate/sign, service is still special cased for a historic pre-oauth flow, and certificate/sign is no longer relevant...
Happy to defer to you on suggested comment change or removing altogether!
There was a problem hiding this comment.
Maybe the original comment is also confusing to me. When we say "pre-oauth", since Desktop just moved to the oauth flow a year ago, that's what I think about, but if I look back through git history it must be referencing... mobile?? I'd have to go look again. I've also seen some other comments in our code about changing service to our client_id or vice versa, not sure.
Like, /certificate/sign is no longer relevant yes, but I imagine this pre-oauth flow is as well, I'm not sure if that's fx_ios_v1 context or what, but we always tell the user they have to upgrade Firefox before they can continue there, and so are we just still special casing because of metrics continuity or something? In which case, we can/should eventually move away from it?
Anyway, maybe we simply modify it to be // `/certificate/sign` event (which is deprecated)? I'll let you make the call 🙂 I guess either way we can remove CERTIFICATE_SIGN_DISABLE_ROLLOUT_RATE from webservices infra if that's still set over there.
There was a problem hiding this comment.
Updated with a clearer comment in parentheses, preserving the original -- thanks!
toufali
force-pushed
the
remove-certificate-sign-ref
branch
from
153b76b to
9632c0b
Compare
There was a problem hiding this comment.
Pull request overview
Removes lingering references to the long-gone /certificate/sign endpoint in the auth-server codebase (metrics/config), aligning the implementation with the endpoint’s removal and reducing dead configuration surface area.
Changes:
- Removed
/certificate/signfrom the route-flow-event ignore list. - Deleted the obsolete
certificateSignDisableRolloutRateconvict config option. - Updated an inline comment referencing the historical
/certificate/signevent mapping.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/routes/oauth/token.js | Updates an inline comment around the legacy metrics mapping for old-sync flows. |
| packages/fxa-auth-server/lib/metrics/events.js | Removes /certificate/sign from ignored route flow event paths. |
| packages/fxa-auth-server/config/index.ts | Removes unused config option tied to certificate signing rollout disablement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
toufali
force-pushed
the
remove-certificate-sign-ref
branch
from
9632c0b to
0234c13
Compare
Because
This pull request
Issue that this pull request solves
Closes: FXA-13290