docs: document the OIDC audience parameter - #137
Merged
Merged
Conversation
The openidconnect app gains an optional "audience" key naming the value the IdP puts into the access token's "aud" claim. It defaults to client-id, and is needed by IdPs that address the resource server instead - RFC 9068 §3 defines an access token's "aud" that way, and Microsoft ADFS follows it, prefixing the application identifier with "microsoft:identityserver:" unless that identifier is already a URL. Documented for 11.0 and 10.16, the two versions the app change ships on. The entry covers the operational consequences rather than just the syntax, because each of them is a way to lock an instance out: the audience becomes authoritative once set, so a token issued to another client of the same IdP is accepted when its "aud" matches, an introspection response that omits "aud" can no longer be used at all, and with token-exchange mode the first list entry is what gets requested from the IdP. Values that cannot be an audience are discarded, and if none is left every access token is rejected. For finding the ADFS value, both cmdlets are named: Get-AdfsWebApiApplication for an OpenID Connect application group, Get-AdfsRelyingPartyTrust for a legacy WS-Federation or SAML trust. Naming only the latter would strand admins whose OIDC registration is an application group, which is the modern default. No app version numbers are claimed: neither release carrying the fix is tagged yet, so the entry points at the issue instead. See owncloud/openidconnect#373 and owncloud/openidconnect#374 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
LukasHirt
previously approved these changes
Sep 18, 2026
oc-tmueller
marked this pull request as draft
September 18, 2026 11:07
Contributor
Author
|
Converted to draft — do not merge as-is. I authored this by hand, and this page is generated.
The pipeline is live, not dead — core changelog What this PR becomes
Verified in the meantime: both pages here are currently byte-in-sync with core, so the regeneration will add only the new entry and no unrelated drift. Two side findings, each getting its own PR: 10.16's sibling page |
kw-tmueller
pushed a commit
to owncloud/openidconnect
that referenced
this pull request
Sep 21, 2026
The README claimed that because another client of the same IdP can often get ownCloud's identifier into "aud" - its own audience mapper, an RFC 8707 resource parameter - the "audience" key is worth setting for that reason too. That is backwards. In exactly that scenario the token's "aud" names ownCloud, so it is accepted whether or not the key is set; setting it changes nothing there, and additionally stops the client-naming claims from being consulted. What the key actually buys is a binding to the resource: a token ownCloud's own client obtained for some *other* resource stops authenticating here. Which clients an IdP may issue ownCloud-audience tokens to is a decision in the IdP, and no ownCloud setting can override it. The three paragraphs are rewritten as one sequence - what it buys, what it does not, what holds when it is unset - rather than two passes over the same two facts, and the Keycloak sentence now sits in the paragraph it belongs to instead of running into the end of the previous one. Same content on the admin manual side (owncloud/docs.owncloud.com#137). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
kw-tmueller
pushed a commit
to owncloud/openidconnect
that referenced
this pull request
Sep 21, 2026
The README claimed that because another client of the same IdP can often get ownCloud's identifier into "aud" - its own audience mapper, an RFC 8707 resource parameter - the "audience" key is worth setting for that reason too. That is backwards. In exactly that scenario the token's "aud" names ownCloud, so it is accepted whether or not the key is set; setting it changes nothing there, and additionally stops the client-naming claims from being consulted. What the key actually buys is a binding to the resource: a token ownCloud's own client obtained for some *other* resource stops authenticating here. Which clients an IdP may issue ownCloud-audience tokens to is a decision in the IdP, and no ownCloud setting can override it. Documented as such now, on the admin manual side too (owncloud/docs.owncloud.com#137). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit 2928e3c69eeaa613cfa25314efda09add16257ad)
The audience parameter alone does not tell an admin whether they need it, and for one supported provider it cannot help at all: Keycloak sends no aud claim unless an audience mapper is configured, so no configured value can ever match. openidconnect 2.4.2 and 2.3.5 therefore also accept the claim naming the client a token was issued to - azp, appid or client_id - and that is what these pages now describe. - oidc.adoc gains an "Access Token Audience" section. Because the behaviour differs per app version and both fixing releases are still unreleased, the applicability is structural rather than a footnote: the table has one column per version - "On 2.4.1" and "On 2.4.2" for ownCloud 11, "On 2.3.4 and earlier" and "On 2.3.5" for ownCloud 10 - so a reader of any single row sees what applies to the version they actually run. On 2.4.1 that means Keycloak needs the audience mapper, Azure needs requestedAccessTokenVersion 2, and ADFS and OneLogin have no remedy short of the upgrade. The section also covers both log lines an admin will meet, the refusal of tokens marked as refresh tokens, the deliberate exception for ID tokens, and the Keycloak audience-mapper recipe with its console path and kcadm.sh command. - config_apps_sample_php_parameters.adoc: the audience entry described the default as "the client-id", which is only half of it, and ended in a placeholder instead of a version. Both fixed, with the versions marked as in preparation since neither 2.4.2 nor 2.3.5 is tagged. - ms-azure-setup.adoc: requestedAccessTokenVersion decides whether aud holds the Application ID URI (v1.0 tokens, the default) or the client id (v2.0). On openidconnect 2.4.1 that decides whether the documented setup works at all, which this walkthrough never said. Each table row records whether it was observed or taken from vendor documentation; PingFederate and cidaas are marked undetermined rather than guessed, and the ownCloud 10 page says so where it would otherwise promise that no upgrade can lock a provider out. Two review rounds folded in. The claims that a token issued to a different client is "rejected either way" and that "an attacker's own client cannot get a token through either check" were both wrong and both contradicted elsewhere in the same section: the audience comparison cannot tell which client asked for a token that names ownCloud. The recommendation to set audience *because* another client might target ownCloud was inverted for the same reason - in that scenario the key changes nothing and removes the client-claim check; it binds the resource, not the client, and the pages now say which of the two they mean. Also: the Keycloak workaround was addressed to ownCloud 10 admins who cannot run 2.4.1 at all; the Azure snippet used the client-id placeholder where the App ID URI belongs, which would have rejected every token once audience was set; and the Azure manifest path read Manifest > Manage instead of Manage > Manifest.A third round corrected the ID-token statement, which claimed to hold "independently of the audience". It does not: with audience set to anything other than the client-id - what ADFS, Azure v1.0 and OneLogin need - an ID token's aud no longer matches and the client-claim fallback is suppressed, so ID tokens are rejected. That is a real benefit of the key and the pages now list it. Also in that round: the ownCloud 11 entry's lead described 2.4.2 behaviour in the present tense while 2.4.1 is what ships; the Keycloak 2.4.2 cell said audience "cannot be used" without allowing for an install that followed the 2.4.1 instruction and added the mapper; OneLogin was missing from the admonition that lists who is affected; the Azure note called the suppressed claim "the appid fallback" where v2.0 tokens use azp; and neither page said that on 2.4.1 the check reaches JWT access tokens only, so an install using opaque tokens with introspection is not affected at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
…iting them config_apps_sample_php_parameters.adoc is generated from core's config/config.apps.sample.php by owncloud/config-to-docs, which the audience entry on this branch did not account for: it existed only here, so the next regeneration would have deleted it. The text now lives in core - one commit per server line - and these two pages are that generator's output. Confirmed before regenerating that both pages were byte-identical to what the generator produces from their respective core branches, so this diff is exactly the new entries and nothing else, and running the generator again is a no-op. Five keys arrive with it that the app has always read and neither page documented: exchange-token-mode-before-introspection, use-access-token-introspection-for-user-info, and the three ocis-routing-policy-* keys. The audience entry is shortened in the move, from ten paragraphs to five. A config sample entry is read in the PHP file as well as in the manual, and the long-form discussion belongs in the hand-written Access Token Audience section, which this entry now links to per server version. Two things it had picked up are gone from here: the refresh-token rule, which is not a property of this key, and the ID-token consequence stated twice. Corrections from review, all verified against the app code: the 11.0 text says the 2.4.1 lockout applies to JWT access tokens (2.4.1 does not check the audience on the introspection path at all), the 10.16 text no longer claims the upgrade cannot lock anyone out (an introspection response with neither "aud" nor "client_id" is rejected), and the client-claim acceptance is version-qualified inline rather than only in the closing paragraph. oidc.adoc and ms-azure-setup.adoc are hand-authored pages and are untouched here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
This was referenced Sep 21, 2026
Contributor
Author
|
The parameter-reference pages in this PR are now generator output, not hand-edited: the That matters because Verified before regenerating that both pages were byte-identical to what the generator produces from their respective core branches, so the diff here is exactly the new entries. Five keys arrive with
|
Regenerated from core after owncloud/core's two `docs/oidc-id-token-position` commits, one per server line. The `audience` entry claimed an ID token "stops being accepted, but only where the configured value differs from the `client-id`", which reads as though this parameter is what decides ID tokens. It is not: where the provider labels the token type in the payload the token is refused whatever the parameter says, and where it does not, what accepts the token is the `client-id` being an accepted audience - so setting the parameter to anything else rejects it as a side effect rather than as its purpose. Generator output only, `php convert.php config:convert-adoc` per server line. Confirmed beforehand that regenerating from the *unmodified* core branch is a no-op against these pages, so this diff is exactly the corrected entry. The app version differs per line by design - 2.4.2 on 11.0, 2.3.5 on 10.16 - because that is when each line's openidconnect release starts requiring a self-labelled token to label itself an access token. Antora builds both pages, and the new text renders as its own paragraph inside the `audience::` list item on both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
oidc.adoc was written against the app at the point where the token-type guard was a list of refresh markers. The app then inverted it to an allowlist, which the generated parameter pages picked up in the previous commit and this page did not - so the two pages in this branch contradicted each other. Four corrections, same on both server lines: - "a token that declares itself *not* to be an access token is refused" and the enumeration of `Refresh`/`Offline`/`refresh` described the old guard. It is now the other way round: a token that labels its own type has to say `Bearer`, `at+jwt`, `access` or the bare `jwt`, and everything else is refused - which is what makes it cover back-channel logout, registration and ID tokens as well. - "ID tokens carry no such marker" was false for exactly the provider the page's own examples use. Keycloak labels them `typ: ID` and Cognito `token_use: id`, so there an ID token presented as a bearer token is refused whatever `audience` says. The case the old sentence described is the *other* one - Azure AD, ADFS, or a payload labelled only `jwt` - and it is now stated as one of two rather than as the rule. - The fallback advice was described as a warning "once per verified token". It is `info`, once per request, and ownCloud's default `loglevel` of 2 means an admin following that paragraph would find nothing in the log at all. The level and the setting to change are named. - The client-naming fallback was listed as "`azp`, `appid` or `client_id`" with no mention that only the most authoritative claim the token carries is consulted. That precedence is the property that makes the fallback safe, so leaving it out overstates what a token needs to carry to be refused. And on 10.16 only: "the upgrade needs no configuration change for any provider whose shape is known" omitted the one shape it can lock out, which the parameter page on the same branch calls out - an introspection response carrying neither `aud` nor `client_id`, unrescuable by any `audience` value. Antora builds both pages and the corrected text renders; the stale sentences are gone from the output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Follow-up on the two commits above, from a review of them. - The client-claim precedence was stated as though it governed every acceptance. It governs the *fallback*: `aud` is compared first and a match there ends the check, so "a token whose `azp` names another client is refused" is only true of tokens that reach the fallback at all. As written it contradicted the same page's own later sentence that any token whose `aud` names ownCloud is accepted whichever client requested it, and would have had an admin build a threat model on a guarantee the code does not give. - The allowlist was given as three values where the code takes four, and the way it was written implied each value belongs to one claim. `access_token` was missing, the comparison is case-insensitive, and either claim accepts any of the values - so a provider sending `token_use: access_token` would have read this page and concluded their deployment was about to break. - `jwt` is no longer on that allowlist; the app reverted it, because a provider that stamps the generic media type into the payload stamps it on its refresh tokens too. The pages say that, rather than listing it as accepted. - "raise it to `1`" for `loglevel` is backwards - 1 is *lower* than the default of 2. An admin following it literally would have set 3 and seen less. - The log section explained the audience mismatch but not the line that precedes it when a client claim decided the rejection. That line exists precisely so an admin does not go tuning `audience` over a token that belongs to another client, and the page was sending them to do exactly that. And on 10.16, "the one shape it can lock out" undercounted: the type allowlist and the claim precedence each lock out a shape of their own on that line, since the whole check arrives at once there, and no `audience` value rescues any of the three. The parameter pages are regenerated from core after dropping `jwt` there too. Antora builds all four pages; each corrected statement is in the output and none of the replaced ones are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Three more from a review of the commit above. - "any of them in either claim" states the wrong logic. `nonAccessTokenMarker()` walks both `typ` and `token_use` and rejects on the first one it finds that is off the allowlist, so *every* label the token carries has to be an accepted value - a token with `typ: JWT` and `token_use: access` is refused on the `typ`, not accepted on the `token_use`. The example is now in the text, because that combination is exactly the one an admin would expect to pass. - On 10.16, the lockout list said three shapes, none rescuable with `audience`. The client-claim one *is* rescuable: the fallback sits behind "`audience` is not set", so configuring it turns the client claims off and leaves `aud` to decide. And a fourth shape was missing - a JWT access token with no usable `exp`, refused with `Access token has no expiry`. Four shapes, three of them unrescuable, one of them not. - The newly quoted `Token was issued to another client` line is truncated like its two neighbours now, since the emitted line carries a second sentence. Antora builds all four pages; the corrected statements are in the output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Two more from a review of the commit above. Calling the client-claim lockout "rescuable with `audience`" was true mechanically and wrong as advice. It only changes anything for a provider that sends an `aud` at all - so not for Keycloak's stock client, the very row the same page marks as having no value to configure - and where it does, the value that lets such a token through is by definition one other clients of the same provider can be issued for. That is the cross-client acceptance the check exists to stop, and the page says so twice elsewhere. The paragraph now says what setting it does and then says not to do it for that reason. And the `Access token has no expiry` lockout was documented on 10.16 only, although the same code ships on this line as 2.4.2. An 11.0 admin whose provider omits `exp` from a JWT access token would have had blanket 401s and nothing on the page to explain them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Generator output only, following two more commits in core's `docs/oidc-id-token-position` branches. A review of those found that the sentence the ID-token paragraph builds on still described the client-naming fallback as accepting a token whose `azp`, `appid` *or* `client_id` names ownCloud, with no mention that only the most authoritative claim the token carries is consulted - so it promised acceptance for a token the app refuses. The `oidc.adoc` pages on this branch already say it; now the parameter reference does too. Core also picked up the app README's closing advice for the case where neither ID-token branch helps, and on 10.16 the list of shapes the upgrade can lock out, which had counted one where there are three. Antora builds both pages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Generator output only. core's `docs/oidc-id-token-position` reflowed the paragraph the previous commit added a sentence to, and reworded the ID-token advice from "where neither applies" - which read as though the two branches were not exhaustive - to "where setting it is not an option", the case it actually means. Confirmed the two pages match what the converter produces from their core branches, so the next regeneration is a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
This was referenced Sep 23, 2026
Contributor
Author
|
Four commits added, and a note on why one check is red.
What the four commits are:
Each of those came out of a review of the commit before it, so the history is a bit stuttery — happy to squash on merge. Sequencing. All of this documents openidconnect 2.4.2 / 2.3.5, which are unreleased (owncloud/openidconnect#374 and #368, still under review). This stays a draft until they ship, and the two core PRs have to merge before it can go green. |
oc-tmueller
added a commit
to owncloud/openidconnect
that referenced
this pull request
Sep 24, 2026
* fix: let the expected access token audience be configured The audience check added in #356 requires the configured client-id to appear in the access token's "aud" claim. That is the ID token rule (OpenID Connect Core 1.0 §2) applied to an access token, whose "aud" RFC 9068 §3 defines as the resource server. Providers that address the resource therefore never send the client-id and could not authenticate at all since 2.4.1: AD FS renders the relying party identifier as "microsoft:identityserver:<identifier>" unless it is a URL, so login succeeded on the ID token and the next request was thrown out of SessionVerifier::verifySession(). Add an optional "audience" key, a string or list of strings, naming what the provider actually puts in "aud". Unset, the expected value stays the client-id, so existing installs are unaffected. Values that cannot be an audience - non-strings, empty strings - are dropped rather than trusted, which leaves nothing to match and so fails closed. That is reported once per request, distinguishing "nothing usable is left" from "one entry of several was ignored", because otherwise a JSON number or an empty list here presents as a site-wide auth outage whose only log line reads like an attack rather than a typo. Setting "audience" also makes the audience authoritative on the introspection path: the RFC 7662 "client_id" shortcut from #365 exists only because "aud" is optional there and cannot be relied on, and once the admin has declared what "aud" holds it can be. Without that, a token this client obtained for a different resource would still be accepted for opaque tokens, leaving the resource binding the admin just configured silently unenforced. Two consequences follow and are documented rather than hidden: a token issued to another client of the same IdP is accepted when its "aud" names us, which is the resource-server model and unavoidable once the client-id is not in "aud" at all; and an introspection response that omits "aud" entirely can no longer be used together with this key. exchangeToken() now requests the configured audience instead of the client-id. Those were the same value before this key existed; without the change, setting "audience" would make the exchanged token fail the very check it has to pass. The JWT branch deliberately does not gain the "client_id" fallback the introspection branch has: RFC 9068 §2.2 makes "aud" REQUIRED there, so no conformant provider needs it, and honouring it would accept a token this client legitimately obtained for a different resource (RFC 8707, RFC 8693) and had replayed here. A test pins that. The error now names the expected audience rather than "the configured client-id", which was wrong once the audience can differ - and misleading enough to have caused a misdiagnosis in the issue report. Fixes #373 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: name the right AD FS cmdlet for the audience identifier Get-AdfsRelyingPartyTrust enumerates WS-Federation and SAML relying party trusts. An OpenID Connect registration on AD FS 2016 or later is an application group with a Web API role, whose identifier - the value that ends up in "aud" - comes from Get-AdfsWebApiApplication instead, so an admin following the previous wording on a modern setup would find no matching trust and be stuck. Name both, and call it the application identifier rather than the relying party identifier, since with this key in play it identifies the resource rather than the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: accept an access token that names this client in azp, appid or client_id The audience check from #356 requires the configured client-id in the access token's "aud". An access token's "aud" is the resource server (RFC 9068 §3), so providers that address the resource never send it, and 2.4.1 locks them out. #374 added the "audience" key for that, but a config key is not enough: - Keycloak (observed, 26.0, confidential client, no audience mapper) sends *no* "aud" claim at all, so no configured value can match. Before this commit a stock Keycloak - a supported IdP - could not authenticate at all on 2.4.1, and #374 did not change that. - Entra ID v1.0 tokens (the default, requestedAccessTokenVersion null or 1) send the App ID URI; AD FS sends microsoft:identityserver:<identifier> (#373); OneLogin API authorization sends the configured API audience URIs. What all of them do send is the client: "azp" (OpenID Connect Core 1.0 §2), "appid" (Entra ID v1.0, AD FS) or "client_id" (RFC 7662 §2.2). With no "audience" configured, a strict string match on one of those is now accepted, which restores pre-2.4.1 behaviour for every documented IdP while keeping the property the check was added for: a token minted for a *different* client of the same issuer still fails (OC10-115, OC10-147), verified against a real Keycloak. Configuring "audience" keeps the strict behaviour on both branches, so the resource binding is opt-in rather than gone. That replaces the deliberate asymmetry #374 documented between the JWT and introspection branches: the JWT branch now honours client_id too when nothing is declared, so the introspection branch's shortcut becomes the same rule instead of a special case. The acceptance is logged once per request with the value to put in "audience". tests/unit/ClientTest.php gains the access token shape of every IdP ownCloud documents - Keycloak stock and with a mapper (observed), Entra ID v1.0 and v2.0, AD FS, Kopano Konnect, OneLogin - asserted in both modes, so a later tightening cannot lock one of them out unnoticed. testVerifyTokenJwtIgnoresClientIdClaim is rescoped to the strict mode it still holds for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: refuse a token that declares itself not to be an access token Review findings on the previous commit. Two are behaviour, four are wording. The client-naming fallback consulted only whether a claim names our client, so any JWT the provider signs with a key from its published JWKS and that carries "azp": <our client-id> became a valid bearer credential - including a refresh or offline token, whose "aud" is the issuer rather than us. A token that declares itself not to be an access token - "typ" of Refresh or Offline as Keycloak sends, "token_use" of refresh as AWS Cognito does - is now refused. That check sits before the audience comparison and applies whether or not "audience" is configured, which does change a pre-existing outcome: a refresh token carrying the expected audience used to pass. It has to, or the guard misses the two shapes that matter most - a provider whose refresh tokens carry the client-id, and Okta's introspection response for a refresh token, which repeats the configured audience and the client_id of the requesting client. It also gets its own log line and its own exception message rather than borrowing the audience mismatch, which would have told the admin to fix an audience that was never the problem - and following that advice would have switched the guard off. Keycloak's own refresh tokens do not reach this code: Keycloak signs them HS512 with a key that is not in the published JWKS, so verifyJWTsignature() throws first (checked against 26.0). That is one provider's default, not a guarantee. ID tokens are deliberately not covered. An ID token's "aud" is the client-id by definition, so it satisfies the default expectation and is accepted as a bearer token - that predates this PR, and refusing it would break any deployment that relies on it today. The README now says so instead of leaving it implied. The advice logged when the fallback is taken said to set "audience" to what the provider sends, and named the value, without the caveat the README carries: for a provider that emits a shared resource identifier, doing that accepts tokens issued to every other client of the same provider - hardening in appearance, OC10-115 in effect. The caveat is now in the line the admin actually reads. The config complaint escaped slashes while the two lines added for it did not, so an admin looking for their own "api://owncloud" found "api:\/\/owncloud". And a security claim that was too broad, in the README and in the docblock: "a token minted for another client never passes" holds for this fallback, not for the audience comparison, which accepts any token whose "aud" names ownCloud whichever client requested it - the resource-server model, unchanged from before this key existed. Another client can often get our identifier into "aud" deliberately, with an audience mapper of its own or an RFC 8707 resource parameter. Stated narrowly now, in both places. Two structural notes from the same review applied to the new code: the "usable audience" filter is one helper used by both sides of the comparison instead of a duplicated closure, and verifyAudience() takes the boolean it actually needs rather than the whole config to re-derive it. OK (209 tests, 352 assertions), php-cs-fixer 0 of 29. Every guard is pinned by a mutation: moving the marker check below the audience comparison fails 7, gating it on the configuration fails 6, dropping the refresh markers fails 4, making their comparison case-sensitive fails 2, dropping the caveat fails 1, re-escaping the slashes fails 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: correct what configuring the audience can and cannot do The README claimed that because another client of the same IdP can often get ownCloud's identifier into "aud" - its own audience mapper, an RFC 8707 resource parameter - the "audience" key is worth setting for that reason too. That is backwards. In exactly that scenario the token's "aud" names ownCloud, so it is accepted whether or not the key is set; setting it changes nothing there, and additionally stops the client-naming claims from being consulted. What the key actually buys is a binding to the resource: a token ownCloud's own client obtained for some *other* resource stops authenticating here. Which clients an IdP may issue ownCloud-audience tokens to is a decision in the IdP, and no ownCloud setting can override it. The three paragraphs are rewritten as one sequence - what it buys, what it does not, what holds when it is unset - rather than two passes over the same two facts, and the Keycloak sentence now sits in the paragraph it belongs to instead of running into the end of the previous one. Same content on the admin manual side (owncloud/docs.owncloud.com#137). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: let the most authoritative client claim decide, and require an access token Review feedback from @kw-fscheuer on #374. Two blocking items and the rest. **tokenNamesThisClient() returned on the first claim that matched, not the first one present.** A truthful "azp" naming another client did not stop the search, so a lower-precedence "client_id" could still name us. On a shared Keycloak realm a tenant can produce exactly that with a hardcoded-claim mapper, and every claim in the resulting token is honest. Reproduced against Keycloak 26.0: a second client with `client_id=owncloud-client` hardcoded authenticated as the victim (HTTP 207), and is now refused (401). The most authoritative claim present decides; no provider sends two of these with conflicting values, so there is no compatibility cost. **The fallback advice was a WARNING, deduped per request.** For every provider the fallback exists for - the documented happy path - that is one warning per token acquisition of every user, which is the log spam #319 fixed once already. It drops to info. Deduping it per *configuration* was considered and rejected: the log should stay a simple sink, and a cache in front of it can swallow the message exactly when something is wrong. The non-blocking items: - The token-type guard is now an allowlist. Enumerating refresh markers closed two members of a class: Keycloak's back-channel logout, registration and initial access tokens, and its ID tokens, are all realm-signed with "aud" equal to the client-id and passed. When a token labels its own type, that label must now say access token ("Bearer", "at+jwt", "access"); when there is no label - Entra ID, AD FS - nothing changes. Verified on the bench: a Keycloak ID token used as a bearer token went from 207 to 401. This is a deliberate change of the ID-token position the README documented, and the README now says so. - That guard moved out of verifyAudience() into assertIsAccessToken(), called from both branches of verifyToken(). A guard hidden inside the audience checker is reachable only by remembering to call it. - getOpenIdConfig() no longer returns a scalar. "123", "true" and a bare string are valid JSON, so a fat-fingered occ config:app:set produced a TypeError against the ?array parameter, and TypeError is not an OpenIDConnectClientException - every request became a 500 instead of a 401. - A JWT access token without an integer "exp" is refused. RFC 9068 §2.2 requires it, and the auth module guards its expiry check with "if ($expiry)", so a missing one skipped expiry verification altogether. The introspection branch stays deliberately asymmetric - RFC 7662 §2.2 makes "exp" optional there and "active" is the authority - so updateCache() takes ?int and cannot 500 on it either. - The raw bearer token is out of the signature-failure log line. A token that fails verification because a key rotated is still live until it expires, and that line is enabled by default. It logs kid, alg and sub instead. - The HS* comment claimed the wrong mechanism. The vendored library routes HS* to an HMAC check against the *client secret*, returning false rather than throwing, so HS* is not refused on algorithm grounds at all. Said plainly at the call site. - README: the client claim is documented as first-present-wins; "first entry" that token exchange requests is the first *usable* entry; the ID-token position is rewritten for the allowlist. - CHANGELOG: the new `audience` key moves to Added, per #253's precedent, so an admin scanning for new configuration on upgrade sees the key that unlocks their AD FS or Entra deployment. Tests: OK (237 tests, 403 assertions), php-cs-fixer 0 of 29. The two test gaps are closed - testIdpAccessTokenShapeWithConfiguredAudience now asserts on $expectAccepted instead of deriving everything from $strictAudience, and the client-naming claims run against the introspection branch as well, so "both branches follow the same rule" is pinned rather than asserted. Nine mutations each turn the suite red, including one that survived a first pass: nothing covered the credential-scrubbing, so that has a test now.A review of this commit caught that one of its own fixes went the wrong way. Widening updateCache() to ?int removed a 500 and put something worse in its place: the cache is written with no TTL, and an entry short-circuits verifyToken() completely, so an introspection response with no "exp" cached ['exp' => null] forever - the expiry check cannot fire without an expiry, and the token is never re-introspected, so it keeps authenticating after the provider revokes it. An unknown expiry now gets a bounded TTL instead, and the comment no longer claims "active" is re-checked per token when the cache is what prevents that. In the same pass: the "exp" guard accepted 0 and negatives, which "if ($expiry)" reads as false exactly like a missing claim, and rejected a non-integral NumericDate, which RFC 7519 §2 permits - it is now int-or-float and greater than zero, a numeric string still being refused since that is not a JSON number. The scalar-config guard covered only the app config, so config.php could still hand a scalar to the same typed parameters. And a rejection on claim precedence was logged as an audience mismatch, which would send an admin to look at an "audience" that was never the problem; it now says which claim named which client. OK (244 tests, 418 assertions). Thirteen mutations covering both rounds each turn the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: bound an unknown expiry from when it was cached, not from the last request Review feedback from @kw-fscheuer on #374, round three. One blocking item, three that are not, all four of them introduced by the previous commit. **UNKNOWN_EXPIRY_CACHE_TTL did not bound what its docblock said it bounds.** authToken() calls updateCache() on every successful auth, and a request served from the cache is a successful auth: verifyToken() returns the cached "exp" and getUserResource() returns the cached uid, then the entry is written again with a fresh 300 seconds. So the TTL was a sliding window. Any client polling more often than five minutes - which every sync client does - kept the entry alive indefinitely, and the token kept authenticating after the provider revoked it, because the expiry check needs an expiry and there is none. Only an idle token was ever really bounded. That is the failure the previous commit claimed to fix. Reproduced end to end against Keycloak 26.0 + owncloud/server:11.0.0 with redis as the distributed cache, seeding the entry exactly as updateCache() writes it for an unknown expiry and watching the key's TTL over three authenticated PROPFINDs: before: 300 -> 300 -> 300 -> 300 (never ages, HTTP 207 each time) after: 300 -> 296 -> 291 -> 287 (ages, HTTP 207 each time) The fix is to write only when this request actually verified something. Chosen over storing an absolute deadline in the entry: no extra cache read, no clamping of a negative remainder, and it also drops a pointless write on every authenticated request in the known-expiry case, where the value being overwritten is identical. testAnUnknownExpiryIsCachedWithATtl() could not catch any of this - one expected set() only ever covers the cache-miss request, and its ICache mock never returns a hit - so the new tests use a stub that actually stores. The core ArrayCache cannot be used for them either: its set() discards the TTL argument outright. The same trace is why the introspection branch's comment was wrong to say that "active": true is re-checked per token. It is re-checked per cache *miss* - on a hit Client::verifyToken() is never reached at all - so what bounds an entry with no expiry is that TTL and nothing else. Said correctly now. The non-blocking items: - The introspected "exp" went out unvalidated. authToken() computes "$expiry - time()", so a non-numeric string raises a TypeError there, and a TypeError is not an OpenIDConnectClientException - the request 500s instead of returning 401 (checked on 8.3: "Unsupported operand types: string - int"). A float, which RFC 7519 §2 permits, trips "Implicit conversion from float ... loses precision" against updateCache()'s ?int. An unusable value now degrades to "unknown" rather than rejecting the token, deliberately asymmetric to the JWT branch: "exp" is OPTIONAL here (RFC 7662 §2.2) with "active" as the authority and that TTL as the bound, so the unknown case is already covered, whereas a 401 would be one more deployment locked out by a patch release. Numeric strings are accepted for the same reason - PHP subtracts them happily, so that shape works today. - The azp/appid/client_id precedence order was written out twice, once in tokenNamesThisClient() where it decides and once in reportClientClaimMismatch() where it is explained. Two copies of a security-deciding order can drift: change one and the log line describes a decision the code did not make, change the other and the rejection goes silent about its reason. One const, one walk in decidingClientClaim(), both callers reading it. Behaviour is unchanged, and the bench confirms it: a second realm client with a hardcoded "client_id" naming ownCloud, alongside a truthful "azp" naming itself, still gets 401. - "jwt" was *not* accepted as a token-type label in the end, which is the one review item declined. It was taken first: the generic JOSE media type says nothing about a token's type, so refusing it looked like an outage for no security gain. A second review pass showed the trade goes the other way. A provider that stamps "jwt" into the payload does so by copying the JOSE header, and the header says "jwt" on every token that provider signs - refresh tokens with it. So the hypothetical provider the entry would accommodate is the same hypothetical provider whose refresh token it would then let through the fallback as a bearer credential, which is the reason this check exists at all. That provider can say what it means with "audience". The allowlist stays at four values, and "typ": "JWT" now sits in providesNonAccessTokenMarkers with the reasoning attached. Live bench rows, all as before this commit: stock Keycloak client 207, the same token on a cache hit 207, "audience" configured to a value Keycloak does not send 401, a token from another realm client with a spoofed "client_id" 401, a refresh token 401, an ID token ("typ": "ID") 401. OK (254 tests, 438 assertions), php-cs-fixer 0 of 29. Baseline was 244/418. Every new guard is pinned by a mutation that turns the suite red: always calling updateCache() fails 2, dropping the flag's reset on a miss fails 1, neutralising the "exp" guard fails 5, the positive floor of usableExpiry() fails 2, its ceiling fails 2, the test after its cast fails 2, putting "jwt" back on the allowlist fails 1, and reversing the claim precedence fails 3. A local review of this commit turned up two more things, both from the previous round. **getOpenIdConfig() still had one exit that could hand out a scalar.** The previous commit routed two of its three returns through systemConfigOrNull(); the malformed-JSON branch kept returning config.php's value raw. So a malformed app config together with a scalar in config.php - a fat-fingered occ config:app:set plus a fat-fingered config.php - still produced the TypeError-as-500 that the filter exists to prevent. appConfigProvider could not catch it: every one of its cases hands back an array from config.php, so the one path that reaches the scalar is the one it never exercises. It has its own test now, and reverting the line fails it. **Two docblocks claimed more than isset() delivers.** tokenNamesThisClient() said the first claim *present* decides, where isset() means present *and not null*, so a claim explicitly set to JSON null falls through to the next. That is the better behaviour - a null claim names nobody, and the only alternative to falling through is rejecting the token - but the comment has to say it rather than assert something narrower than the code. Same for the README's ID-token paragraph, which split providers into "labels the type" and "puts no type claim" without saying that *every* label a token carries has to be an access-token one - the loop rejects on the first that is not, so "typ": "JWT" alongside "token_use": "access" is refused on the typ. Not fixed here, reported instead: SessionVerifier::verifySession() is the other caller of Client::verifyToken() and does not handle the null expiry this PR formalises. It caches the null, so the token is re-introspected on every request, and passes it to refreshToken(), where "null - time()" is always below the five-minute threshold - so every page load of a browser session also performs a refresh_token grant and rotates the session's tokens. That is pre-existing: before this PR the same introspection response made "$introData->exp" return null too, with a warning. It is a different file, a different code path and a design question - what a session with no known expiry should do - so it wants its own issue rather than a hurried line here. OK (255 tests, 440 assertions). A third pass, over this commit's own diff, found two more: - The "exp" comparisons ran on the raw claim while the caller gets the cast value, and a float above zero can cast to 0 - which "if ($expiry)" reads as false and which takes updateCache()'s non-null branch, i.e. an entry with no TTL, the exact thing this commit exists to prevent. "exp": 0.5 is all it takes. Both branches now go through one usableExpiry() helper, which is two conditions because two is what the cases need: the positivity test has to run on the cast result (0.5 -> 0, and INF and NAN cast to 0 too), and the int range has to be checked *before* the cast, at both ends, because casting a float PHP cannot hold is undefined and wraps two's-complement: 2e19 comes out as 1553255926290448384 and -1e19 as 8446744073709551616, so a negative claim can produce a plausible far-future expiry that would then be cached with no TTL forever. Three rounds of mutation shaped that helper, and the docblock now names which of its three tests is the only thing catching which input, because I got that wrong twice while writing it. A first version also tested is_finite() and "$exp <= 0", both of which mutated away with the suite green, so they are gone rather than test-papered; a second bounded only the upper end, and that survived until -1e19 was added; the test after the cast survived until exactly 2^63 was. Identical on 7.4, checked value by value. - An honest note on one thing not fixed. On the JWT branch INF never reaches that helper: the debug line above encodes the payload with JSON_THROW_ON_ERROR, INF is not encodable, and JsonException is not an OpenIDConnectClientException either - so an IdP sending "exp": 1e1000 in a JWT gets a 500 rather than a 401, from the log line rather than from any check. Pre-existing, in five log lines that predate this PR, reported separately. It *does* reach the helper on the introspection branch, where a numeric string carries it past an encode that only sees a string, so that case is in the data provider. - The README and the docblock both offered "audience" as the way out for a provider whose payload says "typ": "jwt". There is no way out: assertIsAccessToken() runs before the audience is looked at and reads nothing from the config. Both now say so. - A CHANGELOG entry after all. The rest of this commit fixes code that never shipped, but updateCache() took "int $expiry" in 2.4.1, and an introspection response with no "exp" - RFC 7662 §2.2 - handed it null. That is a released TypeError-as-500 on every bearer request for those deployments, and an admin upgrading deserves to see it listed. OK (264 tests, 459 assertions), php-cs-fixer 0 of 29. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
to owncloud/openidconnect
that referenced
this pull request
Sep 24, 2026
* ci: run unit tests, code style and trivy on the 2.3.x line The 2.3 line only had release.yml, so anything landing on release-2.3.4 - including security backports - merged with no test evidence at all. The drone pipeline this branch still carries is dead. Port the master workflows, adjusted for the branch: - main.yml runs semantic-git-messages, php-codestyle and php-unit against php 7.4 and core 10.16. Both core-ref and core-ref-php74 have to be set, because the reusable workflows ignore core-ref on 7.4. - security-scan.yml builds the tree to scan with php 7.4. - lint-pr-title.yml is a verbatim copy - the repo squash-merges, so the PR title becomes the commit message. No build job: build.yml has no php-version input and would run "make dist" on the runner default php. release.yml already builds the tarball on tag, which is how v2.3.4 shipped. No acceptance job either - the reusable acceptance workflow installs a hardcoded 11.x daily server, which an app declaring max-version="10" cannot be enabled on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: verify token audience to prevent cross-client account takeover (#356) * fix: verify token audience to prevent cross-client account takeover The OpenID Connect access-token verifier checked the JWT signature and expiry but never validated the "aud" (audience) claim. A correctly signed, unexpired token minted by the same issuer for a different client was therefore accepted, allowing an attacker holding such a token to authenticate as the matching ownCloud account via API/WebDAV bearer auth or the browser session (OC10-115). Client::verifyToken now asserts that the configured client-id is present in the token's audience (handling "aud" as a single string or an array per RFC 7519) and rejects the token otherwise. Both the bearer auth module and the session verifier funnel through this method, so both paths are covered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> * docs(changelog): add entry for #356 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 57782b8) * fix: verify token audience for introspected opaque tokens (#365) * fix: verify token audience for introspected opaque tokens verifyToken() has two branches: JWT access tokens are validated from their decoded payload, while opaque tokens fall through to RFC 7662 token introspection. The audience check added for OC10-115 was wired into the JWT branch only, so the introspection branch validated nothing beyond "error" and "active". An opaque access token minted by the same issuer for a different client is reported as active by the introspection endpoint, so it was accepted and authenticated as its subject - a cross-audience account takeover reachable unauthenticated via the Authorization header. The introspection branch now calls the existing verifyAudience() before returning the expiry, mirroring the JWT branch. Note that this fails closed when "aud" is absent: although the claim is optional in RFC 7662, an unknown audience cannot be shown to name this relying party. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * fix: prefer the introspected client_id over the audience claim RFC 7662 §2.2 defines "client_id" as the client the token was issued to, which is exactly the claim that answers whether an introspected token was issued to this relying party. Unlike RFC 7519, RFC 7662 makes "aud" optional, and providers commonly use it to name the resource server rather than the client - Keycloak emits "account", Okta "api://default", Ory Hydra an empty array unless an audience was requested. Checking "aud" alone therefore rejected legitimate opaque tokens from several mainstream identity providers, which would have stopped those installations from authenticating bearer tokens on upgrade to a security release. The introspection branch now compares "client_id" first and only falls back to the audience claim. This stays fail closed: when neither claim names us, verifyAudience() throws as before, and the check is skipped only for a non-null "client_id" which equals a configured client-id. The JWT branch is untouched, so the semantics shipped for OC10-115 stay as they are. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * docs: note that token exchange introspects the session token In exchange-token-mode-before-introspection, verifyToken() discards its $token argument and introspects the token held in the OIDC session after exchanging it, so the audience check binds that exchanged token rather than the one the caller presented. OpenIdConnectAuthModule then resolves the identity from the presented token, which means the two sides look at different tokens. This is not reachable as a bypass today - without an OIDC session the exchange fatals on the typed string $subjectToken, and the DAV branch which does have a session sets up the filesystem for the session's uid regardless - but nothing records the coupling. Document it so a future change on either side cannot turn it into one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> * test: require the audience rejection to be logged method('logException')->with(...) only constrains the arguments if the method is actually called, so the assertion passed silently if the rejection never logged at all. Use expects(self::once()) in the OC10-147 regression test, where the log line is part of what is being pinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 144431a) * chore: bump version to 2.3.5 (#371) Release the two access-token audience checks on the ownCloud 10 line. #356 shipped on the oc11 line in v2.4.1 but never reached 2.3, so v2.3.5 is the first 10.x release carrying either half of the check. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * fix: let the expected access token audience be configured The audience check added in #356 requires the configured client-id to appear in the access token's "aud" claim. That is the ID token rule (OpenID Connect Core 1.0 §2) applied to an access token, whose "aud" RFC 9068 §3 defines as the resource server. Providers that address the resource therefore never send the client-id and could not authenticate at all since 2.4.1: AD FS renders the relying party identifier as "microsoft:identityserver:<identifier>" unless it is a URL, so login succeeded on the ID token and the next request was thrown out of SessionVerifier::verifySession(). Add an optional "audience" key, a string or list of strings, naming what the provider actually puts in "aud". Unset, the expected value stays the client-id, so existing installs are unaffected. Values that cannot be an audience - non-strings, empty strings - are dropped rather than trusted, which leaves nothing to match and so fails closed. That is reported once per request, distinguishing "nothing usable is left" from "one entry of several was ignored", because otherwise a JSON number or an empty list here presents as a site-wide auth outage whose only log line reads like an attack rather than a typo. Setting "audience" also makes the audience authoritative on the introspection path: the RFC 7662 "client_id" shortcut from #365 exists only because "aud" is optional there and cannot be relied on, and once the admin has declared what "aud" holds it can be. Without that, a token this client obtained for a different resource would still be accepted for opaque tokens, leaving the resource binding the admin just configured silently unenforced. Two consequences follow and are documented rather than hidden: a token issued to another client of the same IdP is accepted when its "aud" names us, which is the resource-server model and unavoidable once the client-id is not in "aud" at all; and an introspection response that omits "aud" entirely can no longer be used together with this key. exchangeToken() now requests the configured audience instead of the client-id. Those were the same value before this key existed; without the change, setting "audience" would make the exchanged token fail the very check it has to pass. The JWT branch deliberately does not gain the "client_id" fallback the introspection branch has: RFC 9068 §2.2 makes "aud" REQUIRED there, so no conformant provider needs it, and honouring it would accept a token this client legitimately obtained for a different resource (RFC 8707, RFC 8693) and had replayed here. A test pins that. The error now names the expected audience rather than "the configured client-id", which was wrong once the audience can differ - and misleading enough to have caused a misdiagnosis in the issue report. Fixes #373 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit b4115dc) * docs: name the right AD FS cmdlet for the audience identifier Get-AdfsRelyingPartyTrust enumerates WS-Federation and SAML relying party trusts. An OpenID Connect registration on AD FS 2016 or later is an application group with a Web API role, whose identifier - the value that ends up in "aud" - comes from Get-AdfsWebApiApplication instead, so an admin following the previous wording on a modern setup would find no matching trust and be stuck. Name both, and call it the application identifier rather than the relying party identifier, since with this key in play it identifies the resource rather than the client. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit 18fd573) * fix: accept an access token that names this client in azp, appid or client_id The audience check from #356 requires the configured client-id in the access token's "aud". An access token's "aud" is the resource server (RFC 9068 §3), so providers that address the resource never send it, and 2.4.1 locks them out. - Keycloak (observed, 26.0, confidential client, no audience mapper) sends *no* "aud" claim at all, so no configured value can match. Before this commit a stock Keycloak - a supported IdP - could not authenticate at all on 2.4.1, and #374 did not change that. - Entra ID v1.0 tokens (the default, requestedAccessTokenVersion null or 1) send the App ID URI; AD FS sends microsoft:identityserver:<identifier> (#373); OneLogin API authorization sends the configured API audience URIs. What all of them do send is the client: "azp" (OpenID Connect Core 1.0 §2), "appid" (Entra ID v1.0, AD FS) or "client_id" (RFC 7662 §2.2). With no "audience" configured, a strict string match on one of those is now accepted, which restores pre-2.4.1 behaviour for every documented IdP while keeping the property the check was added for: a token minted for a *different* client of the same issuer still fails (OC10-115, OC10-147), verified against a real Keycloak. Configuring "audience" keeps the strict behaviour on both branches, so the resource binding is opt-in rather than gone. That replaces the deliberate asymmetry #374 documented between the JWT and introspection branches: the JWT branch now honours client_id too when nothing is declared, so the introspection branch's shortcut becomes the same rule instead of a special case. The acceptance is logged once per request with the value to put in "audience". tests/unit/ClientTest.php gains the access token shape of every IdP ownCloud documents - Keycloak stock and with a mapper (observed), Entra ID v1.0 and v2.0, AD FS, Kopano Konnect, OneLogin - asserted in both modes, so a later tightening cannot lock one of them out unnoticed. testVerifyTokenJwtIgnoresClientIdClaim is rescoped to the strict mode it still holds for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit cc1eec6) * fix: refuse a token that declares itself not to be an access token Review findings on the previous commit. Two are behaviour, four are wording. The client-naming fallback consulted only whether a claim names our client, so any JWT the provider signs with a key from its published JWKS and that carries "azp": <our client-id> became a valid bearer credential - including a refresh or offline token, whose "aud" is the issuer rather than us. A token that declares itself not to be an access token - "typ" of Refresh or Offline as Keycloak sends, "token_use" of refresh as AWS Cognito does - is now refused. That check sits before the audience comparison and applies whether or not "audience" is configured, which does change a pre-existing outcome: a refresh token carrying the expected audience used to pass. It has to, or the guard misses the two shapes that matter most - a provider whose refresh tokens carry the client-id, and Okta's introspection response for a refresh token, which repeats the configured audience and the client_id of the requesting client. It also gets its own log line and its own exception message rather than borrowing the audience mismatch, which would have told the admin to fix an audience that was never the problem - and following that advice would have switched the guard off. Keycloak's own refresh tokens do not reach this code: Keycloak signs them HS512 with a key that is not in the published JWKS, so verifyJWTsignature() throws first (checked against 26.0). That is one provider's default, not a guarantee. ID tokens are deliberately not covered. An ID token's "aud" is the client-id by definition, so it satisfies the default expectation and is accepted as a bearer token - that predates this PR, and refusing it would break any deployment that relies on it today. The README now says so instead of leaving it implied. The advice logged when the fallback is taken said to set "audience" to what the provider sends, and named the value, without the caveat the README carries: for a provider that emits a shared resource identifier, doing that accepts tokens issued to every other client of the same provider - hardening in appearance, OC10-115 in effect. The caveat is now in the line the admin actually reads. The config complaint escaped slashes while the two lines added for it did not, so an admin looking for their own "api://owncloud" found "api:\/\/owncloud". And a security claim that was too broad, in the README and in the docblock: "a token minted for another client never passes" holds for this fallback, not for the audience comparison, which accepts any token whose "aud" names ownCloud whichever client requested it - the resource-server model, unchanged from before this key existed. Another client can often get our identifier into "aud" deliberately, with an audience mapper of its own or an RFC 8707 resource parameter. Stated narrowly now, in both places. Two structural notes from the same review applied to the new code: the "usable audience" filter is one helper used by both sides of the comparison instead of a duplicated closure, and verifyAudience() takes the boolean it actually needs rather than the whole config to re-derive it. OK (209 tests, 352 assertions), php-cs-fixer 0 of 29. Every guard is pinned by a mutation: moving the marker check below the audience comparison fails 7, gating it on the configuration fails 6, dropping the refresh markers fails 4, making their comparison case-sensitive fails 2, dropping the caveat fails 1, re-escaping the slashes fails 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit 63317b9) * docs: correct what configuring the audience can and cannot do The README claimed that because another client of the same IdP can often get ownCloud's identifier into "aud" - its own audience mapper, an RFC 8707 resource parameter - the "audience" key is worth setting for that reason too. That is backwards. In exactly that scenario the token's "aud" names ownCloud, so it is accepted whether or not the key is set; setting it changes nothing there, and additionally stops the client-naming claims from being consulted. What the key actually buys is a binding to the resource: a token ownCloud's own client obtained for some *other* resource stops authenticating here. Which clients an IdP may issue ownCloud-audience tokens to is a decision in the IdP, and no ownCloud setting can override it. Documented as such now, on the admin manual side too (owncloud/docs.owncloud.com#137). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit 2928e3c69eeaa613cfa25314efda09add16257ad) * fix: let the most authoritative client claim decide, and require an access token Review feedback from @kw-fscheuer on #374. Two blocking items and the rest. **tokenNamesThisClient() returned on the first claim that matched, not the first one present.** A truthful "azp" naming another client did not stop the search, so a lower-precedence "client_id" could still name us. On a shared Keycloak realm a tenant can produce exactly that with a hardcoded-claim mapper, and every claim in the resulting token is honest. Reproduced against Keycloak 26.0: a second client with `client_id=owncloud-client` hardcoded authenticated as the victim (HTTP 207), and is now refused (401). The most authoritative claim present decides; no provider sends two of these with conflicting values, so there is no compatibility cost. **The fallback advice was a WARNING, deduped per request.** For every provider the fallback exists for - the documented happy path - that is one warning per token acquisition of every user, which is the log spam #319 fixed once already. It drops to info. Deduping it per *configuration* was considered and rejected: the log should stay a simple sink, and a cache in front of it can swallow the message exactly when something is wrong. The non-blocking items: - The token-type guard is now an allowlist. Enumerating refresh markers closed two members of a class: Keycloak's back-channel logout, registration and initial access tokens, and its ID tokens, are all realm-signed with "aud" equal to the client-id and passed. When a token labels its own type, that label must now say access token ("Bearer", "at+jwt", "access"); when there is no label - Entra ID, AD FS - nothing changes. Verified on the bench: a Keycloak ID token used as a bearer token went from 207 to 401. This is a deliberate change of the ID-token position the README documented, and the README now says so. - That guard moved out of verifyAudience() into assertIsAccessToken(), called from both branches of verifyToken(). A guard hidden inside the audience checker is reachable only by remembering to call it. - getOpenIdConfig() no longer returns a scalar. "123", "true" and a bare string are valid JSON, so a fat-fingered occ config:app:set produced a TypeError against the ?array parameter, and TypeError is not an OpenIDConnectClientException - every request became a 500 instead of a 401. - A JWT access token without an integer "exp" is refused. RFC 9068 §2.2 requires it, and the auth module guards its expiry check with "if ($expiry)", so a missing one skipped expiry verification altogether. The introspection branch stays deliberately asymmetric - RFC 7662 §2.2 makes "exp" optional there and "active" is the authority - so updateCache() takes ?int and cannot 500 on it either. - The raw bearer token is out of the signature-failure log line. A token that fails verification because a key rotated is still live until it expires, and that line is enabled by default. It logs kid, alg and sub instead. - The HS* comment claimed the wrong mechanism. The vendored library routes HS* to an HMAC check against the *client secret*, returning false rather than throwing, so HS* is not refused on algorithm grounds at all. Said plainly at the call site. - README: the client claim is documented as first-present-wins; "first entry" that token exchange requests is the first *usable* entry; the ID-token position is rewritten for the allowlist. - CHANGELOG: the new `audience` key moves to Added, per #253's precedent, so an admin scanning for new configuration on upgrade sees the key that unlocks their AD FS or Entra deployment. Tests: OK (237 tests, 403 assertions), php-cs-fixer 0 of 29. The two test gaps are closed - testIdpAccessTokenShapeWithConfiguredAudience now asserts on $expectAccepted instead of deriving everything from $strictAudience, and the client-naming claims run against the introspection branch as well, so "both branches follow the same rule" is pinned rather than asserted. Nine mutations each turn the suite red, including one that survived a first pass: nothing covered the credential-scrubbing, so that has a test now.A review of this commit caught that one of its own fixes went the wrong way. Widening updateCache() to ?int removed a 500 and put something worse in its place: the cache is written with no TTL, and an entry short-circuits verifyToken() completely, so an introspection response with no "exp" cached ['exp' => null] forever - the expiry check cannot fire without an expiry, and the token is never re-introspected, so it keeps authenticating after the provider revokes it. An unknown expiry now gets a bounded TTL instead, and the comment no longer claims "active" is re-checked per token when the cache is what prevents that. In the same pass: the "exp" guard accepted 0 and negatives, which "if ($expiry)" reads as false exactly like a missing claim, and rejected a non-integral NumericDate, which RFC 7519 §2 permits - it is now int-or-float and greater than zero, a numeric string still being refused since that is not a JSON number. The scalar-config guard covered only the app config, so config.php could still hand a scalar to the same typed parameters. And a rejection on claim precedence was logged as an audience mismatch, which would send an admin to look at an "audience" that was never the problem; it now says which claim named which client. OK (244 tests, 418 assertions). Thirteen mutations covering both rounds each turn the suite red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit 22b3c05) * fix: bound an unknown expiry from when it was cached, not from the last request Review feedback from @kw-fscheuer on #374, round three. One blocking item, three that are not, all four of them introduced by the previous commit. **UNKNOWN_EXPIRY_CACHE_TTL did not bound what its docblock said it bounds.** authToken() calls updateCache() on every successful auth, and a request served from the cache is a successful auth: verifyToken() returns the cached "exp" and getUserResource() returns the cached uid, then the entry is written again with a fresh 300 seconds. So the TTL was a sliding window. Any client polling more often than five minutes - which every sync client does - kept the entry alive indefinitely, and the token kept authenticating after the provider revoked it, because the expiry check needs an expiry and there is none. Only an idle token was ever really bounded. That is the failure the previous commit claimed to fix. Reproduced end to end against Keycloak 26.0 + owncloud/server:11.0.0 with redis as the distributed cache, seeding the entry exactly as updateCache() writes it for an unknown expiry and watching the key's TTL over three authenticated PROPFINDs: before: 300 -> 300 -> 300 -> 300 (never ages, HTTP 207 each time) after: 300 -> 296 -> 291 -> 287 (ages, HTTP 207 each time) The fix is to write only when this request actually verified something. Chosen over storing an absolute deadline in the entry: no extra cache read, no clamping of a negative remainder, and it also drops a pointless write on every authenticated request in the known-expiry case, where the value being overwritten is identical. testAnUnknownExpiryIsCachedWithATtl() could not catch any of this - one expected set() only ever covers the cache-miss request, and its ICache mock never returns a hit - so the new tests use a stub that actually stores. The core ArrayCache cannot be used for them either: its set() discards the TTL argument outright. The same trace is why the introspection branch's comment was wrong to say that "active": true is re-checked per token. It is re-checked per cache *miss* - on a hit Client::verifyToken() is never reached at all - so what bounds an entry with no expiry is that TTL and nothing else. Said correctly now. The non-blocking items: - The introspected "exp" went out unvalidated. authToken() computes "$expiry - time()", so a non-numeric string raises a TypeError there, and a TypeError is not an OpenIDConnectClientException - the request 500s instead of returning 401 (checked on 8.3: "Unsupported operand types: string - int"). A float, which RFC 7519 §2 permits, trips "Implicit conversion from float ... loses precision" against updateCache()'s ?int. An unusable value now degrades to "unknown" rather than rejecting the token, deliberately asymmetric to the JWT branch: "exp" is OPTIONAL here (RFC 7662 §2.2) with "active" as the authority and that TTL as the bound, so the unknown case is already covered, whereas a 401 would be one more deployment locked out by a patch release. Numeric strings are accepted for the same reason - PHP subtracts them happily, so that shape works today. - The azp/appid/client_id precedence order was written out twice, once in tokenNamesThisClient() where it decides and once in reportClientClaimMismatch() where it is explained. Two copies of a security-deciding order can drift: change one and the log line describes a decision the code did not make, change the other and the rejection goes silent about its reason. One const, one walk in decidingClientClaim(), both callers reading it. Behaviour is unchanged, and the bench confirms it: a second realm client with a hardcoded "client_id" naming ownCloud, alongside a truthful "azp" naming itself, still gets 401. - "jwt" was *not* accepted as a token-type label in the end, which is the one review item declined. It was taken first: the generic JOSE media type says nothing about a token's type, so refusing it looked like an outage for no security gain. A second review pass showed the trade goes the other way. A provider that stamps "jwt" into the payload does so by copying the JOSE header, and the header says "jwt" on every token that provider signs - refresh tokens with it. So the hypothetical provider the entry would accommodate is the same hypothetical provider whose refresh token it would then let through the fallback as a bearer credential, which is the reason this check exists at all. That provider can say what it means with "audience". The allowlist stays at four values, and "typ": "JWT" now sits in providesNonAccessTokenMarkers with the reasoning attached. Live bench rows, all as before this commit: stock Keycloak client 207, the same token on a cache hit 207, "audience" configured to a value Keycloak does not send 401, a token from another realm client with a spoofed "client_id" 401, a refresh token 401, an ID token ("typ": "ID") 401. OK (254 tests, 438 assertions), php-cs-fixer 0 of 29. Baseline was 244/418. Every new guard is pinned by a mutation that turns the suite red: always calling updateCache() fails 2, dropping the flag's reset on a miss fails 1, neutralising the "exp" guard fails 5, the positive floor of usableExpiry() fails 2, its ceiling fails 2, the test after its cast fails 2, putting "jwt" back on the allowlist fails 1, and reversing the claim precedence fails 3. A local review of this commit turned up two more things, both from the previous round. **getOpenIdConfig() still had one exit that could hand out a scalar.** The previous commit routed two of its three returns through systemConfigOrNull(); the malformed-JSON branch kept returning config.php's value raw. So a malformed app config together with a scalar in config.php - a fat-fingered occ config:app:set plus a fat-fingered config.php - still produced the TypeError-as-500 that the filter exists to prevent. appConfigProvider could not catch it: every one of its cases hands back an array from config.php, so the one path that reaches the scalar is the one it never exercises. It has its own test now, and reverting the line fails it. **Two docblocks claimed more than isset() delivers.** tokenNamesThisClient() said the first claim *present* decides, where isset() means present *and not null*, so a claim explicitly set to JSON null falls through to the next. That is the better behaviour - a null claim names nobody, and the only alternative to falling through is rejecting the token - but the comment has to say it rather than assert something narrower than the code. Same for the README's ID-token paragraph, which split providers into "labels the type" and "puts no type claim" without saying that *every* label a token carries has to be an access-token one - the loop rejects on the first that is not, so "typ": "JWT" alongside "token_use": "access" is refused on the typ. Not fixed here, reported instead: SessionVerifier::verifySession() is the other caller of Client::verifyToken() and does not handle the null expiry this PR formalises. It caches the null, so the token is re-introspected on every request, and passes it to refreshToken(), where "null - time()" is always below the five-minute threshold - so every page load of a browser session also performs a refresh_token grant and rotates the session's tokens. That is pre-existing: before this PR the same introspection response made "$introData->exp" return null too, with a warning. It is a different file, a different code path and a design question - what a session with no known expiry should do - so it wants its own issue rather than a hurried line here. OK (255 tests, 440 assertions). A third pass, over this commit's own diff, found two more: - The "exp" comparisons ran on the raw claim while the caller gets the cast value, and a float above zero can cast to 0 - which "if ($expiry)" reads as false and which takes updateCache()'s non-null branch, i.e. an entry with no TTL, the exact thing this commit exists to prevent. "exp": 0.5 is all it takes. Both branches now go through one usableExpiry() helper, which is two conditions because two is what the cases need: the positivity test has to run on the cast result (0.5 -> 0, and INF and NAN cast to 0 too), and the int range has to be checked *before* the cast, at both ends, because casting a float PHP cannot hold is undefined and wraps two's-complement: 2e19 comes out as 1553255926290448384 and -1e19 as 8446744073709551616, so a negative claim can produce a plausible far-future expiry that would then be cached with no TTL forever. Three rounds of mutation shaped that helper, and the docblock now names which of its three tests is the only thing catching which input, because I got that wrong twice while writing it. A first version also tested is_finite() and "$exp <= 0", both of which mutated away with the suite green, so they are gone rather than test-papered; a second bounded only the upper end, and that survived until -1e19 was added; the test after the cast survived until exactly 2^63 was. Identical on 7.4, checked value by value. - An honest note on one thing not fixed. On the JWT branch INF never reaches that helper: the debug line above encodes the payload with JSON_THROW_ON_ERROR, INF is not encodable, and JsonException is not an OpenIDConnectClientException either - so an IdP sending "exp": 1e1000 in a JWT gets a 500 rather than a 401, from the log line rather than from any check. Pre-existing, in five log lines that predate this PR, reported separately. It *does* reach the helper on the introspection branch, where a numeric string carries it past an encode that only sees a string, so that case is in the data provider. - The README and the docblock both offered "audience" as the way out for a provider whose payload says "typ": "jwt". There is no way out: assertIsAccessToken() runs before the audience is looked at and reads nothing from the config. Both now say so. - A CHANGELOG entry after all. The rest of this commit fixes code that never shipped, but updateCache() took "int $expiry" in 2.4.1, and an introspection response with no "exp" - RFC 7662 §2.2 - handed it null. That is a released TypeError-as-500 on every bearer request for those deployments, and an admin upgrading deserves to see it listed. OK (264 tests, 459 assertions), php-cs-fixer 0 of 29. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> (cherry picked from commit ef20153) * ci: do not publish ownCloud 10 releases from CI release.yml delegates to owncloud/reusable-workflows, whose signing step can only emit the current signature envelope - the one with `v`, `alg` and a `certificates` array. ownCloud 10.16's IntegrityCheck\Checker::verify() reads `certificate` singular with hard-coded RSA/PSS and has no dispatch on `v`/`alg`, so every artifact that workflow produces for this branch fails `occ integrity:check-app openidconnect` with "App Certificate is not valid". That is not hypothetical: it is exactly what shipped as v2.3.4, which is therefore unusable on the line it was cut for. Leaving the workflow in place means any `v*` tag pushed here republishes the same broken artifact, so remove it rather than hope nobody tags. This release line is built and signed locally instead - `make dist` inside a core 10.16 checkout with the app's G1 key at ~/.owncloud/certificates/openidconnect.{key,crt} - and the resulting tarball is verified with integrity:check-app inside a real owncloud/server:10.16.x before it is published. Do not restore this file on this branch; restoring it silently reintroduces an artifact ownCloud 10 rejects. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * build: refuse to package a signature ownCloud 10 cannot verify With release.yml gone, `make dist` is the only path to an artifact on this branch, and its signing step is silently optional: CAN_SIGN degrades to a printed message when the key, the certificate or occ is missing, so the release path could hand back an unsigned - or wrongly signed - tarball and still exit 0. Two guards, both in the packaging path so they cover the artifact that actually ships rather than a copy nobody publishes: - distdir removes any appinfo/signature.json that came along from the source tree. appinfo/ is copied wholesale and signature.json is not gitignored, so one left behind by a stray `integrity:sign-app --path=.` would be packaged as a signature whose hashes describe a different tree - and with no key present there is no re-sign to overwrite it. - package classifies the envelope and refuses the current format, the one ownCloud 10 cannot read. Anything it cannot classify fails rather than passing. Which format is required comes from appinfo/info.xml's max-version, parsed as XML, so the hunk is inert rather than harmful on a branch targeting ownCloud 11, and reformatting info.xml cannot silently disable it. An unsigned package is tolerated by default, because that is what CI produces - the trivy workflow builds the dist tree with no signing secrets. Cutting a release passes REQUIRE_SIGNATURE=1 to reject it. Deliberately not built on `occ integrity:check-app`: Checker::isCodeCheckEnforced() returns false for the `git` channel, so any occ reachable from a build tree reports success unconditionally - including on a deliberately planted current-format envelope. The authoritative check remains installing the built artifact into a real owncloud/server:10.16.x and running integrity:check-app there. Verified by making it fail, 19 cases in owncloudci/php:7.4. On this branch (max-version="10"): legacy packages, with and without REQUIRE_SIGNATURE; current, v-only, unparseable and unclassifiable envelopes are refused and leave no tarball behind; unsigned passes by default and is refused under REQUIRE_SIGNATURE=1 and =yes; max-version="10.16" takes the same path. On a max-version="11" tree the check is inert for every combination of envelope and REQUIRE_SIGNATURE - which is why the max-version test runs before the unsigned test rather than after it; the other order fails an ownCloud 11 build the check has no opinion about, and that case is now covered. A missing or unparseable max-version is refused. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(changelog): date the 2.3.5 release The section was dated 2026-09-11, the day this branch was prepared, not the day 2.3.5 is released. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(changelog): warn that 2.3.5 can refuse a token 2.3.4 accepted The 2.3.5 section lists the two audience fixes and the token-type guard, but nothing told an admin what they change. This is the first 2.3.x release with any audience verification at all - 2.3.4 accepted any correctly signed, unexpired token from the configured issuer - so the upgrade can lock out a deployment whose IdP names a resource in "aud" rather than this client, and there is no way to know that from a list of fix titles. Spell out the three things that decide: an "aud" naming this client, the azp/appid/client_id fallback when it does not, the new "audience" key for the resource-server case, and the type-claim allowlist that no setting relaxes. Accuracy note, since it is easy to get backwards: an "aud" that names this client is accepted no matter which client requested the token - Client::verifyAudience() returns as soon as the audience comparison matches, before the client-naming claims are consulted at all. So the claim precedence cannot narrow an "aud" match; it only orders the fallback that runs when "aud" does not name us. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
oc-tmueller
added a commit
to owncloud/core
that referenced
this pull request
Sep 24, 2026
…41858) The `audience` paragraph said an ID token "stops being accepted, but only where the configured value differs from the `client-id`", which made this key the thing that decides ID tokens. It is not, and has not been since the openidconnect app started requiring a token that labels its own type to label itself an access token. Where the provider puts that label in the payload - Keycloak's `typ` of `ID`, AWS Cognito's `token_use` of `id` - an ID token is refused whatever this key is set to, including not set at all. Where the provider puts no type claim in the payload, which is Entra ID and ADFS, the old sentence was right but for the wrong reason: what accepts the token is the `client-id` being an accepted audience, so setting this key to anything else rejects it as a side effect rather than as its purpose. Both halves now say so, in their own paragraph rather than wedged into the one about binding a token to a resource, which is a different property. Discussed on owncloud/openidconnect#374; the app's own README carries the same position. The admin manual's parameter reference is generated from this file, so owncloud/docs.owncloud.com#137 gets the same paragraph. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
oc-tmueller
added a commit
to owncloud/core
that referenced
this pull request
Sep 24, 2026
…[10.16] (#41859) The `audience` paragraph said an ID token "stops being accepted, but only where the configured value differs from the `client-id`", which made this key the thing that decides ID tokens. It is not, and has not been since the openidconnect app started requiring a token that labels its own type to label itself an access token. Where the provider puts that label in the payload - Keycloak's `typ` of `ID`, AWS Cognito's `token_use` of `id` - an ID token is refused whatever this key is set to, including not set at all. Where the provider puts no type claim in the payload, which is Entra ID and ADFS, the old sentence was right but for the wrong reason: what accepts the token is the `client-id` being an accepted audience, so setting this key to anything else rejects it as a side effect rather than as its purpose. Both halves now say so, in their own paragraph rather than wedged into the one about binding a token to a resource, which is a different property. Discussed on owncloud/openidconnect#374; the app's own README carries the same position. The admin manual's parameter reference is generated from this file, so owncloud/docs.owncloud.com#137 gets the same paragraph. Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
https://github.com/owncloud/docs.owncloud.com/actions/runs/35921916790?pr=137 I reran the config-docs-in-sync workflow, and it passes now - good. IMO this can be merged. |
phil-davis
approved these changes
Sep 25, 2026
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
3 participants
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.
Documents the
audiencekey the openidconnect app gains in owncloud/openidconnect#374 (fixes owncloud/openidconnect#373).Why
An access token is accepted only if it names the ownCloud relying party. Until now the expected value was always the configured
client-id— which is the ID token rule (OpenID Connect Core 1.0 §2), not the access-token one. RFC 9068 §3 defines an access token'saudas the resource server, so providers that address the resource send something else entirely and could not authenticate.Microsoft ADFS is the case that made this necessary. It takes the identifier of the relying party trust and prefixes it with
microsoft:identityserver:unless that identifier is already a URL — so an admin whose relying party identifier is their client-id GUID seesaud: "microsoft:identityserver:<guid>"and has nothing to configure. ADFS is already listed as a supported IdP inconfiguration/user/oidc/oidc.adoc, so this gap was reachable from a documented setup.What
Adds an
audience::entry to the OIDC parameter reference inconfig_apps_sample_php_parameters.adoc(the page bothoidc.adocandkopano-setup.adocxref for the full key list), for the two versions the app change ships on:content/server/11.0/...— app 2.4.2content/server/10.16/...— app 2.3.5The entry covers the default (
client-id), the accepted shapes, that it replaces rather than extends the client-id, and how to find the ADFS value.It deliberately documents the operational consequences rather than just the syntax, because each one is a way to take an instance offline:
audmatches — the RFC 7662client_idcheck no longer applies. The entry says to pick a value only ownCloud can be issued for, and not to reuse a tenant-wide resource identifier.aud(which RFC 7662 permits), because every opaque token would then be rejected.exchange-token-mode-before-introspection, the first usable list entry is also what the token exchange requests from the IdP.Two deliberate choices:
Both ADFS cmdlets are named.
Get-AdfsWebApiApplicationfor an OpenID Connect application group,Get-AdfsRelyingPartyTrustfor a legacy WS-Federation or SAML trust. Naming only the latter — as the first draft did — would strand admins whose OIDC registration is an application group, which is the modern default on ADFS 2016+.No app version is claimed. Neither release carrying the fix is tagged yet (tags stop at
v2.4.1; the oC11 change sits under## [Unreleased]), so the entry points at the issue instead of asserting a number that could be off by a release.Placed alphabetically between
allowed-user-backendsandauth-params. The two files' entries are byte-identical.Checks
Full local CI equivalent —
npm ci,npm run antora,npm test— all pass, and the built pages carry the entry in both versions.Rendered output confirms the entry is a proper definition term in the right position:
Note this is the file's first use of
+list continuation (the entry needs three paragraphs). Verified in the rendered HTML that all three attach toaudienceand that every following term still parses as its own<dt>.Every factual claim was checked line-by-line against the implementation in owncloud/openidconnect#374 rather than against its README alone.
Docs-only; no nav or xref changes.
🤖 Generated with Claude Code