Site Service: refactored site service to use groups to manage site users

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@10375 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

config/alfresco/messages/patch-service.properties

@@ -219,4 +219,7 @@ patch.updateDmPermissions.description=Update ACLs on all DM node objects to the
 patch.updateDmPermissions.result=Updated ACLs. Created {0} defining ACLs.
 
 patch.createSiteStore.description=Create the AVM store for site data structure for 3.0 web-tier clients.
-patch.createSiteStore.result=Created the AVM site data store.
+patch.createSiteStore.result=Created the AVM site data store.
+
+patch.sitePermissionRefactorPatch.description=Create permission groups for sites.
+patch.sitePermissionRefactorPatch.result=Groups have been created for all sites and user's allocated accordingly.

config/alfresco/patch/patch-services-context.xml

@@ -1470,4 +1470,22 @@
         </property>
     </bean>
     
+    <bean id="patch.sitePermissionRefactorPatch" class="org.alfresco.repo.admin.patch.impl.SitePermissionRefactorPatch" parent="basePatch">
+        <property name="id"><value>patch.sitePermissionRefactorPatch</value></property>
+        <property name="description"><value>patch.sitePermissionRefactorPatch.description</value></property>
+        <property name="fixesFromSchema"><value>0</value></property>
+        <property name="fixesToSchema"><value>128</value></property>
+        <property name="targetSchema"><value>129</value></property>
+        <!-- helper beans -->
+        <property name="siteService">
+            <ref bean="siteService" />
+        </property>
+        <property name="permissionService">
+            <ref bean="PermissionService" />
+        </property>
+        <property name="authorityService">
+            <ref bean="AuthorityService" />
+        </property>
+    </bean>
+    
 </beans>

config/alfresco/site-services-context.xml

@@ -16,6 +16,40 @@
     	<property name="model" value="alfresco/model/sitePermissionDefinitions.xml"/>
     </bean>
     
+    <!-- Site service bean -->
+    <bean id="SiteService" class="org.springframework.aop.framework.ProxyFactoryBean">
+        <property name="proxyInterfaces">
+            <value>org.alfresco.repo.site.SiteService</value>
+        </property>
+        <property name="target">
+            <ref bean="siteService"/>
+        </property>
+        <property name="interceptorNames">
+            <list>
+                <idref local="SiteService_transaction"/>
+                <idref bean="AuditMethodInterceptor"/>
+                <idref bean="exceptionTranslator"/>
+                <idref local="SiteService_security"/>
+            </list>
+        </property>
+    </bean>
+
+    <!-- Site service transaction bean -->
+    <bean id="SiteService_transaction" class="org.springframework.transaction.interceptor.TransactionInterceptor">
+        <property name="transactionManager">
+            <ref bean="transactionManager"/>
+        </property>
+        <property name="transactionAttributes">
+            <props>
+                <prop key="*">${server.transaction.mode.default}</prop>
+            </props>
+        </property>
+    </bean>
+    
+    <!-- Site service security bean -->
+    <bean id="SiteService_security" class="org.alfresco.repo.security.permissions.impl.AlwaysProceedMethodInterceptor" />
+    
+    
     <bean id="siteService" class="org.alfresco.repo.site.SiteServiceImpl">
        <property name="nodeService" ref="NodeService"/>
        <property name="fileFolderService" ref="FileFolderService"/>
@@ -25,6 +59,7 @@
        <property name="personService" ref="PersonService"/>
        <property name="activityService" ref="activityService"/>
        <property name="taggingService" ref="TaggingService"/>
+       <property name="authorityService" ref="AuthorityService"/>
     </bean>
     
     <bean id="siteServiceScript" parent="baseJavaScriptExtension" class="org.alfresco.repo.site.script.ScriptSiteService">

config/alfresco/version.properties

@@ -19,4 +19,4 @@ version.build=@build-number@
 
 # Schema number
 
-version.schema=128
+version.schema=129

source/java/org/alfresco/repo/admin/patch/impl/SitePermissionRefactorPatch.java

