exceptionfactory commented on code in PR #9566:
URL: https://github.com/apache/nifi/pull/9566#discussion_r1870069339
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -82,6 +85,27 @@ public String getUserIdentityFromToken(final String
base64EncodedToken) throws J
}
}
+ public Set<String> getUserGroupsFromToken(final String base64EncodedToken)
throws JwtException {
+ // The library representations of the JWT should be kept internal to
this service.
+ try {
+ final Jws<Claims> jws =
parseTokenFromBase64EncodedString(base64EncodedToken);
+
+ if (jws == null) {
+ throw new JwtException("Unable to parse token");
+ }
+
+ @SuppressWarnings("unchecked")
+ ArrayList<String> groupsString =
jws.getPayload().get(GROUPS_CLAIM, ArrayList.class);
+
+ return new HashSet<>(groupsString);
+ } catch (JwtException e) {
+ logger.debug("The Base64 encoded JWT: " + base64EncodedToken);
+ final String errorMessage = "There was an error validating the
JWT";
+ logger.error(errorMessage, e);
+ throw e;
Review Comment:
Logging the exception as an error here should be unnecessary. If it is
necessary to convey additional details, throwing a new exception that has the
`JwtException` as a cause could work.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -34,11 +34,13 @@
import org.apache.nifi.registry.security.key.KeyService;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.security.core.GrantedAuthority;
import org.springframework.stereotype.Service;
import java.nio.charset.StandardCharsets;
-import java.util.Calendar;
+import java.util.*;
Review Comment:
Coding conventions require explicit imports, so this change should be
reverted.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -82,6 +85,27 @@ public String getUserIdentityFromToken(final String
base64EncodedToken) throws J
}
}
+ public Set<String> getUserGroupsFromToken(final String base64EncodedToken)
throws JwtException {
Review Comment:
As mentioned on the calling reference, decoding, parsing, and verifying the
token more than once is a concern because it happens on every HTTP request.
Evaluating an alternative approach that parses and verifies the token,
returning a structured set of claims, would be a better path forward.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -82,6 +85,27 @@ public String getUserIdentityFromToken(final String
base64EncodedToken) throws J
}
}
+ public Set<String> getUserGroupsFromToken(final String base64EncodedToken)
throws JwtException {
+ // The library representations of the JWT should be kept internal to
this service.
+ try {
+ final Jws<Claims> jws =
parseTokenFromBase64EncodedString(base64EncodedToken);
+
+ if (jws == null) {
+ throw new JwtException("Unable to parse token");
+ }
+
+ @SuppressWarnings("unchecked")
+ ArrayList<String> groupsString =
jws.getPayload().get(GROUPS_CLAIM, ArrayList.class);
+
+ return new HashSet<>(groupsString);
+ } catch (JwtException e) {
+ logger.debug("The Base64 encoded JWT: " + base64EncodedToken);
Review Comment:
The token itself should never be logged because it provides access to the
REST API, so this line should be removed.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtIdentityProvider.java:
##########
@@ -70,7 +71,8 @@ public AuthenticationResponse
authenticate(AuthenticationRequest authenticationR
try {
final String jwtPrincipal =
jwtService.getUserIdentityFromToken(jwtAuthToken);
- return new AuthenticationResponse(jwtPrincipal, jwtPrincipal,
expiration, issuer);
+ final Set<String> groups =
jwtService.getUserGroupsFromToken(jwtAuthToken);
Review Comment:
This approach requires parsing the authentication token string twice, which
is not very efficient. It probably requires some refactoring to do this once,
and then extract the claims required.
##########
nifi-registry/nifi-registry-core/nifi-registry-security-api/src/main/java/org/apache/nifi/registry/security/authorization/UserGroupProvider.java:
##########
@@ -75,6 +75,30 @@ public interface UserGroupProvider {
*/
Group getGroup(String identifier) throws AuthorizationAccessException;
+ /**
+ * Retrieves a Group by name.
+ *
+ * @param name the name of the group to retrieve
+ * @return the Group with the given name, or null if no matching group was
found
+ * @throws AuthorizationAccessException if there was an unexpected error
performing the operation
+ */
+ default Group getGroupByName(String name) throws
AuthorizationAccessException {
Review Comment:
As this method appears to be used only in the `StandardManagedAuthorizer`,
it does not seem like a good candidate for adding to the `UserGroupProvider`
interface right now.
--
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]
