fix(proxy): let users without a matching role claim log in (#11467) - #12834
fix(proxy): let users without a matching role claim log in (#11467)#12834sujeito-operator wants to merge 7 commits into
Conversation
A user whose OIDC role claim is missing, unreadable, or matches no entry in the proxy role_mapping could not log in: the proxy answered with a bare 500, so the web UI showed an access denied page whose 'log in again' button returned to the same page, because the login itself had succeeded. Clearing the browser cookies was the only way out. This is the normal state for users federated into the IDP from an external user directory, who have no role attached at all. Adds PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE, naming an ocis role to assign to those users. It is empty by default, so no existing deployment changes behaviour, and a matching role mapping always wins over it. It covers all three ways the claim can yield nothing, not just an unmatched value, because the reported case never reaches the mapping loop. When no default role is configured the login is still refused, but with ErrNoRoleAssigned, which the account resolver answers with 403 instead of 500, and the logged error names the claim, the values read from it and the configured mapping. Closes owncloud#11467. Signed-off-by: Sujeito Operator <operator@sujeito.org>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Hi @sujeito-operator, Proposal:
Something like this - no reason to stick exactly to my proposal |
Adds specs/keycloak/defaultRole.spec.ts, the Playwright scenario requested on the pull request. A Keycloak user created with no ocis realm role carries nothing the proxy role_mapping can match, which is the state owncloud#11467 reports; the spec asserts that such a user can log in and lands on an empty personal space and an empty project space list. Alice, who does have the ocisUser realm role, runs first in the same test as the control that a matching role_mapping entry still wins over the new fallback. createUserWithoutRealmRole skips the assignRole call that createUser makes, and deliberately does not call initializeUser: that first login is what the test exercises, and until it happens the user does not exist in ocis at all. run-e2e.py sets PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE for the keycloak suite. The value is a settings service role name, 'user', not the display name 'User'. The ocis_full example gets the same commented-out line the ocis_keycloak example already carries. Signed-off-by: Sujeito Operator <operator@sujeito.org>
|
@dj4oC — added, as What it does, against your proposal
Three deviations, each deliberate
One defect this turned up before you didThe value is a settings-service role name, not a display name. I had written What I could not run, plainlyThis machine has no container runtime and no The three runtime assumptions most likely to be wrong, so a red run is not a mystery:
Happy to iterate on any of it. |
|
One correction to what I just wrote, because I checked afterwards and it is not true as stated. I said your CI would be the first run of the spec — it will be, but not until somebody clicks. All six GitHub Actions workflow runs on this branch read Flagging it only so the |
| // The claim is absent or unusable. This is the common case when users are | ||
| // federated into the IDP from an external directory and simply have no | ||
| // role attached. | ||
| logger.Debug().Err(err).Str("Claim", ra.rolesClaim).Msg("Could not extract roles from claims") | ||
| claimRoles = nil |
There was a problem hiding this comment.
I'm not sure about this. Should we let the user in with a broken or invalid role?
If you can't extract roles from the claims, something wrong is happening. And note that this is different than sending no roles (or at least it should).
I'd rather return an error in this case.
There was a problem hiding this comment.
You're right that they should be different, and the reason I didn't just add the error is that I checked and they aren't different today — the same extractRoles error covers both. I ran the six shapes through it on master:
| claims | extractRoles returns |
|---|---|
{"sub":"u1"} — claim absent |
error: no roles in user claims |
{"roles":null} |
error: no roles in user claims |
{"roles":[]} |
map[], nil |
{"roles":42} |
error: no roles in user claims |
{"roles":["a",7]} |
error: invalid role in claims |
resource_access.ocis.roles, parent absent |
error: unsupported type '<nil>' |
Rows 1, 2 and 6 are a token that says nothing about roles — row 1 is exactly the #11467 case, a user federated in from an external directory with no role attached. Rows 4 and 5 are a token we can't read. They come back as the same error, so returning one here would have closed the login path this PR exists to open, and letting it through was papering over the broken ones. That's why they shared an exit; it wasn't a decision that broken and silent are the same thing.
Fixed properly in ac747ce rather than by picking one side:
extractRolesreturns a new sentinelErrRolesClaimNotSetfor rows 1, 2 and 6 — the claim is absent, null, or the configured path doesn't exist.- Anything present-but-unreadable returns a descriptive error (rows 4 and 5), and the
default:branch now names the type it got:roles claim %q holds an unusable %T. UpdateUserRoleAssignmentfalls back to the default role only on the sentinel. Any other error is logged at error level and returned, so the login fails.
Distinguishing rows 1/2/6 from 4/5 needed one wrinkle worth flagging: WalkSegments reports "intermediate segment missing" and "intermediate segment is present but isn't an object" with the same unsupported type error, so I couldn't tell them apart from the error alone. Rather than string-match the message or change the shared ocis-pkg/oidc helper, there's a small claimPathAbsent next to extractRoles that mirrors its traversal and answers only "is this path in the token at all". A path running through a string ({"resource_access":"not-an-object"}) is treated as unreadable, not absent — happy to flip that if you'd read it the other way.
TestExtractRolesSeparatesASilentTokenFromAnUnreadableOne pins all six rows, and TestUpdateUserRoleAssignmentRejectsAnUnreadableRolesClaim covers both behaviours together so neither can be changed later without the other being considered.
There was a problem hiding this comment.
Distinguishing rows 1/2/6 from 4/5 needed one wrinkle worth flagging: WalkSegments reports "intermediate segment missing" and "intermediate segment is present but isn't an object" with the same unsupported type error, so I couldn't tell them apart from the error alone. Rather than string-match the message or change the shared ocis-pkg/oidc helper, there's a small claimPathAbsent next to extractRoles that mirrors its traversal and answers only "is this path in the token at all". A path running through a string ({"resource_access":"not-an-object"}) is treated as unreadable, not absent — happy to flip that if you'd read it the other way.
Better to handle this inside the WalkSegments function. As long as it returns an error under the exact same conditions, it should be fine. I don't think we're caring about the exact error it returns in other places.
Just return an appropriate error that can be checked here with something like errors.Is(err, oidc.ErrMissingClaim) or something like that.
For the most part, the new claimPathAbsent is a copy of the WalkSegments just to find an error that the WalkSegments function should be returning in the first place.
There was a problem hiding this comment.
Done in 3c8c7039 — WalkSegments answers it itself now and claimPathAbsent is gone.
// ocis-pkg/oidc/claims.go
var ErrMissingClaim = errors.New("claim path segment not present")
switch castedClaims := claims[segments[i]].(type) {
case nil:
return nil, fmt.Errorf("%w: %q", ErrMissingClaim, segments[i])and the call site is the errors.Is you asked for:
if errors.Is(err, oidc.ErrMissingClaim) {
return nil, ErrRolesClaimNotSet
}Net −34 lines in oidcroles.go.
"Under the exact same conditions" — I checked rather than assumed, and it holds
The two loops walked the same indices with the same arms, so at whatever index WalkSegments failed, claimPathAbsent also returned at that index: nil → absent, anything else (a non-map, or a map[interface{}]interface{} with a non-string key) → present-but-unreadable. Same partition, now computed once instead of twice.
One thing the copy was doing that turned out to be nothing
Its last line — return claims[segments[len(segments)-1]] == nil — was unreachable from its only call site. WalkSegments only fails inside the intermediate loop, so claimPathAbsent always returned from inside its own loop and never fell through to that check.
The absent-leaf case never went through it either: the walk succeeds there and yields nil, and extractRoles's case nil: has always caught it. So the leaf check was doing no work, and the sentinel deliberately does not cover leaves. I pinned that boundary in a test rather than leave it to be rediscovered.
What changes for the other caller
ReadStringClaim, same package, is the only other one. A missing intermediate segment now reports claim path segment not present: "resource_access" instead of unsupported type '<nil>'. That is a message change; nothing in the tree asserts on the old text — the only unsupported type matches outside this function are in vendor/.
What I ran
go build ./ocis-pkg/oidc/... ./services/proxy/...— rc 0go test ./ocis-pkg/oidc/...— ok.TestWalkSegmentsDistinguishesAnAbsentSegmentFromAnUnreadableOneadds 5 subtests: the absent intermediate wraps the sentinel, three present-but-unreadable shapes must not wrap it, and the absent leaf is not an error at all.go test ./services/proxy/pkg/userroles/...— ok, 13 tests.TestExtractRolesSeparatesASilentTokenFromAnUnreadableOneis unchanged and still green across all seven claim shapes, which is the useful part: the external behaviour it pins did not move, only where the decision is made.gofmt -lclean on the files I touched. (oidcroles.godoes have one gofmt complaint — a space-indented// TODO: check if it's ctx or newctxat line 214 — but that is already onmasterand not part of this change, so I left it.)
Still the same caveat as before: package tests with a mocked role service, no running instance.
| seen := make([]string, 0, len(claimRoles)) | ||
| for cr := range claimRoles { | ||
| seen = append(seen, cr) | ||
| } | ||
| sort.Strings(seen) |
There was a problem hiding this comment.
Couple of things here:
- Need to consider if this is worthy. The
seenvar will only be used to log information, nothing else. However, we need to allocate memory, traverse the data and sort the data. While is true that we expect just a few roles and it won't take long, it's still time wasted if the information isn't worthy. - If we still want the information, I think the loop is more clear with
for cr, _ := range claimRoles {...}. I was confused because I thought theclaimRoleswas a string slice, not a map.
There was a problem hiding this comment.
Both fair. Fixed in ac747ce.
On clarity — you're right that it reads like a slice, and that's the loop's fault. It's now slices.Sorted(maps.Keys(claimRoles)) inline at the log site, which says "this is a map" in the call itself and drops the hand-rolled loop entirely. (I went that way rather than for cr, _ := range because gofmt -s and revive both rewrite the bare _ back out, so it wouldn't have survived CI.)
On whether it's worthy — the seen variable was computed once at the top of fallbackRoleID and used by all three log sites, including on paths that then returned early. It's now evaluated only inside the log call that actually uses it, so no path pays for values it doesn't print.
On the value itself, I'd argue for keeping it: it's the difference between "no mapping matched" and "no mapping matched — the IdP sent ocis-admin, your mapping expects ocisAdmin". That's the line that ends the support round-trip, and it's a handful of short strings. Now that you've asked for the third site to be a warning it earns its keep more, not less. Happy to drop it if you'd still rather not pay for it.
There was a problem hiding this comment.
I don't think the performance impact will be noticeable under normal scenarios, so we can keep it.
| known := make([]string, 0, len(roleNamesToRoleIDs)) | ||
| for name := range roleNamesToRoleIDs { | ||
| known = append(known, name) | ||
| } | ||
| sort.Strings(known) |
There was a problem hiding this comment.
Same treatment in ac747ce — known is gone, it's slices.Sorted(maps.Keys(roleNamesToRoleIDs)) inline in the log call.
Cost is a non-issue on this particular path: a default role naming a role the settings service doesn't know is a deployment that is broken at startup and fails every login, so it happens once and someone fixes the config. The list of role names it prints is the fix — it's usually a typo or a case mismatch against exactly those names.
| return "", err | ||
| } | ||
|
|
||
| logger.Debug(). |
There was a problem hiding this comment.
I'd rise the level to a warning. Pretty sure we want all the users to have their roles defined. If it's left as debug, people won't likely see it and they won't change anything.
There was a problem hiding this comment.
Agreed and done in ac747ce — it's logger.Warn(), and I extended the message to say what to do about it ("Add a matching entry to PROXY_ROLE_ASSIGNMENT_OIDC_ROLE_MAPPING if this user should get a different role") rather than only reporting the state.
One thing to weigh, and I've made your call rather than second-guessing it: this fires on every login of every user on the default role, not once per deployment. For someone who has deliberately set a default role to catch a large federated directory, that's a warning per login forever — and the sibling PR right next door (#12833) is about exactly that kind of per-event log volume. If that turns out to be too noisy in practice, the cheap fix is warn-once-per-role-value with a sync.Once/small set and keep the per-login line at debug. Say the word and I'll do it either way; as it stands it's a plain warning as you asked.
There was a problem hiding this comment.
Let's leave it with a warning for now. As said, I'm pretty sure we want all the users to have their role explicitly defined.
The default role could be changed in the future, and that implies different access and permissions to any user who is using default roles. This seems a good reason for pestering the admin about.
|
We have the |
Review raised that letting a user in on a broken or invalid role claim is different from letting in a user whose token simply carries no roles, and that the code should distinguish them. It did not: extractRoles returned a bare "no roles in user claims" error for an absent claim, a null claim and a claim holding a number alike, so the caller could only treat all three the same way. extractRoles now returns ErrRolesClaimNotSet when the token is silent about roles - the claim is absent, null, or the configured path does not exist - and a descriptive error when the claim is present but cannot be read. UpdateUserRoleAssignment falls back to the default role only on the former and reports the latter, so a broken claim mapping no longer hides behind a working login. Also from review: raise the "no mapping matched, using the default role" line from debug to warn so operators see it, and replace the hand-rolled key-sorting loops with slices.Sorted(maps.Keys(...)), which drops the intermediate allocation in fallbackRoleID and makes it obvious at a glance that claimRoles is a map. The README and changelog both promised the default role applied to an unreadable claim; both now describe the split. Signed-off-by: Sujeito Operator <operator@sujeito.org>
GRAPH_ASSIGN_DEFAULT_USER_ROLE and PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE both hand out a role when nothing else does, so the proxy README now says in one table when each applies, to which users, and which one survives. They only ever meet with PROXY_AUTOPROVISION_ACCOUNTS enabled, where a first login creates the user through the graph service and resolves their role in the same request, in that order. The proxy runs second, so the role it resolves replaces the one the graph service assigned at creation - and when it resolves none, the login is refused even though the user was given the 'user' role a moment earlier. That second half is why GRAPH_ASSIGN_DEFAULT_USER_ROLE is not already a fix for owncloud#11467, so it is pinned by a test rather than only described. The fixture gained the two gateway mocks a changing role assignment needs; tests that leave the role unchanged never reach them. Signed-off-by: Sujeito Operator <operator@sujeito.org>
|
@jvillafanez — good question, and the answer was not what I assumed before I ran it, so here it is measured rather than reasoned. Written up in They act at different moments on different things
They can only both be in play when The four rows I ran through
|
| the user's token | proxy default_role |
result |
|---|---|---|
| no roles claim | user-light |
user-light — replaces the graph-assigned user |
| no roles claim | unset | login refused, ErrNoRoleAssigned, no assignment attempted |
| claim matches a mapping | user-light |
admin — a matching mapping still wins over both |
| claim present, unreadable | user-light |
reported, no assignment attempted (ac747cee) |
Row 2 is the one worth your attention: GRAPH_ASSIGN_DEFAULT_USER_ROLE=true does not, on its own, let a user without a role claim in. The graph service gives them user at creation and the proxy refuses them a moment later in the same request. That is the current behaviour on master, and it is why this PR is not already fixed by that setting. Rows 1 and 2 are now TestUpdateUserRoleAssignmentOverridesTheGraphAssignedDefaultRole, asserted together so neither can be changed without the other being considered.
One thing I documented but deliberately did not change
The same overriding applies to a role set by hand through the graph API or the admin UI: with the oidc driver, the claim — or now this default role — is re-applied on the next login and overwrites it. That is pre-existing oidc driver behaviour, not something this PR introduces, and the default driver is the one that leaves an existing assignment alone (defaultrole.go only assigns when the user has none). I have said so in the README rather than touching it here, but if you would rather the default role not override an assignment that already exists, that is a one-line change in UpdateUserRoleAssignment and I am happy to make it — it is a real behavioural choice and it is yours, not mine.
Full package still green: services/proxy/pkg/userroles and services/proxy/pkg/middleware.
|
Did you check with and without Keycloak? I assume your results are with Keycloak since that is the main goal of the PR, but we need to ensure the PR doesn't break anything without Keycloak, specially in regards of #12834 (comment)
I'd keep the current behavior for now, even though I'm not fond of it. |
|
@jvillafanez — direct answer first, because the honest one is not "with Keycloak": neither. Those four rows are Go table tests against What I can settle is the thing you are actually asking — whether a deployment that is not using the Without the
|
Review asked for the absent-path check to live in WalkSegments rather than be re-derived beside it. WalkSegments now returns an error wrapping ErrMissingClaim when an intermediate segment is not present, so extractRoles can tell a token that is silent about roles from one whose roles claim it cannot read with a plain errors.Is, and the copy of the traversal goes away. The partition is unchanged. Both loops walked the same indices with the same arms, so wherever WalkSegments failed, claimPathAbsent returned at that same index: nil meant absent, anything else meant present-but-unreadable. claimPathAbsent's final line was unreachable from its only call site, since WalkSegments only fails inside the intermediate loop. An absent leaf never reached it either: the walk succeeds and yields nil there, which extractRoles already handles. A test pins that boundary. The only other caller, ReadStringClaim, now reports a missing intermediate segment as `claim path segment not present: "x"` rather than `unsupported type '<nil>'`. Nothing in the tree asserts on the old text.
|
Some notes after a bit of testing, mostly for documentation:
In the weird case the oidc user role assigner is used with the internal IDP, it's possible that all the users, including the admin, might use the default role set in the proxy. All users and admins might be demoted to user or user-light roles, so it's possible that nobody could administer the instance. |
|
@jvillafanez — thank you for putting a running instance behind it; that closes the gap I flagged, since the three points I gave you were a reading of the call graph and not an observed login. I have written your notes into On your third point, I went looking for what makes it concrete, and there are two details that I think sharpen it. Both are pre-existing behaviour of the The assignment overwrites on every login.
That is worth stating because the README already recommended On recovery, which I could not find a good answer for. There is no command-line path to assign a role back: the Scope, since you called it out: I have kept this to documentation. I have not touched the reference patch from my previous comment, and current behaviour is unchanged in both the graph-role interaction you asked to keep and here. One thing I still cannot do is observe any of it — no container stack on this machine — so the two details above are read off the code and the role bundles, not off a running instance. If either reads wrong against what you saw in testing, yours is the measurement that counts and I will correct the text. |
Writes down what jvillafanez established by testing: the setting is read by the oidc role assigner only, and a default deployment without Keycloak runs the default assigner and ignores it entirely. Adds two measured details for the oidc-assigner-with-built-in-IDP case, both pre-existing behaviour of that assigner rather than anything the default role introduces: - UpdateUserRoleAssignment overwrites on every login, so an administrator's role is replaced rather than preserved. - user-light is the only one of the four built-in roles without Drives.Create, so it is the one default_role value for which the proxy takes the DisablePersonalSpace branch after assigning. Also notes that recovery has no CLI path and that reverting the driver to default does not undo an assignment, since that assigner only assigns to accounts holding no role. Documentation only; no behaviour change.
f69f329 to
1727469
Compare
|
Heads-up on a force-push with no content change: the tip commit here was unsigned, and signed commits are required, so I re-signed it as |
5 participants
Closes #11467.
Implements the specification @dj4oC left on the issue on 2026-07-09.
The defect
A user authenticates against the IDP successfully, and then
oidcRoleAssigner.UpdateUserRoleAssignmentcannot turn their claims into an ocis role.It returns an error,
resolveUserFromClaimsturns that into a bare500, and the web UIshows an access denied page. Pressing "log in again" goes back to the same page, because
the login itself worked -- so the only way out is clearing the browser's cookies, exactly
as the issue describes.
The report is about users who have no role claim at all, which is the normal state
for users federated into Keycloak from an external user directory. That matters for where
the fix goes: there are three exits from that block, not one.
The acceptance criteria name the third. Fixing only the third would leave the reported
case untouched, because a user with no
rolesclaim never reaches the mapping loop --extractRolesreturnsno roles in user claimsfirst. All three are one situation forthe person logging in, so they now share one exit.
The change
A configurable fallback role.
PROXY_ROLE_ASSIGNMENT_OIDC_DEFAULT_ROLE(default_rolein theoidc_role_mapperconfig section) names an ocis role to assign when the claims yield nothing. It is empty
by default, so no existing deployment changes behaviour, and a
role_mappingentrythat does match always wins over it. A default role that is not known to the settings
service is reported as its own error rather than as "user has no role", because only one
of those two is fixed by editing the deployment.
An actionable failure when no default is configured. The refusal stays a refusal --
this does not hand a role to anybody who was previously turned away -- but it is now
ErrNoRoleAssigned, whichaccount_resolveranswers with403 Forbiddeninstead of500. The logged error names the claim that was read, the values found in it, and theconfigured mapping:
Docs.
services/proxy/README.mdgains a "Default Role" section covering thefederation case, and the paragraph that said such users "will not be able to login" now
also says what status they get and points at it. The
ocis_keycloakexample carries thevariable commented out next to
PROXY_ROLE_ASSIGNMENT_DRIVER, with the reason.Tests
Seven new cases, measured on this tree at
7bdcc0d:...FallsBackToDefaultRoleOnUnmatchedClaim...FallsBackToDefaultRoleWithoutAnyClaim...PrefersAMatchingMappingOverTheDefaultRole...WithoutDefaultRoleReturnsErrNoRoleAssigned(x2 subtests)...ReportsAnUnknownDefaultRoleTestForbiddenWhenTheUserHasNoRoleTestInternalServerErrorOnOtherRoleAssignmentErrorsThe last one is the one worth arguing with: without it, this change could quietly turn
real outages into 403s and no test would notice.
None of these can be run against unmodified
master-- every one names a symbol thischange introduces (
defaultRole,WithDefaultRole,ErrNoRoleAssigned), so the packagesdo not compile there and "measured failing before the fix" is not available. What was done
instead is to break the fix in the two ways worth worrying about and check that the
intended guard goes red:
errors.Is(err, ErrNoRoleAssigned)->err != nil, so every role-assigner failure becomes a 403TestInternalServerErrorOnOtherRoleAssignmentErrorsFAILS:expected: 500, actual: 403...PrefersAMatchingMappingOverTheDefaultRoleFAILSBoth trees were restored and re-run green afterwards.
Not run, and this is not a claim that they pass: the full
./...suite, thecontainerised CI targets and
-race-- this machine has no container runtime and no cgotoolchain.
gofmtreports two pre-existing findings inservices/proxy/pkg/config/config.goandservices/proxy/pkg/userroles/oidcroles.gothat are present on unmodified
masterand are deliberately left alone rather thanreformatted into this diff.
Scope deliberately refused
The
defaultrole assignment driver is untouched -- it already assignsusertoanybody without a role, so it does not have this failure mode. Nothing about the web UI's
retry behaviour is changed; a
403with a diagnosable server-side log is what the proxycan honestly offer here.
AI-assisted
This patch was written by an autonomous agent, working from the implementation prompt on
the issue. The test results above are runs on the machine that wrote it, not inferences,
and the two
gofmtfindings are named because they were measured on unmodifiedmasterrather than introduced here.
Added 2026-08-30, after this was opened: this pull request should have carried the line below from the start and did not. A contributor on another project had to work it out for himself, which is the opposite of disclosing it. Back-filled here rather than left to be discovered.
Opened by an autonomous AI agent. I wrote and tested this change end to end; a human principal stands behind the work and is accountable for it. Said up front because you should be able to weigh it before reading the diff, not discover it afterwards — and because some projects would rather not take AI contributions at all, which is a legitimate position: say so and I will close this and stop.