anchela commented on code in PR #26:
URL:
https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/26#discussion_r2244744502
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -278,6 +279,7 @@ public AuthenticationInfo extractCredentials(
UserInfo userInfo =
userInfoResponse.toSuccessResponse().getUserInfo();
// process credentials
+ UserInfoProcessor userInfoProcessor =
userInfoProcessors.get(connection.name());
Review Comment:
if there is no matching processor in the map this will return null and
result in a NPE on the next line. i would add defense against NPE.
you can still throw a runtime exception like IllegalStateException but that
indicates this you know what's happening and inform the caller accordingly
about a misconfiguration. NPE looks a bit sloppy
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcAuthenticationHandler.java:
##########
@@ -289,6 +291,7 @@ public AuthenticationInfo extractCredentials(
throw new RuntimeException(e);
}
} else {
+ UserInfoProcessor userInfoProcessor =
userInfoProcessors.get(connection.name());
Review Comment:
same here -> maybe refactor it into a method that avoids duplicating the
handling and exception throwing
##########
src/test/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImplTest.java:
##########
Review Comment:
i would add tests for edge case values of 'connection' like empty string and
null.... if those values are permitted.
##########
src/test/java/org/apache/sling/auth/oauth_client/AuthorizationCodeFlowIT.java:
##########
@@ -437,7 +437,8 @@ void
accessTokenIsPresentOnSuccessfulAuthenticationHandlerLogin(boolean withPkce
null,
Map.of(
"storeAccessToken", "true",
- "storeRefreshToken", "true")));
+ "storeRefreshToken", "true",
+ "connection", oidcConnectionName)));
Review Comment:
if the connection value can be null -> add a test for that case.
in general: also add a test case with a connection name that doesn't match
or is empty string or null if that is an option at all
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -187,4 +192,9 @@ public SlingUserInfoProcessorImpl(
throw new RuntimeException("Failed to parse TokenResponse in
UserInfoProcessor", e);
}
}
+
+ @Override
+ public @NotNull String connection() {
Review Comment:
it's marked as @notnull.... is this guaranteed? see my question above about
the possibility that config.connection() is null
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -67,6 +67,9 @@ public class SlingUserInfoProcessorImpl implements
UserInfoProcessor {
description = "Name of the claim in the ID Token or UserInfo
that contains the groups. "
+ "If not set, the default 'groups' is used")
String groupsClaimName() default "groups";
+
+ @AttributeDefinition(name = "connection", description = "OIDC
Connection Name")
Review Comment:
the default value is null in this case, right?
would that lead to more NPE in the code above where you call
connection.name()?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -85,6 +89,7 @@ public SlingUserInfoProcessorImpl(
this.storeRefreshToken = config.storeRefreshToken();
this.groupsInIdToken = config.groupsInIdToken();
this.groupsClaimName = config.groupsClaimName();
+ this.connection = config.connection();
Review Comment:
if connection shall never be null (see notnull annotation in the interface)
we need to make sure config.connection() is never null. see my question above
and below
--
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]