@@ -0,0 +1,141 @@
+/*
+ * Copyright (C) 2005-2008 Alfresco Software Limited.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+ * As a special exception to the terms and conditions of version 2.0 of 
+ * the GPL, you may redistribute this Program in connection with Free/Libre 
+ * and Open Source Software ("FLOSS") applications as described in Alfresco's 
+ * FLOSS exception.  You should have recieved a copy of the text describing 
+ * the FLOSS exception, and it is also available here: 
+ * http://www.alfresco.com/legal/licensing"
+ */
+package org.alfresco.repo.admin.patch.impl;
+
+import java.util.List;
+import java.util.Set;
+
+import org.alfresco.i18n.I18NUtil;
+import org.alfresco.repo.admin.patch.AbstractPatch;
+import org.alfresco.repo.site.SiteInfo;
+import org.alfresco.repo.site.SiteModel;
+import org.alfresco.repo.site.SiteService;
+import org.alfresco.repo.site.SiteServiceImpl;
+import org.alfresco.service.cmr.security.AccessPermission;
+import org.alfresco.service.cmr.security.AuthorityService;
+import org.alfresco.service.cmr.security.AuthorityType;
+import org.alfresco.service.cmr.security.PermissionService;
+
+/**
+ * Patch's the site permission model to use groups to contain users.
+ * 
+ * @author Roy Wetherall
+ */
+public class SitePermissionRefactorPatch extends AbstractPatch
+{
+    /** Messages */
+    private static final String STATUS_MSG = "patch.sitePermissionRefactorPatch.result";
+    
+    /** Services */
+    private SiteService siteService;
+    private PermissionService permissionService;
+    private AuthorityService authorityService;
+    
+    /**
+     * Set site service
+     * 
+     * @param siteService   the site service
+     */
+    public void setSiteService(SiteService siteService)
+    {
+        this.siteService = siteService;
+    }
+    
+    /**
+     * Set the permission service
+     * 
+     * @param permissionService     the permission service
+     */
+    public void setPermissionService(PermissionService permissionService)
+    {
+        this.permissionService = permissionService;
+    }
+    
+    /**
+     * The authority service
+     * 
+     * @param authorityService  the authority service
+     */
+    public void setAuthorityService(AuthorityService authorityService)
+    {
+        this.authorityService = authorityService;
+    }
+    
+    /**
+     * @see org.alfresco.repo.admin.patch.AbstractPatch#applyInternal()
+     */
+    @Override
+    protected String applyInternal() throws Exception
+    {
+        // Set all the sites in the repository
+        List<SiteInfo> sites = this.siteService.listSites(null, null);
+        for (SiteInfo siteInfo : sites)
+        {
+            // Create the site's groups
+            String siteGroup = authorityService.createAuthority(
+                    AuthorityType.GROUP, 
+                    null, 
+                    ((SiteServiceImpl)this.siteService).getSiteGroupName(siteInfo.getShortName(), 
+                    false));
+            Set<String> permissions = permissionService.getSettablePermissions(SiteModel.TYPE_SITE);
+            for (String permission : permissions)
+            {
+                // Create a group for the permission
+                String permissionGroup = authorityService.createAuthority(
+                                            AuthorityType.GROUP, 
+                                            siteGroup, 
+                                            ((SiteServiceImpl)this.siteService).getSitePermissionGroupName(
+                                                    siteInfo.getShortName(), 
+                                                    permission, 
+                                                    false));
+                
+                // Assign the group the relevant permission on the site
+                permissionService.setPermission(siteInfo.getNodeRef(), permissionGroup, permission, true);
+            }
+            
+            // Take the current members and assign them to the appropriate groups
+            Set<AccessPermission> currentPermissions = this.permissionService.getAllSetPermissions(siteInfo.getNodeRef());
+            for (AccessPermission permission : currentPermissions)
+            {
+                // Only support user's being transfered (if public the everyone group will stay on the node)
+                if (permission.getAuthorityType() == AuthorityType.USER)
+                {
+                    // Add this authority to the appropriate group
+                    String group = ((SiteServiceImpl)this.siteService).getSitePermissionGroupName(
+                            siteInfo.getShortName(), 
+                            permission.getPermission(), 
+                            true);
+                   this.authorityService.addAuthority(group, permission.getAuthority()); 
+                   
+                   // Remove the permission from the node
+                   this.permissionService.deletePermission(siteInfo.getNodeRef(), permission.getAuthority(), permission.getPermission());
+                }
+            }
+        }
+     
+        // Report status
+        return I18NUtil.getMessage(STATUS_MSG);
+    }
+}

source/java/org/alfresco/repo/site/SiteServiceImpl.java

@@ -47,6 +47,7 @@ import org.alfresco.service.cmr.repository.StoreRef;
 import org.alfresco.service.cmr.search.ResultSet;
 import org.alfresco.service.cmr.search.SearchService;
 import org.alfresco.service.cmr.security.AccessPermission;
+import org.alfresco.service.cmr.security.AuthorityService;
 import org.alfresco.service.cmr.security.AuthorityType;
 import org.alfresco.service.cmr.security.PermissionService;
 import org.alfresco.service.cmr.security.PersonService;
@@ -85,6 +86,7 @@ public class SiteServiceImpl implements SiteService, SiteModel
     private PersonService personService;
     private AuthenticationComponent authenticationComponent;
     private TaggingService taggingService;
+    private AuthorityService authorityService;
     
     /** The site root node reference */
     private NodeRef siteRootNodeRef;
@@ -169,15 +171,31 @@ public class SiteServiceImpl implements SiteService, SiteModel
         this.taggingService = taggingService;
     }
     
+    /**
+     * Set the authority service
+     * 
+     * @param authorityService  authority service
+     */
+    public void setAuthorityService(AuthorityService authorityService)
+    {
+        this.authorityService = authorityService;
+    }
+    
     /**
      * @see org.alfresco.repo.site.SiteService#createSite(java.lang.String, java.lang.String, java.lang.String, java.lang.String, boolean)
      */
