-
Notifications
You must be signed in to change notification settings - Fork 388
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
Changes from 1 commit
289958c
3d7a7e5
e641c4c
ce2fc61
1fc6f92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,7 @@ | |
import org.apache.jackrabbit.oak.jcr.delegate.NodeDelegate; | ||
import org.apache.jackrabbit.oak.jcr.delegate.PropertyDelegate; | ||
import org.apache.jackrabbit.oak.jcr.delegate.SessionDelegate; | ||
import org.apache.jackrabbit.oak.jcr.repository.RepositoryImpl; | ||
import org.apache.jackrabbit.oak.jcr.security.AccessManager; | ||
import org.apache.jackrabbit.oak.jcr.session.operation.SessionOperation; | ||
import org.apache.jackrabbit.oak.jcr.xml.ImportHandler; | ||
|
@@ -248,11 +249,15 @@ public String getUserID() { | |
public String[] getAttributeNames() { | ||
Set<String> names = newTreeSet(sessionContext.getAttributes().keySet()); | ||
Collections.addAll(names, sd.getAuthInfo().getAttributeNames()); | ||
names.add(RepositoryImpl.BOUND_PRINCIPALS); | ||
return names.toArray(new String[names.size()]); | ||
} | ||
|
||
@Override | ||
public Object getAttribute(String name) { | ||
if (RepositoryImpl.BOUND_PRINCIPALS.equals(name)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair point..... so let's keep it the way it is. |
||
return sd.getAuthInfo().getPrincipals(); | ||
} | ||
Object attribute = sd.getAuthInfo().getAttribute(name); | ||
if (attribute == null) { | ||
attribute = sessionContext.getAttributes().get(name); | ||
|
There was a problem hiding this comment.
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.......
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 3d7a7e5.