fix(sso): OIDC JIT provisioning granted Participant on every competition - #129
Merged
Conversation
A just-in-time-provisioned SSO user was given the Participant role with
`competition_id=None`. That is the *site-wide* shape — `user_has_permission`
short-circuits on a NULL competition — so a single OIDC login granted
challenge_view, ticket_view and ticket_respond on every competition on the
install, with no join and no invite code.
It got worse downstream. `has_global_role` treated any unscoped assignment as
administrator-equivalent, which is what `_can_see` and `list_competitions` use
to decide private-competition visibility. `CompetitionOut` carries
`invite_code`, so `GET /api/competitions` returned every private competition on
the install together with its invite code.
The docstring claimed this mirrored public registration. It did not: local
registration creates a user with no role assignment at all, and earns
Participant per-competition via `ensure_participant_role` on join. The admin
API already refused to create this exact shape ("A competition-scoped role
needs a competition"), and ARCHITECTURE.md §7.5 already documented the
invariant — nothing enforced it.
Fixed in three layers, because one was clearly not enough:
- JIT provisioning grants no role assignment, matching local registration.
- Resolution requires the *role* to be `scope="global"` before an unscoped
assignment grants anything, in `user_has_permission`,
`effective_permissions` and `has_global_role`. A malformed row now grants
nothing instead of everything, however it got written.
- A migration deletes existing rows. Verified against real Postgres: the
unscoped Participant is removed, while a global Administrator and a
competition-scoped Participant are both preserved.
The existing test asserted the role *name* and passed throughout. The new
tests assert the assignment set is empty and that a JIT user cannot see a
private competition; both fail against the previous code, as does the
resolver test.
Migrations aren't covered by the suite (ADR-0006), so the migration was run
against a throwaway Postgres 16 rather than SQLite only.
Co-Authored-By: Claude <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
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.
Scope note — read first
No tagged release is affected. OIDC (#58) landed in the v1.2.0 milestone, which is not yet tagged;
git ls-tree v1.1.1 | grep oidcreturns nothing. This is exploitable only onmain/source builds, which is why it's an ordinary public PR rather than a security advisory.The bug
JIT provisioning gave every SSO user the Participant role with
competition_id=None. That's the site-wide shape —user_has_permissionshort-circuits on a NULL competition — so one OIDC login grantedchallenge_view,ticket_viewandticket_respondon every competition on the install, with no join and no invite code.It compounded downstream:
oidc_identity._assign_participantwritescompetition_id=Nonedeps.user_has_permission→if assignment_competition_id is None: return Truemembership.has_global_roletreats any unscoped row as administrator-equivalentcompetitions.list_competitionsshort-circuits its visibility filter on thatCompetitionOutcarriesinvite_codeNet effect:
GET /api/competitionsreturned every private competition on the install, with its invite code, to anyone who could authenticate at the configured IdP.The docstring claimed this mirrored public registration. It didn't —
routers/auth.py:186creates a registered user with no role assignment, and Participant is earned per-competition viaensure_participant_roleon join. The admin API already refused this exact shape ("A competition-scoped role needs a competition"), and ARCHITECTURE.md §7.5 already documented the invariant. Nothing enforced it.Fix — three layers
user_has_permission,effective_permissionsandhas_global_rolerequire the role to bescope="global"before an unscoped assignment grants anythingb3f7c21a9d04deletes existing rowsLayer 2 is the important one: a malformed row now grants nothing instead of everything, however it gets written — including by a restored backup.
Verification
assert not Truewhere the old resolver grantedchallenge_viewsite-wide. The pre-existing test asserted the role name and passed the entire time the scope was wrong.Operator impact
Installs running
mainwith SSO enabled have these rows today; the migration removes them on upgrade. Affected users lose nothing in practice — they regain Participant per-competition on join, exactly as locally-registered users always have.🤖 Generated with Claude Code