Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAK-11498: Expose Session-bound principals via JackrabbitSession #2093

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

kwin
Copy link
Member

@kwin kwin commented Feb 18, 2025

No description provided.

@kwin kwin requested review from reschke and anchela February 18, 2025 15:35
Copy link

Commit-Check ✔️

@kwin kwin force-pushed the feature/OAK-11498-expose-bound-principals-via-jrsession branch from ae3e61b to 7b7a00d Compare February 18, 2025 15:36
@kwin kwin force-pushed the feature/OAK-11498-expose-bound-principals-via-jrsession branch from 7b7a00d to fc756c5 Compare February 18, 2025 15:46
@kwin
Copy link
Member Author

kwin commented Mar 3, 2025

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

Copy link
Contributor

@anchela anchela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

reschke commented Mar 5, 2025

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

@kwin
Copy link
Member Author

kwin commented Mar 5, 2025

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
Copy link
Contributor

reschke commented Mar 12, 2025

@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
Copy link
Member Author

kwin commented Mar 12, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants