OAK-11498: Expose Session-bound principals via JackrabbitSession

[kwin]

No description provided.

[github-actions]

Commit-Check ❌

Commit rejected by Commit-Check.                                  
                                                                  
  (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)    (c).-.(c)  
   / ._. \      / ._. \      / ._. \      / ._. \      / ._. \   
 __\( C )/__  __\( H )/__  __\( E )/__  __\( C )/__  __\( K )/__ 
(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)(_.-/'-'\-._)
   || E ||      || R ||      || R ||      || O ||      || R ||   
 _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._  _.' '-' '._ 
(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)(.-./`-´\.-.)
 `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´  `-´     `-´ 
                                                                  
Commit rejected.                                                  
                                                                  
Type message check failed => Merge branch 'trunk' into feature/OAK-11498-expose-bound-principals-via-jrsession

 
It doesn't match regex: ^OAK-\d+:?\s\S+.*
The commit message must start with 'OAK-<ID>[:] ' followed by some descriptive text
Suggest: Please check your commit message whether it matches above regex

[kwin]

I am gonna merge end of this week, if there is no other feedback.

[anchela]

without a default implementation for the new API call, this modification is a breaking change that will cause issues for our downstream project AEM.

as i said before the fact that principals can be exposed on the jackrabbit API is very much an implementation detail of oak and i am not really convinced that this should be exposed in jackrabbit API.

[reschke]

@anchela - how would a default implementation help? (FWIW, I'd like to understand as well how we would implement this in Jackrabbit Classic).

[kwin]

as i said before the fact that principals can be exposed on the jackrabbit API is very much an implementation detail of oak and i am not really convinced that this should be exposed in jackrabbit API.

We do have several principal related API classes already in https://github.com/apache/jackrabbit-oak/tree/trunk/oak-jackrabbit-api/src/main/java/org/apache/jackrabbit/api/security/principal. As JCR 2.0 spec relies on principals for access control (https://developer.adobe.com/experience-manager/reference-materials/spec/jcr/2.0/16_Access_Control_Management.html#16.5.1%20Access%20Control%20Entries) it is natural that somehow you have it bound to a session as each impl would need to somehow compute those to check for access.

For JR2 I think the subject.getPrincipals (https://github.com/apache/jackrabbit/blob/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/SessionImpl.java#L384) should work in general. Maybe with some additional logic to get transitive group memberships like in https://github.com/apache/jackrabbit/blob/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/security/authentication/AbstractLoginModule.java#L682C30-L682C43.

[reschke]

@anchela - I do not understand how having a default impl would avoid bumping up the package version - or did you have something else in mind?

@kwin - that said, would it be possible to have a meaningful default impl?

[kwin]

I don't think a default impl in the JR-API module without additional dependencies would work, rather one impl for Oak and one for JR2.

[anchela]

imho it should be fairly easy to come up with a best-effort default implementation.
and if that is not considered good enough the default implementation should at the very least throw an UnsupportedRepositoryOperationException to avoid runtime exceptions

best-effort default could e.g. be an attempt to compute a set of principals from existing public API and if that is not possible throw an exception.

[kwin]

Having a default implementation which just throws an exception is actually worse than requiring any provider. to come up with a reasonable impl. Otherwise semantic versioning becomes useless

[anchela]

@kwin , i was not thinking about an impl that throws but one that leverages public API. i will work with @Amoratinos to get this unblocked.

[Amoratinos]

@kwin what do you think about:

    @NotNull
    default Set<Principal> getPrincipals() throws RepositoryException {
        String userId = getUserID();
        if (userId == null) {
            throw new UnsupportedRepositoryOperationException("No user ID associated with this session.");
        }

        Authorizable authorizable = getUserManager().getAuthorizable(userId);
        if (authorizable == null) {
            throw new UnsupportedRepositoryOperationException("No authorizable found for user ID: " + userId);
        }

        Principal userPrincipal = authorizable.getPrincipal();
        Set<Principal> principals = new java.util.HashSet<>();
        principals.add(userPrincipal);
        PrincipalIterator iterator =  getPrincipalManager().getGroupMembership(userPrincipal);
        while (iterator.hasNext()) {
            principals.add(iterator.nextPrincipal());
        }
        return principals;
    }

I've aligned with @anchela about the implementation, we thought about returning an empty set instead of throwing an exception but this could be misleading