From ff1372bed8e772b131169f2d30b87992d690bbaf Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Fri, 20 Oct 2023 10:01:18 -0700 Subject: [PATCH 1/2] cadc-gms: fix GroupURI validation and tests --- cadc-gms/build.gradle | 2 +- .../main/java/org/opencadc/gms/GroupURI.java | 54 +++++++++++------- .../java/org/opencadc/gms/GroupURITest.java | 57 +++++++++++++------ 3 files changed, 74 insertions(+), 39 deletions(-) diff --git a/cadc-gms/build.gradle b/cadc-gms/build.gradle index 47490068..4b5000b6 100644 --- a/cadc-gms/build.gradle +++ b/cadc-gms/build.gradle @@ -16,7 +16,7 @@ sourceCompatibility = 1.8 group = 'org.opencadc' -version = '1.0.8' +version = '1.0.9' description = 'OpenCADC GMS API library' def git_url = 'https://github.com/opencadc/ac' diff --git a/cadc-gms/src/main/java/org/opencadc/gms/GroupURI.java b/cadc-gms/src/main/java/org/opencadc/gms/GroupURI.java index 9d82dc6e..22444408 100644 --- a/cadc-gms/src/main/java/org/opencadc/gms/GroupURI.java +++ b/cadc-gms/src/main/java/org/opencadc/gms/GroupURI.java @@ -91,40 +91,37 @@ public class GroupURI implements Comparable { */ public GroupURI(URI uri) throws IllegalArgumentException { if (uri == null) { - throw new IllegalArgumentException("Null URI"); + throw new IllegalArgumentException("null URI"); } // Ensure the scheme is correct if (uri.getScheme() == null || !"ivo".equals(uri.getScheme())) { - throw new IllegalArgumentException("GroupURI scheme must be 'ivo'."); + throw new IllegalArgumentException("scheme must be 'ivo' in resourceID: " + uri); } if (uri.getAuthority() == null) { - throw new IllegalArgumentException("Group authority is required."); + throw new IllegalArgumentException("authority is required in resourceID: " + uri); } if (uri.getPath() == null || uri.getPath().length() == 0) { - throw new IllegalArgumentException("Missing authority and/or path."); + throw new IllegalArgumentException("path is required in resourceID: " + uri); + } + + if (uri.getFragment() != null) { + throw new IllegalArgumentException("fragment not allowed in uri: " + uri); + } + + if (uri.getQuery() == null) { + throw new IllegalArgumentException("query (group name) required in uri: " + uri); } - String fragment = uri.getFragment(); - String query = uri.getQuery(); - String name = null; - if (query == null) { - throw new IllegalArgumentException("Group name is required."); - } else { - if (fragment != null) { - throw new IllegalArgumentException("Fragment not allowed in group URIs"); - } - - if (isValidGroupName(query)) { - name = query; - } else { - throw new IllegalArgumentException(GROUP_NAME_ERRORMSG); - } + String name = uri.getQuery(); + if (!isValidGroupName(name)) { + throw new IllegalArgumentException("invalid group name: " + name + " reason: " + GROUP_NAME_ERRORMSG); + } - this.uri = URI.create(uri.getScheme() + "://" + uri.getAuthority() + uri.getPath() + "?" + name); + this.uri = uri; } /** @@ -142,7 +139,10 @@ public GroupURI(String uri) throws IllegalArgumentException, URISyntaxException public GroupURI(URI resourceID, String name) { if (resourceID == null) { - throw new IllegalArgumentException("Null URI"); + throw new IllegalArgumentException("null GMS resourceID"); + } + if (name == null) { + throw new IllegalArgumentException("null group name"); } // Ensure the scheme is correct @@ -157,9 +157,19 @@ public GroupURI(URI resourceID, String name) { if (resourceID.getPath() == null || resourceID.getPath().length() == 0) { throw new IllegalArgumentException("path is required in resourceID: " + resourceID); } + + if (resourceID.getQuery() != null) { + throw new IllegalArgumentException("query not allowed in resourceID: " + resourceID); + } + if (resourceID.getFragment() != null) { - throw new IllegalArgumentException("fragment not allowed in resourceID: " + resourceID.getFragment()); + throw new IllegalArgumentException("fragment not allowed in resourceID: " + resourceID); + } + + if (!isValidGroupName(name)) { + throw new IllegalArgumentException("invalid group name: " + name + " reason: " + GROUP_NAME_ERRORMSG); } + this.uri = URI.create(resourceID.toASCIIString() + "?" + name); } diff --git a/cadc-gms/src/test/java/org/opencadc/gms/GroupURITest.java b/cadc-gms/src/test/java/org/opencadc/gms/GroupURITest.java index 875c4709..765eea0a 100644 --- a/cadc-gms/src/test/java/org/opencadc/gms/GroupURITest.java +++ b/cadc-gms/src/test/java/org/opencadc/gms/GroupURITest.java @@ -16,7 +16,7 @@ public class GroupURITest { private static Logger log = Logger.getLogger(GroupURITest.class); static { - Log4jInit.setLevel("ca.nrc.cadc.ac", Level.DEBUG); + Log4jInit.setLevel("org.opencadc.gms", Level.INFO); } @Test @@ -28,24 +28,39 @@ public void testEquals() { @Test public void testMalformed() { + try { + + assertIllegalArgument(URI.create("ivo://example.org/gms?first.last@idp.com")); + assertIllegalArgument(URI.create("ivo://example.org/gms"), "first.last@idp.com"); + // no scheme - assertIllegalArgument("example.org/gms?gname", "scheme"); + assertIllegalArgument(URI.create("example.org/gms?gname")); + assertIllegalArgument(URI.create("example.org/gms"), "gname"); // wrong scheme - assertIllegalArgument("gms://example.org/gms?gname", "scheme"); + assertIllegalArgument(URI.create("gms://example.org/gms?gname")); + assertIllegalArgument(URI.create("gms://example.org/gms"), "gname"); // no authority - assertIllegalArgument("ivo://gms?gname", "authority"); + assertIllegalArgument(URI.create("ivo://gms?gname")); + assertIllegalArgument(URI.create("ivo://gms"), "gname"); // no path - assertIllegalArgument("ivo://example.org/gname", "name"); + assertIllegalArgument(URI.create("ivo://example.org?gname")); + assertIllegalArgument(URI.create("ivo://example.org"), "gname"); - // group name in fragment no longer allowed - assertIllegalArgument("ivo://my.authority/gms#name", "name"); + // no group name + assertIllegalArgument(URI.create("ivo://example.org/gms")); + assertIllegalArgument(URI.create("ivo://example.org/gms"), null); + assertIllegalArgument(URI.create("ivo://example.org/gms"), ""); + assertIllegalArgument(URI.create("ivo://example.org/gms"), " "); // fragment not allowed - assertIllegalArgument("ivo://my.authority/gms?name#name", "fragment"); + assertIllegalArgument(URI.create("ivo://my.authority/gms#name")); + assertIllegalArgument(URI.create("ivo://my.authority/gms#name"), null); + assertIllegalArgument(URI.create("ivo://my.authority/gms?name#name")); + assertIllegalArgument(URI.create("ivo://my.authority/gms?name#name")); } catch (Exception unexpected) { log.error("unexpected exception", unexpected); @@ -56,7 +71,7 @@ public void testMalformed() { @Test public void testSimpleGroupName() { try { - GroupURI g = new GroupURI("ivo://my.authority/gms?name"); + GroupURI g = new GroupURI(URI.create("ivo://my.authority/gms?name")); Assert.assertEquals("ivo", g.getURI().getScheme()); Assert.assertEquals("/gms", g.getURI().getPath()); Assert.assertEquals("name", g.getName()); @@ -72,7 +87,7 @@ public void testSimpleGroupName() { public void testHierarchicalGroupName() { try { String name = "hierachical/group/structure"; - GroupURI g = new GroupURI("ivo://my.authority/gms?" + name); + GroupURI g = new GroupURI(URI.create("ivo://my.authority/gms?" + name)); Assert.assertEquals("ivo", g.getURI().getScheme()); Assert.assertEquals("/gms", g.getURI().getPath()); Assert.assertEquals(name, g.getName()); @@ -84,14 +99,24 @@ public void testHierarchicalGroupName() { } } - private void assertIllegalArgument(String uri, String message) throws URISyntaxException { + private void assertIllegalArgument(URI uri) { + try { + GroupURI gu = new GroupURI(uri); + Assert.fail("expected IllegalArgumentException, got: " + gu); + } catch (IllegalArgumentException e) { + log.info("caught expected: " + e); + } catch (Exception unexpected) { + log.error("unexpected exception", unexpected); + Assert.fail("unexpected exception: " + unexpected); + } + } + + private void assertIllegalArgument(URI resourceID, String name) { try { - new GroupURI(uri); - Assert.fail("expected IllegalArgumentException, got: " + uri); + GroupURI gu = new GroupURI(resourceID, name); + Assert.fail("expected IllegalArgumentException, got: " + gu); } catch (IllegalArgumentException e) { - // expected - log.debug("Checking if message '" + e.getMessage() + "' contains '" + message + "'"); - Assert.assertTrue(e.getMessage().toLowerCase().contains(message)); + log.info("caught expected: " + e); } catch (Exception unexpected) { log.error("unexpected exception", unexpected); Assert.fail("unexpected exception: " + unexpected); From 961f1e5c24e6c23858664ce739b84e65ded8fdeb Mon Sep 17 00:00:00 2001 From: Patrick Dowler Date: Fri, 20 Oct 2023 13:12:35 -0700 Subject: [PATCH 2/2] cadc-access-control-server: handle invalid group names in ldap --- cadc-access-control-server/build.gradle | 2 +- .../java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/cadc-access-control-server/build.gradle b/cadc-access-control-server/build.gradle index c8bc364c..15df99d2 100644 --- a/cadc-access-control-server/build.gradle +++ b/cadc-access-control-server/build.gradle @@ -13,7 +13,7 @@ sourceCompatibility = 1.8 group = 'org.opencadc' -version = '1.3.33' +version = '1.3.34' description = 'OpenCADC User+Group server library' def git_url = 'https://github.com/opencadc/ac' diff --git a/cadc-access-control-server/src/main/java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java b/cadc-access-control-server/src/main/java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java index dc295b7f..0d303630 100755 --- a/cadc-access-control-server/src/main/java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java +++ b/cadc-access-control-server/src/main/java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java @@ -797,7 +797,9 @@ public Group getGroup(final int gid) Group ldapGroup = createGroupFromSearchResult(searchEntry, PUB_GROUP_ATTRS, ldapConn); return ldapGroup; - + } catch (IllegalArgumentException ex) { + // invalid group name + throw new GroupNotFoundException("porobably invalid group name: " + ex); } catch (LDAPException e1) { logger.debug("getGroup Exception: " + e1, e1); LdapDAO.checkLdapResult(e1.getResultCode()); @@ -867,9 +869,10 @@ private Group getGroup(final DN groupDN, final String loggableID, String[] attri profiler.checkpoint("getGroup.addMembers"); return ldapGroup; - } - catch (LDAPException e1) - { + } catch (IllegalArgumentException ex) { + // invalid group name + throw new GroupNotFoundException("porobably invalid group name: " + ex); + } catch (LDAPException e1) { logger.debug("getGroup Exception: " + e1, e1); LdapDAO.checkLdapResult(e1.getResultCode()); throw new RuntimeException("BUG: checkLdapResult didn't throw an exception");