fix: Revised the principal field retrieval#27014
Merged
yhwang merged 1 commit intoprestodb:masterfrom Apr 3, 2026
Merged
Conversation
prestodb-ci
requested review from
a team,
ShahimSharafudeen and
anandamideShakyan
and removed request for
a team
Contributor
Reviewer's GuideRefactors OAuth2/OIDC principal and claims handling to prioritize ID token per OIDC spec, optionally cache UserInfo responses, and propagate rich user claims through token cookies and authenticators while tightening and clarifying UserInfo parsing, audience validation, and error handling. Sequence diagram for OIDC principal resolution using ID token and fallback UserInfosequenceDiagram
actor User
participant Browser
participant OAuth2Authenticator
participant TokenPairSerializer
participant OAuth2Client as NimbusOAuth2Client
participant IdP
User->>Browser: Access protected resource
Browser->>OAuth2Authenticator: HTTP request with auth cookie
OAuth2Authenticator->>TokenPairSerializer: deserialize(cookieValue)
TokenPairSerializer-->>OAuth2Authenticator: TokenPair (accessToken, expiration, claims)
OAuth2Authenticator->>OAuth2Authenticator: validate expiration
OAuth2Authenticator->>OAuth2Authenticator: claims = tokenPair.getClaims()
alt claims present
OAuth2Authenticator->>OAuth2Authenticator: use claims for principalField
else claims absent
OAuth2Authenticator->>OAuth2Client: getClaims(accessToken)
OAuth2Client->>OAuth2Client: getJWTClaimsSetFromIdToken(idToken)
alt principalField in ID token claims
OAuth2Client-->>OAuth2Authenticator: claims with principalField
else principalField missing in ID token
OAuth2Client->>OAuth2Client: queryUserInfo(accessToken)
OAuth2Client->>IdP: /userinfo with Bearer accessToken
IdP-->>OAuth2Client: 200 OK with userinfo JSON
OAuth2Client->>OAuth2Client: parse(httpResponse)
OAuth2Client-->>OAuth2Authenticator: claims with principalField
end
end
OAuth2Authenticator->>OAuth2Authenticator: build Principal from claims[principalField]
OAuth2Authenticator-->>Browser: successful authentication
Sequence diagram for OAuth2 UserInfo caching and principal extractionsequenceDiagram
participant Component as Caller
participant NimbusOAuth2Client as OAuth2Client
participant UserInfoCache as Cache
participant IdP
Component->>NimbusOAuth2Client: toResponse(Tokens tokens, existingRefreshToken)
NimbusOAuth2Client->>NimbusOAuth2Client: getUserClaims(accessToken)
alt userinfoUrl configured
NimbusOAuth2Client->>NimbusOAuth2Client: queryUserInfo(accessToken)
alt cache enabled
NimbusOAuth2Client->>NimbusOAuth2Client: cacheKey = computeCacheKey(accessToken)
NimbusOAuth2Client->>UserInfoCache: getIfPresent(cacheKey)
alt cache hit
UserInfoCache-->>NimbusOAuth2Client: cached JWTClaimsSet
NimbusOAuth2Client-->>Component: Response(accessToken, expiration, refreshToken, claims)
else cache miss
UserInfoCache-->>NimbusOAuth2Client: null
NimbusOAuth2Client->>IdP: /userinfo with Bearer accessToken
IdP-->>NimbusOAuth2Client: 200 OK with userinfo JSON
NimbusOAuth2Client->>NimbusOAuth2Client: claims = fetchUserInfoClaims(accessToken)
NimbusOAuth2Client->>UserInfoCache: put(cacheKey, claims)
NimbusOAuth2Client-->>Component: Response(accessToken, expiration, refreshToken, claims)
end
else cache disabled
NimbusOAuth2Client->>IdP: /userinfo with Bearer accessToken
IdP-->>NimbusOAuth2Client: 200 OK with userinfo JSON
NimbusOAuth2Client-->>Component: Response(accessToken, expiration, refreshToken, claims)
end
else userinfoUrl not configured
NimbusOAuth2Client->>NimbusOAuth2Client: parseAccessToken(accessToken)
NimbusOAuth2Client-->>Component: Response with accessToken claims or error
end
Sequence diagram for token cookie serialization with embedded claimssequenceDiagram
participant OAuth2Flow as OAuth2CallbackHandler
participant NimbusOAuth2Client as OAuth2Client
participant TokenPair as TokenPair
participant TokenPairSerializer as AccessTokenClaimsSerializer
participant Browser
OAuth2Flow->>NimbusOAuth2Client: exchange authorization code
NimbusOAuth2Client-->>OAuth2Flow: Response(accessToken, expiration, refreshToken, claims)
OAuth2Flow->>TokenPair: fromOAuth2Response(Response)
TokenPair-->>OAuth2Flow: TokenPair(accessToken, expiration, refreshToken, claims)
OAuth2Flow->>TokenPairSerializer: serialize(TokenPair)
alt claims present
TokenPairSerializer->>TokenPairSerializer: build Map(accessToken, claims)
TokenPairSerializer->>TokenPairSerializer: json = ObjectMapper.writeValueAsString(map)
TokenPairSerializer->>TokenPairSerializer: cookieValue = Base64.encode(json)
else no claims
TokenPairSerializer->>TokenPairSerializer: cookieValue = accessToken
end
TokenPairSerializer-->>OAuth2Flow: cookieValue
OAuth2Flow->>Browser: Set-Cookie auth=cookieValue
Class diagram for updated OAuth2/OIDC principal and claims handlingclassDiagram
class OAuth2Config {
-Optional~String~ stateKey
-Optional~String~ principalField
-boolean enableRefreshTokens
-boolean enableDiscovery
-boolean userinfoCacheEnabled
-Duration userinfoCacheTtl
+boolean isUserinfoCacheEnabled()
+OAuth2Config setUserinfoCacheEnabled(boolean userinfoCacheEnabled)
+Duration getUserinfoCacheTtl()
+OAuth2Config setUserinfoCacheTtl(Duration userinfoCacheTtl)
}
class OAuth2Client {
}
class OAuth2Client_Response {
-String accessToken
-Instant expiration
-Optional~String~ refreshToken
-Map~String,Object~ claims
+Response(String accessToken, Instant expiration, Optional~String~ refreshToken)
+Response(String accessToken, Instant expiration, Optional~String~ refreshToken, Map~String,Object~ claims)
+String getAccessToken()
+Instant getExpiration()
+Optional~String~ getRefreshToken()
+Optional~Map~String,Object~~ getClaims()
}
class NimbusOAuth2Client {
-Issuer issuer
-ClientID clientId
-ClientSecretBasic clientAuth
-Optional~URI~ userinfoUrl
-JWTProcessor~SecurityContext~ accessTokenProcessor
-AuthorizationCodeFlow flow
-Cache~String,JWTClaimsSet~ userInfoCache
-boolean userinfoCacheEnabled
+NimbusOAuth2Client(OAuth2Config oauthConfig, OAuth2ServerConfigProvider serverConfigurationProvider, NimbusHttpClient httpClient)
+void load()
-Response toResponse(Tokens tokens, Optional~String~ existingRefreshToken)
-Response toResponse(OIDCTokens tokens, Optional~String~ existingRefreshToken)
-Optional~JWTClaimsSet~ getUserClaims(String accessToken)
-Optional~JWTClaimsSet~ getJWTClaimsSet(String accessToken)
-Optional~JWTClaimsSet~ getJWTClaimsSetFromIdToken(JWT idToken)
-Optional~JWTClaimsSet~ queryUserInfo(String accessToken)
-String computeCacheKey(String accessToken)
-JWTClaimsSet fetchUserInfoClaims(String accessToken)
+UserInfoResponse parse(HTTPResponse httpResponse)
-Optional~JWTClaimsSet~ parseAccessToken(String accessToken)
}
class TokenPairSerializer {
<<interface>>
+TokenPair deserialize(String token)
+String serialize(TokenPair tokenPair)
}
class TokenPairSerializer_ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER {
+TokenPair deserialize(String token)
+String serialize(TokenPair tokenPair)
}
class TokenPair {
-String accessToken
-Date expiration
-Optional~String~ refreshToken
-Optional~Map~String,Object~~ claims
-TokenPair(String accessToken, Date expiration, Optional~String~ refreshToken)
-TokenPair(String accessToken, Date expiration, Optional~String~ refreshToken, Optional~Map~String,Object~~ claims)
+static TokenPair accessToken(String accessToken)
+static TokenPair fromOAuth2Response(OAuth2Client_Response tokens)
+static TokenPair accessAndRefreshTokens(String accessToken, Date expiration, String refreshToken)
+static TokenPair withAccessAndRefreshTokens(String accessToken, Date expiration, String refreshToken)
+String getAccessToken()
+Date getExpiration()
+Optional~String~ getRefreshToken()
+Optional~Map~String,Object~~ getClaims()
}
class OAuth2Authenticator {
-OAuth2Client client
-TokenPairSerializer tokenPairSerializer
+Principal authenticate(HttpServletRequest request)
}
class JweTokenSerializer {
-OAuth2Client client
-String principalField
-Clock clock
-Duration tokenExpiration
-String audience
-String issuer
+String serialize(TokenPair tokenPair)
}
class NimbusHttpClient {
}
OAuth2Client o-- OAuth2Client_Response : uses
NimbusOAuth2Client ..|> OAuth2Client
OAuth2Client_Response <-- TokenPair : fromOAuth2Response
TokenPairSerializer <|.. TokenPairSerializer_ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER
OAuth2ServiceModule ..> TokenPairSerializer_ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER : binds
TokenPairSerializer ..> TokenPair : create/parse
OAuth2Authenticator --> TokenPairSerializer : uses
OAuth2Authenticator --> OAuth2Client : uses
JweTokenSerializer --> OAuth2Client : client
JweTokenSerializer --> TokenPair : consumes
NimbusOAuth2Client --> NimbusHttpClient : httpClient
NimbusOAuth2Client --> OAuth2Config : configuration
NimbusOAuth2Client --> OAuth2Client_Response : creates
NimbusOAuth2Client --> JWTClaimsSet : produces
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-main/src/test/java/com/facebook/presto/server/security/oauth2/TestNimbusOAuth2ClientUserInfoParser.java:318-327` </location>
<code_context>
+ }
+
+ @Test
+ public void testParseErrorResponse()
+ throws Exception
+ {
+ NimbusOAuth2Client client = createClient("sub", ADDITIONAL_AUDIENCE);
+ HTTPResponse httpResponse = new HTTPResponse(401);
+ httpResponse.setContentType("application/json");
+ JSONObject body = new JSONObject();
+ body.put("error", "invalid_token");
+ body.put("error_description", "The access token is invalid");
+ httpResponse.setBody(body.toJSONString());
+
+ assertThat(client.parse(httpResponse).indicatesSuccess()).isFalse();
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen error response test by asserting error details, not only indicatesSuccess()
Since non-200 responses now return `UserInfoErrorResponse.parse(httpResponse)` directly, this test could assert the parsed error details as well, not just `indicatesSuccess()`. For example:
```java
UserInfoResponse response = client.parse(httpResponse);
assertThat(response.indicatesSuccess()).isFalse();
UserInfoErrorResponse errorResponse = response.toErrorResponse();
assertThat(errorResponse.getErrorObject().getCode()).isEqualTo("invalid_token");
assertThat(errorResponse.getErrorObject().getDescription()).isEqualTo("The access token is invalid");
```
This verifies the error body is correctly propagated, not only that the response is non-success.
Suggested implementation:
```java
@Test
public void testParseErrorResponse()
throws Exception
{
NimbusOAuth2Client client = createClient("sub", ADDITIONAL_AUDIENCE);
HTTPResponse httpResponse = new HTTPResponse(401);
httpResponse.setContentType("application/json");
JSONObject body = new JSONObject();
body.put("error", "invalid_token");
body.put("error_description", "The access token is invalid");
httpResponse.setBody(body.toJSONString());
UserInfoResponse response = client.parse(httpResponse);
assertThat(response.indicatesSuccess()).isFalse();
UserInfoErrorResponse errorResponse = response.toErrorResponse();
assertThat(errorResponse.getErrorObject().getCode()).isEqualTo("invalid_token");
assertThat(errorResponse.getErrorObject().getDescription()).isEqualTo("The access token is invalid");
}
```
```java
import com.nimbusds.oauth2.sdk.http.HTTPResponse;
import com.nimbusds.openid.connect.sdk.UserInfoErrorResponse;
import com.nimbusds.openid.connect.sdk.UserInfoResponse;
import com.nimbusds.openid.connect.sdk.UserInfoSuccessResponse;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...st/java/com/facebook/presto/server/security/oauth2/TestNimbusOAuth2ClientUserInfoParser.java
Show resolved
Hide resolved
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
4b42156 to
92337bb
Compare
yhwang
changed the title
yhwang
changed the title
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
92337bb to
1e807dc
Compare
Member
Author
|
Manually verified the integration (both Web UI and CLI) with Not able to integrate with GitHub OAuth, the reason is that its issuer and the OIDC discovery path are different. We may need to provide an extra config setting for the OIDC discovery path to fulfill this case. Plan to also verify the Okta service as well. I will provide updates later. |
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
1e807dc to
edab2ab
Compare
yhwang
changed the title
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
NimbusOAuth2Clientthe userinfo cache TTL is converted to minutes using integer division oftoMillis(); consider passing the configuredDurationdirectly toexpireAfterWrite(e.g., withTimeUnit.MILLISECONDS) to avoid silent rounding and ensure the cache TTL matches the configuration exactly. - The
BaseOAuth2AuthenticationFilterTest.extractAccessTokenhelper assumes all cookies are in the new Base64-encoded JSON format; if backward compatibility with plain access tokens is important, it may be worth updating this helper to transparently support both formats similar toACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER.deserialize.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `NimbusOAuth2Client` the userinfo cache TTL is converted to minutes using integer division of `toMillis()`; consider passing the configured `Duration` directly to `expireAfterWrite` (e.g., with `TimeUnit.MILLISECONDS`) to avoid silent rounding and ensure the cache TTL matches the configuration exactly.
- The `BaseOAuth2AuthenticationFilterTest.extractAccessToken` helper assumes all cookies are in the new Base64-encoded JSON format; if backward compatibility with plain access tokens is important, it may be worth updating this helper to transparently support both formats similar to `ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER.deserialize`.
## Individual Comments
### Comment 1
<location> `presto-main/src/main/java/com/facebook/presto/server/security/oauth2/TokenPairSerializer.java:49-58` </location>
<code_context>
+ */
+ private Optional<JWTClaimsSet> getJWTClaimsSetFromIdToken(com.nimbusds.jwt.JWT idToken)
+ {
+ try {
+ return Optional.of(idToken.getJWTClaimsSet());
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Consider limiting exception-driven control flow in deserialize() to avoid repeated decode/parse failures on the hot path
The new `ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER.deserialize` always tries Base64 decode + JSON parse and uses `IllegalArgumentException` / `IOException` to detect legacy plain tokens. While many tokens are still in the old format, this means an exception per deserialize call, which is costly on hot paths like authentication. To avoid that, consider adding a version prefix/marker (e.g. `v2:`) to the new format and branching on that, so you only run Base64/JSON when the marker is present, preserving backward compatibility without relying on exceptions for control flow.
Suggested implementation:
```java
private static final Logger LOG = Logger.get(TokenPairSerializer.class);
private final ObjectMapper objectMapper = new ObjectMapper();
// Prefix marker for new Base64-encoded JSON token format with claims
private static final String NEW_FORMAT_PREFIX = "v2:";
```
```java
@Override
public TokenPair deserialize(String token)
{
// Old format: plain access token without prefix (backward compatibility, no exceptions on hot path)
if (!token.startsWith(NEW_FORMAT_PREFIX)) {
LOG.debug("Using plain access token format (no claims available, will need to query on authentication)");
return TokenPair.accessToken(token);
}
// New format: strip prefix, then decode from Base64 and parse as JSON with claims
String encodedPayload = token.substring(NEW_FORMAT_PREFIX.length());
try {
byte[] decodedBytes = Base64.getDecoder().decode(encodedPayload);
String decodedJson = new String(decodedBytes, UTF_8);
@SuppressWarnings("unchecked")
Map<String, Object> data = objectMapper.readValue(decodedJson, Map.class);
if (data.containsKey("accessToken") && data.containsKey("claims")) {
String accessToken = (String) data.get("accessToken");
@SuppressWarnings("unchecked")
Map<String, Object> claims = (Map<String, Object>) data.get("claims");
LOG.debug("Deserialized token with claims from new Base64-encoded JSON format");
return new TokenPair(accessToken, new Date(MAX_VALUE), Optional.empty(), Optional.of(claims));
}
// If the JSON structure is not as expected, fall back to plain access token
LOG.warn("Token with new-format prefix does not contain expected fields, treating as plain access token");
}
catch (IllegalArgumentException | IOException e) {
// If decoding/parsing fails for a supposedly new-format token, fall back to plain token
LOG.warn("Failed to deserialize token in new Base64-encoded JSON format, treating as plain access token", e);
}
return TokenPair.accessToken(token);
}
```
To fully implement the versioned format and keep things consistent:
1. Update the corresponding `serialize(TokenPair tokenPair)` implementation inside `ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER` to prepend `NEW_FORMAT_PREFIX` to the Base64-encoded JSON string it currently produces, e.g.:
`return NEW_FORMAT_PREFIX + Base64.getEncoder().encodeToString(jsonBytes);`
This ensures that only tokens produced in the new format carry the prefix and go through Base64/JSON parsing on deserialize, while legacy tokens (without prefix) remain fast-path without exception-driven control flow.
2. If there are tests for `ACCESS_TOKEN_CLAIMS_ONLY_SERIALIZER`, update them to expect the `v2:` prefix for newly serialized tokens and to verify that legacy, unprefixed tokens still deserialize correctly as plain access tokens.
</issue_to_address>
### Comment 2
<location> `presto-main/src/test/java/com/facebook/presto/server/security/oauth2/BaseOAuth2AuthenticationFilterTest.java:332-341` </location>
<code_context>
}
}
+ private String extractAccessToken(String cookieValue)
+ {
+ // Decode Base64-encoded JSON to extract access token
+ byte[] decodedBytes = java.util.Base64.getDecoder().decode(cookieValue);
+ String decodedJson = new String(decodedBytes, java.nio.charset.StandardCharsets.UTF_8);
+ java.util.Map<String, Object> data = JsonCodec.mapJsonCodec(String.class, Object.class).fromJson(decodedJson);
+ String accessToken = (String) data.get("accessToken");
+ if (accessToken == null) {
+ throw new IllegalStateException("Cookie value does not contain 'accessToken' field: " + decodedJson);
+ }
+ return accessToken;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Helper claims to handle both old and new cookie formats but only supports the new Base64-encoded JSON format
`extractAccessToken` is described as supporting both the old plain-token cookie and the new Base64-encoded JSON format, but it always Base64-decodes and parses JSON. A legacy cookie containing just the raw token will therefore fail. Either update the helper to detect and handle the old format (and add tests for both), or change the documentation/tests to state that only the new format is supported and add a test that explicitly rejects the old format.
Suggested implementation:
```java
private String extractAccessToken(String cookieValue)
{
if (cookieValue == null) {
throw new IllegalArgumentException("cookieValue is null");
}
// First, attempt to interpret the cookie value as Base64-encoded JSON containing an "accessToken" field
try {
byte[] decodedBytes = java.util.Base64.getDecoder().decode(cookieValue);
String decodedJson = new String(decodedBytes, java.nio.charset.StandardCharsets.UTF_8);
java.util.Map<String, Object> data = JsonCodec.mapJsonCodec(String.class, Object.class).fromJson(decodedJson);
Object accessToken = data.get("accessToken");
if (accessToken instanceof String && !((String) accessToken).isEmpty()) {
return (String) accessToken;
}
// If the JSON is valid but doesn't contain a non-empty "accessToken", fall back to the legacy format
}
catch (IllegalArgumentException | RuntimeException ignored) {
// If Base64 decoding or JSON parsing fails, fall back to the legacy plain-token format
}
// Legacy behavior: cookie value is the raw access token
return cookieValue;
}
```
`) to cover both formats and a negative case.
Here are the code changes:
<file_operations>
<file_operation operation="edit" file_path="presto-main/src/test/java/com/facebook/presto/server/security/oauth2/BaseOAuth2AuthenticationFilterTest.java">
<<<<<<< SEARCH
private String extractAccessToken(String cookieValue)
{
// Decode Base64-encoded JSON to extract access token
byte[] decodedBytes = java.util.Base64.getDecoder().decode(cookieValue);
String decodedJson = new String(decodedBytes, java.nio.charset.StandardCharsets.UTF_8);
java.util.Map<String, Object> data = JsonCodec.mapJsonCodec(String.class, Object.class).fromJson(decodedJson);
String accessToken = (String) data.get("accessToken");
if (accessToken == null) {
throw new IllegalStateException("Cookie value does not contain 'accessToken' field: " + decodedJson);
}
return accessToken;
}
=======
private String extractAccessToken(String cookieValue)
{
if (cookieValue == null) {
throw new IllegalArgumentException("cookieValue is null");
}
// First, attempt to interpret the cookie value as Base64-encoded JSON containing an "accessToken" field
try {
byte[] decodedBytes = java.util.Base64.getDecoder().decode(cookieValue);
String decodedJson = new String(decodedBytes, java.nio.charset.StandardCharsets.UTF_8);
java.util.Map<String, Object> data = JsonCodec.mapJsonCodec(String.class, Object.class).fromJson(decodedJson);
Object accessToken = data.get("accessToken");
if (accessToken instanceof String && !((String) accessToken).isEmpty()) {
return (String) accessToken;
}
// If the JSON is valid but doesn't contain a non-empty "accessToken", fall back to the legacy format
}
catch (IllegalArgumentException | RuntimeException ignored) {
// If Base64 decoding or JSON parsing fails, fall back to the legacy plain-token format
}
// Legacy behavior: cookie value is the raw access token
return cookieValue;
}
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement your comment, the following tests should be added/updated in `BaseOAuth2AuthenticationFilterTest` (or the corresponding test class that uses `extractAccessToken`):
1. **New format test**: A test that constructs a cookie whose value is a Base64-encoded JSON string like `{"accessToken":"token-from-json"}` and verifies that `extractAccessToken` (via the existing flow) uses `token-from-json` as the Bearer token and successfully validates it.
2. **Legacy format test**: A test that constructs a cookie whose value is a plain token string (no Base64, no JSON) and verifies that this token is used as-is and successfully validated.
3. **Negative/invalid format test**: A test that passes an invalid Base64 string or JSON without an `accessToken` field and verifies that the legacy path is used (i.e., the cookie value is treated as the token). If your intent is instead to reject such malformed cookies, adjust the fallback logic accordingly and assert that an appropriate exception is thrown or that validation fails with the expected status.
You will need to integrate these tests with the existing setup that creates cookies, hits the `/userinfo` endpoint, and asserts on the response/claims so that both formats are exercised end-to-end.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/TokenPairSerializer.java
Show resolved
Hide resolved
...test/java/com/facebook/presto/server/security/oauth2/BaseOAuth2AuthenticationFilterTest.java
Show resolved
Hide resolved
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
2 times, most recently
from
af9f392 to
60ea316
Compare
Member
Author
|
Updated the code to address this comment:
With the changes, I verified the following IdPs:
In summary, if the user specifies |
steveburnett
previously approved these changes
Feb 12, 2026
Contributor
steveburnett
left a comment
steveburnett
left a comment
There was a problem hiding this comment.
LGTM! (docs)
Pull branch, local doc build, looks good. Thanks!
Contributor
auden-woolfson
left a comment
auden-woolfson
left a comment
There was a problem hiding this comment.
Overall LGTM, I think you should comb through all the comments and remove some of them that might be redundant/the code speaks for itself.
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/NimbusOAuth2Client.java
Show resolved
Hide resolved
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
60ea316 to
962e3c0
Compare
Member
Author
|
Hi @tdcmeehan, not sure who I should tag to bring awareness of the build failures about the version of the testcontainer that we are currently using: I tried to look up solutions for this and here are the potential ones:
I can update the testcontainer version in my PR and see if those errors in the CI would be fixed. How do you think? |
Member
Author
|
I found that updating the |
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
962e3c0 to
499d4b5
Compare
Member
Author
|
@prestodb/committers could anyone help with the review? Thanks! |
Contributor
|
@auden-woolfson can you do a first pass? |
auden-woolfson
requested changes
Feb 25, 2026
Contributor
auden-woolfson
left a comment
auden-woolfson
left a comment
There was a problem hiding this comment.
Few small changes
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/JweTokenSerializer.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/NimbusOAuth2Client.java
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/OAuth2Authenticator.java
Outdated
Show resolved
Hide resolved
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
499d4b5 to
19695c2
Compare
Member
Author
|
@auden-woolfson Thank you for the review! I've implemented the nested condition suggestions in JweTokenSerializer and OAuth2Authenticator. But I keep the |
Enhance OAuth2/OIDC authentication to be more standards-compliant and efficient: Features: - Extract principal from ID token per OIDC specification - Preserve user claims in cookies without refresh tokens - Add configurable UserInfo response caching - Serialize cookies as Base64-encoded JSON for safety Improvements: - Reduce redundant UserInfo endpoint queries - Better performance and lower IdP load - Improved error handling and validation Configuration: - Add http-server.authentication.oauth2.userinfo-cache (default: false) - Add http-server.authentication.oauth2.userinfo-cache-ttl (default: 10m) Signed-off-by: Yihong Wang <yh.wang@ibm.com>
yhwang
force-pushed
the
oauth-aud-optional-userinfo
branch
from
19695c2 to
24e0235
Compare
Member
Author
|
Just updated the commit message to rerun the CI jobs. No code change. There were some stale jobs earlier. |
auden-woolfson
approved these changes
Feb 27, 2026
Contributor
auden-woolfson
left a comment
auden-woolfson
left a comment
There was a problem hiding this comment.
LGTM! Thanks for your work on this
Member
Author
|
@tdcmeehan , anyone you'd like them to review the changes? |
Member
Author
|
I believe the changes improve compliance with the OAuth/OIDC spec. The changes also properly work with the IdPs I listed here. I'd like to know if we are okay to proceed with the change or if there are any comments or suggestions. |
Member
Author
|
Want to poke again @prestodb/committers. Any comment on the changes? @auden-woolfson is the one who integrated the OAuth commits from Trino into Presto. He approved the changes. I see it's a good sign to move forward. |
tdcmeehan
approved these changes
Apr 3, 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
5 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.
Description
Motivation and Context
audclaim is not mandatory in the userinfo response. This change makes theaudvalidation in the userinfo optional.principal fieldfrom ID token if it's OIDC flow. For the OAuth2 flow, retrieve theprincipal fieldfrom userinfo and fallback to the access token.Impact
Make the OAuth flow comply with the spec and correctly verify the
audclaim and handle the principal field that the user specified.Test Plan
Make sure no regression and existing integration tests pass. Add test cases for the userinfo parsing.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Relax OAuth2 user info audience and principal handling while enforcing JWT access token validation and improving error handling.
Bug Fixes:
Enhancements:
Tests:
Summary by Sourcery
Align OAuth2/OIDC authentication with spec by deriving principals from ID tokens or userinfo, caching userinfo responses, and preserving user claims in cookies for flows without refresh tokens.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: