From 98131766ba3857e849397d154754ceb43a908141 Mon Sep 17 00:00:00 2001 From: chasegawa Date: Fri, 23 Jul 2021 12:16:33 -0700 Subject: [PATCH] SHIBUI-1774 Changes from feedback --- pac4j-module/build.gradle | 3 +- .../unicon/shibui/pac4j/AddNewUserFilter.java | 46 ++++--------------- .../shibui/pac4j/BetterSAML2Profile.java | 28 +++++++++-- ...calUserProfileAuthorizationGenerator.java} | 14 +++--- .../shibui/pac4j/Pac4jConfiguration.java | 11 +++-- .../pac4j/Pac4jConfigurationProperties.java | 4 +- .../src/main/resources/application.yml | 2 +- .../src/test/docker/conf/application.yml | 2 +- .../shibui/pac4j/AddNewUserFilterTests.groovy | 9 ++-- 9 files changed, 55 insertions(+), 64 deletions(-) rename pac4j-module/src/main/java/net/unicon/shibui/pac4j/{SAML2ModelAuthorizationGenerator.java => LocalUserProfileAuthorizationGenerator.java} (73%) diff --git a/pac4j-module/build.gradle b/pac4j-module/build.gradle index 4318595df..a84d2fe11 100644 --- a/pac4j-module/build.gradle +++ b/pac4j-module/build.gradle @@ -28,7 +28,8 @@ generateLombokConfig.enabled = false dependencies { compileOnly project(':backend') - compile "org.pac4j:spring-security-pac4j:6.0.0" // pac4j is "off" - spring 6.0.0 here uses 5.1 core, thus differences in versions + compile "org.pac4j:spring-security-pac4j:6.0.0" // pac4j is "off" - spring 6.0.0 here uses 5.1 core, thus differences in versions + compile "org.pac4j:pac4j-core:5.1.0" compile "org.pac4j:pac4j-http:5.1.0" compile "org.pac4j:pac4j-saml:5.1.0", { // opensaml libraries are provided diff --git a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/AddNewUserFilter.java b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/AddNewUserFilter.java index 06195db23..c379690c5 100644 --- a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/AddNewUserFilter.java +++ b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/AddNewUserFilter.java @@ -44,7 +44,7 @@ public class AddNewUserFilter implements Filter { private Optional emailService; private Pac4jConfigurationProperties pac4jConfigurationProperties; private RoleRepository roleRepository; - private Pac4jConfigurationProperties.SAML2ProfileMapping saml2ProfileMapping; + private Pac4jConfigurationProperties.SimpleProfileMapping simpleProfileMapping; private UserRepository userRepository; private Matcher matcher; @@ -54,7 +54,7 @@ public AddNewUserFilter(Pac4jConfigurationProperties pac4jConfigurationPropertie this.emailService = emailService; this.pac4jConfigurationProperties = pac4jConfigurationProperties; this.matcher = matcher; - saml2ProfileMapping = this.pac4jConfigurationProperties.getSaml2ProfileMapping(); + simpleProfileMapping = this.pac4jConfigurationProperties.getSimpleProfileMapping(); } @Transactional @@ -69,11 +69,11 @@ private User buildAndPersistNewUserFromProfile(CommonProfile profile) { User user = new User(); user.getRoles().add(newUserRole); - user.setUsername(getAttributeFromProfile(profile, "username")); + user.setUsername(profile.getUsername()); user.setPassword(BCrypt.hashpw(RandomStringUtils.randomAlphanumeric(20), BCrypt.gensalt())); - user.setFirstName(getAttributeFromProfile(profile, "firstName")); - user.setLastName(getAttributeFromProfile(profile, "lastName")); - user.setEmailAddress(getAttributeFromProfile(profile, "email")); + user.setFirstName(profile.getFirstName()); + user.setLastName(profile.getFamilyName()); + user.setEmailAddress(profile.getEmail()); User persistedUser = userRepository.save(user); if (log.isDebugEnabled()) { log.debug("Persisted new user:\n" + user); @@ -95,11 +95,11 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha if (authentication != null) { CommonProfile profile = (CommonProfile) authentication.getPrincipal(); if (profile != null) { - String username = getAttributeFromProfile(profile, "username"); + String username = profile.getUsername(); if (username != null) { Optional persistedUser = userRepository.findByUsername(username); User user; - if (!persistedUser.isPresent()) { + if (persistedUser.isEmpty()) { user = buildAndPersistNewUserFromProfile(profile); emailService.ifPresent(e -> { try { @@ -122,36 +122,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } } - private String getAttributeFromProfile(CommonProfile profile, String stringKey) { - if (profile instanceof SAML2Profile) { - return getAttributeFromSAML2Profile(profile, stringKey); - } - return stringKey.equalsIgnoreCase("username") ? profile.getId() : null; - } - - @SuppressWarnings("unchecked") - private String getAttributeFromSAML2Profile(CommonProfile profile, String stringKey) { - String attributeKey = null; - switch (stringKey) { - case "username": - attributeKey = saml2ProfileMapping.getUsername(); - break; - case "firstName": - attributeKey = saml2ProfileMapping.getFirstName(); - break; - case "lastName": - attributeKey = saml2ProfileMapping.getLastName(); - break; - case "email": - attributeKey = saml2ProfileMapping.getEmail(); - break; - default: - // do we care? Not yet. - } - List attributeList = (List) profile.getAttribute(attributeKey); - return attributeList.size() < 1 ? null : attributeList.get(0); - } - @Override public void init(FilterConfig filterConfig) throws ServletException { } diff --git a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/BetterSAML2Profile.java b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/BetterSAML2Profile.java index 58d16a9ec..17a5edd64 100644 --- a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/BetterSAML2Profile.java +++ b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/BetterSAML2Profile.java @@ -2,22 +2,40 @@ import org.pac4j.saml.profile.SAML2Profile; +import net.unicon.shibui.pac4j.Pac4jConfigurationProperties.SimpleProfileMapping; + import java.util.Collection; public class BetterSAML2Profile extends SAML2Profile { - private final String usernameAttribute; + private SimpleProfileMapping profileMapping; + + public BetterSAML2Profile(final SimpleProfileMapping simpleProfileMapping) { + this.profileMapping = simpleProfileMapping; + } - public BetterSAML2Profile(final String usernameAttribute) { - this.usernameAttribute = usernameAttribute; + @Override + public String getEmail() { + return (String) getAttribute(profileMapping.getEmail()); + } + + @Override + public String getFamilyName() { + return (String) getAttribute(profileMapping.getLastName()); + } + + @Override + public String getFirstName() { + return (String) getAttribute(profileMapping.getFirstName()); } @Override public String getUsername() { - Object username = getAttribute(usernameAttribute); + Object username = getAttribute(profileMapping.getUsername()); if (username instanceof Collection) { - return (String) ((Collection)username).toArray()[0]; + return (String) ((Collection) username).toArray()[0]; } else { return (String) username; } } + } diff --git a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/SAML2ModelAuthorizationGenerator.java b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/LocalUserProfileAuthorizationGenerator.java similarity index 73% rename from pac4j-module/src/main/java/net/unicon/shibui/pac4j/SAML2ModelAuthorizationGenerator.java rename to pac4j-module/src/main/java/net/unicon/shibui/pac4j/LocalUserProfileAuthorizationGenerator.java index 4286d4e38..1f0f0cba1 100644 --- a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/SAML2ModelAuthorizationGenerator.java +++ b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/LocalUserProfileAuthorizationGenerator.java @@ -1,26 +1,26 @@ package net.unicon.shibui.pac4j; -import edu.internet2.tier.shibboleth.admin.ui.security.model.User; -import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository; +import java.util.Optional; + import org.pac4j.core.authorization.generator.AuthorizationGenerator; import org.pac4j.core.context.WebContext; import org.pac4j.core.context.session.SessionStore; import org.pac4j.core.profile.UserProfile; -import org.pac4j.saml.profile.SAML2Profile; -import java.util.Optional; +import edu.internet2.tier.shibboleth.admin.ui.security.model.User; +import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository; -public class SAML2ModelAuthorizationGenerator implements AuthorizationGenerator { +public class LocalUserProfileAuthorizationGenerator implements AuthorizationGenerator { private final UserRepository userRepository; - public SAML2ModelAuthorizationGenerator(UserRepository userRepository) { + public LocalUserProfileAuthorizationGenerator(UserRepository userRepository) { this.userRepository = userRepository; } @Override public Optional generate(WebContext context, SessionStore sessionStore, UserProfile profile) { Optional user = userRepository.findByUsername(profile.getUsername()); - user.ifPresent( u -> ((SAML2Profile)profile).addRole(u.getRole())); + user.ifPresent( u -> profile.addRole(u.getRole()) ); return Optional.of(profile); } } diff --git a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfiguration.java b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfiguration.java index b73d7593d..5e173f151 100644 --- a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfiguration.java +++ b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfiguration.java @@ -50,13 +50,13 @@ public class Pac4jConfiguration { * Custom class that ensures we add the user's roles to the information when doing SAML2 auth */ @Bean - public SAML2ModelAuthorizationGenerator saml2ModelAuthorizationGenerator(UserRepository userRepository) { - return new SAML2ModelAuthorizationGenerator(userRepository); + public LocalUserProfileAuthorizationGenerator saml2ModelAuthorizationGenerator(UserRepository userRepository) { + return new LocalUserProfileAuthorizationGenerator(userRepository); } @Bean(name = "pac4j-config") public Config config(final Pac4jConfigurationProperties pac4jConfigProps, - final SAML2ModelAuthorizationGenerator saml2ModelAuthorizationGenerator) { + final LocalUserProfileAuthorizationGenerator saml2ModelAuthorizationGenerator) { log.info("**** Configuring PAC4J "); final Config config = new Config(); final Clients clients = new Clients(pac4jConfigProps.getCallbackUrl()); @@ -80,7 +80,7 @@ public Config config(final Pac4jConfigurationProperties pac4jConfigProps, saml2Config.setServiceProviderMetadataPath(pac4jConfigProps.getServiceProviderMetadataPath()); saml2Config.setForceServiceProviderMetadataGeneration(pac4jConfigProps.isForceServiceProviderMetadataGeneration()); saml2Config.setWantsAssertionsSigned(pac4jConfigProps.isWantAssertionsSigned()); - saml2Config.setAttributeAsId(pac4jConfigProps.getSaml2ProfileMapping().getUsername()); + saml2Config.setAttributeAsId(pac4jConfigProps.getSimpleProfileMapping().getUsername()); //saml2Config.setPostLogoutURL(pac4jConfigProps.getPostLogoutURL()); // consideration needed? //saml2Config.setSpLogoutRequestBindingType(pac4jConfigProps.getSpLogoutRequestBindingType()); @@ -88,7 +88,7 @@ public Config config(final Pac4jConfigurationProperties pac4jConfigProps, saml2Client.setName("Saml2Client"); saml2Client.addAuthorizationGenerator(saml2ModelAuthorizationGenerator); SAML2Authenticator saml2Authenticator = new SAML2Authenticator(saml2Config.getAttributeAsId(), saml2Config.getMappedAttributes()); - saml2Authenticator.setProfileDefinition(new CommonProfileDefinition(p -> new BetterSAML2Profile(pac4jConfigProps.getSaml2ProfileMapping().getUsername()))); + saml2Authenticator.setProfileDefinition(new CommonProfileDefinition(p -> new BetterSAML2Profile(pac4jConfigProps.getSimpleProfileMapping()))); saml2Client.setAuthenticator(saml2Authenticator); saml2Client.setName(PAC4J_CLIENT_NAME); @@ -112,6 +112,7 @@ public void validate(Credentials credentials, WebContext context, SessionStore s final CommonProfile profile = new CommonProfile(); String token = ((TokenCredentials)credentials).getToken(); profile.setId(token); + profile.addAttribute("username", token); profile.setRoles(userService.getUserRoles(token)); credentials.setUserProfile(profile); } diff --git a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfigurationProperties.java b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfigurationProperties.java index 8a44baf47..977164958 100644 --- a/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfigurationProperties.java +++ b/pac4j-module/src/main/java/net/unicon/shibui/pac4j/Pac4jConfigurationProperties.java @@ -28,7 +28,7 @@ public class Pac4jConfigurationProperties { private String keystorePath = "/tmp/samlKeystore.jks"; private int maximumAuthenticationLifetime = 3600; private String privateKeyPassword = "changeit"; - private SAML2ProfileMapping saml2ProfileMapping; + private SimpleProfileMapping simpleProfileMapping; private String serviceProviderEntityId = "https://unicon.net/shibui"; private String serviceProviderMetadataPath = "/tmp/sp-metadata.xml"; private String typeOfAuth = "SAML2"; @@ -37,7 +37,7 @@ public class Pac4jConfigurationProperties { @Getter @Setter - public static class SAML2ProfileMapping { + public static class SimpleProfileMapping { private String email; private String firstName; private String lastName; diff --git a/pac4j-module/src/main/resources/application.yml b/pac4j-module/src/main/resources/application.yml index 4042fd939..ebba4fcd9 100644 --- a/pac4j-module/src/main/resources/application.yml +++ b/pac4j-module/src/main/resources/application.yml @@ -1,6 +1,6 @@ shibui: pac4j: - saml2ProfileMapping: + simpleProfileMapping: username: urn:oid:0.9.2342.19200300.100.1.3 firstName: givenName lastName: sn diff --git a/pac4j-module/src/test/docker/conf/application.yml b/pac4j-module/src/test/docker/conf/application.yml index 90ddce36a..b49782897 100644 --- a/pac4j-module/src/test/docker/conf/application.yml +++ b/pac4j-module/src/test/docker/conf/application.yml @@ -31,7 +31,7 @@ shibui: forceServiceProviderMetadataGeneration: true callbackUrl: "https://localhost:8443/callback" maximumAuthenticationLifetime: 3600000 - saml2ProfileMapping: + simpleProfileMapping: username: urn:oid:0.9.2342.19200300.100.1.1 firstName: urn:oid:2.5.4.42 lastName: urn:oid:2.5.4.4 diff --git a/pac4j-module/src/test/groovy/net/unicon/shibui/pac4j/AddNewUserFilterTests.groovy b/pac4j-module/src/test/groovy/net/unicon/shibui/pac4j/AddNewUserFilterTests.groovy index 6a1c7e976..d15b70f0a 100644 --- a/pac4j-module/src/test/groovy/net/unicon/shibui/pac4j/AddNewUserFilterTests.groovy +++ b/pac4j-module/src/test/groovy/net/unicon/shibui/pac4j/AddNewUserFilterTests.groovy @@ -45,7 +45,7 @@ class AddNewUserFilterTests extends Specification { @Autowired Pac4jConfigurationProperties pac4jConfigurationProperties - Pac4jConfigurationProperties.SAML2ProfileMapping saml2ProfileMapping + Pac4jConfigurationProperties.SimpleProfileMapping profileMapping @Subject AddNewUserFilter addNewUserFilter @@ -56,7 +56,7 @@ class AddNewUserFilterTests extends Specification { authentication.getPrincipal() >> saml2Profile addNewUserFilter = new AddNewUserFilter(pac4jConfigurationProperties, userRepository, roleRepository, new PathMatcher(), Optional.of(emailService)) - saml2ProfileMapping = pac4jConfigurationProperties.saml2ProfileMapping + profileMapping = pac4jConfigurationProperties.simpleProfileMapping } def "new users are redirected"() { @@ -65,8 +65,9 @@ class AddNewUserFilterTests extends Specification { 'FirstName': 'New', 'LastName': 'User', 'Email': 'newuser@institution.edu'].each { key, value -> - saml2Profile.getAttribute(saml2ProfileMapping."get${key}"()) >> [value] + saml2Profile.getAttribute(profileMapping."get${key}"()) >> [value] } + saml2Profile.getUsername() >> "newUser" userRepository.findByUsername('newUser') >> Optional.empty() roleRepository.findByName('ROLE_NONE') >> Optional.of(new Role('ROLE_NONE')) @@ -82,7 +83,7 @@ class AddNewUserFilterTests extends Specification { def "existing users are not redirected"() { given: - saml2Profile.getAttribute(saml2ProfileMapping.getUsername()) >> ['existingUser'] + saml2Profile.getUsername() >> "existingUser" userRepository.findByUsername('existingUser') >> Optional.of(new User().with { it.username = 'existingUser' it.roles = [new Role('ROLE_USER')]