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

cadc-gms: fix GroupURI validation and tests #160

Merged
merged 2 commits into from
Oct 20, 2023
Merged

cadc-gms: fix GroupURI validation and tests #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff1372b
cadc-gms: fix GroupURI validation and tests
pdowler Oct 20, 2023
961f1e5
cadc-access-control-server: handle invalid group names in ldap
pdowler Oct 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cadc-access-control-server/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
11 changes: 7 additions & 4 deletions cadc-access-control-server/src/main/java/ca/nrc/cadc/ac/server/ldap/LdapGroupDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion cadc-gms/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
54 changes: 32 additions & 22 deletions cadc-gms/src/main/java/org/opencadc/gms/GroupURI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,40 +91,37 @@ public class GroupURI implements Comparable<GroupURI> {
*/
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;
}

/**
Expand All @@ -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
Expand All @@ -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);
}

Expand Down
57 changes: 41 additions & 16 deletions cadc-gms/src/test/java/org/opencadc/gms/GroupURITest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -28,24 +28,39 @@ public void testEquals() {

@Test
public void testMalformed() {

try {

assertIllegalArgument(URI.create("ivo://example.org/[email protected]"));
assertIllegalArgument(URI.create("ivo://example.org/gms"), "[email protected]");

// 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);
Expand All @@ -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());
Expand All @@ -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());
Expand All @@ -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);
Expand Down
Loading