juanpablo-santos commented on code in PR #407:
URL: https://github.com/apache/jspwiki/pull/407#discussion_r2532051020
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/DefaultAuthorizationManager.java:
##########
@@ -191,6 +191,12 @@ public boolean hasRoleOrPrincipal( final Session session,
final Principal princi
final Principal[] userPrincipals = session.getPrincipals();
return Arrays.stream(userPrincipals).anyMatch(userPrincipal ->
userPrincipal.getName().equals(principalName));
}
+ // this little hack enables use to use container defined roles that
are not expllicitly
+ // listed in the web.xml of our war file. A common use case when the
container is wired up
+ // to keycloak or a LDAP type capability.
+ if (session.getHttpRequestContext()!=null) {
Review Comment:
hmm, now I see the why, but I wonder, since this is / should always be
related to the `Session.hasPrincipal/getPrincipals/userPrincipals` methods, if
this check could be pushed to the wikisession. This would entail, when
constructing the session using a request, asking for the dynamic roles and
storing them.
Again, this "asking for the dynamic roles", it's more easier than done, as
there isn't a way to extract all roles of the user for a given request, we only
have `isUserInRole`. So perhaps that list could be extracted from the wiki
properties? It¡d be less flexible, but OTOH, session would only store the roles
that it needs to store instead of everything returned by a 3rd party.
I dunno, not sure about what would be the best way to go; my concern is
storing the request at session level, and exposing it through the public API
opens up to creative uses, which would end up returning as problems (i.e., the
request has expired, the request is not the last request, the request with my
data, etc.)
##########
jspwiki-it-tests/jspwiki-it-container-auth/build.xml:
##########
@@ -0,0 +1,97 @@
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+-->
+
+<!--
+ This project builds a "WikiOnAStick" based on JSPWiki.
+-->
+<project name="makeDistro" default="makeDistro" basedir=".">
Review Comment:
why a portable build? this could be set up as the rest of the jspwiki-it-*
modules, so the integration tests can be run against this new config
##########
jspwiki-api/src/main/java/org/apache/wiki/api/core/Session.java:
##########
@@ -250,5 +252,21 @@ public interface Session extends WikiEventListener {
static Object doPrivileged( final Session session, final
PrivilegedAction<?> action ) throws AccessControlException {
return Subject.doAsPrivileged( session.getSubject(), action, null );
}
+
+ /**
+ * sets the raw http servlet request object. typically used for
+ * verifying externally defined access controls.
+ * @since 3.0.0
+ * @param session
+ */
+ void setHttpRequestContext(HttpServletRequest session);
+
+ /**
+ * gets the raw http servlet request object. typically used for
+ * verifying externally defined access controls. Result may be null
+ * @since 3.0.0
+ * @return HttpServletRequest
+ */
+ HttpServletRequest getHttpRequestContext();
Review Comment:
`HttpServletRequest` is already accessible through
`Context.getHttpRequest()` and also, WikiSession is akin to HttpSession, and
treated like that, so storing the request here may cause issues later on.
##########
jspwiki-main/src/main/java/org/apache/wiki/auth/acl/adv/AdvancedAcl.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Copyright 2025 The Apache Software Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wiki.auth.acl.adv;
+
+import java.security.Permission;
+import java.security.Principal;
+import java.util.Enumeration;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.wiki.api.core.AclEntry;
+import org.apache.wiki.auth.AdvancedAuthorizationManager;
+import org.apache.wiki.auth.acl.Acl;
+import org.apache.wiki.auth.acl.AdvancedAclManager;
+
+/**
+ * an extension to the base ACL classes that provides boolean logic, tyical use
+ * case is for role/group membership
+ *
+ * @since 3.0.0
+ * @see AdvancedAclManager
+ * @see AdvancedAuthorizationManager
+ */
+//org.apache.wiki.auth.acl.Acl
+public class AdvancedAcl implements org.apache.wiki.api.core.Acl, Acl {
Review Comment:
if previous wiki syntax still works as is, I'd rather move all this to the
ACL and take over the AclManager, instead of having two separate AclManagers
##########
jspwiki-main/src/test/java/org/apache/wiki/auth/acl/adv/RuleParserTest.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Copyright 2025 The Apache Software Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wiki.auth.acl.adv;
+
+import java.util.List;
+import java.util.Set;
+import org.apache.wiki.auth.AdvancedAuthorizationManager;
+import org.apache.wiki.auth.acl.AdvancedAclManager;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * tests the rule parser for the advanced acl tool chain
+ *
+ * @see AdvancedAcl
+ * @see AdvancedAclManager
+ * @see AdvancedAuthorizationManager
+ * @since 3.0.0
+ */
+public class RuleParserTest {
+
+ public RuleParserTest() {
+ }
+
+ @Test
+ public void simpleOrStatement() {
+ RuleParser instance = new RuleParser("bob or mary");
+ RuleNode result = instance.parse();
+ Assertions.assertEquals(2, result.getAllRoles().size());
+ Assertions.assertTrue(result.evaluate(Set.of("bob")));
+ Assertions.assertTrue(result.evaluate(Set.of("mary")));
+ Assertions.assertFalse(result.evaluate(Set.of("john")));
+ }
+
+ @Test
+ public void simpleAndStatement() {
+ RuleParser instance = new RuleParser("accounting AND finance");
+ RuleNode result = instance.parse();
+ Assertions.assertEquals(2, result.getAllRoles().size());
+ Assertions.assertTrue(result.evaluate(Set.of("accounting",
"finance")));
+ Assertions.assertTrue(result.evaluate(Set.of("accounting", "finance",
"otherDepartment")));
+ Assertions.assertFalse(result.evaluate(Set.of("accounting")));
+ Assertions.assertFalse(result.evaluate(Set.of("finance")));
+ Assertions.assertFalse(result.evaluate(Set.of("john")));
+ }
+
+ @Test
+ public void complexSetup() {
+ RuleParser instance = new RuleParser("accounting AND (finance OR
admin)");
+ RuleNode result = instance.parse();
+ Assertions.assertEquals(3, result.getAllRoles().size());
+ Assertions.assertTrue(result.evaluate(Set.of("accounting",
"finance")));
+ Assertions.assertTrue(result.evaluate(Set.of("accounting", "finance",
"otherDepartment")));
+ Assertions.assertTrue(result.evaluate(Set.of("accounting", "finance",
"admin")));
+ Assertions.assertTrue(result.evaluate(Set.of("accounting", "admin")));
+ Assertions.assertFalse(result.evaluate(Set.of("accounting")));
+ Assertions.assertFalse(result.evaluate(Set.of("finance")));
+ Assertions.assertFalse(result.evaluate(Set.of("john")));
+ }
+
+ @Test
+ public void complexSetupNot() {
+ RuleParser instance = new RuleParser("accounting AND finance AND NOT
(bob)");
+ instance.parse();
+
+ instance = new RuleParser("(accounting AND finance AND NOT (bob))");
+ RuleNode result = instance.parse();
+
+ Assertions.assertEquals(3, result.getAllRoles().size());
+ Assertions.assertTrue(result.evaluate(Set.of("accounting",
"finance")));
+ Assertions.assertTrue(result.evaluate(Set.of("accounting", "finance",
"otherDepartment")));
+ Assertions.assertFalse(result.evaluate(Set.of("accounting", "finance",
"bob")));
+ Assertions.assertFalse(result.evaluate(Set.of("accounting", "admin")));
+ Assertions.assertFalse(result.evaluate(Set.of("accounting")));
+ Assertions.assertFalse(result.evaluate(Set.of("finance")));
+ Assertions.assertFalse(result.evaluate(Set.of("john")));
+ }
+
+ @Test
+ public void mismatchedParan() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> {
+ RuleParser instance = new RuleParser("(accounting AND finance AND
NOT(bob)");
+ RuleNode parse = instance.parse();
+ System.out.println(parse);
+ });
+
+ RuleParser instance = new RuleParser("(accounting AND finance AND
NOT(bob))");
+ instance.parse();
+
+ List<String> invalidExprs = List.of(
Review Comment:
`@ParameterizedTest` should be really handy for this kind of tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
