[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r312012092
##
File path: java/org/apache/catalina/realm/RealmBase.java
##
@@ -928,6 +928,15 @@ public boolean hasRole(Wrapper wrapper, Principal
principal, String role) {
}
}
+// Check for a role alias/mapping defined on context level
+if (getContainer() instanceof Context) {
+Context context = (Context) getContainer();
+String realRole = context.findRoleMapping(role);
Review comment:
You can argue for and against the null check. If you prefer to skip it, I'm
fine with that.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole() URL: https://github.com/apache/tomcat/pull/186#discussion_r311727060 ## File path: webapps/docs/changelog.xml ## @@ -47,6 +47,10 @@ + +63636: Context.findRoleMapping() never called +in RealmBase#hasRole(). (michaelo) + Review comment: BZ issues should appear in order so the entry for BZ 63636 should appear after BZ 63608. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311726365
##
File path: test/org/apache/catalina/realm/TestRealmBase.java
##
@@ -789,4 +791,45 @@ public void testHttpConstraint() throws IOException {
Assert.assertFalse(mapRealm.hasResourcePermission(
request, response, constraintsDelete, null));
}
+
+@Test
+public void testRoleMapping() throws Exception {
+Context context = new TesterContext() {
+private Map roleMapping = new HashMap<>();
+
+public void addRoleMapping(String role, String link) {
+roleMapping.put(role, link);
+}
+
+@Override
+public String findRoleMapping(String role) {
+return roleMapping.get(role);
+}
+};
+
+context.addRoleMapping(ROLE2, "very-complex-role-name");
Review comment:
I was thinking more:
``
but that is a discussion better suited to BZ 55477
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311725976
##
File path: java/org/apache/catalina/realm/RealmBase.java
##
@@ -928,6 +928,15 @@ public boolean hasRole(Wrapper wrapper, Principal
principal, String role) {
}
}
+// Check for a role alias/mapping defined on context level
+if (getContainer() instanceof Context) {
+Context context = (Context) getContainer();
+String realRole = context.findRoleMapping(role);
Review comment:
`findSecurityReference()` != `findSecurityReferences()`
`findSecurityReference()` is the method that tries to map a single role and
should be changed.
`findSecurityReferences()` is closer to a getter for the current settings
for the Wrapper and should be unchanged.
So:
```
@Override
public String findSecurityReference(String name) {
String reference;
// First check the Wrapper
referencesLock.readLock().lock();
try {
reference = references.get(name);
} finally {
referencesLock.readLock().unlock();
}
// If not specified on the Wrapper, check the Context
if (reference == null && getParent() instanceof Context) {
reference = ((Context) getParent()).findRoleMapping(name);
}
return reference;
}
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311499556
##
File path: java/org/apache/catalina/realm/RealmBase.java
##
@@ -928,6 +928,15 @@ public boolean hasRole(Wrapper wrapper, Principal
principal, String role) {
}
}
+// Check for a role alias/mapping defined on context level
+if (getContainer() instanceof Context) {
+Context context = (Context) getContainer();
+String realRole = context.findRoleMapping(role);
Review comment:
`findSecurityReferences()` should be unchanged.
Parent of a Wrapper should only ever be a Context but an `instance of` check
is probably prudent. It would also protect against nulls (may be possible
during shutdown - I didn't check).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311472300
##
File path: test/org/apache/catalina/realm/TestRealmBase.java
##
@@ -789,4 +791,45 @@ public void testHttpConstraint() throws IOException {
Assert.assertFalse(mapRealm.hasResourcePermission(
request, response, constraintsDelete, null));
}
+
+@Test
+public void testRoleMapping() throws Exception {
+Context context = new TesterContext() {
+private Map roleMapping = new HashMap<>();
+
+public void addRoleMapping(String role, String link) {
+roleMapping.put(role, link);
+}
+
+@Override
+public String findRoleMapping(String role) {
+return roleMapping.get(role);
+}
+};
+
+context.addRoleMapping(ROLE2, "very-complex-role-name");
Review comment:
I was thinking more along the lines of a nested element in the Context as
they would be consistent with other Tomcat configuration.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311457083
##
File path: test/org/apache/catalina/realm/TestRealmBase.java
##
@@ -789,4 +791,45 @@ public void testHttpConstraint() throws IOException {
Assert.assertFalse(mapRealm.hasResourcePermission(
request, response, constraintsDelete, null));
}
+
+@Test
+public void testRoleMapping() throws Exception {
+Context context = new TesterContext() {
+private Map roleMapping = new HashMap<>();
+
+public void addRoleMapping(String role, String link) {
+roleMapping.put(role, link);
+}
+
+@Override
+public String findRoleMapping(String role) {
+return roleMapping.get(role);
+}
+};
+
+context.addRoleMapping(ROLE2, "very-complex-role-name");
Review comment:
The only way to add these Context mappings (currently) is via code. There is
no mechanism to do this via configuration. Possibly as a separate enhancement,
consider adding such an option. See
https://bz.apache.org/bugzilla/show_bug.cgi?id=55477
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636:
Context#findRoleMapping() never called in RealmBase#hasRole()
URL: https://github.com/apache/tomcat/pull/186#discussion_r311451173
##
File path: java/org/apache/catalina/realm/RealmBase.java
##
@@ -928,6 +928,15 @@ public boolean hasRole(Wrapper wrapper, Principal
principal, String role) {
}
}
+// Check for a role alias/mapping defined on context level
+if (getContainer() instanceof Context) {
+Context context = (Context) getContainer();
+String realRole = context.findRoleMapping(role);
Review comment:
The above code is only used when the Realm is defined at the Context level.
That isn't what is required here. The role mappings need to be checked for the
current Context irrespective of where the Realm is defined. Something like:
`Context context = (Context) wrapper.getParent();`
A similar change would also need to be made to `UserDatabaseRealm` and
potentially other sub-classes. Consider moving this to
`Wrapper.findSecurityReference`. i.e. Look up Wrapper references first and if
none found try the Context.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole()
markt-asf commented on a change in pull request #186: BZ 63636: Context#findRoleMapping() never called in RealmBase#hasRole() URL: https://github.com/apache/tomcat/pull/186#discussion_r311445088 ## File path: webapps/docs/changelog.xml ## @@ -47,6 +47,10 @@ + +63636: Context.findRoleMapping() never called +in RealmBase#hasRole(). (michaelo) + Review comment: Please see the notes at the top of this file regarding the ordering of sections and the ordering of elements in each section. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
