SHIBUI-1774

Changes from feedback

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

pac4j-module/src/main/java/net/unicon/shibui/pac4j/AddNewUserFilter.java

@@ -44,7 +44,7 @@ public class AddNewUserFilter implements Filter {
     private Optional<EmailService> 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<User> 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<String> attributeList = (List<String>) profile.getAttribute(attributeKey);
-        return attributeList.size() < 1 ? null : attributeList.get(0);
-    }
-
     @Override
     public void init(FilterConfig filterConfig) throws ServletException {
     }

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;
         }
     }
+
 }

pac4j-module/src/main/java/net/unicon/shibui/pac4j/SAML2ModelAuthorizationGenerator.java → 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<UserProfile> generate(WebContext context, SessionStore sessionStore, UserProfile profile) {
         Optional<User> user = userRepository.findByUsername(profile.getUsername());
-        user.ifPresent( u -> ((SAML2Profile)profile).addRole(u.getRole()));
+        user.ifPresent( u -> profile.addRole(u.getRole()) );
         return Optional.of(profile);
     }
 }

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,15 +80,15 @@ 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());
 
             final SAML2Client saml2Client = new SAML2Client(saml2Config);
             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);
                                 }

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;

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

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

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')]