exceptionfactory commented on a change in pull request #4630:
URL: https://github.com/apache/nifi/pull/4630#discussion_r530754016
##########
File path:
nifi-commons/nifi-properties/src/main/java/org/apache/nifi/util/NiFiProperties.java
##########
@@ -982,6 +984,21 @@ public String getOidcClaimIdentifyingUser() {
return getProperty(SECURITY_USER_OIDC_CLAIM_IDENTIFYING_USER,
"email").trim();
}
+ /**
+ * Returns the list of fallback claims to be used to identify a user when
the configured claim is empty for a user
+ *
+ * @return The list of fallback claims to be used to identify the user
+ */
+ public List<String> getOidcFallbackClaimsIdentifyingUser() {
+ String rawProperty =
getProperty(SECURITY_USER_OIDC_FALLBACK_CLAIMS_IDENTIFYING_USER, "").trim();
+ if (rawProperty.isEmpty()) {
+ return new ArrayList<>();
Review comment:
Recommend returning Collections.emptyList() to avoid unnecessary
ArrayList creation.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/oidc/StandardOidcIdentityProvider.java
##########
@@ -439,8 +439,21 @@ private LoginAuthenticationToken
convertOIDCTokenToLoginAuthenticationToken(OIDC
identity = claimsSet.getStringClaim(EMAIL_CLAIM);
logger.info("The 'email' claim was present. Using that claim
to avoid extra remote call");
} else {
- identity = retrieveIdentityFromUserInfoEndpoint(oidcTokens);
- logger.info("Retrieved identity from UserInfo endpoint");
+ final List<String> fallbackClaims =
properties.getOidcFallbackClaimsIdentifyingUser();
+ if (fallbackClaims.size() > 0) {
+ logger.info("fallbackClaims.size() : " +
fallbackClaims.size());
Review comment:
This is not necessary to log at the info level, and since it is a
configuration property, it seems unnecessary to log even at the debug level.
Recommend at least changing to debug and using parameterized message strings or
removing altogether.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/oidc/StandardOidcIdentityProvider.java
##########
@@ -439,8 +439,21 @@ private LoginAuthenticationToken
convertOIDCTokenToLoginAuthenticationToken(OIDC
identity = claimsSet.getStringClaim(EMAIL_CLAIM);
logger.info("The 'email' claim was present. Using that claim
to avoid extra remote call");
} else {
- identity = retrieveIdentityFromUserInfoEndpoint(oidcTokens);
- logger.info("Retrieved identity from UserInfo endpoint");
+ final List<String> fallbackClaims =
properties.getOidcFallbackClaimsIdentifyingUser();
+ if (fallbackClaims.size() > 0) {
+ logger.info("fallbackClaims.size() : " +
fallbackClaims.size());
+ for (String fallbackClaim : fallbackClaims) {
+ if (availableClaims.contains(fallbackClaim)) {
+ identity = claimsSet.getStringClaim(fallbackClaim);
+ break;
+ }
+ }
+ }
+ if (StringUtils.isBlank(identity)) {
+ identity =
retrieveIdentityFromUserInfoEndpoint(oidcTokens);
+ logger.info("Retrieved identity from UserInfo endpoint");
Review comment:
This log message is somewhat unclear and may not be necessary at the
info level. Recommend at least changing it to debug an parameterizing the
message with the retrieved identity along the following lines:
```suggestion
logger.debug("Identity [{}] retrieved from UserInfo
endpoint", identity);
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/oidc/StandardOidcIdentityProvider.java
##########
@@ -439,8 +439,21 @@ private LoginAuthenticationToken
convertOIDCTokenToLoginAuthenticationToken(OIDC
identity = claimsSet.getStringClaim(EMAIL_CLAIM);
logger.info("The 'email' claim was present. Using that claim
to avoid extra remote call");
} else {
- identity = retrieveIdentityFromUserInfoEndpoint(oidcTokens);
- logger.info("Retrieved identity from UserInfo endpoint");
+ final List<String> fallbackClaims =
properties.getOidcFallbackClaimsIdentifyingUser();
+ if (fallbackClaims.size() > 0) {
Review comment:
This size check seems unnecessary, particularly if the following log
line is removed, since the for loop will not run over an empty list.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/groovy/org/apache/nifi/web/security/oidc/StandardOidcIdentityProviderGroovyTest.groovy
##########
@@ -411,10 +411,29 @@ class StandardOidcIdentityProviderGroovyTest extends
GroovyTestCase {
assert exp <= System.currentTimeMillis() + 10_000
}
+ @Test
+ void
testconvertOIDCTokenToLoginAuthenticationTokenShouldHandleNoEmailClaimHasFallbackClaims()
{
+ // Arrange
+ StandardOidcIdentityProvider soip =
buildIdentityProviderWithMockTokenValidator(["getOidcClaimIdentifyingUser":
"email", "getOidcFallbackClaimsIdentifyingUser": ["upn"] ])
+
+ OIDCTokenResponse mockResponse = mockOIDCTokenResponse(["email": null,
"upn": "xxx@aaddomain"])
+ logger.info("OIDC Token Response with no email and upn:
${mockResponse.dump()}")
+
+ String loginToken =
soip.convertOIDCTokenToLoginAuthenticationToken(mockResponse)
+ logger.info("NiFi token create with upn: ${loginToken}")
+ // Assert
+ // Split JWT into components and decode Base64 to JSON
+ def (String contents, String expiration) =
loginToken.tokenize("\\[\\]")
+ logger.info("Token contents: ${contents} | Expiration: ${expiration}")
+ assert contents =~ "LoginAuthenticationToken for xxx@aaddomain issued
by https://accounts\\.issuer\\.com expiring at"
Review comment:
Recommend declaring a variable for the email address so that both the
mockOIDCTokenResponse() and the assert clearly use the same string.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
