Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Restore coverage for the active handler’s key behavior before removing the existing integration tests.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Removes the unused legacy POST /v1/key-data endpoint while preserving /v1/account/scoped-key-data.
Changes:
- Removes the legacy route and unused imports.
- Removes its Swagger documentation.
- Removes obsolete tests and the unused client constant.
- Handler behavior coverage should be moved to the replacement route before approval.
| File | Summary |
|---|---|
packages/fxa-auth-server/test/remote/oauth_api.in.spec.ts |
Removes legacy endpoint tests and constant; existing handler behavior coverage must be adapted to the replacement route. |
packages/fxa-auth-server/lib/routes/oauth/key_data.js |
Removes the legacy route. |
packages/fxa-auth-server/docs/swagger/oauth-server-api.ts |
Removes legacy endpoint documentation. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
## Because - `POST /v1/key-data` is a legacy alias of `POST /v1/account/scoped-key-data`. Both routes run the same `keyDataHandler`. - No FxA client calls `/v1/key-data`. We checked fxa-auth-client, fxa-settings, fxa-content-server and PyFxA. ## This pull request - Removes the `POST /v1/key-data` route and two orphaned imports from `key_data.js`. `POST /v1/account/scoped-key-data` and `keyDataHandler` do not change. - Removes the `KEY_DATA_POST` swagger entry and its overview link from `oauth-server-api.ts`. - Moves the `POST /key-data` integration tests in `oauth_api.in.spec.ts` to `POST /account/scoped-key-data`. They use session-token credentials. The bad-assertion case is now an invalid-session-token case, because the route does not accept `assertion`. - Sets `oauth.secretKey` in `test/lib/server.ts`, so that the in-process oauth server can verify the assertions that the route signs. ## Issue that this pull request solves Closes: https://mozilla-hub.atlassian.net/browse/FXA-14618
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
POST /v1/key-dataroute is unused. It is an old alias ofPOST /v1/account/scoped-key-dataand uses the samekeyDataHandler.fxa-auth-client,fxa-settings,fxa-content-serverand PyFxA.This pull request
POST /v1/key-dataroute fromkey_data.js, plus two orphaned imports.KEY_DATA_POSTswagger entry and its overview link fromoauth-server-api.ts.POST /key-datatest block and the orphanedBAD_CLIENT_IDconst fromoauth_api.in.spec.ts.POST /v1/account/scoped-key-dataorkeyDataHandler.Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-14618
Checklist
Put an
xin the boxes that applyHow to review (Optional)
key_data.jskey_data.js,oauth-server-api.ts,oauth_api.in.spec.tsPOST /v1/key-datanow gets a 404.Screenshots (Optional)
Other information (Optional)
Local checks:
npx jest lib/routes/oauth/index.spec.ts: 8 passed, 0 failed. This covers/account/scoped-key-data.npx nx lint fxa-auth-server: exit 0.tscshows no errors in the changed files.git grepfinds no remaining references.*.in.spec.tstests or the functional tests locally. CI runs them.Follow-ups, out of scope:
keyDataHandlerbranches: multiple scopes, unknown client, disallowed scopes, and thefxa-keysChangedAt/fxa-generationfallback. A follow-up can move these tests to/account/scoped-key-data.