-    public SiteInfo createSite(String sitePreset, String shortName, String title, String description, boolean isPublic)
+    public SiteInfo createSite(final String sitePreset, String passedShortName, final String title, final String description, final boolean isPublic)
     {
-        /// TODO check for shortname duplicates
-        
         // Remove spaces from shortName
-        shortName = shortName.replaceAll(" ", "");
+        final String shortName = passedShortName.replaceAll(" ", "");
+        
+        // Check to see if we already have a site of this name
+        NodeRef existingSite = getSiteNodeRef(shortName);
+        if (existingSite != null)
+        {
+            // Throw an exception since we have a duplicate site name
+            throw new AlfrescoRuntimeException("Unable to create site because the site short name '" + shortName + "' is already in use.  Site short names must be unique.");
+        }
         
         // Get the site parent node reference
         NodeRef siteParent = getSiteParent(shortName);
@@ -188,7 +206,7 @@ public class SiteServiceImpl implements SiteService, SiteModel
         properties.put(SiteModel.PROP_SITE_PRESET, sitePreset);
         properties.put(ContentModel.PROP_TITLE, title);
         properties.put(ContentModel.PROP_DESCRIPTION, description);
-        NodeRef siteNodeRef = this.nodeService.createNode(
+        final NodeRef siteNodeRef = this.nodeService.createNode(
                 siteParent, 
                 ContentModel.ASSOC_CONTAINS, 
                 QName.createQName(NamespaceService.CONTENT_MODEL_1_0_URI, shortName), 
@@ -198,21 +216,80 @@ public class SiteServiceImpl implements SiteService, SiteModel
         // Make the new site a tag scope
         this.taggingService.addTagScope(siteNodeRef);
         
-       // Set the memberhips details
+        // Clear the sites inherited permissions
-       //    - give all authorities read permissions if site is public
+        this.permissionService.setInheritParentPermissions(siteNodeRef, false);        
-       //    - give all authorities read permission on permissions so memberships can be calculated
-       //    - give current user role of site manager
-       this.permissionService.setInheritParentPermissions(siteNodeRef, false);
-       if (isPublic == true)
-       {
-           this.permissionService.setPermission(siteNodeRef, PermissionService.ALL_AUTHORITIES, SITE_CONSUMER, true);
-       }
-       this.permissionService.setPermission(siteNodeRef, PermissionService.ALL_AUTHORITIES, PermissionService.READ_PERMISSIONS, true);
-       this.permissionService.setPermission(siteNodeRef, authenticationComponent.getCurrentUserName(), SiteModel.SITE_MANAGER, true);
         
-       // Return created site information
+        // Get the current user
-       SiteInfo siteInfo = new SiteInfo(sitePreset, shortName, title, description, isPublic, siteNodeRef);
+        final String currentUser = authenticationComponent.getCurrentUserName();
-       return siteInfo;
+        
+        // Create the relevant groups and assign permissions
+        AuthenticationUtil.runAs(
+            new AuthenticationUtil.RunAsWork<Object>()
+            {
+                public String doWork() throws Exception
+                {
+                    // Create the site's groups
+                    String siteGroup = authorityService.createAuthority(AuthorityType.GROUP, null, getSiteGroupName(shortName, false));
+                    Set<String> permissions = permissionService.getSettablePermissions(SiteModel.TYPE_SITE);
+                    for (String permission : permissions)
+                    {
+                        // Create a group for the permission
+                        String permissionGroup = authorityService.createAuthority(AuthorityType.GROUP, siteGroup, getSitePermissionGroupName(shortName, permission, false));
+                        
+                        // Assign the group the relevant permission on the site
+                        permissionService.setPermission(siteNodeRef, permissionGroup, permission, true);
+                    }
+                    
+                    // Set the memberhips details
+                    //    - give all authorities read permissions if site is public
+                    //    - give all authorities read permission on permissions so memberships can be calculated
+                    //    - add the current user to the site manager group
+                    if (isPublic == true)
+                    {
+                        permissionService.setPermission(siteNodeRef, PermissionService.ALL_AUTHORITIES, SITE_CONSUMER, true);
+                    }
+                    permissionService.setPermission(siteNodeRef, PermissionService.ALL_AUTHORITIES, PermissionService.READ_PERMISSIONS, true);
+                    authorityService.addAuthority(getSitePermissionGroupName(shortName, SiteModel.SITE_MANAGER, true), currentUser);                  
+                    
+                    // Return nothing
+                    return null;
+                }
+                    
+            }, AuthenticationUtil.getSystemUserName());               
+           
+        // Return created site information
+        SiteInfo siteInfo = new SiteInfo(sitePreset, shortName, title, description, isPublic, siteNodeRef);
+        return siteInfo;
+    }
+    
+    /**
+     * Helper method to get the name of the site group 
+     * 
+     * @param shortName     site short name
+     * @return String       site group name
+     */
+    public String getSiteGroupName(String shortName, boolean withGroupPrefix)
+    {
+        StringBuffer sb = new StringBuffer(64);
+        if (withGroupPrefix == true)
+        {
+            sb.append(PermissionService.GROUP_PREFIX);
+        }
+        sb.append("site_");
+        sb.append(shortName);
+        return sb.toString();
+    }
+    
+    /**
+     * Helper method to get the name of the site permission group
+     * 
+     * @param shortName     site short name
+     * @param permission    permission name
+     * @return String       site permission group name
+     */
+    public String getSitePermissionGroupName(String shortName, String permission, boolean withGroupPrefix)
+    {
+        return getSiteGroupName(shortName, withGroupPrefix) + "_" + permission;
     }
     
     /**
@@ -420,7 +497,7 @@ public class SiteServiceImpl implements SiteService, SiteModel
     /**
      * @see org.alfresco.repo.site.SiteService#deleteSite(java.lang.String)
      */
-    public void deleteSite(String shortName)
+    public void deleteSite(final String shortName)
     {
         NodeRef siteNodeRef = getSiteNodeRef(shortName);
         if (siteNodeRef == null)
@@ -428,7 +505,19 @@ public class SiteServiceImpl implements SiteService, SiteModel
             throw new AlfrescoRuntimeException("Can not delete site " + shortName + " because it does not exist.");
         }
         
-        this.nodeService.deleteNode(siteNodeRef);        
+        // Delete the node
+        this.nodeService.deleteNode(siteNodeRef);
+
+        // Delete the associatated group's
+        AuthenticationUtil.runAs(
+            new AuthenticationUtil.RunAsWork<Object>()
+            {
+                public Object doWork() throws Exception
+                {
+                    authorityService.deleteAuthority(getSiteGroupName(shortName, true));
+                    return null;
+                }
+            }, AuthenticationUtil.getSystemUserName());
     }
     
     /**
@@ -443,29 +532,18 @@ public class SiteServiceImpl implements SiteService, SiteModel
         }
         
         Map<String, String> members = new HashMap<String, String>(23);
-        Set<AccessPermission> permissions = this.permissionService.getAllSetPermissions(siteNodeRef);
+        
-        for (AccessPermission permission : permissions)
+        Set<String> permissions = permissionService.getSettablePermissions(SiteModel.TYPE_SITE);
+        for (String permission : permissions)
         {
-            String authority = permission.getAuthority();      
+            String groupName = getSitePermissionGroupName(shortName, permission, true);
-            if (permission.getAuthority().startsWith(PermissionService.GROUP_PREFIX) == true)
+            Set<String> users = this.authorityService.getContainedAuthorities(AuthorityType.USER, groupName, true);
+            for (String user : users)
             {
-                // TODO .. collapse groups into users
+                // Add the user and their permission to the returned map
-            }                
+                members.put(user, permission);
-            else
-            {
-                // Check to see if we already have an entry for the user in the map
-                if (members.containsKey(authority) == true)
-                {
-                    // TODO .. we need to resolve the permission in the map to the 'highest'
-                    //         for now do nothing as we shouldn't have more than on anyhow
-                }
-                else
-                {
-                    // Add the user and permission to the map
-                    members.put(authority, permission.getPermission());
-                }
             }
-        }
+        }        
         
         return members;
     }
@@ -475,8 +553,40 @@ public class SiteServiceImpl implements SiteService, SiteModel
      */
     public String getMembersRole(String shortName, String userName)
     {
-        Map<String, String> members = listMembers(shortName, null, null);
+        String result = null;
-        return members.get(userName);
+        String group = getPermissionGroup(shortName, userName);
+        if (group != null)
+        {
+            int index = group.lastIndexOf('_');
+            if (index != -1)
+            {
+                result = group.substring(index+1);
+            }
+        }
+        return result;
+    }
+    
+    /**
+     * Helper method to get the permission group for a given user on a site.
+     * Returns null if the user does not have a explicit membership to the site.
+     * 
+     * @param siteShortName     site short name
+     * @param userName          user name
+     * @return String           permission group, null if no explicit membership set
+     */
+    private String getPermissionGroup(String siteShortName, String userName)
+    {
+        String result = null;
+        Set<String> groups = this.authorityService.getContainingAuthorities(AuthorityType.GROUP, userName, true);
+        for (String group : groups)
+        {
+            if (group.startsWith(PermissionService.GROUP_PREFIX + "site_" + siteShortName) == true)
+            {
+                result = group;
+                break;
+            }
+        }
+        return result;
     }
 
     /**
@@ -484,7 +594,7 @@ public class SiteServiceImpl implements SiteService, SiteModel
      */
     public boolean isMember(String shortName, String userName)
     {
-        return (getMembersRole(shortName, userName) != null);
+        return (getPermissionGroup(shortName, userName) != null);
     }
     
     /**
@@ -499,57 +609,80 @@ public class SiteServiceImpl implements SiteService, SiteModel
         }
      
         // TODO what do we do about the user if they are in a group that has rights to the site?        
-        // TODO do not remove the only site manager
         
         // Determine whether the site is private or not
         boolean isPublic = isSitePublic(siteNodeRef);
         
         // Get the current user
         String currentUserName = AuthenticationUtil.getCurrentUserName();
+        String currentUserRole = getMembersRole(shortName, currentUserName);
         
         // Get the user current role 
-        String role = getMembersRole(shortName, userName);
+        final String role = getMembersRole(shortName, userName);        
-        
+        if (role != null)
-        // If ...
+        {       
-        //  -- the site is public and
+            // Check that we are not about to remove the last site manager
-        //  -- the user is ourselves and
+            if (SiteModel.SITE_MANAGER.equals(role) == true)
-        //  -- the users current role is consumer
-        if (isPublic == true &&
-            currentUserName.equals(userName) == true &&
-            role != null &&
-            role.equals(SiteModel.SITE_CONSUMER) == true)
-        {
-            // Run as system user
-            AuthenticationUtil.runAs(new AuthenticationUtil.RunAsWork<Object>()
             {
-                public Object doWork() throws Exception
+                Set<String> siteMangers = this.authorityService.getContainedAuthorities(
+                        AuthorityType.USER, 
+                        getSitePermissionGroupName(shortName, SITE_MANAGER, true), 
+                        true);
+                if (siteMangers.size() == 1)
                 {
-                    permissionService.clearPermission(siteNodeRef, userName);
+                    throw new AlfrescoRuntimeException("A site requires at least one site manager.  You can not remove '" + userName + "' from the site memebership because they are currently the only site manager.");
-                    return null;
                 }
-            }, AuthenticationUtil.SYSTEM_USER_NAME);
+            }
-        }
+            
-        else
+            // If ...
-        {
+            //  -- the current user is a site manager
-            // Clear the permissions for the user 
+            // or
-            this.permissionService.clearPermission(siteNodeRef, userName);
+            //  -- the site is public and
-        }
+            //  -- the user is ourselves and
-
+            //  -- the users current role is consumer
-        if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
+            if ((currentUserRole != null &&
-        {
+                 SiteModel.SITE_MANAGER.equals(currentUserRole) == true)
-            activityService.postActivity(ActivityType.SITE_USER_REMOVED, shortName, ACTIVITY_TOOL, getActivityData(userName, ""));
+                || 
-        }
+                (isPublic == true &&
-        else
+                 currentUserName.equals(userName) == true &&
-        {
+                 role.equals(SiteModel.SITE_CONSUMER) == true))
-            // TODO - update this, if sites support groups
+            {
-            logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
+                // Run as system user
-        }
+                AuthenticationUtil.runAs(new AuthenticationUtil.RunAsWork<Object>()
+                {
+                    public Object doWork() throws Exception
+                    {
+                        // Remove the user from the current permission group
+                        String currentGroup = getSitePermissionGroupName(shortName, role, true);
+                        authorityService.removeAuthority(currentGroup, userName);
+                        
+                        return null;
+                    }
+                }, AuthenticationUtil.SYSTEM_USER_NAME);
+                
+                // Raise events
+                if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
+                {
+                    activityService.postActivity(ActivityType.SITE_USER_REMOVED, shortName, ACTIVITY_TOOL, getActivityData(userName, ""));
+                }
+                else
+                {
+                    // TODO - update this, if sites support groups
+                    logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
+                }
+            }
+            else
+            {
+                // Throw a permission exception
+                throw new AlfrescoRuntimeException("Access denied, user does not have permissions to delete membership details of the site '" + shortName + "'");
+            }
+        }        
     }
 
     /**
      * @see org.alfresco.repo.site.SiteService#setMembership(java.lang.String, java.lang.String, java.lang.String)
      */
-    public void setMembership(String shortName, final String userName, final String role)
+    public void setMembership(final String shortName, final String userName, final String role)
     {
         final NodeRef siteNodeRef = getSiteNodeRef(shortName);
         if (siteNodeRef == null)
@@ -557,87 +690,100 @@ public class SiteServiceImpl implements SiteService, SiteModel
             throw new AlfrescoRuntimeException("Site " + shortName + " does not exist.");
         }
         
-        // Determine whether the site is private or not
+        // Get the user's current role
-        boolean isPublic = isSitePublic(siteNodeRef);
+        final String currentRole = getMembersRole(shortName, userName);
         
-        // Determine whether the user is already a member of the site
+        if (currentRole == null || role.equals(currentRole) == false)
-        boolean alreadyMember = false;
-        Set<AccessPermission> permissions = this.permissionService.getAllSetPermissions(siteNodeRef);
-        for (AccessPermission permission : permissions)
-        {
-            String authority = permission.getAuthority();
-            if (authority.equals(userName))
-            {
-                alreadyMember = true;
-                break;
-            }
-        }
-        
-        // TODO if this is the only site manager do not downgrade their permissions
-        
-        // If we are:
-        // -- refering to a public site and
-        // -- the role being set is consumer and
-        // -- the user being added is ourselves and
-        // -- the member does not already have permissions 
-        // ... then we can set the permissions as system user
-        String currentUserName = AuthenticationUtil.getCurrentUserName();
-        if (isPublic == true &&
-            role.equals(SiteModel.SITE_CONSUMER) == true &&
-            userName.equals(currentUserName) == true &&
-            alreadyMember == false)
-        {
-            // Run as system user
-            AuthenticationUtil.runAs(new AuthenticationUtil.RunAsWork<Object>()
-            {
-                public Object doWork() throws Exception
-                {
-                    // Clear any existing permissions
-                    permissionService.clearPermission(siteNodeRef, userName);
-                    
-                    // Set the permissions
-                    permissionService.setPermission(siteNodeRef, userName, role, true);
-                    
-                    return null;
-                }
-                    
-            }, AuthenticationUtil.SYSTEM_USER_NAME);
-            
-        }
-        else
         {        
-            // Clear any existing permissions
+            // Determine whether the site is private or not
-            this.permissionService.clearPermission(siteNodeRef, userName);
+            boolean isPublic = isSitePublic(siteNodeRef);
             
-            // Set the permissions
+            // TODO if this is the only site manager do not downgrade their permissions
-            this.permissionService.setPermission(siteNodeRef, userName, role, true);
+            
-        }
+            // If we are ...
-        
+            // -- the site manager
-        if (! alreadyMember)
+            // or we are ...
-        {
+            // -- refering to a public site and
-            if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
+            // -- the role being set is consumer and
+            // -- the user being added is ourselves and
+            // -- the member does not already have permissions 
+            // ... then we can set the permissions as system user
+            final String currentUserName = AuthenticationUtil.getCurrentUserName();
+            final String currentUserRole = getMembersRole(shortName, currentUserName);
+            if  ((currentUserRole != null && 
+                  SiteModel.SITE_MANAGER.equals(currentUserRole) == true) 
+                  ||
+                 (isPublic == true &&
+                  role.equals(SiteModel.SITE_CONSUMER) == true &&
+                  userName.equals(currentUserName) == true &&
+                  currentRole == null))
             {
-                activityService.postActivity(ActivityType.SITE_USER_JOINED, shortName, ACTIVITY_TOOL, getActivityData(userName, role));
+                // Check that we are not about to remove the last site manager
+                if (SiteModel.SITE_MANAGER.equals(currentRole) == true)
+                {
+                    Set<String> siteMangers = this.authorityService.getContainedAuthorities(
+                            AuthorityType.USER, 
+                            getSitePermissionGroupName(shortName, SITE_MANAGER, true), 
+                            true);
+                    if (siteMangers.size() == 1)
+                    {
+                        throw new AlfrescoRuntimeException("A site requires at least one site manager.  You can not change '" + userName + "' role from the site memebership because they are currently the only site manager.");
+                    }
+                }
+                
+                // Run as system user
+                AuthenticationUtil.runAs(new AuthenticationUtil.RunAsWork<Object>()
+                {
+                    public Object doWork() throws Exception
+                    {
+                        if (currentRole != null)
+                        {
+                            // Remove the user from the current permission group
+                            String currentGroup = getSitePermissionGroupName(shortName, currentRole, true);
+                            authorityService.removeAuthority(currentGroup, userName);
+                        }
+                        
+                        // Add the user to the new permission group
+                        String newGroup = getSitePermissionGroupName(shortName, role, true);
+                        authorityService.addAuthority(newGroup, userName);
+                        
+                        return null;
+                    }
+                        
+                }, AuthenticationUtil.SYSTEM_USER_NAME);
+                
+                if (currentRole == null)
+                {
+                    if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
+                    {
+                        activityService.postActivity(ActivityType.SITE_USER_JOINED, shortName, ACTIVITY_TOOL, getActivityData(userName, role));
+                    }
+                    else
+                    {
+                        // TODO - update this, if sites support groups
+                        logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
+                    }
+                }
+                else
+                {
+                    if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
+                    {
+                        activityService.postActivity(ActivityType.SITE_USER_ROLE_UPDATE, shortName, ACTIVITY_TOOL, getActivityData(userName, role));
+                    }
+                    else
+                    {
+                        // TODO - update this, if sites support groups
+                        logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
+                    }
+                }                
             }
             else
-            {
+            {        
-                // TODO - update this, if sites support groups
+                // Raise a permission exception
-                logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
+                throw new AlfrescoRuntimeException("Access denied, user does not have permissions to modify membership details of the site '" + shortName + "'");
             }
         }
-        else
+    }    
-        {
-            if (AuthorityType.getAuthorityType(userName) == AuthorityType.USER)
-            {
-                activityService.postActivity(ActivityType.SITE_USER_ROLE_UPDATE, shortName, ACTIVITY_TOOL, getActivityData(userName, role));
-            }
-            else
-            {
-                // TODO - update this, if sites support groups
-                logger.error("setMembership - failed to post activity: unexpected authority type: " + AuthorityType.getAuthorityType(userName));
-            }
-        }
-    }
 
     /**
      * @see org.alfresco.repo.site.SiteService#createContainer(java.lang.String, java.lang.String, org.alfresco.service.namespace.QName, java.util.Map)

source/java/org/alfresco/repo/site/SiteServiceImplTest.java

@@ -37,9 +37,10 @@ import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
 import org.alfresco.service.cmr.repository.ScriptLocation;
 import org.alfresco.service.cmr.repository.ScriptService;
+import org.alfresco.service.cmr.security.PersonService;
 import org.alfresco.service.cmr.tagging.TaggingService;
 import org.alfresco.util.BaseAlfrescoSpringTest;
-import org.alfresco.util.TestWithUserUtils;
+import org.alfresco.util.PropertyMap;
 
 /**
  * Thumbnail service implementation unit test
@@ -53,15 +54,16 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
     private static final String TEST_TITLE = "This is my title";
     private static final String TEST_DESCRIPTION = "This is my description";
     
-    private static final String USER_ONE = "UserOne";
+    private static final String USER_ONE = "UserOne_SiteServiceImplTest";
-    private static final String USER_TWO = "UserTwo";
+    private static final String USER_TWO = "UserTwo_SiteServiceImplTest";
-    private static final String USER_THREE = "UserThree";
+    private static final String USER_THREE = "UserThree_SiteServiceImplTest";
     
     private SiteService siteService;    
     private ScriptService scriptService;
     private NodeService nodeService;
     private AuthenticationComponent authenticationComponent;
     private TaggingService taggingService;
+    private PersonService personService;
     
     /**
      * Called during the transaction setup
@@ -71,14 +73,35 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         super.onSetUpInTransaction();
         
         // Get the required services
-        this.siteService = (SiteService)this.applicationContext.getBean("siteService");
+        this.siteService = (SiteService)this.applicationContext.getBean("SiteService");
         this.scriptService = (ScriptService)this.applicationContext.getBean("ScriptService");
         this.nodeService = (NodeService)this.applicationContext.getBean("NodeService");
         this.authenticationComponent = (AuthenticationComponent)this.applicationContext.getBean("authenticationComponent");
         this.taggingService = (TaggingService)this.applicationContext.getBean("TaggingService");
+        this.personService = (PersonService)this.applicationContext.getBean("PersonService");
         
         // Do the test's as userOne
-        TestWithUserUtils.authenticateUser(USER_ONE, "PWD", this.authenticationService, this.authenticationComponent);
+        createUser(USER_ONE);
+        createUser(USER_TWO);
+        createUser(USER_THREE);
+        this.authenticationComponent.setCurrentUser(USER_ONE);
+    }
+    
+    private void createUser(String userName)
+    {
+        if (this.authenticationService.authenticationExists(userName) == false)
+        {
+            this.authenticationService.createAuthentication(userName, "PWD".toCharArray());
+            
+            PropertyMap ppOne = new PropertyMap(4);
+            ppOne.put(ContentModel.PROP_USERNAME, userName);
+            ppOne.put(ContentModel.PROP_FIRSTNAME, "firstName");
+            ppOne.put(ContentModel.PROP_LASTNAME, "lastName");
+            ppOne.put(ContentModel.PROP_EMAIL, "email@email.com");
+            ppOne.put(ContentModel.PROP_JOBTITLE, "jobTitle");
+            
+            this.personService.createPerson(ppOne);
+        }        
     }
 	
     public void testCreateSite() throws Exception
@@ -87,7 +110,7 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         SiteInfo siteInfo = this.siteService.createSite(TEST_SITE_PRESET, "mySiteTest", TEST_TITLE, TEST_DESCRIPTION, true);
         checkSiteInfo(siteInfo, TEST_SITE_PRESET, "mySiteTest", TEST_TITLE, TEST_DESCRIPTION, true);     
         
-        String name = "!\"£$%^&*()_+=-[]{}";
+        String name = "!£$%^&*()_+=-[]{}";
         siteInfo = this.siteService.createSite(TEST_SITE_PRESET, name, TEST_TITLE, TEST_DESCRIPTION, true);
         checkSiteInfo(siteInfo, TEST_SITE_PRESET, name, TEST_TITLE, TEST_DESCRIPTION, true); 
         siteInfo = this.siteService.getSite(name);
@@ -98,6 +121,17 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         checkSiteInfo(siteInfo, TEST_SITE_PRESET, name, TEST_TITLE, TEST_DESCRIPTION, true); 
         siteInfo = this.siteService.getSite(name);
         checkSiteInfo(siteInfo, TEST_SITE_PRESET, name, TEST_TITLE, TEST_DESCRIPTION, true); 
+        
+        // Test for duplicate site error
+        try
+        {
+            this.siteService.createSite(TEST_SITE_PRESET, "mySiteTest", TEST_TITLE, TEST_DESCRIPTION, true);
+            fail("Shouldn't allow duplicate site short names.");
+        }
+        catch (AlfrescoRuntimeException exception)
+        {
+            // Expected
+        }
     }
     
     private void checkSiteInfo( SiteInfo siteInfo, String expectedSitePreset, String expectedShortName, String expectedTitle, 
@@ -257,18 +291,19 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         assertEquals(2, sites.size());
         
         // Now get the sites as user two
-        TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_TWO);
         sites = this.siteService.listSites(null, null);
         assertNotNull(sites);
         assertEquals(1, sites.size());
         checkSiteInfo(sites.get(0), TEST_SITE_PRESET, "isPublicTrue", TEST_TITLE, TEST_DESCRIPTION, true);
         
         // Make user 2 a member of the site
-        TestWithUserUtils.authenticateUser(USER_ONE, "PWD", this.authenticationService, this.authenticationComponent);
+        //TestWithUserUtils.authenticateUser(USER_ONE, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_ONE);
         this.siteService.setMembership("isPublicFalse", USER_TWO, SiteModel.SITE_CONSUMER);
         
         // Now get the sites as user two
-        TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_TWO);
         sites = this.siteService.listSites(null, null);
         assertNotNull(sites);
         assertEquals(2, sites.size());
@@ -328,7 +363,7 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         assertEquals(SiteModel.SITE_COLLABORATOR, members.get(USER_THREE));
         
         // Check that a non-manager and non-member cannot edit the memberships
-        TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_TWO);
         try
         {
             this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_COLLABORATOR);
@@ -347,7 +382,7 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         {
             // As expected            
         }
-        TestWithUserUtils.authenticateUser(USER_THREE, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_THREE);
         try
         {
             this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_COLLABORATOR);
@@ -367,9 +402,34 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
             // As expected            
         }
         
-        // TODO .. try and change the permissions of the only site manager
+        this.authenticationComponent.setCurrentUser(USER_ONE);        
+        // Try and change the permissions of the only site manager
+        this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_MANAGER);
+        this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_COLLABORATOR);
+        try
+        {
+            this.siteService.setMembership("testMembership", USER_ONE, SiteModel.SITE_COLLABORATOR);
+            fail("You can not change the role of the last site memnager");
+        }
+        catch (AlfrescoRuntimeException exception)
+        {
+            // Expected
+            //exception.printStackTrace();
+        }
         
-        // TODO .. try and remove the only site manager and should get a failure
+        // Try and remove the only site manager and should get a failure
+        this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_MANAGER);
+        this.siteService.removeMembership("testMembership", USER_ONE);
+        try
+        {
+            this.siteService.removeMembership("testMembership", USER_TWO);
+            fail("You can not remove the last site memnager from a site");
+        }
+        catch (AlfrescoRuntimeException exception)
+        {
+            // Expected
+            //exception.printStackTrace();
+        }
     }
     
     public void testJoinLeave()
@@ -379,7 +439,8 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         this.siteService.createSite(TEST_SITE_PRESET, "testMembershipPrivate", TEST_TITLE, TEST_DESCRIPTION, false);
         
         // Become user two
-        TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        //TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_TWO);
         
         // As user two try and add self as contributor
         try
@@ -398,7 +459,7 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         // As user two try and add self as consumer to private site
         try
         {
-            this.siteService.setMembership("testMembership", USER_TWO, SiteModel.SITE_CONSUMER);
+            this.siteService.setMembership("testMembershipPrivate", USER_TWO, SiteModel.SITE_CONSUMER);
             fail("This should have failed because you can't do this to a private site unless you are site manager");
         }
         catch (AlfrescoRuntimeException exception)
@@ -419,10 +480,10 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         
         
         // add some members use in remove tests
-        TestWithUserUtils.authenticateUser(USER_ONE, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_ONE);
         this.siteService.setMembership("testMembership", USER_THREE, SiteModel.SITE_COLLABORATOR);
         this.siteService.setMembership("testMembershipPrivate", USER_TWO, SiteModel.SITE_CONSUMER);
-        TestWithUserUtils.authenticateUser(USER_TWO, "PWD", this.authenticationService, this.authenticationComponent);
+        this.authenticationComponent.setCurrentUser(USER_TWO);
         
         // try and remove user two permissions from private site
         try
@@ -505,7 +566,6 @@ public class SiteServiceImplTest extends BaseAlfrescoSpringTest
         assertEquals(ForumModel.TYPE_FORUM, nodeService.getType(container8));
     }
     
-    
     // == Test the JavaScript API ==
     
     public void testJSAPI() throws Exception

source/java/org/alfresco/repo/site/script/Site.java

@@ -215,10 +215,10 @@ public class Site implements Serializable
      */
     public ScriptableHashMap<String, String> listMembers(String nameFilter, String roleFilter)
     {
-        Map<String, String> sites =  this.siteService.listMembers(getShortName(), nameFilter, roleFilter);
+        Map<String, String> members =  this.siteService.listMembers(getShortName(), nameFilter, roleFilter);
         
         ScriptableHashMap<String, String> result = new ScriptableHashMap<String, String>();
-        result.putAll(sites);
+        result.putAll(members);
         
         return result;
     }

