Fixed oauth - #279
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Slack OpenID Connect implementation is missing required authorization/token parameters (response_type, nonce, and grant_type), which can break sign-in in production.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Slack sign-in flow by switching from legacy Slack OAuth v2 to Slack OpenID Connect endpoints, and streamlines the fantasy module by removing Sleeper account-linking functionality across backend and frontend.
Changes:
- Migrate Slack auth endpoints and payload expectations to Slack OpenID Connect (
/openid/connect/authorize,openid.connect.token,openid.connect.userInfo) and update tests/docs accordingly. - Remove Sleeper account-linking API/UI, including the
/fantasy/profileendpoint, hook logic, and related tests. - Update README Slack configuration guidance to reflect the OpenID Connect sign-in flow.
File summaries
| File | Description |
|---|---|
| README.md | Updates Slack setup instructions to describe OpenID Connect login scope/behavior. |
| packages/frontend/src/pages/FantasyPage.tsx | Removes Sleeper-linking UI and replaces it with a “linking unavailable” message. |
| packages/frontend/src/pages/FantasyPage.spec.tsx | Updates UI tests to reflect removal of account-linking. |
| packages/frontend/src/hooks/useFantasy.ts | Removes linkSleeperUser API call and exposed hook method. |
| packages/frontend/src/hooks/useFantasy.spec.ts | Removes tests covering the deleted account-linking hook behavior. |
| packages/backend/src/fantasy/fantasy.service.ts | Removes Sleeper-linking service method. |
| packages/backend/src/fantasy/fantasy.service.spec.ts | Removes unit tests for the deleted service method. |
| packages/backend/src/fantasy/fantasy.controller.ts | Removes the /profile route. |
| packages/backend/src/fantasy/fantasy.controller.spec.ts | Updates controller tests to confirm the linking endpoint is no longer exposed. |
| packages/backend/src/auth/auth.controller.ts | Switches to Slack OpenID Connect endpoints and adjusts token/userinfo handling. |
| packages/backend/src/auth/auth.controller.spec.ts | Updates auth tests to the OpenID Connect URLs/response shapes. |
| packages/backend/src/auth/auth.const.ts | Updates Slack auth constants to OpenID Connect URLs. |
Review details
Suppressed comments (2)
packages/backend/src/auth/auth.controller.ts:106
- Slack’s
openid.connect.tokenendpoint expectsgrant_type=authorization_codein the form body. Without it, the code exchange can fail even with a validcode.
new URLSearchParams({
client_id: clientId,
client_secret: clientSecret,
code,
redirect_uri: redirectUri,
}).toString(),
packages/backend/src/auth/auth.controller.spec.ts:100
- Once
grant_type=authorization_codeis added to the token exchange, this test should assert it’s present in the posted form body (otherwise the test won’t catch a broken OpenID token exchange payload).
expect(Axios.post).toHaveBeenCalledWith(
'https://slack.com/api/openid.connect.token',
expect.stringContaining('code=valid-code'),
{ headers: { 'Content-Type': 'application/x-www-form-urlencoded' } },
);
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The Slack OpenID token exchange is missing a required grant_type=authorization_code parameter, which can break authentication in production.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/backend/src/auth/auth.controller.ts:108
- Slack's OpenID Connect token exchange requires
grant_type=authorization_code. The current request omits it, which can cause the/openid.connect.tokencall to fail even with a valid code.
new URLSearchParams({
client_id: clientId,
client_secret: clientSecret,
code,
redirect_uri: redirectUri,
}).toString(),
packages/frontend/src/pages/FantasyPage.tsx:266
- This change removes the "Pending waivers" UI, but the PR description only mentions removing Sleeper account-linking and migrating Slack auth. If the pending-waivers removal is intentional, the PR description should call it out (and you may want to remove
pendingWaiversfrom the fantasy API/model to avoid computing/sending data that the frontend no longer renders).
<section>
<div className="mb-3 flex flex-wrap items-center justify-between gap-3">
<div>
<h2 className="flex items-center gap-2 text-lg font-semibold">
<UserPlus className="h-5 w-5 text-primary" aria-hidden="true" /> Waiver wire proposals
</h2>
<p className="mt-1 text-sm text-muted-foreground">Waivers process Wednesday and Sunday.</p>
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
|
|
||
| expect(blocks[0].text.text).toBe('AAPL :chart:'); | ||
| }); |
This pull request makes two major changes: it migrates Slack authentication from the legacy OAuth v2 flow to Slack's OpenID Connect flow, and it removes the backend and frontend support for linking Sleeper fantasy accounts. The authentication update improves security and simplifies the login process, while the removal of Sleeper account-linking endpoints and related code streamlines the fantasy module.
Slack Authentication Migration:
/oauth/v2/authorize) to OpenID Connect (/openid/connect/authorize) for user authentication, updating all relevant endpoints, parameters, and test cases to use the new flow and OpenID scopes. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12]Sleeper Account-Linking Removal:
/fantasy/profileendpoint and all backend logic for linking Sleeper accounts, including controller routes, service methods, and related tests. [1] [2] [3] [4] [5] [6] [7]linkSleeperUserhook and its usage/tests. [1] [2] [3] [4]Documentation Updates:
README.mdto clarify the new Slack authentication setup and to remove references to legacy scopes and flows. [1] [2]