rombert commented on code in PR #44:
URL:
https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/44#discussion_r2878063724
##########
src/test/java/org/apache/sling/auth/oauth_client/AuthorizationCodeFlowIT.java:
##########
@@ -335,6 +365,247 @@ void
accessTokenIsPresentOnSuccessfulAuthenticationHandlerLoginWithoutPkceWithNo
accessTokenIsPresentOnSuccessfulAuthenticationHandlerLogin(false);
}
+ @Test
+ void oidcSingleLogoutRedirectsToIdpEndSessionEndpoint() throws Exception {
+
performOidcLoginWithLogoutRedirectPathAndAssertSingleLogoutRedirect(true);
+ }
+
+ void
performOidcLoginWithLogoutRedirectPathAndAssertSingleLogoutRedirect(boolean
withPkce) throws Exception {
Review Comment:
Can we extend the existing OIDC test to also perform the SP-initiated logout?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -75,31 +90,68 @@ public class SlingUserInfoProcessorImpl implements
UserInfoProcessor {
name = "idpNameInPrincipals",
description = "Add a suffix with the idp in the username and
to the groups created by this processor")
boolean idpNameInPrincipals() default false;
+
+ @AttributeDefinition(
+ name = "cleanupServiceUserName",
+ description = "Service user name for cleaning up user tokens
during logout. This user must be "
+ + "configured with read/write access to user profile
properties.")
+ String cleanupServiceUserName() default
DEFAULT_CLEANUP_SERVICE_USER_NAME;
+
+ @AttributeDefinition(
+ name = "enableTokenCleanup",
+ description = "Enable automatic cleanup of stored tokens
during logout. When enabled, tokens "
+ + "(access_token, refresh_token, id_token) are removed
from the user profile on logout. "
+ + "Disable this to preserve tokens for debugging,
auditing, or manual cleanup. "
+ + "Default: true")
+ boolean enableTokenCleanup() default true;
}
private static final Logger logger =
LoggerFactory.getLogger(SlingUserInfoProcessorImpl.class);
+ private static final String DEFAULT_CLEANUP_SERVICE_USER_NAME =
"oidc-cleanup-service";
+ private static final String PROFILE_PREFIX = "profile/";
Review Comment:
A similar constant is introduced in OidcLogoutHandler, please keep only one.
##########
src/main/java/org/apache/sling/auth/oauth_client/spi/UserInfoProcessor.java:
##########
@@ -47,6 +47,20 @@ OidcAuthCredentials process(
@Nullable String userInfo, @NotNull String tokenResponse, @NotNull
String oidcSubject, @NotNull String idp)
throws RuntimeException;
+ /**
+ * Clean up user data after logout. This method is called by the OIDC
authentication handler
+ * during the logout process to remove sensitive data such as access
tokens, refresh tokens,
+ * and ID tokens from persistent storage.
+ * <p>
+ * Implementations should remove stored tokens from the user's profile in
the repository
+ * to ensure they cannot be reused after logout.
+ *
+ * @param userId the user identifier (e.g., from request.getRemoteUser())
+ */
+ default void cleanupUserData(@NotNull String userId) {
Review Comment:
This might need to move/change, see my comments in SlingUserInfoProcessorImpl
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -75,31 +90,68 @@ public class SlingUserInfoProcessorImpl implements
UserInfoProcessor {
name = "idpNameInPrincipals",
description = "Add a suffix with the idp in the username and
to the groups created by this processor")
boolean idpNameInPrincipals() default false;
+
+ @AttributeDefinition(
+ name = "cleanupServiceUserName",
+ description = "Service user name for cleaning up user tokens
during logout. This user must be "
+ + "configured with read/write access to user profile
properties.")
+ String cleanupServiceUserName() default
DEFAULT_CLEANUP_SERVICE_USER_NAME;
+
+ @AttributeDefinition(
+ name = "enableTokenCleanup",
+ description = "Enable automatic cleanup of stored tokens
during logout. When enabled, tokens "
+ + "(access_token, refresh_token, id_token) are removed
from the user profile on logout. "
+ + "Disable this to preserve tokens for debugging,
auditing, or manual cleanup. "
+ + "Default: true")
+ boolean enableTokenCleanup() default true;
}
private static final Logger logger =
LoggerFactory.getLogger(SlingUserInfoProcessorImpl.class);
+ private static final String DEFAULT_CLEANUP_SERVICE_USER_NAME =
"oidc-cleanup-service";
+ private static final String PROFILE_PREFIX = "profile/";
private final CryptoService cryptoService;
private final boolean storeAccessToken;
private final boolean storeRefreshToken;
+ private final boolean storeIdToken;
private final boolean groupsInIdToken;
private final String groupsClaimName;
private final String connection;
private final boolean idpNameInPrincipals;
+ private final String cleanupServiceUserName;
+ private final boolean enableTokenCleanup;
+ private final SlingRepository repository;
@Activate
public SlingUserInfoProcessorImpl(
- @Reference(policyOption = ReferencePolicyOption.GREEDY)
CryptoService service, Config config) {
+ @Reference(policyOption = ReferencePolicyOption.GREEDY)
CryptoService service,
+ @Reference(cardinality = ReferenceCardinality.OPTIONAL,
policyOption = ReferencePolicyOption.GREEDY)
Review Comment:
What is the reason for making the SlingRepository reference optional?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/SlingUserInfoProcessorImpl.java:
##########
@@ -209,6 +266,111 @@ private String getGroupName(@NotNull String idp, Object
group) {
}
}
+ @Override
+ public void cleanupUserData(@NotNull String userId) {
Review Comment:
Why did you choose to implement the cleanup logic here? We have all the
persistence handled in the OAuthTokenStore (and sub-classes).
This is also tying the cleanup logic to JCR only and I think makes the
UserInfoProcessor have too many responsibilities.
--
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]