source/java/org/alfresco/repo/site/script/test_siteService.js

@@ -13,29 +13,29 @@ function checkSite(site, sitePreset, shortName, title, description, isPublic)
 function testCRUD()
 {
 	// Try and get a site that doesn't exist
-	var site = siteService.getSite("siteShortName");
+	var site = siteService.getSite("siteShortNameCRUD");
 	test.assertNull(site, "Site should not have been found.");
 	
 	// Try and create a site
-	site = siteService.createSite("sitePreset", "siteShortName", "siteTitle", "siteDescription", true);
+	site = siteService.createSite("sitePreset", "siteShortNameCRUD", "siteTitle", "siteDescription", true);
-	checkSite(site, "sitePreset", "siteShortName", "siteTitle", "siteDescription", true);
+	checkSite(site, "sitePreset", "siteShortNameCRUD", "siteTitle", "siteDescription", true);
 	
 	// Try and get the created site
-	site = siteService.getSite("siteShortName");
+	site = siteService.getSite("siteShortNameCRUD");
-	checkSite(site, "sitePreset", "siteShortName", "siteTitle", "siteDescription", true);
+	checkSite(site, "sitePreset", "siteShortNameCRUD", "siteTitle", "siteDescription", true);
 	
 	// Try and update the values of the site
 	site.title = "abc123abc";
 	site.description = "abc123abc";
 	site.isPublic = false;
-	checkSite(site, "sitePreset", "siteShortName", "abc123abc", "abc123abc", false);
+	checkSite(site, "sitePreset", "siteShortNameCRUD", "abc123abc", "abc123abc", false);
 	site.save();
-	site = siteService.getSite("siteShortName");
+	site = siteService.getSite("siteShortNameCRUD");
-	checkSite(site, "sitePreset", "siteShortName", "abc123abc", "abc123abc", false);
+	checkSite(site, "sitePreset", "siteShortNameCRUD", "abc123abc", "abc123abc", false);
 	
 	// Delete the site
 	site.deleteSite();
-	site = siteService.getSite("siteShortName");
+	site = siteService.getSite("siteShortNameCRUD");
 	test.assertNull(site, "");
 }
 
