Merge pull request #1142 from drewwills/UP-5002

UP-5002:  Access the 'org.apereo.portal.security.PersonFactory.guest_…

uPortal-api/uPortal-api-rest/src/test/java/org/apereo/portal/rest/SessionRESTControllerTest.java

@@ -18,6 +18,7 @@
 import org.apereo.portal.events.PortalEventFactoryImpl;
 import org.apereo.portal.security.IPerson;
 import org.apereo.portal.security.IPersonManager;
+import org.apereo.portal.security.PersonFactory;
 import org.apereo.portal.security.provider.PersonImpl;
 import org.junit.Assert;
 import org.junit.Before;
@@ -47,6 +48,9 @@ public class SessionRESTControllerTest {
 
     @Before
     public void setup() {
+        PersonFactory fac = new PersonFactory();
+        fac.init();
+
         sessionRESTController = new SessionRESTController();
         res = new MockHttpServletResponse();
         req = new MockHttpServletRequest();

uPortal-core/src/main/java/org/apereo/portal/security/PersonFactory.java

@@ -17,31 +17,43 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
-import org.apereo.portal.properties.PropertiesManager;
+import javax.annotation.PostConstruct;
 import org.apereo.portal.security.provider.PersonImpl;
 import org.apereo.portal.security.provider.RestrictedPerson;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.stereotype.Component;
 
 /**
- * Creates a person.
+ * Responsible for creating {@link IPerson} instances. Historically, the capabilities of this class
+ * were accessed through static methods and constants, but with uP5 (and beyond) we need to
+ * configure this class through the <code>PropertySourcesPlaceholderConfigurer</code>. At present
+ * this class brideges both worlds, but in the future it would be good to move away from static
+ * methods.
  *
- * <p>Can create representations of a <i>system</i> user and a <i>guest</i> user.
+ * <p>Can create representations of a <i>system</i> as well as <i>guest</i> users.
  *
- * <p><i>system</i> users have an ID of 0
+ * <p>The <i>system</i> user has an ID of 0
  *
- * <p><i>guest</i> users have both of the following characteristics<br>
+ * <p><i>guest</i> users exhibit both of the following characteristics<br>
  *
  * <ol>
- *   <li>User is not successfully authenticated with the portal.
- *   <li>User name matches the value of the property <code>
- *       org.apereo.portal.security.PersonFactory.guest_user_name</code> in <code>portal.properties
- *       </code>.
+ *   <li>User is not (successfully) authenticated with the portal.
+ *   <li>Username is included in the list specified by the property <code>
+ *       org.apereo.portal.security.PersonFactory.guest_user_names</code>.
  * </ol>
  */
+@Component
 public class PersonFactory {
 
-    private static final String GUEST_USERNAMES_PROPERTY =
-            PropertiesManager.getProperty(
-                    "org.apereo.portal.security.PersonFactory.guest_user_names", "guest");
+    private String guestUsernamesProperty = "guest"; // default;  for unit tests
+
+    private static List<String> guestUsernames = null;
+
+    @PostConstruct
+    public void init() {
+        guestUsernames =
+                Collections.unmodifiableList(Arrays.asList(guestUsernamesProperty.split(",")));
+    }
 
     /**
      * Collection of guest user names specified in portal.properties as <code>
@@ -50,8 +62,13 @@ public class PersonFactory {
      *
      * @since 5.0
      */
-    public static final List<String> GUEST_USERNAMES =
-            Collections.unmodifiableList(Arrays.asList(GUEST_USERNAMES_PROPERTY.split(",")));
+    public static List<String> getGuestUsernames() {
+        if (guestUsernames == null) {
+            throw new IllegalStateException(
+                    "The guestUsernames collection has not been initialized");
+        }
+        return guestUsernames;
+    }
 
     /**
      * Creates an empty <code>IPerson</code> implementation.
@@ -83,4 +100,15 @@ public static RestrictedPerson createRestrictedPerson() {
         IPerson person = createPerson();
         return new RestrictedPerson(person);
     }
+
+    /**
+     * In addition to supporting the Spring context, this setter allows unit tests to bootstrap this
+     * class so that downstream features won't break.
+     *
+     * @since 5.0
+     */
+    @Value("${org.apereo.portal.security.PersonFactory.guest_user_names:guest}")
+    public void setGuestUsernamesProperty(String guestUsernamesProperty) {
+        this.guestUsernamesProperty = guestUsernamesProperty;
+    }
 }

uPortal-core/src/main/java/org/apereo/portal/security/provider/PersonImpl.java

@@ -234,7 +234,7 @@ public void setFullName(String sFullName) {
     @Override
     public boolean isGuest() {
         String userName = (String) getAttribute(IPerson.USERNAME);
-        boolean isGuestUsername = PersonFactory.GUEST_USERNAMES.contains(userName);
+        boolean isGuestUsername = PersonFactory.getGuestUsernames().contains(userName);
         boolean isAuthenticated = m_securityContext != null && m_securityContext.isAuthenticated();
         return isGuestUsername && !isAuthenticated;
     }

uPortal-groups/uPortal-groups-pags/src/test/java/org/apereo/portal/groups/pags/PAGSTest.java

@@ -30,6 +30,7 @@
 import org.apereo.portal.groups.pags.testers.ValueExistsTester;
 import org.apereo.portal.groups.pags.testers.ValueMissingTester;
 import org.apereo.portal.security.IPerson;
+import org.apereo.portal.security.PersonFactory;
 import org.apereo.portal.security.provider.PersonImpl;
 
 /** Tests the PAGS testers. */
@@ -86,6 +87,10 @@ private static void print(String msg) {
     }
 
     protected void setUp() {
+
+        PersonFactory fac = new PersonFactory();
+        fac.init();
+
         try {
             if (IPERSON_CLASS == null) {
                 IPERSON_CLASS = Class.forName("org.apereo.portal.security.IPerson");

uPortal-groups/uPortal-groups-pags/src/test/java/org/apereo/portal/groups/pags/testers/GuestUserTesterTest.java

@@ -16,12 +16,20 @@
 
 import org.apereo.portal.groups.pags.TestPersonAttributesGroupTestDefinition;
 import org.apereo.portal.security.IPerson;
+import org.apereo.portal.security.PersonFactory;
 import org.apereo.portal.security.provider.PersonImpl;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 public class GuestUserTesterTest {
 
+    @Before
+    public void setUp() {
+        PersonFactory fac = new PersonFactory();
+        fac.init();
+    }
+
     @Test
     public void testGuestTrue() throws Exception {
         GuestUserTester tester =

uPortal-layout/uPortal-layout-core/src/test/java/org/apereo/portal/layout/profile/ServerNameGuestChainingProfileMapperTest.java

@@ -19,8 +19,10 @@
 import java.util.List;
 import java.util.Map;
 import org.apereo.portal.security.IPerson;
+import org.apereo.portal.security.PersonFactory;
 import org.apereo.portal.security.provider.PersonImpl;
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.BlockJUnit4ClassRunner;
@@ -29,14 +31,20 @@
 @RunWith(BlockJUnit4ClassRunner.class)
 public class ServerNameGuestChainingProfileMapperTest {
 
-    private Map<String, String> configMap = new HashMap<String, String>();
+    private Map<String, String> configMap = new HashMap<>();
 
     {
         configMap.put("test-lycee.portail.ent", "lycees");
         configMap.put("test-cfa.portail.ent", "cfa");
         configMap.put("test.college.ent", "clg37");
     }
 
+    @Before
+    public void setUp() {
+        PersonFactory fac = new PersonFactory();
+        fac.init();
+    }
+
     @Test
     public void testGuestAuthorizedDomain() throws Exception {
         ServerNameGuestChainingProfileMapper profileMapper =
@@ -106,12 +114,12 @@ public void testNonGuestWithSubMapperMatching() throws Exception {
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -132,12 +140,12 @@ public void testNonGuestWithSubMapperNotMatching() throws Exception {
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -158,12 +166,12 @@ public void testGuestNonAuthorizedDomainWithSubMapperMatching() throws Exception
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -184,12 +192,12 @@ public void testGuestAuthorizedDomainWithSubMapperMatching() throws Exception {
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -210,12 +218,12 @@ public void testGuestAuthorizedDomainWithSubMapperNotMatchingWithSubDefault() th
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -237,13 +245,13 @@ public void testGuestAuthorizedDomainWithSubMapperNotMatchingWithoutSubDefault()
         ServerNameGuestChainingProfileMapper profileMapper =
                 new ServerNameGuestChainingProfileMapper();
         profileMapper.setAuthorizedServerNames(configMap);
-        List<IProfileMapper> subMappers = new ArrayList<IProfileMapper>();
+        List<IProfileMapper> subMappers = new ArrayList<>();
         SessionAttributeProfileMapperImpl subMapper = new SessionAttributeProfileMapperImpl();
         subMappers.add(subMapper);
         subMapper.setDefaultProfileName("submapper_default");
         subMapper.setAttributeName("session_attr");
         subMapper.setDefaultProfileName("");
-        Map<String, String> mapping = new HashMap<String, String>();
+        Map<String, String> mapping = new HashMap<>();
         mapping.put("session_attr", "sub_fname");
         subMapper.setMappings(mapping);
         profileMapper.setSubMappers(subMappers);
@@ -259,14 +267,14 @@ public void testGuestAuthorizedDomainWithSubMapperNotMatchingWithoutSubDefault()
         Assert.assertEquals("guest-clg37-default", fname);
     }
 
-    protected static IPerson createGuestPerson() throws Exception {
+    private static IPerson createGuestPerson() throws Exception {
         IPerson person = new PersonImpl();
         person.setAttribute(IPerson.USERNAME, "guest");
 
         return person;
     }
 
-    protected static IPerson createPerson() throws Exception {
+    private static IPerson createPerson() throws Exception {
         IPerson person = new PersonImpl();
         person.setAttribute(IPerson.USERNAME, "non_guest");
 

uPortal-layout/uPortal-layout-impl/src/main/java/org/apereo/portal/layout/dlm/DistributedLayoutManager.java

@@ -1383,7 +1383,7 @@ public boolean resetLayout(String loginId) {
     /** Resets the layout of the specified user. */
     private boolean resetLayout(IPerson person) {
         final String userName = person.getUserName();
-        if (PersonFactory.GUEST_USERNAMES.contains(userName)) {
+        if (PersonFactory.getGuestUsernames().contains(userName)) {
             throw new IllegalArgumentException("CANNOT RESET LAYOUT FOR A GUEST USER: " + person);
         }
         LOG.warn("Resetting user layout for: " + userName, new Throwable());

uPortal-persondir/src/main/java/org/apereo/portal/RDBMUserIdentityStore.java

@@ -164,7 +164,7 @@ public void removePortalUID(final String userName) {
                 new TransactionCallbackWithoutResult() {
                     @Override
                     protected void doInTransactionWithoutResult(TransactionStatus arg0) {
-                        if (PersonFactory.GUEST_USERNAMES.contains(userName)) {
+                        if (PersonFactory.getGuestUsernames().contains(userName)) {
                             throw new IllegalArgumentException(
                                     "CANNOT RESET LAYOUT FOR A GUEST USER");
                         }
@@ -295,7 +295,7 @@ public int getPortalUID(IPerson person, boolean createPortalData)
         String username = (String) person.getAttribute(IPerson.USERNAME);
 
         // only synchronize a non-guest request.
-        if (PersonFactory.GUEST_USERNAMES.contains(username)) {
+        if (PersonFactory.getGuestUsernames().contains(username)) {
             uid = __getPortalUID(person, createPortalData);
         } else {
             Object lock = getLock(person);

uPortal-portlets/src/main/java/org/apereo/portal/portlets/favorites/FavoritesController.java

@@ -117,7 +117,7 @@ public String initializeView(PortletRequest req, Model model) {
         final String username =
                 req.getRemoteUser() != null
                         ? req.getRemoteUser()
-                        : PersonFactory.GUEST_USERNAMES.get(0); // First item is the default
+                        : PersonFactory.getGuestUsernames().get(0); // First item is the default
         final IAuthorizationPrincipal principal =
                 authorizationService.newPrincipal(username, IPerson.class);
         final List<IUserLayoutNodeDescription> favorites = new ArrayList<>();

uPortal-security/uPortal-security-core/src/main/java/org/apereo/portal/security/provider/BasePersonManager.java

@@ -103,7 +103,7 @@ public IPerson getPerson(HttpServletRequest request) throws PortalSecurityExcept
     protected IPerson createGuestPerson(HttpServletRequest request) throws Exception {
 
         // First we need to know the guest username
-        String username = PersonFactory.GUEST_USERNAMES.get(0); // First item is the default
+        String username = PersonFactory.getGuestUsernames().get(0); // First item is the default
 
         // Pluggable strategy for supporting multiple guest users
         for (IGuestUsernameSelector selector : guestUsernameSelectors) {
@@ -115,7 +115,7 @@ protected IPerson createGuestPerson(HttpServletRequest request) throws Exception
         }
 
         // Sanity check...
-        if (!PersonFactory.GUEST_USERNAMES.contains(username)) {
+        if (!PersonFactory.getGuestUsernames().contains(username)) {
             final String msg =
                     "The specified guest username is not in the configured list:  " + username;
             throw new IllegalStateException(msg);

uPortal-webapp/src/main/resources/layout/theme/respondr/navigation.xsl

@@ -75,26 +75,8 @@
  | Sets the use of the sub navigation row, which lists out links to the portlets on the active tab.
  | Values are 'true' or 'false'
 -->
-<!-- Use the INSTITUTION parameter to configure the subnavigation row on a per skin/institution basis. -->
 <xsl:param name="USE_SUBNAVIGATION_ROW" select="false" />
 
-  <!--
-   | The unofficial "theme-switcher".
-   | The INSTITUTION variable can be used to make logical tests and configure the theme on a per skin basis.
-   | Allows the the theme to configure differently for a skin or group of skins, yet not break for other skins that might require a different configuration.
-   | The implementation is hard-coded, but it works.
-   | May require the addition of an xsl:choose statement around parameters, vairables, and template calls.
-  -->
-  <xsl:variable name="INSTITUTION">
-    <xsl:choose>
-      <xsl:when test="$SKIN='university' or $SKIN='university-div1' or $SKIN='university-div2'">university</xsl:when> <!-- Set all institution skins to a specific theme configuration  -->
-      <xsl:when test="$SKIN='coal'">coal</xsl:when>
-      <xsl:when test="$SKIN='ivy'">ivy</xsl:when>
-      <xsl:when test="$SKIN='hc'">hc</xsl:when>
-      <xsl:otherwise>uportal</xsl:otherwise>
-    </xsl:choose>
-  </xsl:variable>
-
   <!-- ========== TEMPLATE: NAVIGATION ========== -->
   <!-- ========================================== -->
   <!--