rombert commented on code in PR #44:
URL:
https://github.com/apache/sling-org-apache-sling-auth-oauth-client/pull/44#discussion_r2974108799
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/OidcLogoutHandler.java:
##########
@@ -226,50 +223,22 @@ URI getEndSessionEndpoint(@NotNull ClientConnection
connection) {
}
/**
- * Reads the id_token from the user's OAK profile using a service account,
for use as id_token_hint
- * at the IdP end_session_endpoint. The token must have been stored
previously (e.g. by
- * SlingUserInfoProcessorImpl with storeIdToken and the sync layer
persisting credentials to OAK).
- * <p>
- * The service user configured via logoutServiceUserName must be
configured in Apache Jackrabbit Oak
- * (e.g. system users / External Principal Configuration) with minimal
read-only access to user profile
- * properties. DO NOT grant write or administrative permissions to this
service user.
+ * Reads the id_token for the current user, for use as id_token_hint at
the IdP end_session_endpoint.
*
- * @param userId the current user id (e.g. from request.getRemoteUser())
- * @param logoutServiceUserName service user name for reading from JCR
+ * @param resolver the resource resolver for the current user
* @param connection the resolved client connection to use with the token
store
* @return the id_token string, or null if not found or on error
*/
@Nullable
- String getIdTokenFromOak(
- @NotNull String userId, @NotNull String logoutServiceUserName,
@Nullable ClientConnection connection) {
- Session serviceSession = null;
+ String getIdTokenFromOak(@Nullable ResourceResolver resolver, @Nullable
ClientConnection connection) {
Review Comment:
`getIdTokenFromTokenStore` ?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/RedisOAuthTokenStore.java:
##########
@@ -123,12 +121,8 @@ public void clearAccessToken(@NotNull ClientConnection
connection, @NotNull Reso
}
@Override
- public @Nullable String getIdToken(
- @NotNull ClientConnection connection, @NotNull Session
serviceSession, @NotNull String userId)
+ public @Nullable String getIdToken(@NotNull ClientConnection connection,
@NotNull ResourceResolver resolver)
throws OAuthException {
- // Redis token store does not support ID token storage via service
session.
- // ID tokens are stored in JCR user profiles when using
JcrUserHomeOAuthTokenStore.
- // For Redis-based token storage, this method is not applicable and
returns null.
return null;
Review Comment:
Is there a particular reason not to implement this method? I think it would
look exactly like the other two methods in this class.
##########
src/test/java/org/apache/sling/auth/oauth_client/InMemoryOAuthTokenStore.java:
##########
@@ -154,11 +152,8 @@ public void clearAccessToken(@NotNull ClientConnection
connection, @NotNull Reso
}
@Override
- public @Nullable String getIdToken(
- @NotNull ClientConnection connection, @NotNull Session
serviceSession, @NotNull String userId)
+ public @Nullable String getIdToken(@NotNull ClientConnection connection,
@NotNull ResourceResolver resolver)
throws OAuthException {
- // InMemoryOAuthTokenStore does not support ID token retrieval via
service session.
- // This is only used for testing and ID tokens are not stored in this
implementation.
return null;
Review Comment:
Like in the redis token store, any reason not to implement it?
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/RedisOAuthTokenStore.java:
##########
@@ -32,7 +34,7 @@
import static
org.osgi.service.component.annotations.ConfigurationPolicy.REQUIRE;
-@Component(configurationPolicy = REQUIRE)
+@Component(configurationPolicy = REQUIRE, property =
"service.ranking:Integer=100")
Review Comment:
As commented above, I think this is no longer necessary.
##########
src/main/java/org/apache/sling/auth/oauth_client/impl/JcrUserHomeOAuthTokenStore.java:
##########
@@ -40,10 +43,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import static
org.osgi.service.component.annotations.ConfigurationPolicy.REQUIRE;
-
-// a config class is intentionally not defined, but a config is required to
select an implementation
-@Component(configurationPolicy = REQUIRE)
+@Component(property = "service.ranking:Integer=0")
Review Comment:
With your latest changes that is already addressed, right?
`OidcLogoutHandler` has an optional dependency to the `TokenStore` so the
service ranking/configuration policy changes can be reverted.
--
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]
