Few more improvements for nullability

Expect the domain object to be non-null in SecurityService.

Make UserDetailManager.getCurrentUsername() non-null

Fix getGroupsUserCanEdit() to pass the supplied username to
getGroupsUserCanView(), otherwise the groups are based on the current
user.

Author: arteymix

gsec/src/main/java/gemma/gsec/SecurityServiceImpl.java

@@ -261,7 +261,7 @@ public List<String> getGroupAuthoritiesNameFromGroupName( String groupName ) {
     @Override
     @Transactional(readOnly = true)
     public <T extends Securable> Map<T, Collection<String>> getGroupsEditableBy( Collection<T> securables ) {
-        Collection<String> groupNames = getGroupsUserCanView();
+        Collection<String> groupNames = getGroupsUserCanView( userDetailsManager.getCurrentUsername() );
 
         Map<T, Collection<String>> result = new HashMap<>( securables.size() );
 
@@ -278,7 +278,7 @@ public <T extends Securable> Map<T, Collection<String>> getGroupsEditableBy( Col
     @Override
     @Transactional(readOnly = true)
     public Collection<String> getGroupsEditableBy( Securable s ) {
-        Collection<String> groupNames = getGroupsUserCanView();
+        Collection<String> groupNames = getGroupsUserCanView( userDetailsManager.getCurrentUsername() );
 
         Collection<String> result = new HashSet<>();
 
@@ -299,7 +299,7 @@ public <T extends Securable> Map<T, Collection<String>> getGroupsReadableBy( Col
 
         if ( securables.isEmpty() ) return result;
 
-        Collection<String> groupNames = getGroupsUserCanView();
+        Collection<String> groupNames = getGroupsUserCanView( userDetailsManager.getCurrentUsername() );
 
         for ( String groupName : groupNames ) {
             populateGroupPermissions( result, groupName,
@@ -314,7 +314,7 @@ public <T extends Securable> Map<T, Collection<String>> getGroupsReadableBy( Col
     @Override
     @Transactional(readOnly = true)
     public Collection<String> getGroupsReadableBy( Securable s ) {
-        Collection<String> groupNames = getGroupsUserCanView();
+        Collection<String> groupNames = getGroupsUserCanView( userDetailsManager.getCurrentUsername() );
 
         Collection<String> result = new HashSet<>();
 
@@ -330,7 +330,7 @@ public Collection<String> getGroupsReadableBy( Securable s ) {
     @Override
     @Transactional(readOnly = true)
     public Collection<String> getGroupsUserCanEdit( String userName ) {
-        Collection<String> groupNames = getGroupsUserCanView();
+        Collection<String> groupNames = getGroupsUserCanView( userName );
 
         Collection<String> result = new HashSet<>();
         for ( String gname : groupNames ) {
@@ -497,11 +497,6 @@ public boolean isPublic( Securable s ) {
     @Override
     @Transactional(readOnly = true)
     public boolean isPrivate( Securable s ) {
-        if ( s == null ) {
-            log.warn( "Null object: considered public!" );
-            return false;
-        }
-
         /*
          * Note: in theory, it should pay attention to the sid we ask for and return nothing if there is no acl.
          * However, the implementation actually ignores the sid argument.
@@ -517,10 +512,6 @@ public boolean isPrivate( Securable s ) {
     @Override
     @Transactional(readOnly = true)
     public boolean isShared( Securable s ) {
-        if ( s == null ) {
-            return false;
-        }
-
         /*
          * Implementation note: this code mimics AclEntryVoter.vote, but in adminsitrative mode so no auditing etc
          * happens.
@@ -576,10 +567,6 @@ public void makePrivate( Collection<? extends Securable> objs ) {
     @Override
     @Transactional
     public void makePrivate( Securable object ) {
-        if ( object == null ) {
-            return;
-        }
-
         if ( isPrivate( object ) ) {
             log.warn( "Object is already private: " + object );
             return;
@@ -608,13 +595,8 @@ public void makePublic( Collection<? extends Securable> objs ) {
     @Override
     @Transactional
     public void makePublic( Securable object ) {
-
-        if ( object == null ) {
-            return;
-        }
-
         if ( isPublic( object ) ) {
-            log.warn( "Object is already public" );
+            log.warn( "Object is already public: " + object );
             return;
         }
 
@@ -639,9 +621,8 @@ public void makePublic( Securable object ) {
     @Override
     @Transactional
     public void makeReadableByGroup( Securable s, String groupName ) throws AccessDeniedException {
-
         if ( StringUtils.isEmpty( groupName.trim() ) ) {
-            throw new IllegalArgumentException( "'group' cannot be null" );
+            throw new IllegalArgumentException( "The group name cannot be empty." );
         }
 
         Collection<String> groups = checkForGroupAccessByCurrentUser( groupName );
@@ -661,7 +642,6 @@ public void makeReadableByGroup( Securable s, String groupName ) throws AccessDe
     @Override
     @Transactional
     public void makeUnreadableByGroup( Securable s, String groupName ) throws AccessDeniedException {
-
         if ( StringUtils.isEmpty( groupName.trim() ) ) {
             throw new IllegalArgumentException( "'group' cannot be null" );
         }
@@ -675,7 +655,6 @@ public void makeUnreadableByGroup( Securable s, String groupName ) throws Access
     @Override
     @Transactional
     public void makeUneditableByGroup( Securable s, String groupName ) throws AccessDeniedException {
-
         if ( StringUtils.isEmpty( groupName.trim() ) ) {
             throw new IllegalArgumentException( "'group' cannot be null" );
         }
@@ -688,7 +667,6 @@ public void makeUneditableByGroup( Securable s, String groupName ) throws Access
     @Override
     @Transactional
     public void makeEditableByGroup( Securable s, String groupName ) throws AccessDeniedException {
-
         if ( StringUtils.isEmpty( groupName.trim() ) ) {
             throw new IllegalArgumentException( "'group' cannot be null" );
         }
@@ -819,15 +797,15 @@ private Collection<String> checkForGroupAccessByCurrentUser( String groupName )
     /**
      * @return groups that the current user can view. For administrators, this is all groups.
      */
-    private Collection<String> getGroupsUserCanView() {
+    private Collection<String> getGroupsUserCanView( String userName ) {
         Collection<String> groupNames;
         try {
             // administrator...
             groupNames = groupManager.findAllGroups();
         } catch ( AccessDeniedException e ) {
             // I'm not sure this actually happens. Usermanager.findAllGroups should just show all of the user's viewable
             // groups.
-            groupNames = groupManager.findGroupsForUser( userDetailsManager.getCurrentUsername() );
+            groupNames = groupManager.findGroupsForUser( userName );
         }
         return groupNames;
     }

gsec/src/main/java/gemma/gsec/acl/ObjectIdentityRetrievalStrategyImpl.java

@@ -37,7 +37,8 @@ public class ObjectIdentityRetrievalStrategyImpl implements ObjectIdentityRetrie
 
     @Override
     public ObjectIdentity getObjectIdentity( Object domainObject ) {
-        Assert.isInstanceOf( Securable.class, domainObject, "The domain object must implement the Securable interface" );
+        Assert.notNull( domainObject, "The domain object must be non-null." );
+        Assert.isInstanceOf( Securable.class, domainObject, "The domain object must implement the " + Securable.class.getName() + " interface." );
         if ( domainObject instanceof SecureValueObject ) {
             SecureValueObject svo = ( SecureValueObject ) domainObject;
             return new AclObjectIdentity( svo.getSecurableClass(), svo.getId() );

gsec/src/main/java/gemma/gsec/authentication/UserDetailsManager.java

@@ -2,7 +2,6 @@
 
 import org.springframework.security.core.userdetails.UsernameNotFoundException;
 
-import javax.annotation.Nullable;
 import java.util.Collection;
 
 /**
@@ -44,8 +43,9 @@ public interface UserDetailsManager extends org.springframework.security.provisi
 
     /**
      * Returns a String username (the principal).
+     *
+     * @throws IllegalStateException if no user is currently authenticated.
      */
-    @Nullable
     String getCurrentUsername();
 
     boolean loggedIn();