mirror of
https://github.com/arsvendg/Stirling-PDF.git
synced 2026-07-16 19:33:11 +02:00
feat(oauth2): opt-in claim-dump diagnostics for OIDC login failures (#6456)
# Description of Changes ## What & why Customers using ADFS (or any generic OIDC provider that doesn't emit `email`) hit `Attribute value for 'email' cannot be null` during OAuth2 login with no visibility into what claims the provider actually sent. The only available remedy was guessing at `security.oauth2.useAsUsername` until something worked. This PR adds a new opt-in `security.oauth2.debugLogging` flag (default `false`). When enabled, `CustomOAuth2UserService` logs: - All ID token claims (sorted, with values) - All UserInfo endpoint claims (if any) - The merged attribute key set Spring exposes to `getAttribute()` - The value the configured `useAsUsername` actually resolved to - A **`Hint:`** line listing the claim keys present in the token that map to a valid `UsernameAttribute` enum value — i.e. exactly what the operator could put in `useAsUsername` to make login work Logged at `INFO` on the success path and `ERROR` on failure (inside the existing `catch (IllegalArgumentException)` block that throws `OAuth2AuthenticationException`). The block is wrapped with a `[OAUTH2 DEBUG] ... [/OAUTH2 DEBUG]` banner and ends with a PII warning so operators don't leave it on in production. Default off → zero observable change for anyone not actively troubleshooting. ## Files changed | File | Why | |---|---| | `app/common/.../ApplicationProperties.java` | New `debugLogging` field on the `OAUTH2` config class with javadoc warning about PII | | `app/core/src/main/resources/settings.yml.template` | Documents `oauth2.debugLogging` so it appears on next startup | | `app/proprietary/.../security/service/CustomOAuth2UserService.java` | Emits the claim dump + suggestion hint when the flag is on | | `app/proprietary/.../security/service/CustomOAuth2UserServiceDebugLoggingTest.java` (new) | Unit test: mocks the OIDC delegate, asserts off-path is silent and on-path emits the dump with the right Hint contents | ## End-to-end verification Ran the bundled `testing/compose/docker-compose-keycloak-oauth.yml` Keycloak realm, configured `security.oauth2.useAsUsername: mail` (Keycloak emits `email`, not `mail`) and `provider: demarest` (matches the original customer bug report). Triggered the OAuth flow at `http://localhost:8080/oauth2/authorization/demarest` and confirmed: - The ERROR-level dump fires with the full 19-claim ID token decoded - `-- Value at 'mail' : <NULL — this is why login fails>` correctly identifies the missing claim - `-- Hint:` correctly suggests `[email, family_name, given_name, preferred_username]` (the four keys present that map to valid `UsernameAttribute` values) - Auth still fails with the original `OAuth2AuthenticationException` — no change to control flow, just added diagnostic logging Unit test (`CustomOAuth2UserServiceDebugLoggingTest`) covers both branches. ## Reviewer notes - **No new public APIs.** The flag is config-only; no servlet endpoints exposed. - **PII is logged when the flag is on.** This is the whole point — operators need to see the claims to fix their config — but it's gated, defaults off, and the dump self-documents with a `WARNING: ... Set security.oauth2.debugLogging=false once troubleshooting is complete.` footer. - **Why log everything, not just sub/email?** Because the operator doesn't know in advance which claim they actually want. ADFS uses `upn` in some configs and `preferred_username` in others; Azure AD uses `oid`; the customer here had neither. Dumping the full set is the only way to make the diagnostic self-service. - **Out of scope for this PR (follow-ups):** - The `UsernameAttribute` enum doesn't include `upn` / `unique_name` (common ADFS claims). If the customer's token only has `upn`, the Hint will be empty even though the operator can see `upn` in the dump. Worth a separate PR to extend the enum. - The known-provider validator in `Provider.java` (rejects e.g. `useAsUsername: mail` for `provider: keycloak` at startup) bypasses our diagnostic for those provider names. ADFS customers using `provider: <name>` fall into the `default` branch so are not affected — but it's a sharp edge worth documenting. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) — N/A, backend-only change - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] Doc-repo update (if functionality has heavily changed) — diagnostic flag is self-documenting via the `settings.yml.template` comment and the in-log warning; happy to add a doc-repo entry if reviewers want one - [ ] Translation tags — N/A ### UI Changes (if applicable) - [ ] N/A — backend-only ### Testing (if applicable) - [x] Unit test added (`CustomOAuth2UserServiceDebugLoggingTest`) covering on/off paths and Hint correctness - [x] End-to-end verified locally against bundled Keycloak compose with intentionally misconfigured `useAsUsername` - [x] Full `:proprietary:test` suite passes
This commit is contained in:
ConnorYoh
+171
-4
@@ -1,6 +1,10 @@
|
||||
package stirling.software.proprietary.security.service;
|
||||
|
||||
import java.util.Collections;
|
||||
import java.util.Map;
|
||||
import java.util.Optional;
|
||||
import java.util.Set;
|
||||
import java.util.TreeSet;
|
||||
|
||||
import org.springframework.security.authentication.LockedException;
|
||||
import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserRequest;
|
||||
@@ -8,6 +12,8 @@ import org.springframework.security.oauth2.client.oidc.userinfo.OidcUserService;
|
||||
import org.springframework.security.oauth2.client.userinfo.OAuth2UserService;
|
||||
import org.springframework.security.oauth2.core.OAuth2AuthenticationException;
|
||||
import org.springframework.security.oauth2.core.OAuth2Error;
|
||||
import org.springframework.security.oauth2.core.oidc.OidcIdToken;
|
||||
import org.springframework.security.oauth2.core.oidc.OidcUserInfo;
|
||||
import org.springframework.security.oauth2.core.oidc.user.DefaultOidcUser;
|
||||
import org.springframework.security.oauth2.core.oidc.user.OidcUser;
|
||||
|
||||
@@ -39,20 +45,37 @@ public class CustomOAuth2UserService implements OAuth2UserService<OidcUserReques
|
||||
|
||||
@Override
|
||||
public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2AuthenticationException {
|
||||
String registrationId = userRequest.getClientRegistration().getRegistrationId();
|
||||
boolean debugLogging = Boolean.TRUE.equals(oauth2Properties.getDebugLogging());
|
||||
// Resolved inside the try so a bad/null useAsUsername (IllegalArgumentException from
|
||||
// valueOf, or NPE on toUpperCase) is caught and wrapped as OAuth2AuthenticationException
|
||||
// by the existing handlers below, matching the pre-debugLogging behaviour.
|
||||
String usernameAttributeKey = null;
|
||||
|
||||
try {
|
||||
OidcUser user = delegate.loadUser(userRequest);
|
||||
String usernameAttributeKey =
|
||||
usernameAttributeKey =
|
||||
UsernameAttribute.valueOf(oauth2Properties.getUseAsUsername().toUpperCase())
|
||||
.getName();
|
||||
OidcUser user = delegate.loadUser(userRequest);
|
||||
|
||||
if (debugLogging) {
|
||||
logClaimDump(
|
||||
"OAuth2/OIDC login claims received",
|
||||
registrationId,
|
||||
usernameAttributeKey,
|
||||
user.getIdToken(),
|
||||
user.getUserInfo(),
|
||||
user.getAttributes(),
|
||||
false);
|
||||
}
|
||||
|
||||
// Extract SSO provider information
|
||||
String ssoProviderId = user.getSubject(); // Standard OIDC 'sub' claim
|
||||
String ssoProvider = userRequest.getClientRegistration().getRegistrationId();
|
||||
String username = user.getAttribute(usernameAttributeKey);
|
||||
|
||||
log.debug(
|
||||
"OAuth2 login - Provider: {}, ProviderId: {}, Username: {}",
|
||||
ssoProvider,
|
||||
registrationId,
|
||||
ssoProviderId,
|
||||
username);
|
||||
|
||||
@@ -79,10 +102,154 @@ public class CustomOAuth2UserService implements OAuth2UserService<OidcUserReques
|
||||
usernameAttributeKey);
|
||||
} catch (IllegalArgumentException e) {
|
||||
log.error("Error loading OIDC user: {}", e.getMessage());
|
||||
// Only emit the claim dump if we successfully resolved usernameAttributeKey. A null
|
||||
// value here means UsernameAttribute.valueOf rejected the configured useAsUsername
|
||||
// before delegate.loadUser ran — that error message is self-explanatory and a claim
|
||||
// dump would have no resolved-key to compare against.
|
||||
if (debugLogging && usernameAttributeKey != null) {
|
||||
// The DefaultOidcUser constructor (or our own checks) rejected the chosen
|
||||
// username attribute. Dump the claims we DID receive so the operator can pick
|
||||
// a different value for security.oauth2.useAsUsername.
|
||||
logClaimDump(
|
||||
"OAuth2/OIDC login FAILED - dumping received claims",
|
||||
registrationId,
|
||||
usernameAttributeKey,
|
||||
userRequest.getIdToken(),
|
||||
null,
|
||||
userRequest.getIdToken() == null
|
||||
? Collections.emptyMap()
|
||||
: userRequest.getIdToken().getClaims(),
|
||||
true);
|
||||
}
|
||||
throw new OAuth2AuthenticationException(new OAuth2Error(e.getMessage()), e);
|
||||
} catch (Exception e) {
|
||||
log.error("Unexpected error loading OIDC user", e);
|
||||
if (debugLogging && usernameAttributeKey != null && userRequest.getIdToken() != null) {
|
||||
logClaimDump(
|
||||
"OAuth2/OIDC login FAILED (unexpected error) - dumping ID token claims",
|
||||
registrationId,
|
||||
usernameAttributeKey,
|
||||
userRequest.getIdToken(),
|
||||
null,
|
||||
userRequest.getIdToken().getClaims(),
|
||||
true);
|
||||
}
|
||||
throw new OAuth2AuthenticationException("Unexpected error during authentication");
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Emits a multi-line diagnostic dump of the claims returned by the OAuth2/OIDC provider. Only
|
||||
* invoked when {@code security.oauth2.debugLogging=true}.
|
||||
*
|
||||
* @param banner short title for the log block
|
||||
* @param registrationId Spring client registration id (e.g. "demarest", "keycloak")
|
||||
* @param usernameAttributeKey the claim key the application is configured to use as username
|
||||
* @param idToken the decoded ID token, may be null on unexpected failures
|
||||
* @param userInfo the decoded UserInfo response, may be null if the provider returned none
|
||||
* @param mergedAttributes the merged attribute map Spring uses for {@code getAttribute()}
|
||||
* @param failure true if logging in the error path (uses ERROR level), false for INFO
|
||||
*/
|
||||
private void logClaimDump(
|
||||
String banner,
|
||||
String registrationId,
|
||||
String usernameAttributeKey,
|
||||
OidcIdToken idToken,
|
||||
OidcUserInfo userInfo,
|
||||
Map<String, Object> mergedAttributes,
|
||||
boolean failure) {
|
||||
StringBuilder sb = new StringBuilder();
|
||||
sb.append("\n========== [OAUTH2 DEBUG] ").append(banner).append(" ==========\n");
|
||||
sb.append("Provider registrationId : ").append(registrationId).append('\n');
|
||||
sb.append("Configured useAsUsername: ")
|
||||
.append(oauth2Properties.getUseAsUsername())
|
||||
.append(" (looks up claim key '")
|
||||
.append(usernameAttributeKey)
|
||||
.append("')\n");
|
||||
|
||||
if (idToken != null) {
|
||||
Map<String, Object> idClaims = idToken.getClaims();
|
||||
sb.append("\n-- ID token claims (")
|
||||
.append(idClaims == null ? 0 : idClaims.size())
|
||||
.append(") --\n");
|
||||
appendClaims(sb, idClaims);
|
||||
sb.append("ID token issued at : ").append(idToken.getIssuedAt()).append('\n');
|
||||
sb.append("ID token expires at: ").append(idToken.getExpiresAt()).append('\n');
|
||||
} else {
|
||||
sb.append("\n-- ID token: <null> --\n");
|
||||
}
|
||||
|
||||
if (userInfo != null && userInfo.getClaims() != null) {
|
||||
sb.append("\n-- UserInfo endpoint claims (")
|
||||
.append(userInfo.getClaims().size())
|
||||
.append(") --\n");
|
||||
appendClaims(sb, userInfo.getClaims());
|
||||
} else {
|
||||
sb.append("\n-- UserInfo endpoint claims: none returned --\n");
|
||||
}
|
||||
|
||||
if (mergedAttributes != null) {
|
||||
sb.append("\n-- Merged attribute keys available to useAsUsername: ")
|
||||
.append(new TreeSet<>(mergedAttributes.keySet()))
|
||||
.append("\n");
|
||||
Object resolved = mergedAttributes.get(usernameAttributeKey);
|
||||
sb.append("-- Value at '")
|
||||
.append(usernameAttributeKey)
|
||||
.append("' : ")
|
||||
.append(resolved == null ? "<NULL — this is why login fails>" : resolved)
|
||||
.append('\n');
|
||||
|
||||
if (resolved == null) {
|
||||
Set<String> hints = suggestUsernameClaims(mergedAttributes.keySet());
|
||||
if (!hints.isEmpty()) {
|
||||
sb.append(
|
||||
"-- Hint: the following claim(s) are present and map to a"
|
||||
+ " known UsernameAttribute value — try setting"
|
||||
+ " security.oauth2.useAsUsername to one of: ")
|
||||
.append(hints)
|
||||
.append('\n');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
sb.append(
|
||||
"\nWARNING: this block contains PII. Set security.oauth2.debugLogging=false once"
|
||||
+ " troubleshooting is complete.\n");
|
||||
sb.append("========== [/OAUTH2 DEBUG] ==========");
|
||||
|
||||
if (failure) {
|
||||
log.error(sb.toString());
|
||||
} else {
|
||||
log.info(sb.toString());
|
||||
}
|
||||
}
|
||||
|
||||
private static void appendClaims(StringBuilder sb, Map<String, Object> claims) {
|
||||
if (claims == null || claims.isEmpty()) {
|
||||
sb.append(" (no claims)\n");
|
||||
return;
|
||||
}
|
||||
// Sort for stable, scannable output
|
||||
new TreeSet<>(claims.keySet())
|
||||
.forEach(
|
||||
key -> {
|
||||
Object value = claims.get(key);
|
||||
sb.append(" ").append(key).append(" = ").append(value).append('\n');
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the intersection of the claim keys the provider actually returned and the keys that
|
||||
* {@link UsernameAttribute} accepts — i.e. valid values the operator could put in {@code
|
||||
* security.oauth2.useAsUsername} to make this login work.
|
||||
*/
|
||||
private static Set<String> suggestUsernameClaims(Set<String> availableClaimKeys) {
|
||||
Set<String> supported = new TreeSet<>();
|
||||
for (UsernameAttribute attr : UsernameAttribute.values()) {
|
||||
if (availableClaimKeys.contains(attr.getName())) {
|
||||
supported.add(attr.getName());
|
||||
}
|
||||
}
|
||||
return supported;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user