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.

Sign up for GitHub

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

Jump to bottom

OAK-9415 expose the bound principals of a session as attribute #291

Closed
wants to merge 5 commits into from
Closed

OAK-9415 expose the bound principals of a session as attribute #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
289958c
OAK-9415 exposed the bound principals of a session as attribute
kwin Apr 28, 2021
3d7a7e5
clarify documentation
kwin Apr 28, 2021
e641c4c
fix test which incorrectly assumes that the refresh interval is always
kwin Apr 28, 2021
ce2fc61
make tests more stable by not relying on attribute name order
kwin Apr 28, 2021
1fc6f92
extend tests to cover attribute "oak.bound-principals"
kwin Apr 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion oak-doc/src/site/markdown/differences.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
limitations under the License.
-->

<!-- MACRO{toc} -->

Backward compatibility
======================

Expand Down Expand Up @@ -316,6 +318,21 @@ Node Name Length Limit

With the document storage backend (MongoDB, RDBMS), there is currently
a limit of 150 UTF-8 bytes on the length of the node names.
See also OAK-2644.
See also [OAK-2644](https://issues.apache.org/jira/browse/OAK-2644).

Session Attributes
------------------

Oak exposes the following attributes via [`Session.getAttribute(...)`][1] and [`Session.getAttributeNames()`][2] in addition to the ones set through [Credentials][3]' attributes passed to [Repository.login(...)][4].

Attribute Name | Attribute Value Type | Description
--- | --- | ---
`oak.refresh-interval` | `Long` | The session refresh interval in seconds.
`oak.relaxed-locking` | `Boolean` | Whether relaxed locking behaviour is enabled for the session. See [OAK-1329](https://issues.apache.org/jira/browse/OAK-1329).
`oak.bound-principals` | `Set<Principal>` | The principals associated with the JCR session. See [OAK-9415](https://issues.apache.org/jira/browse/OAK-9415)

[0]: https://docs.adobe.com/content/docs/en/spec/jsr170/javadocs/jcr-2.0/javax/jcr/Session.html#setNamespacePrefix(java.lang.String,%20java.lang.String)
[1]: https://docs.adobe.com/content/docs/en/spec/jsr170/javadocs/jcr-2.0/javax/jcr/Session.html#getAttribute(java.lang.String)
[2]: https://docs.adobe.com/content/docs/en/spec/jsr170/javadocs/jcr-2.0/javax/jcr/Session.html#getAttributeNames()
[3]: https://docs.adobe.com/content/docs/en/spec/jsr170/javadocs/jcr-2.0/javax/jcr/Credentials.html
[4]: https://docs.adobe.com/content/docs/en/spec/jsr170/javadocs/jcr-2.0/javax/jcr/Repository.html#login(javax.jcr.Credentials,%20java.lang.String)
7 changes: 7 additions & 0 deletions oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/repository/RepositoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,13 @@ public class RepositoryImpl implements JackrabbitRepository {
*/
public static final String RELAXED_LOCKING = "oak.relaxed-locking";

/**
* Name of the session attribute exposing the associated principals
*
* @see <a href="https://issues.apache.org/jira/browse/OAK-9415">OAK-9415</a>
*/
public static final String BOUND_PRINCIPALS = "oak.bound-principals";

/**
* logger instance
*/
Expand Down
5 changes: 5 additions & 0 deletions oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/session/SessionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
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 •
edited

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.

return sd.getAuthInfo().getPrincipals();
}
Object attribute = sd.getAuthInfo().getAttribute(name);
if (attribute == null) {
attribute = sessionContext.getAttributes().get(name);
Expand Down
30 changes: 23 additions & 7 deletions oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/RepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,16 @@
import java.io.InputStreamReader;
import java.io.Reader;
import java.math.BigDecimal;
import java.security.Principal;
import java.util.Arrays;
import java.util.Calendar;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import javax.jcr.Binary;
import javax.jcr.GuestCredentials;
Expand Down Expand Up @@ -86,6 +90,7 @@
import org.apache.jackrabbit.commons.cnd.ParseException;
import org.apache.jackrabbit.commons.jackrabbit.SimpleReferenceBinary;
import org.apache.jackrabbit.core.data.RandomInputStream;
import org.apache.jackrabbit.core.security.principal.AdminPrincipal;
import org.apache.jackrabbit.oak.fixture.NodeStoreFixture;
import org.apache.jackrabbit.oak.jcr.repository.RepositoryImpl;
import org.apache.jackrabbit.oak.spi.nodetype.NodeTypeConstants;
Expand All @@ -96,6 +101,7 @@
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.mockito.Matchers;
import org.slf4j.LoggerFactory;

public class RepositoryTest extends AbstractRepositoryTest {
Expand Down Expand Up @@ -132,19 +138,30 @@ public void createRepository() throws RepositoryException {

@Test
public void login() throws RepositoryException {
assertNotNull(getAdminSession());
Session session = getAdminSession();
assertNotNull(session);
Set<Principal> principals = (Set<Principal>) session.getAttribute(RepositoryImpl.BOUND_PRINCIPALS);
assertNotNull(principals);
Set<String> expectedPrincipalNames = new HashSet<>(Arrays.asList("admin", "everyone"));
assertEquals(expectedPrincipalNames, principals.stream().map(Principal::getName).collect(Collectors.toSet()));
}

@Test
public void loginWithAttribute() throws RepositoryException {
Map<String, Object> attributes = new HashMap<>();
attributes.put(RepositoryImpl.REFRESH_INTERVAL, 42);
attributes.put(RepositoryImpl.BOUND_PRINCIPALS, Collections.singleton(new AdminPrincipal("admin")));
Session session = ((JackrabbitRepository) getRepository()).login(
new GuestCredentials(), null,
Collections.<String, Object>singletonMap(RepositoryImpl.REFRESH_INTERVAL, 42));
new GuestCredentials(), null, attributes);

String[] attributeNames = session.getAttributeNames();
assertEquals(1, attributeNames.length);
assertEquals(RepositoryImpl.REFRESH_INTERVAL, attributeNames[0]);
assertTrue(Arrays.asList(attributeNames).contains(RepositoryImpl.REFRESH_INTERVAL));
assertEquals(42L, session.getAttribute(RepositoryImpl.REFRESH_INTERVAL));
Set<Principal> principals = (Set<Principal>) session.getAttribute(RepositoryImpl.BOUND_PRINCIPALS);
assertNotNull(principals);
// admin must not be contained in the principals (altough added in the login attributes)
Set<String> expectedPrincipalNames = new HashSet<>(Arrays.asList("anonymous", "everyone"));
assertEquals(expectedPrincipalNames, principals.stream().map(Principal::getName).collect(Collectors.toSet()));
session.logout();
}

Expand All @@ -157,8 +174,7 @@ public void loginWithCredentialsAttribute() throws RepositoryException {
try {
session = getRepository().login(sc, null);
String[] attributeNames = session.getAttributeNames();
assertEquals(1, attributeNames.length);
assertEquals("attr", attributeNames[0]);
assertTrue(Arrays.asList(attributeNames).contains("attr"));
assertEquals("val", session.getAttribute("attr"));
} finally {
if (session != null) {
Expand Down