From d5d7ad7077f9dedaa1d30c934a96615aabec1144 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Wed, 16 Jan 2019 18:50:24 -0700 Subject: [PATCH] [SHIBUI-1030] Modified the User<->Role relationship, removing Cascade completely. Added eager fetching to roles w/in users, lazy to users w/in roles. Modified DevConfig to create roles first, then create users. Replaced existing email-sending unit test with one that checks the results from MailHog. It is a bit messy due to all of the junk that ends up in the email json that Mailhog returns that isn't the same each time. --- .../admin/ui/configuration/DevConfig.groovy | 35 ++++++++++++++++--- .../admin/ui/security/model/Role.java | 2 +- .../admin/ui/security/model/User.java | 4 +-- .../admin/ui/service/EmailService.java | 1 + .../admin/ui/service/EmailServiceImpl.java | 4 +-- .../ui/configuration/TestConfiguration.groovy | 2 +- .../ui/service/EmailServiceImplTests.groovy | 30 +++++++++++++++- 7 files changed, 67 insertions(+), 11 deletions(-) diff --git a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy index a225d7271..dcf255601 100644 --- a/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy +++ b/backend/src/main/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/DevConfig.groovy @@ -11,6 +11,7 @@ import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.ReloadableMetadat import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository import edu.internet2.tier.shibboleth.admin.ui.security.model.Role import edu.internet2.tier.shibboleth.admin.ui.security.model.User +import edu.internet2.tier.shibboleth.admin.ui.security.repository.RoleRepository import edu.internet2.tier.shibboleth.admin.ui.security.repository.UserRepository import edu.internet2.tier.shibboleth.admin.util.ModelRepresentationConversions import org.springframework.context.annotation.Bean @@ -24,17 +25,35 @@ import javax.annotation.PostConstruct @Profile('dev') class DevConfig { private final UserRepository adminUserRepository + private final RoleRepository roleRepository private final MetadataResolverRepository metadataResolverRepository - DevConfig(UserRepository adminUserRepository, MetadataResolverRepository metadataResolverRepository) { + DevConfig(UserRepository adminUserRepository, MetadataResolverRepository metadataResolverRepository, RoleRepository roleRepository) { this.adminUserRepository = adminUserRepository this.metadataResolverRepository = metadataResolverRepository + this.roleRepository = roleRepository } @Transactional @PostConstruct void createDevUsers() { + if (roleRepository.count() == 0) { + def roles = [new Role().with { + name = 'ROLE_ADMIN' + it + }, new Role().with { + name = 'ROLE_USER' + it + }, new Role().with { + name = 'ROLE_NONE' + it + }] + roles.each { + roleRepository.save(it) + } + } + roleRepository.flush() if (adminUserRepository.count() == 0) { def users = [new User().with { username = 'admin' @@ -42,7 +61,7 @@ class DevConfig { firstName = 'Joe' lastName = 'Doe' emailAddress = 'joe@institution.edu' - roles.add(new Role(name: 'ROLE_ADMIN')) + roles.add(roleRepository.findByName('ROLE_ADMIN').get()) it }, new User().with { username = 'nonadmin' @@ -50,13 +69,21 @@ class DevConfig { firstName = 'Peter' lastName = 'Vandelay' emailAddress = 'peter@institution.edu' - roles.add(new Role(name: 'ROLE_USER')) + roles.add(roleRepository.findByName('ROLE_USER').get()) + it + }, new User().with { + username = 'admin2' + password = '{noop}anotheradmin' + firstName = 'Rand' + lastName = 'al\'Thor' + emailAddress = 'rand@institution.edu' + roles.add(roleRepository.findByName('ROLE_ADMIN').get()) it }] users.each { adminUserRepository.save(it) } - + adminUserRepository.flush() } } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java index c7f1112b3..41acabdca 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/Role.java @@ -45,7 +45,7 @@ public Role(String name, int rank) { //Ignore properties annotation here is to prevent stack overflow recursive error during JSON serialization @JsonIgnoreProperties("roles") - @ManyToMany(mappedBy = "roles", fetch = FetchType.EAGER) + @ManyToMany(mappedBy = "roles", fetch = FetchType.LAZY) private Set users = new HashSet<>(); } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java index edb7c542a..1a36ffdcc 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/model/User.java @@ -10,9 +10,9 @@ import lombok.ToString; import org.apache.commons.lang.StringUtils; -import javax.persistence.CascadeType; import javax.persistence.Column; import javax.persistence.Entity; +import javax.persistence.FetchType; import javax.persistence.JoinColumn; import javax.persistence.JoinTable; import javax.persistence.ManyToMany; @@ -50,7 +50,7 @@ public class User extends AbstractAuditable { private String role; @JsonIgnore - @ManyToMany(cascade = CascadeType.PERSIST) + @ManyToMany(fetch = FetchType.EAGER) @JoinTable(name = "user_role", joinColumns = @JoinColumn(name = "user_id"), inverseJoinColumns = @JoinColumn(name = "role_id")) private Set roles = new HashSet<>(); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailService.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailService.java index 39b441e9a..a4ace393f 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailService.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailService.java @@ -9,4 +9,5 @@ public interface EmailService { void sendMail(String emailTemplate, String fromAddress, String[] recipients, String subject, Locale locale) throws MessagingException; void sendNewUserMail(String newUsername) throws MessagingException; + String[] getSystemAdminEmailAddresses(); } diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImpl.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImpl.java index 601ad48a0..6e8d20f28 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImpl.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImpl.java @@ -62,10 +62,10 @@ public void sendMail(String emailTemplate, String fromAddress, String[] recipien public void sendNewUserMail(String newUsername) throws MessagingException { String subject = String.format("User Access Request for %s", newUsername); - sendMail("new-user", systemEmailAddress, getSystemAdmins(), subject, Locale.getDefault()); + sendMail("new-user", systemEmailAddress, getSystemAdminEmailAddresses(), subject, Locale.getDefault()); } - private String[] getSystemAdmins() { + public String[] getSystemAdminEmailAddresses() { Set systemAdmins = userRepository.findByRoles_Name("ROLE_ADMIN"); if (systemAdmins == null || systemAdmins.size() == 0) { //TODO: Should this be an exception? diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/TestConfiguration.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/TestConfiguration.groovy index 2ee2a5638..a01645a09 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/TestConfiguration.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/configuration/TestConfiguration.groovy @@ -40,7 +40,7 @@ class TestConfiguration { @Bean JavaMailSender javaMailSender() { return new JavaMailSenderImpl().with { - it.host = 'mailhog' + it.host = 'localhost' it.port = 1025 it } diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImplTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImplTests.groovy index ce735d105..8960f807e 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImplTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/service/EmailServiceImplTests.groovy @@ -6,10 +6,13 @@ import edu.internet2.tier.shibboleth.admin.ui.configuration.EmailConfiguration import edu.internet2.tier.shibboleth.admin.ui.configuration.InternationalizationConfiguration import edu.internet2.tier.shibboleth.admin.ui.configuration.SearchConfiguration import edu.internet2.tier.shibboleth.admin.ui.configuration.TestConfiguration +import groovy.json.JsonOutput +import groovy.json.JsonSlurper import org.springframework.beans.factory.annotation.Autowired import org.springframework.boot.autoconfigure.domain.EntityScan import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest import org.springframework.boot.test.context.SpringBootTest +import org.springframework.core.env.Environment import org.springframework.data.jpa.repository.config.EnableJpaRepositories import org.springframework.test.context.ActiveProfiles import org.springframework.test.context.ContextConfiguration @@ -30,13 +33,38 @@ class EmailServiceImplTests extends Specification { @Autowired EmailService emailService + @Autowired + Environment env + + JsonSlurper jsonSlurper = new JsonSlurper() + // Ignoring until we can figure out how to get this to pass on Jenkins @Ignore def "emailService can successfully send an email"() { + given: + + def mailhogHost = 'localhost' + def mailhogPort = '8025' + + def newUserName = 'foobar' + def expectedToAddresses = emailService.systemAdminEmailAddresses + def expectedFromAddress = env.getProperty('shibui.mail.system-email-address') + def expectedNewUserEmailSubject = "User Access Request for ${newUserName}" + def expectedTextEmailBody = new File(this.class.getResource('/mail/text/new-user.txt').toURI()).text + when: emailService.sendNewUserMail("foobar") + def getEmailsURL = new URL("http://${mailhogHost}:${mailhogPort}/api/v2/messages") + def resultJson = jsonSlurper.parse(getEmailsURL) + println(JsonOutput.prettyPrint(JsonOutput.toJson(resultJson))) then: - noExceptionThrown() + // There's a bunch of other noise in here that changes per email + resultJson.total == 1 + expectedToAddresses.size() == resultJson.items[0].To.size() + expectedToAddresses.join(', ') == resultJson.items[0].Content.Headers.To[0] + expectedFromAddress == resultJson.items[0].From.Mailbox + '@' + resultJson.items[0].From.Domain + expectedNewUserEmailSubject == resultJson.items[0].Content.Headers.Subject[0] + resultJson.items[0].Content.Body.contains(expectedTextEmailBody) } }