fix: treat the OAuth2 user id as opaque

[oc-tmueller]

Description

OAuthApiController::generateToken() (both the authorization_code and the refresh_token
grant) and AuthModule::authToken() split the user id stored on an authorization code, access
token or refresh token at the first colon, and then resolved whatever user the remainder named:

$userId = $authorizationCode->getUserId();

// strip off username if it exists
if (\strstr($userId, ':')) {
    list(, $userId) = \explode(':', $userId, 2);
}

This is a leftover of the login name:user id format that only v0.5.0 - v0.5.2 ever wrote.
A user id is opaque, so one that legitimately contains a colon must not be reinterpreted as a
pair - the token would then be resolved to, and issued for, a different account than the one the
authorization code belongs to.

Why removing the parsing is safe

• PageController stores the raw IUser::getUID() since e5b508c ("use userid on api calls,
  show username in login form", 2022-03-10), so nothing writes the pair format any more. The
  login name survives only as the OIDC login_hint.
• No stored pair value can still reach the parsers: authorization codes expire after 10 minutes
  (AuthorizationCode::EXPIRATION_TIME), access tokens after 1 hour
  (AccessToken::EXPIRATION_TIME), and refresh tokens - which never expire - were always
  persisted with the already-split value.

So this is a plain removal of dead code, with no migration or compatibility shim needed.

Changes

• lib/Controller/OAuthApiController.php: drop the colon parsing in both grant paths.
• lib/AuthModule.php: drop it in authToken(), where it ran on every bearer request.
• tests/unit/Controller/OAuthApiControllerTest.php: two regression tests asserting that a
  colon-bearing user id is passed to the user manager verbatim and ends up unchanged on the
  issued access token, refresh token and in the user_id response field.
• tests/unit/AuthModuleTest.php: the ConcatUserID case asserted the splitting behaviour and
  is replaced by one asserting that the stored user id is used verbatim.

Testing

--testsuite unit run inside a core checkout:

• oc11 / php 8.3 (this branch): OK (163 tests, 525 assertions)
• oc10.16 / php 7.4 (same patch on top of v0.6.1): OK (163 tests, 525 assertions)
• without the lib/ change, exactly the three new tests fail

make test-php-style clean, make test-php-phpstan reports [OK] No errors.

Note for the 10.x line

The same patch applies cleanly to v0.6.1, but there is no maintenance branch for the 0.6 /
ownCloud 10 line to target - one has to be cut from the tag first. Happy to open that PR once
the branch exists.

[CLAassistant]

All committers have signed the CLA.

[phil-davis]

This looks like a good idea (removing "dead/useless" code).
In practice, there won't be (and never were) usernames containing a colon such as "phil:davis". So the ability to opaquely process such usernames is nice-to-have, but usernames in ownCloud Classic already cannot contain a colon.
(usernames containing a colon would create hassles for Basic Auth API requests that tried to send a "username:password"!)

[kw-fscheuer]

Code review

No issues found. I verified the dead-code argument against the repo history rather than the description alone.

All three parsing sites are removed, and nothing else in lib/ splits a user id on a colon — a repo-wide search for explode/strstr leaves only the hostname split in lib/Utilities.php#L105-L108:

• lib/Controller/OAuthApiController.php#L193-L197 — authorization_code grant
• lib/Controller/OAuthApiController.php#L231-L233 — refresh_token grant
• lib/AuthModule.php#L114-L117 — authToken()

The compatibility argument holds:

• 4685ce0c wrote the pair format onto the authorization code and the implicit-grant access token only. Both records minted in generateToken() already used the post-split value, so no refresh token ever stored a pair.
• e5b508c3 restored raw getUID() storage and left only the parsers behind, which is what this PR removes.
• git compare places 4685ce0 inside v0.5.0, and e5b508c outside v0.5.2 but inside v0.5.3 — so the v0.5.0 - v0.5.2 range in the commit message is exact. Combined with the 10 minute / 1 hour expiries, no stored pair value can still reach the removed code.

One non-blocking test note: testAuthTokenKeepsUserIdWithColon asserts only that authToken() returns null, which would also pass if the token were rejected for an unrelated reason. Asserting that a colon-bearing user id resolves to that user would pin the behaviour down more tightly.

Generated with Claude Code