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-9415 expose the bound principals of a session as attribute #291

Closed

Conversation

kwin
Copy link
Member

@kwin kwin commented Apr 28, 2021

No description provided.

@kwin kwin changed the title OAK-9415 exposed the bound principals of a session as attribute OAK-9415 expose the bound principals of a session as attribute Apr 28, 2021
Session Attributes
------------------

Oak exposes the following attributes via [`Session.getAttribute(...)`][1] and [`Session.getAttributeNames()`][2].
Copy link
Contributor

Choose a reason for hiding this comment

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

that's not entirely correct.... it also exposes credentials attributes as intended by the JCR specification. maybe rather state something like: in addition to attributes defined on credentials.......

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 3d7a7e5.

return names.toArray(new String[names.size()]);
}

@Override
public Object getAttribute(String name) {
if (RepositoryImpl.BOUND_PRINCIPALS.equals(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would rather move that to the end of the method.... as the last piece to check.... in other words: if someone came up with the crazy idea to set "oak.bound_principals" attribute on his credentials, it probably should take precedence to fulfill the API contract defined by the JCR specification, shouldn't it?

Copy link
Member Author

@kwin kwin Apr 28, 2021

Choose a reason for hiding this comment

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

That is IMHO dangerous, as the bound principals can be used for checking if certain code paths should be allowed (e.g. in the context of FileVault). If someone can forge these attributes by just creating a (otherwise restricted) session with those parameters, this could easily be abused for privilege escalation. IMHO those values should not be allowed to be overwritten by consumers.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair point..... so let's keep it the way it is.

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.

@kwin , i would suggest to also add a dedicated test to RepositoryTest.class for the new attribute.

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.

@kwin thanks for the tests.

@kwin
Copy link
Member Author

kwin commented Apr 29, 2021

Merged in cde9070.

@kwin kwin closed this Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants