Fix: group-editing authorization dead-ends on Approve - #12
Merged
Conversation
Approving the group-editing grant could end at "Authorization failed. Tell a coach." The callback decided which of the two per-person grants had come back by reading a `metadata` marker put into the OAuth state. When that marker did not survive the round trip, the callback fell through to the enrolment branch, where the scope guard correctly refused to overwrite a mentor's DM token with one that cannot read DMs — and the refusal took the whole authorization down with it. The guard was right. Routing on a marker was not: it made one hint a single point of failure for a flow whose failure mode is a dead end. Grants are now told apart by what the token can actually do. A token carrying `usergroups:write` and no DM history scope cannot be an enrolment, whatever the state parameter says. The marker is still honoured when present, and the scope guard stays as the backstop it was meant to be rather than the tripwire it became. Verified: with the marker absent, a group grant now lands in the admin row and the mentor's DM token is untouched; a normal enrolment still stores as one; and a genuinely degraded enrolment token is still refused. The failure page also stops being a dead end. An authorization link is valid for ten minutes, and expiring is both the most likely failure and the only one the person reading the page can fix, so it now says so and tells them to run the command again. Everything else names the error code to quote to a coach, instead of asking them to report that something, somewhere, went wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
This is the commit that missed the #11 merge window — it was pushed a few minutes after #11 was merged, so it never made it into
main. It's the fix for the failure actually reported: clicking Approve on the group-editing authorization and landing on "Authorization failed. Tell a coach."Everything else from #11 (the
@studentslookup, channel names) is already inmain. This is the remaining piece.What went wrong
The OAuth callback decided which of the two per-person grants had come back by reading a
metadatamarker placed in the OAuth state. When that marker didn't survive the round trip, the callback fell through to the enrolment branch — where the scope guard correctly refused to overwrite a mentor's DM token with one that cannot read DMs. The refusal was right, and it took the whole authorization down with it.The guard wasn't the mistake. Routing on a marker was: it made one hint a single point of failure for a flow whose failure mode is a dead end.
The fix
Grants are told apart by what the token can actually do. A token carrying
usergroups:writeand no DM-history scope cannot be an enrolment, whatever the state parameter claims. The marker is still honoured when present, and the scope guard goes back to being the backstop it was meant to be rather than the tripwire it became.Verified against the store directly:
The failure page stops being a dead end
Authorization links are valid for ten minutes. Expiring is both a likely failure and the only one the person reading the page can fix, so it now says exactly that and tells them to run the command again. Every other failure names the error code to quote to a coach, instead of asking them to report that something, somewhere, went wrong.
This matters for diagnosis: an expired link and a lost marker produced an identical dead end, which is why the original report couldn't be pinned down without server logs.
npm run typecheck && npm test && npm run format:check && npm run build— 103 tests, all green.