Copilot commented on code in PR #10503:
URL: https://github.com/apache/gravitino/pull/10503#discussion_r2972536216
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/JwksTokenValidator.java:
##########
@@ -140,7 +151,13 @@ public Principal validateToken(String token, String
serviceAudience) {
}
// Use principal mapper to extract username
- return principalMapper.map(principal);
+ Principal userPrincipal = principalMapper.map(principal);
+ List<String> groups = extractGroups(validatedClaims);
+ if (groups != null && !groups.isEmpty()) {
+ List<String> mappedGroups = groupMapper.map(groups);
+ return new UserPrincipal(userPrincipal.getName(), mappedGroups);
+ }
Review Comment:
Group support was added here, but there’s no corresponding unit test
exercising group-claim extraction + mapping for the JWKS validator. Please add
coverage in `TestJwksTokenValidator` for at least: (1) groups claim present ->
`UserPrincipal` contains mapped groups, and (2) missing/invalid groups claim ->
behavior remains unchanged.
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/OAuthConfig.java:
##########
@@ -138,6 +138,15 @@ public interface OAuthConfig {
.toSequence()
.createWithDefault(java.util.Arrays.asList("sub"));
+ ConfigEntry<List<String>> GROUP_FIELDS =
+ new ConfigBuilder(OAUTH_CONFIG_PREFIX + "groupsFields")
+ .doc(
+ "JWT claim field(s) to use as groups. Comma-separated list for
fallback in order (e.g., 'groups,roles').")
+ .version(ConfigConstants.VERSION_1_3_0)
+ .stringConf()
+ .toSequence()
+ .createWithDefault(java.util.Arrays.asList("groups"));
Review Comment:
`GROUP_FIELDS` is registered under the config key
`gravitino.authenticator.oauth.groupsFields`, but the naming pattern in this
file (e.g., `principalFields`) and the PR description/tests use `groupFields`.
With the current key, user-provided `gravitino.authenticator.oauth.groupFields`
will be ignored and the default will always apply, breaking custom group-claim
configuration.
##########
server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java:
##########
@@ -485,4 +485,36 @@ public void testPrincipalFieldsWithMapper() {
assertNotNull(principal);
assertEquals("john.doe", principal.getName()); // Mapper extracted local
part
}
+
+ @Test
+ public void testValidateTokenWithGroups() {
+ Map<String, String> config = createBaseConfig();
+ config.put("gravitino.authenticator.oauth.groupFields", "groups");
+ validator.initialize(createConfig(config));
+
+ // Create token with groups
+ String token =
+ Jwts.builder()
+ .setSubject("test-user")
+ .claim("groups", Arrays.asList("group1", "group2"))
Review Comment:
This test sets `gravitino.authenticator.oauth.groupFields`, but the
implementation reads `OAuthConfig.GROUP_FIELDS` (currently keyed as
`gravitino.authenticator.oauth.groupsFields`). Because the default claim is
already `groups`, the test will pass even if the config is ignored; consider
using the actual config key and a non-default claim name (e.g., configure
`roles` and put `roles` in the token) so the test validates the behavior.
```suggestion
config.put("gravitino.authenticator.oauth.groupsFields", "roles");
validator.initialize(createConfig(config));
// Create token with groups in the configured claim "roles"
String token =
Jwts.builder()
.setSubject("test-user")
.claim("roles", Arrays.asList("group1", "group2"))
```
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/StaticSignKeyValidator.java:
##########
@@ -110,7 +126,13 @@ public Principal validateToken(String token, String
serviceAudience) {
String principal = extractPrincipal(jwt.getBody());
// Use principal mapper to extract username
- return principalMapper.map(principal);
+ Principal userPrincipal = principalMapper.map(principal);
+ List<String> groups = extractGroups(jwt.getBody());
+ if (groups != null && !groups.isEmpty()) {
+ List<String> mappedGroups = groupMapper.map(groups);
Review Comment:
`groupMapper.map(groups)` exceptions (e.g., from a custom `GroupMapper`
implementation) are not caught here, so a bad mapper can surface as an uncaught
runtime exception (likely 500) rather than an authentication failure. Consider
wrapping group extraction/mapping in a try/catch (e.g., `RuntimeException`) and
rethrowing as `UnauthorizedException` with a clear message.
```suggestion
List<String> mappedGroups;
try {
mappedGroups = groupMapper.map(groups);
} catch (RuntimeException e) {
throw new UnauthorizedException(e, "Failed to map user groups from
token claims");
}
```
##########
core/src/main/java/org/apache/gravitino/UserPrincipal.java:
##########
@@ -21,20 +21,39 @@
import com.google.common.base.Preconditions;
import java.security.Principal;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Objects;
/** A simple implementation of Principal that holds a username. */
public class UserPrincipal implements Principal {
private final String username;
+ private final List<String> groups;
Review Comment:
Class JavaDoc says `UserPrincipal` only holds a username, but the class now
also stores group membership. Please update the JavaDoc to reflect the new
behavior so API consumers understand the additional data carried by this
principal.
##########
core/src/main/java/org/apache/gravitino/auth/RegexGroupMapper.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.auth;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Regex-based group mapper that extracts group names using regex patterns
with capturing groups.
+ *
+ * <p>This implementation is thread-safe as Pattern.matcher() creates
thread-local Matcher
+ * instances.
+ */
+public class RegexGroupMapper implements GroupMapper {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(RegexGroupMapper.class);
+
+ private final Pattern pattern;
+
+ /**
+ * Creates a new regex group mapper.
+ *
+ * @param patternStr the regex pattern with a capturing group (required)
+ * @throws IllegalArgumentException if the pattern string has invalid regex
syntax
+ */
Review Comment:
The JavaDoc implies the regex pattern must include a capturing group, but
the implementation falls back to returning the original group when there is no
capturing group. Either enforce the requirement (fail fast on patterns without
a capturing group) or update the JavaDoc to describe the fallback behavior.
##########
server-common/src/test/java/org/apache/gravitino/server/authentication/TestStaticSignKeyValidator.java:
##########
@@ -485,4 +485,36 @@ public void testPrincipalFieldsWithMapper() {
assertNotNull(principal);
assertEquals("john.doe", principal.getName()); // Mapper extracted local
part
}
+
+ @Test
+ public void testValidateTokenWithGroups() {
+ Map<String, String> config = createBaseConfig();
+ config.put("gravitino.authenticator.oauth.groupFields", "groups");
+ validator.initialize(createConfig(config));
+
+ // Create token with groups
+ String token =
+ Jwts.builder()
+ .setSubject("test-user")
+ .claim("groups", Arrays.asList("group1", "group2"))
+ .setAudience(serviceAudience)
+ .setIssuedAt(Date.from(Instant.now()))
+ .setExpiration(Date.from(Instant.now().plusSeconds(3600)))
+ .signWith(hmacKey, SignatureAlgorithm.HS256)
+ .compact();
+
+ Principal principal = validator.validateToken(token, serviceAudience);
+ assertNotNull(principal);
+ assertEquals("test-user", principal.getName());
+
+ // Check if principal is UserPrincipal and has groups
+ if (principal instanceof org.apache.gravitino.UserPrincipal) {
+ org.apache.gravitino.UserPrincipal userPrincipal =
+ (org.apache.gravitino.UserPrincipal) principal;
+ assertEquals(Arrays.asList("group1", "group2"),
userPrincipal.getGroups());
+ } else {
+ // This fails if the principal is not UserPrincipal or groups are missing
+ throw new AssertionError("Principal should be UserPrincipal");
Review Comment:
Avoid using fully-qualified class names in the method body here
(`org.apache.gravitino.UserPrincipal`). Add an import and use
`assertInstanceOf` (or `assertTrue` + cast) instead of the manual `if/else` +
`AssertionError` for clearer test failures and to match the repository’s Java
import/style guidelines.
##########
core/src/main/java/org/apache/gravitino/UserPrincipal.java:
##########
@@ -47,25 +66,34 @@ public String getName() {
return username;
}
+ /**
+ * Returns the groups of this principal.
+ *
+ * @return the groups
+ */
+ public List<String> getGroups() {
+ return groups;
+ }
+
@Override
public int hashCode() {
- return username.hashCode();
+ return Objects.hash(username, groups);
}
@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
- if (o instanceof UserPrincipal) {
- UserPrincipal that = (UserPrincipal) o;
- return this.username.equals(that.username);
+ if (!(o instanceof UserPrincipal)) {
+ return false;
}
- return false;
+ UserPrincipal that = (UserPrincipal) o;
+ return Objects.equals(username, that.username) && Objects.equals(groups,
that.groups);
}
Review Comment:
`equals`/`hashCode` now include `groups`, which changes `UserPrincipal`
identity semantics: the same username with different groups will no longer be
equal. This can cause duplicate `UserPrincipal`s in `Subject` principal sets
and make lookups/caching keyed by principal unstable as group membership
changes over time. Consider keeping equality/hashCode based only on `username`
(treating groups as attributes), or introduce a separate type for “user +
groups” if you need value semantics.
##########
server-common/src/main/java/org/apache/gravitino/server/authentication/StaticSignKeyValidator.java:
##########
@@ -138,6 +160,26 @@ private String extractPrincipal(Claims claims) {
"No valid principal found in token. Checked fields: %s",
principalFields);
}
+ /** Extracts the groups from the validated JWT claims using configured
field(s). */
+ private List<String> extractGroups(Claims claims) {
+ if (groupFields != null && !groupFields.isEmpty()) {
+ for (String field : groupFields) {
+ if (StringUtils.isNotBlank(field)) {
+ try {
+ Object groups = claims.get(field);
+ if (groups instanceof List) {
+ return (List<String>) groups;
+ }
+ } catch (Exception e) {
+ LOG.warn("Failed to parse groups from claim field: {}", field, e);
+ }
Review Comment:
`extractGroups` performs an unchecked cast to `List<String>` after only
checking `instanceof List`, which can throw `ClassCastException` at runtime or
silently drop groups (because it’s caught as `Exception`). Safer options are to
read a typed list (if supported) or convert `List<?>` to `List<String>` by
mapping each element via `String.valueOf`, and narrow the catch to the expected
exception type(s).
##########
core/src/main/java/org/apache/gravitino/auth/GroupMapperFactory.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.auth;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Factory class for creating {@link GroupMapper} instances. */
+public class GroupMapperFactory {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(GroupMapperFactory.class);
+
+ private GroupMapperFactory() {}
+
+ /**
+ * Creates a GroupMapper instance based on the configuration.
+ *
+ * @param mapperType the type of the mapper (e.g., "regex" or fully
qualified class name)
+ * @param regexPattern the regex pattern to use (only for "regex" mapper
type)
+ * @return a configured GroupMapper instance
+ * @throws IllegalArgumentException if the mapper type is invalid or
initialization fails
+ */
+ public static GroupMapper create(String mapperType, String regexPattern) {
+ if ("regex".equalsIgnoreCase(mapperType)) {
+ if (regexPattern == null) {
+ throw new IllegalArgumentException("Regex pattern cannot be null for
regex mapper");
+ }
+ return new RegexGroupMapper(regexPattern);
+ }
+
+ try {
+ Class<?> clazz = Class.forName(mapperType);
+ if (!GroupMapper.class.isAssignableFrom(clazz)) {
+ throw new IllegalArgumentException(
+ "Class " + mapperType + " does not implement GroupMapper");
+ }
+ return (GroupMapper) clazz.getDeclaredConstructor().newInstance();
+ } catch (Exception e) {
+ LOG.error("Failed to create GroupMapper: {}", mapperType, e);
+ throw new IllegalArgumentException("Failed to create GroupMapper: " +
mapperType, e);
+ }
Review Comment:
`GroupMapperFactory.create` catches a generic `Exception`, which makes it
harder to distinguish misconfiguration cases (e.g., class not found vs.
constructor failure) and conflicts with the project guideline to handle
exceptions specifically. Consider catching `ClassNotFoundException` separately
(to report an “unknown mapper type”), and catching
`ReflectiveOperationException` for instantiation issues; also consider
documenting that custom mappers must have a public no-arg constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