@@ -63,20 +63,20 @@ function testMembership()
 	var members = site.listMembers(null, null);
 	test.assertNotNull(members);
 	test.assertEquals(1, members.length);
-	test.assertEquals("SiteManager", members["UserOne"]);
+	test.assertEquals("SiteManager", members["UserOne_SiteServiceImplTest"]);
 	
-	site.setMembership("UserTwo", "SiteCollaborator");
+	site.setMembership("UserTwo_SiteServiceImplTest", "SiteCollaborator");
 	members = site.listMembers(null, null);
 	test.assertNotNull(members);
 	test.assertEquals(2, members.length);
-	test.assertEquals("SiteManager", members["UserOne"]);
+	test.assertEquals("SiteManager", members["UserOne_SiteServiceImplTest"]);
-	test.assertEquals("SiteCollaborator", members["UserTwo"]);
+	test.assertEquals("SiteCollaborator", members["UserTwo_SiteServiceImplTest"]);
 	
-	site.removeMembership("UserTwo");
+	site.removeMembership("UserTwo_SiteServiceImplTest");
 	members = site.listMembers(null, null);
 	test.assertNotNull(members);
 	test.assertEquals(1, members.length);
-	test.assertEquals("SiteManager", members["UserOne"]);
+	test.assertEquals("SiteManager", members["UserOne_SiteServiceImplTest"]);
 	
 }
 

source/java/org/alfresco/repo/tagging/UpdateTagScopesActionExecuter.java

@@ -29,7 +29,6 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
-import org.alfresco.error.AlfrescoRuntimeException;
 import org.alfresco.model.ContentModel;
 import org.alfresco.repo.action.ParameterDefinitionImpl;
 import org.alfresco.repo.action.executer.ActionExecuterAbstractBase;