From ef06e9f4a94fc8f9c000377c339481b17c9b25a1 Mon Sep 17 00:00:00 2001 From: Alinga Yeung <Alinga.Yeung@nrc-cnrc.gc.ca> Date: Wed, 2 Sep 2015 11:55:40 -0700 Subject: [PATCH] Story 1657 rework. Updated based on Brian's review comments. Moved IdentityType to cadcUtil to consolidate its usage. --- projects/cadcAccessControl-Server/build.xml | 3 +- .../ac/server/web/users/GetUserAction.java | 104 +++++++----- .../server/web/users/UserActionFactory.java | 3 +- .../cadc/ac/server/RequestValidatorTest.java | 9 +- .../web/groups/AddUserMemberActionTest.java | 6 +- .../groups/RemoveUserMemberActionTest.java | 7 +- .../src/ca/nrc/cadc/ac/client/UserClient.java | 160 +++++------------- .../nrc/cadc/ac/xml/AbstractReaderWriter.java | 4 +- 8 files changed, 132 insertions(+), 164 deletions(-) diff --git a/projects/cadcAccessControl-Server/build.xml b/projects/cadcAccessControl-Server/build.xml index b5ad9f91..e0c3d437 100644 --- a/projects/cadcAccessControl-Server/build.xml +++ b/projects/cadcAccessControl-Server/build.xml @@ -93,6 +93,7 @@ <property name="cadcUtil" value="${lib}/cadcUtil.jar"/> <property name="cadcUWS" value="${lib}/cadcUWS.jar"/> <property name="wsUtil" value="${lib}/wsUtil.jar"/> + <property name="wsUtil-augment" value="${lib}/wsUtil-augment.jar"/> <property name="javacsv" value="${ext.lib}/javacsv.jar"/> <property name="jdom2" value="${ext.lib}/jdom2.jar"/> @@ -102,7 +103,7 @@ <property name="xerces" value="${ext.lib}/xerces.jar"/> <property name="jars" - value="${javacsv}:${jdom2}:${log4j}:${servlet}:${unboundid}:${xerces}:${cadcAccessControl}:${cadcLog}:${cadcRegistry}:${cadcUtil}:${cadcUWS}:${wsUtil}"/> + value="${javacsv}:${jdom2}:${log4j}:${servlet}:${unboundid}:${xerces}:${cadcAccessControl}:${cadcLog}:${cadcRegistry}:${cadcUtil}:${cadcUWS}:${wsUtil}:${wsUtil-augment}"/> <target name="build" depends="compile"> <jar jarfile="${build}/lib/${project}.jar" diff --git a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/GetUserAction.java b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/GetUserAction.java index 92a345af..591e863f 100644 --- a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/GetUserAction.java +++ b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/GetUserAction.java @@ -71,6 +71,8 @@ import ca.nrc.cadc.ac.PersonalDetails; import ca.nrc.cadc.ac.User; import ca.nrc.cadc.ac.UserNotFoundException; import ca.nrc.cadc.ac.server.UserPersistence; +import ca.nrc.cadc.auth.HttpPrincipal; +import ca.nrc.cadc.auth.NumericPrincipal; import java.security.AccessControlContext; import java.security.AccessController; @@ -82,6 +84,8 @@ import javax.security.auth.Subject; import org.apache.log4j.Logger; +import com.sun.security.auth.X500Principal; + public class GetUserAction extends AbstractUserAction { @@ -98,16 +102,18 @@ public class GetUserAction extends AbstractUserAction public void doAction() throws Exception { + log.debug("alinga-- GetUserAction.doAction(): enter"); User<Principal> user; - if (isServops()) + if (isAugmentUser()) { - Subject subject = new Subject(); + log.debug("alinga-- GetUserAction.doAction(): is an augment user"); + Subject subject = new Subject(); subject.getPrincipals().add(this.userID); - user = (User<Principal>) Subject.doAs(subject, new PrivilegedExceptionAction<Object>() + user = Subject.doAs(subject, new PrivilegedExceptionAction<User<Principal>>() { @Override - public Object run() throws Exception + public User<Principal> run() throws Exception { return getUser(userID); } @@ -116,10 +122,12 @@ public class GetUserAction extends AbstractUserAction } else { + log.debug("alinga-- GetUserAction.doAction(): is not an augment user"); user = getUser(this.userID); } writeUser(user); + log.debug("alinga-- GetUserAction.doAction(): exit"); } protected User<Principal> getUser(Principal principal) throws Exception @@ -130,58 +138,78 @@ public class GetUserAction extends AbstractUserAction try { user = userPersistence.getUser(principal); - if (detail != null) - { - // Only return user principals - if (detail.equals("identity")) - { - user.details.clear(); - } - // Only return user profile info, first and last name. - else if (detail.equals("display")) - { - user.getIdentities().clear(); - Set<PersonalDetails> details = user.getDetails(PersonalDetails.class); - if (details.isEmpty()) - { - String error = principal.getName() + " missing required PersonalDetails"; - throw new IllegalStateException(error); - } - PersonalDetails pd = details.iterator().next(); - user.details.clear(); - user.details.add(new PersonalDetails(pd.getFirstName(), pd.getLastName())); - } - else - { - throw new IllegalArgumentException("Illegal detail parameter " + detail); - } - } } catch (UserNotFoundException e) { user = userPersistence.getPendingUser(principal); } - return user; + if (detail != null) + { + // Only return user principals + if (detail.equals("identity")) + { + user.details.clear(); + } + // Only return user profile info, first and last name. + else if (detail.equals("display")) + { + user.getIdentities().clear(); + Set<PersonalDetails> details = user.getDetails(PersonalDetails.class); + if (details.isEmpty()) + { + String error = principal.getName() + " missing required PersonalDetails"; + throw new IllegalStateException(error); + } + PersonalDetails pd = details.iterator().next(); + user.details.clear(); + user.details.add(new PersonalDetails(pd.getFirstName(), pd.getLastName())); + } + else + { + throw new IllegalArgumentException("Illegal detail parameter " + detail); + } + } + + return user; } - protected boolean isServops() + protected boolean isAugmentUser() { - boolean isServops = false; AccessControlContext acc = AccessController.getContext(); Subject subject = Subject.getSubject(acc); if (subject != null) { - for (Principal principal : subject.getPrincipals()) + log.debug("alinga-- GetUserAction.isAugmentUser(): subject is not null."); + for (Principal principal : subject.getPrincipals(X500Principal.class)) { - if (principal.getName().equals(this.getAugmentUserDN())) + log.debug("alinga-- GetUserAction.isAugmentUser(): principal = " + principal); + log.debug("alinga-- GetUserAction.isAugmentUser(): principal name = " + principal.getName()); + log.debug("alinga-- GetUserAction.isAugmentUser(): augmentUserDN = " + this.getAugmentUserDN()); + if (principal instanceof X500Principal) + { + log.debug("alinga-- UserClientTest constructor(): servops is X500Principal."); + } + else if (principal instanceof HttpPrincipal) + { + log.debug("alinga-- UserClientTest constructor(): servops is X500Principal."); + } + else if (principal instanceof NumericPrincipal) + { + log.debug("alinga-- UserClientTest constructor(): servops is X500Principal."); + } + else + { + log.debug("alinga-- UserClientTest constructor(): servops is unknown principal."); + } + + if (principal.getName().equals(this.getAugmentUserDN())) { - isServops = true; - break; + return true; } } } - return isServops; + return false; } } diff --git a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/UserActionFactory.java b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/UserActionFactory.java index 457dd4b1..30583c7f 100644 --- a/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/UserActionFactory.java +++ b/projects/cadcAccessControl-Server/src/ca/nrc/cadc/ac/server/web/users/UserActionFactory.java @@ -68,17 +68,18 @@ */ package ca.nrc.cadc.ac.server.web.users; -import ca.nrc.cadc.ac.IdentityType; import ca.nrc.cadc.ac.User; import ca.nrc.cadc.ac.server.web.WebUtil; import ca.nrc.cadc.auth.CookiePrincipal; import ca.nrc.cadc.auth.HttpPrincipal; +import ca.nrc.cadc.auth.IdentityType; import ca.nrc.cadc.auth.NumericPrincipal; import ca.nrc.cadc.auth.OpenIdPrincipal; import java.io.IOException; import java.net.URL; import java.security.Principal; + import javax.security.auth.x500.X500Principal; import javax.servlet.http.HttpServletRequest; diff --git a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/RequestValidatorTest.java b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/RequestValidatorTest.java index e54ea3da..87abcdac 100644 --- a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/RequestValidatorTest.java +++ b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/RequestValidatorTest.java @@ -71,15 +71,18 @@ package ca.nrc.cadc.ac.server; import ca.nrc.cadc.ac.Role; import ca.nrc.cadc.ac.server.web.groups.AddUserMemberActionTest; import ca.nrc.cadc.auth.AuthenticationUtil; +import ca.nrc.cadc.auth.IdentityType; import ca.nrc.cadc.util.Log4jInit; import ca.nrc.cadc.uws.Parameter; import java.util.ArrayList; import java.util.List; + import org.apache.log4j.Level; import org.apache.log4j.Logger; import org.junit.BeforeClass; import org.junit.Test; + import static org.junit.Assert.*; /** @@ -152,7 +155,7 @@ public class RequestValidatorTest paramList.clear(); paramList.add(new Parameter("ID", "foo")); - paramList.add(new Parameter("IDTYPE", AuthenticationUtil.AUTH_TYPE_HTTP)); + paramList.add(new Parameter("IDTYPE", IdentityType.USERNAME.getValue())); paramList.add(new Parameter("ROLE", "foo")); try { @@ -163,7 +166,7 @@ public class RequestValidatorTest paramList.clear(); paramList.add(new Parameter("ID", "foo")); - paramList.add(new Parameter("IDTYPE", AuthenticationUtil.AUTH_TYPE_HTTP)); + paramList.add(new Parameter("IDTYPE", IdentityType.USERNAME.getValue())); paramList.add(new Parameter("ROLE", "foo")); paramList.add(new Parameter("GROUPID", "")); try @@ -175,7 +178,7 @@ public class RequestValidatorTest paramList.clear(); paramList.add(new Parameter("ID", "foo")); - paramList.add(new Parameter("IDTYPE", AuthenticationUtil.AUTH_TYPE_HTTP)); + paramList.add(new Parameter("IDTYPE", IdentityType.USERNAME.getValue())); paramList.add(new Parameter("ROLE", Role.MEMBER.getValue())); rv.validate(paramList); diff --git a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberActionTest.java b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberActionTest.java index 8eae8ace..d079de6f 100644 --- a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberActionTest.java +++ b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/AddUserMemberActionTest.java @@ -74,7 +74,9 @@ import ca.nrc.cadc.ac.User; import ca.nrc.cadc.ac.server.GroupPersistence; import ca.nrc.cadc.ac.server.UserPersistence; import ca.nrc.cadc.auth.AuthenticationUtil; +import ca.nrc.cadc.auth.IdentityType; import ca.nrc.cadc.util.Log4jInit; + import java.security.Principal; import org.apache.log4j.Level; @@ -107,7 +109,7 @@ public class AddUserMemberActionTest try { String userID = "foo"; - String userIDType = AuthenticationUtil.AUTH_TYPE_HTTP; + String userIDType = IdentityType.USERNAME.getValue(); Principal userPrincipal = AuthenticationUtil.createPrincipal(userID, userIDType); User<Principal> user = new User<Principal>(userPrincipal); @@ -159,7 +161,7 @@ public class AddUserMemberActionTest try { String userID = "foo"; - String userIDType = AuthenticationUtil.AUTH_TYPE_HTTP; + String userIDType = IdentityType.USERNAME.getValue(); Principal userPrincipal = AuthenticationUtil.createPrincipal(userID, userIDType); User<Principal> user = new User<Principal>(userPrincipal); diff --git a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberActionTest.java b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberActionTest.java index 92f5263d..dcba09b6 100644 --- a/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberActionTest.java +++ b/projects/cadcAccessControl-Server/test/src/ca/nrc/cadc/ac/server/web/groups/RemoveUserMemberActionTest.java @@ -74,8 +74,11 @@ import ca.nrc.cadc.ac.User; import ca.nrc.cadc.ac.server.GroupPersistence; import ca.nrc.cadc.ac.server.UserPersistence; import ca.nrc.cadc.auth.AuthenticationUtil; +import ca.nrc.cadc.auth.IdentityType; import ca.nrc.cadc.util.Log4jInit; + import java.security.Principal; + import org.apache.log4j.Level; import org.apache.log4j.Logger; import org.easymock.EasyMock; @@ -106,7 +109,7 @@ public class RemoveUserMemberActionTest try { String userID = "foo"; - String userIDType = AuthenticationUtil.AUTH_TYPE_HTTP; + String userIDType = IdentityType.USERNAME.getValue(); Principal userPrincipal = AuthenticationUtil.createPrincipal(userID, userIDType); User<Principal> user = new User<Principal>(userPrincipal); @@ -156,7 +159,7 @@ public class RemoveUserMemberActionTest try { String userID = "foo"; - String userIDType = AuthenticationUtil.AUTH_TYPE_HTTP; + String userIDType = IdentityType.USERNAME.getValue(); Principal userPrincipal = AuthenticationUtil.createPrincipal(userID, userIDType); User<Principal> user = new User<Principal>(userPrincipal); diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/UserClient.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/UserClient.java index a22a7519..3e1f8e24 100644 --- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/UserClient.java +++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/client/UserClient.java @@ -71,13 +71,10 @@ package ca.nrc.cadc.ac.client; import java.io.*; import java.net.MalformedURLException; import java.net.URL; -import java.net.URLEncoder; -import java.security.AccessControlContext; -import java.security.AccessController; import java.security.Principal; +import java.util.Iterator; import java.util.Set; -import javax.net.ssl.SSLSocketFactory; import javax.security.auth.Subject; import javax.security.auth.x500.X500Principal; @@ -87,9 +84,9 @@ import ca.nrc.cadc.auth.HttpPrincipal; import org.apache.log4j.Logger; import ca.nrc.cadc.ac.xml.UserReader; +import ca.nrc.cadc.auth.AuthenticationUtil; import ca.nrc.cadc.auth.CookiePrincipal; import ca.nrc.cadc.auth.NumericPrincipal; -import ca.nrc.cadc.auth.SSLUtil; import ca.nrc.cadc.net.HttpDownload; @@ -102,8 +99,6 @@ public class UserClient private static final Logger log = Logger.getLogger(UserClient.class); // socket factory to use when connecting - private SSLSocketFactory sslSocketFactory; - private SSLSocketFactory mySocketFactory; private String baseURL; /** @@ -148,12 +143,11 @@ public class UserClient */ public void augmentSubject(Subject subject) { - URL url = this.getURL(subject); + Principal principal = this.getPrincipal(subject); + URL url = this.getURL(principal); log.debug("augmentSubject request to " + url.toString()); ByteArrayOutputStream out = new ByteArrayOutputStream(); HttpDownload download = new HttpDownload(url, out); - - download.setSSLSocketFactory(getSSLSocketFactory()); download.run(); this.handleThrowable(download); @@ -162,7 +156,7 @@ public class UserClient protected void augmentSubject(Subject subject, Set<Principal> principals) { - if (principals.isEmpty()) + if (!principals.iterator().hasNext()) { String name = subject.getPrincipals().iterator().next().getName(); String msg = "No UserIdentity in LDAP server for principal: " + name; @@ -197,6 +191,33 @@ public class UserClient } } + protected Principal getPrincipal(final Subject subject) + { + Set<Principal> principals = subject.getPrincipals(); + Iterator<Principal> iterator = principals.iterator(); + if (iterator.hasNext()) + { + Principal principal = iterator.next(); + log.debug("alinga-- UserClient.getPrincipal(): principal = " + principal); + if (iterator.hasNext()) + { + Principal principal1 = iterator.next(); + log.debug("alinga-- UserClient.getPrincipal(): principal1 = " + principal1); + log.debug("alinga-- UserClient.getPrincipal(): number of principals = " + principals.size()); + // Should only have one principal + final String msg = "Subject has more than one principal."; + throw new IllegalArgumentException(msg); + } + + return principal; + } + else + { + final String msg = "Subject has no principal."; + throw new IllegalArgumentException(msg); + } + } + protected Set<Principal> getPrincipals(ByteArrayOutputStream out) { try @@ -224,127 +245,34 @@ public class UserClient } } - protected URL getURL(Subject subject) + protected URL getURL(Principal principal) { try { - String userID = subject.getPrincipals().iterator().next().getName(); - String encodedUserID = URLEncoder.encode(userID, "UTF-8"); - URL url = new URL(this.baseURL + "/users/" + encodedUserID + - "?idType=" + this.getIdType(subject) + "&detail=identity"); + String userID = principal.getName(); + URL url = new URL(this.baseURL + "/users/" + userID + + "?idType=" + this.getIdType(principal) + "&detail=identity"); log.debug("getURL(): returned url =" + "" + " " + url.toString()); return url; } - catch (UnsupportedEncodingException e) - { - throw new RuntimeException(e); - } catch (MalformedURLException e) { throw new RuntimeException(e); } } - protected String getIdType(Subject subject) + protected String getIdType(Principal principal) { - Set<Principal> principals = subject.getPrincipals(); - if (principals.size() > 0) - { - String idTypeStr = null; - Principal principal = principals.iterator().next(); - if (principal instanceof HttpPrincipal) - { - idTypeStr = IdentityType.USERNAME.getValue(); - } - else if (principal instanceof X500Principal) - { - idTypeStr = IdentityType.X500.getValue(); - } - else if (principal instanceof NumericPrincipal) - { - idTypeStr = IdentityType.CADC.getValue(); - } - else if (principal instanceof CookiePrincipal) - { - idTypeStr = IdentityType.COOKIE.getValue(); - } - else - { - final String msg = "Subject has unsupported principal " + - principal.getName() + - ", not one of (X500, Cookie, HTTP or Cadc)."; - throw new IllegalArgumentException(msg); - } - - return idTypeStr; - } - else - { - final String msg = "Subject has no principal."; - throw new IllegalArgumentException(msg); - } - } - - /** - * @param sslSocketFactory the sslSocketFactory to set - */ - public void setSSLSocketFactory(SSLSocketFactory sslSocketFactory) - { - if (mySocketFactory != null) - { - throw new IllegalStateException( - "Illegal use of GMSClient: cannot set SSLSocketFactory " + - "after using one created from Subject"); - } - this.sslSocketFactory = sslSocketFactory; - clearCache(); - } - - private int subjectHashCode = 0; - - private SSLSocketFactory getSSLSocketFactory() - { - AccessControlContext ac = AccessController.getContext(); - Subject s = Subject.getSubject(ac); - - // no real Subject: can only use the one from setSSLSocketFactory - if (s == null || s.getPrincipals().isEmpty()) - { - return sslSocketFactory; - } - - // lazy init - if (this.mySocketFactory == null) + String idTypeStr = AuthenticationUtil.getPrincipalType(principal); + if (idTypeStr == null) { - log.debug("getSSLSocketFactory: " + s); - this.mySocketFactory = SSLUtil.getSocketFactory(s); - this.subjectHashCode = s.hashCode(); - } - else - { - int c = s.hashCode(); - if (c != subjectHashCode) - { - throw new IllegalStateException( - "Illegal use of " + this.getClass().getSimpleName() + - ": subject change not supported for internal " + - "SSLSocketFactory"); - } - } - return this.mySocketFactory; - } - - protected void clearCache() - { - AccessControlContext acContext = AccessController.getContext(); - Subject subject = Subject.getSubject(acContext); - - if (subject != null) - { - log.debug("Clearing cache"); - subject.getPrivateCredentials().clear(); + final String msg = "Subject has unsupported principal " + + principal.getName(); + throw new IllegalArgumentException(msg); } + + return idTypeStr; } } diff --git a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/xml/AbstractReaderWriter.java b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/xml/AbstractReaderWriter.java index 7401185b..732e4065 100644 --- a/projects/cadcAccessControl/src/ca/nrc/cadc/ac/xml/AbstractReaderWriter.java +++ b/projects/cadcAccessControl/src/ca/nrc/cadc/ac/xml/AbstractReaderWriter.java @@ -72,7 +72,6 @@ package ca.nrc.cadc.ac.xml; import ca.nrc.cadc.ac.AC; import ca.nrc.cadc.ac.Group; import ca.nrc.cadc.ac.GroupProperty; -import ca.nrc.cadc.ac.IdentityType; import ca.nrc.cadc.ac.PersonalDetails; import ca.nrc.cadc.ac.PosixDetails; import ca.nrc.cadc.ac.ReaderException; @@ -81,9 +80,11 @@ import ca.nrc.cadc.ac.UserDetails; import ca.nrc.cadc.ac.UserRequest; import ca.nrc.cadc.ac.WriterException; import ca.nrc.cadc.auth.HttpPrincipal; +import ca.nrc.cadc.auth.IdentityType; import ca.nrc.cadc.auth.NumericPrincipal; import ca.nrc.cadc.auth.OpenIdPrincipal; import ca.nrc.cadc.date.DateUtil; + import org.jdom2.Attribute; import org.jdom2.Document; import org.jdom2.Element; @@ -91,6 +92,7 @@ import org.jdom2.output.Format; import org.jdom2.output.XMLOutputter; import javax.security.auth.x500.X500Principal; + import java.io.IOException; import java.io.Writer; import java.security.Principal; -- GitLab