If a library is not public, ensure that the roles displayed on the upload form or any permission form for a contained folder or dataset are derived from the set of users that have access to the library.  Add error checks to the upload tool to display appropriate error messages if ill-legitimate roles were selected ( shouldn't happen since they are filtered out ) or if roles are selected such that the dataset would be in-accessible to everyone.

lib/galaxy/model/__init__.py

@@ -5,7 +5,7 @@ Naming: try to use class names that have a distinct plural form so that
 the relationship cardinalities are obvious (e.g. prefer Dataset to Data)
 """
 
-import os.path, os, errno, sys, codecs
+import os.path, os, errno, sys, codecs, operator
 import galaxy.datatypes
 from galaxy.util.bunch import Bunch
 from galaxy import util
@@ -435,6 +435,12 @@ class Dataset( object ):
             os.remove(self.data.file_name)
         except OSError, e:
             log.critical('%s delete error %s' % (self.__class__.__name__, e))
+    def get_access_roles( self, trans ):
+        roles = []
+        for dp in self.actions:
+            if dp.action == trans.app.security_agent.permitted_actions.DATASET_ACCESS.action:
+                roles.append( dp.role )
+        return roles
 
 class DatasetInstance( object ):
     """A base class for all 'dataset instances', HDAs, LDAs, etc"""
@@ -747,6 +753,49 @@ class Library( object ):
                     return template.get_widgets( trans.user, contents=info.content )
             return template.get_widgets( trans.user )
         return []
+    def get_access_roles( self, trans ):
+        roles = []
+        for lp in self.actions:
+            if lp.action == trans.app.security_agent.permitted_actions.LIBRARY_ACCESS.action:
+                roles.append( lp.role )
+        return roles
+    def get_legitimate_roles( self, trans ):
+        if trans.app.security_agent.library_is_public( self ):
+            return trans.sa_session.query( trans.app.model.Role ) \
+                                   .filter( trans.app.model.Role.table.c.deleted==False ) \
+                                   .order_by( trans.app.model.Role.table.c.name )
+        def sort_by_attr( seq, attr ):
+            """
+            Sort the sequence of objects by object's attribute
+            Arguments:
+            seq  - the list or any sequence (including immutable one) of objects to sort.
+            attr - the name of attribute to sort by
+            """
+            # Use the "Schwartzian transform"
+            # Create the auxiliary list of tuples where every i-th tuple has form
+            # (seq[i].attr, i, seq[i]) and sort it. The second item of tuple is needed not
+            # only to provide stable sorting, but mainly to eliminate comparison of objects
+            # (which can be expensive or prohibited) in case of equal attribute values.
+            intermed = map( None, map( getattr, seq, ( attr, ) * len( seq ) ), xrange( len( seq ) ), seq )
+            intermed.sort()
+            return map( operator.getitem, intermed, ( -1, ) * len( intermed ) )            
+        roles = set()
+        # If a library has roles associated with the LIBRARY_ACCESS permission, we need to start with them.
+        access_roles = self.get_access_roles( trans )
+        for role in access_roles:
+            roles.add( role )
+            # Each role potentially has users.  We need to find all roles that each of those users have.
+            for ura in role.users:
+                roles.add( ura.role )
+            # Each role also potentially has groups which, in turn, have members ( users ).  We need to 
+            # find all roles that each group's members have.
+            for gra in role.groups:
+                group = gra.group
+                for uga in group.users:
+                    user = uga.user
+                    for ura in user.roles:
+                        roles.add( ura.role )
+        return sort_by_attr( [ role for role in roles ], 'name' )
     def get_display_name( self ):
         # Library name can be either a string or a unicode object. If string, 
         # convert to unicode object assuming 'utf-8' format.
@@ -937,6 +986,8 @@ class LibraryDatasetDatasetAssociation( DatasetInstance ):
         return ldda
     def clear_associated_files( self, metadata_safe = False, purge = False ):
         return
+    def get_access_roles( self, trans ):
+        return self.dataset.get_access_roles( trans )
     def get_info_association( self, restrict=False, inherited=False ):
         # If restrict is True, we will return this ldda's info_association whether it
         # exists or not.  If restrict is False, we'll return the next available info_association

lib/galaxy/security/__init__.py

@@ -5,6 +5,7 @@ Galaxy Security
 import logging, socket
 from datetime import datetime, timedelta
 from galaxy.util.bunch import Bunch
+from galaxy.util import listify
 from galaxy.model.orm import *
 
 log = logging.getLogger(__name__)
@@ -17,6 +18,8 @@ class Action( object ):
 
 class RBACAgent:
     """Class that handles galaxy security"""
+    IN_ACCESSIBLE = 'access_error'
+    ILL_LEGITIMATE = 'legitimate_error'
     permitted_actions = Bunch(
         DATASET_MANAGE_PERMISSIONS = Action( "manage permissions", "Role members can manage the roles associated with this dataset", "grant" ),
         DATASET_ACCESS = Action( "access", "Role members can import this dataset into their history for analysis", "restrict" ),
@@ -74,6 +77,10 @@ class RBACAgent:
         raise "Unimplemented Method"
     def make_library_public( self, library ):
         raise "Unimplemented Method"
+    def get_library_dataset_permissions( self, library_dataset ):
+        raise "Unimplemented Method"
+    def check_library_dataset_access( self, trans, library_id, **kwd ):
+        raise "Unimplemented Method"
     def get_component_associations( self, **kwd ):
         raise "Unimplemented Method"
     def components_are_associated( self, **kwd ):
@@ -283,8 +290,6 @@ class GalaxyRBACAgent( RBACAgent ):
         permissions looks like: { Action : [ Role, Role ] }
         """
         # Delete all of the current permissions on the dataset
-        # TODO: If setting ACCESS permission, at least 1 user must have every role associated with this dataset,
-        # or the dataset is inaccessible.  See admin/library_dataset_dataset_association()
         for dp in dataset.actions:
             self.sa_session.delete( dp )
         # Add the new permissions on the dataset
@@ -299,8 +304,6 @@ class GalaxyRBACAgent( RBACAgent ):
         Set a specific permission on a dataset, leaving all other current permissions on the dataset alone
         permissions looks like: { Action : [ Role, Role ] }
         """
-        # TODO: If setting ACCESS permission, at least 1 user must have every role associated with this dataset,
-        # or the dataset is inaccessible.  See admin/library_dataset_dataset_association()
         for action, roles in permission.items():
             if isinstance( action, Action ):
                 action = action.action
@@ -405,18 +408,92 @@ class GalaxyRBACAgent( RBACAgent ):
             else:
                 permissions[ action ] = [ library_dataset_permission.role ]
         return permissions
+    def check_library_dataset_access( self, trans, library_id, **kwd ):
+        # library_id must be decoded before being sent
+        msg = ''
+        permissions = {}
+        # accessible will be True only if at least 1 user has every role in DATASET_ACCESS_in
+        accessible = False 
+        # legitimate will be True only if all roles in DATASET_ACCESS_in are in the set
+        # of roles returned from library.get_legitimate_roles()
+        legitimate = False
+        error = None
+        for k, v in get_permitted_actions( filter='DATASET' ).items():
+            in_roles = [ self.sa_session.query( self.model.Role ).get( x ) for x in listify( kwd.get( k + '_in', [] ) ) ]
+            if v == self.permitted_actions.DATASET_ACCESS and in_roles:
+                library = self.model.Library.get( library_id )
+                if not self.library_is_public( library ):
+                    # Ensure that roles being associated with DATASET_ACCESS are a subset of the legitimate roles
+                    # derived from the roles associated with the LIBRARY_ACCESS permission on the library if it's
+                    # not public.  This will keep ill-legitimate roles from being associated with the DATASET_ACCESS
+                    # permission on the dataset (i.e., if Role1 is associated with LIBRARY_ACCESS, then only those users
+                    # that have Role1 should be associated with DATASET_ACCESS.
+                    legitimate_roles = library.get_legitimate_roles( trans )
+                    ill_legitimate_roles = []
+                    for role in in_roles:
+                        if role not in legitimate_roles:
+                            ill_legitimate_roles.append( role )
+                    if ill_legitimate_roles:
+                        error = self.ILL_LEGITIMATE
+                        msg += "The following roles are not associated with users that have the 'access library' permission on this "
+                        msg += "library, so they cannot be associated with the 'access' permission on the datasets: "
+                        for role in ill_legitimate_roles:
+                            msg += "%s, " % role.name
+                        msg = msg.rstrip( ", " )
+                        new_in_roles = []
+                        for role in in_roles:
+                            if role in legitimate_roles:
+                                new_in_roles.append( role )
+                        in_roles = new_in_roles
+                    else:
+                        legitimate = True
+                if len( in_roles ) == 1:
+                    accessible = True
+                else:
+                    # At least 1 user must have every role associated with the access 
+                    # permission on this dataset, or the dataset is not accessible.
+                    in_roles_set = set()
+                    for role in in_roles:
+                        in_roles_set.add( role )
+                    users_set = set()
+                    for role in in_roles:
+                        for ura in role.users:
+                            users_set.add( ura.user )
+                        for gra in role.groups:
+                            group = gra.group
+                            for uga in group.users:
+                                users_set.add( uga.user )
+                    # Make sure that at least 1 user has every role being associated with the dataset
+                    for user in users_set:
+                        user_roles_set = set()
+                        for ura in user.roles:
+                            user_roles_set.add( ura.role )
+                        if in_roles_set.issubset( user_roles_set ):
+                            accessible = True
+                            break
+                if not accessible:
+                    error = self.IN_ACCESSIBLE
+                    # Don't set the permissions for DATASET_ACCESS if inaccessible, but set all other permissions
+                    permissions[ self.get_action( v.action ) ] = []
+                    msg += "At least 1 user must have every role associated with accessing datasets.  The roles you "
+                    msg += "attempted to associate for access would make the datasets in-accessible by everyone, "
+                    msg += "so access permissions were left in their original state (or not set).  All other "
+                    msg += "permissions were updated for the datasets."
+                else:
+                    permissions[ self.get_action( v.action ) ] = in_roles
+            else:
+                permissions[ self.get_action( v.action ) ] = in_roles
+        return permissions, in_roles, error, msg
     def copy_library_permissions( self, source_library_item, target_library_item, user=None ):
         # Copy all relevant permissions from source.
         permissions = {}
         for role_assoc in source_library_item.actions:
-            if role_assoc.action == self.permitted_actions.LIBRARY_ACCESS and \
-                not( isinstance( source_library_item, galaxy.model.Libary ) and isinstance( target_library_item, galaxy.model.Libary ) ):
+            if role_assoc.action != self.permitted_actions.LIBRARY_ACCESS.action:
                 # LIBRARY_ACCESS is a special permission that is set only at the library level.
-                continue
-            if role_assoc.action in permissions:
-                permissions[role_assoc.action].append( role_assoc.role )
-            else:
-                permissions[role_assoc.action] = [ role_assoc.role ]
+                if role_assoc.action in permissions:
+                    permissions[role_assoc.action].append( role_assoc.role )
+                else:
+                    permissions[role_assoc.action] = [ role_assoc.role ]
         self.set_all_library_permissions( target_library_item, permissions )
         if user:
             item_class = None

lib/galaxy/tools/actions/__init__.py

@@ -51,8 +51,8 @@ class DefaultToolAction( object ):
                             trans.sa_session.add( assoc )
                             trans.sa_session.flush()
                             data = new_data
-                roles = trans.get_current_user_roles()
-                if data and not trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+                current_user_roles = trans.get_current_user_roles()
+                if data and not trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
                     raise "User does not have permission to use a dataset (%s) provided for input." % data.id
                 return data
             if isinstance( input, DataToolParameter ):
@@ -269,10 +269,10 @@ class DefaultToolAction( object ):
         #        parameters to the command as a special case.
         for name, value in tool.params_to_strings( incoming, trans.app ).iteritems():
             job.add_parameter( name, value )
-        roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         for name, dataset in inp_data.iteritems():
             if dataset:
-                if not trans.app.security_agent.can_access_dataset( roles, dataset.dataset ):
+                if not trans.app.security_agent.can_access_dataset( current_user_roles, dataset.dataset ):
                     raise "User does not have permission to use a dataset (%s) provided for input." % data.id
                 job.add_input_dataset( name, dataset )
             else:

lib/galaxy/tools/actions/upload_common.py

@@ -59,7 +59,7 @@ def handle_library_params( trans, params, folder_id, replace_dataset=None ):
         role = trans.sa_session.query( trans.app.model.Role ).get( role_id )
         library_bunch.roles.append( role )
     return library_bunch
-def get_precreated_datasets( trans, params, data_obj, controller='root' ):    
+def get_precreated_datasets( trans, params, data_obj, controller='root' ):
     """
     Get any precreated datasets (when using asynchronous uploads).
     """
@@ -67,7 +67,7 @@ def get_precreated_datasets( trans, params, data_obj, controller='root' ):
     async_datasets = []
     if params.get( 'async_datasets', None ) not in ["None", "", None]:
         async_datasets = params['async_datasets'].split(',')
-    roles = trans.get_current_user_roles()
+    current_user_roles = trans.get_current_user_roles()
     for id in async_datasets:
         try:
             data = trans.sa_session.query( data_obj ).get( int( id ) )
@@ -82,7 +82,7 @@ def get_precreated_datasets( trans, params, data_obj, controller='root' ):
             else:
                 rval.append( data )
         elif data_obj is trans.app.model.LibraryDatasetDatasetAssociation:
-            if controller == 'library' and not trans.app.security_agent.can_add_library_item( roles, data.library_dataset.folder ):
+            if controller == 'library' and not trans.app.security_agent.can_add_library_item( current_user_roles, data.library_dataset.folder ):
                 log.error( 'Got a precreated dataset (%s) but this user (%s) is not allowed to write to it' % ( data.id, trans.user.id ) )
             else:
                 rval.append( data )
@@ -122,8 +122,8 @@ def new_history_upload( trans, uploaded_dataset, state=None ):
     trans.sa_session.flush()
     return hda
 def new_library_upload( trans, uploaded_dataset, library_bunch, state=None ):
-    roles = trans.get_current_user_roles()
-    if not ( trans.app.security_agent.can_add_library_item( roles, library_bunch.folder ) \
+    current_user_roles = trans.get_current_user_roles()
+    if not ( trans.app.security_agent.can_add_library_item( current_user_roles, library_bunch.folder ) \
              or trans.user.email in trans.app.config.get( "admin_users", "" ).split( "," ) ):
         # This doesn't have to be pretty - the only time this should happen is if someone's being malicious.
         raise Exception( "User is not authorized to add datasets to this library." )

lib/galaxy/tools/parameters/basic.py

@@ -1177,7 +1177,7 @@ class DataToolParameter( ToolParameter ):
         field = form_builder.SelectField( self.name, self.multiple, None, self.refresh_on_change, refresh_on_change_values = self.refresh_on_change_values )
         # CRUCIAL: the dataset_collector function needs to be local to DataToolParameter.get_html_field()
         def dataset_collector( hdas, parent_hid ):
-            roles = trans.get_current_user_roles()
+            current_user_roles = trans.get_current_user_roles()
             for i, hda in enumerate( hdas ):
                 if len( hda.name ) > 30:
                     hda_name = '%s..%s' % ( hda.name[:17], hda.name[-11:] )
@@ -1189,7 +1189,7 @@ class DataToolParameter( ToolParameter ):
                     hid = str( hda.hid )
                 if not hda.dataset.state in [galaxy.model.Dataset.states.ERROR, galaxy.model.Dataset.states.DISCARDED] and \
                     hda.visible and \
-                    trans.app.security_agent.can_access_dataset( roles, hda.dataset ):
+                    trans.app.security_agent.can_access_dataset( current_user_roles, hda.dataset ):
                     # If we are sending data to an external application, then we need to make sure there are no roles
                     # associated with the dataset that restrict it's access from "public".
                     if self.tool and self.tool.tool_type == 'data_destination' and not trans.app.security_agent.dataset_is_public( hda.dataset ):
@@ -1204,7 +1204,7 @@ class DataToolParameter( ToolParameter ):
                         if target_ext:
                             if converted_dataset:
                                 hda = converted_dataset
-                            if not trans.app.security_agent.can_access_dataset( roles, hda.dataset ):
+                            if not trans.app.security_agent.can_access_dataset( current_user_roles, hda.dataset ):
                                 continue
                             selected = ( value and ( hda in value ) )
                             field.add_option( "%s: (as %s) %s" % ( hid, target_ext, hda_name ), hda.id, selected )

lib/galaxy/web/controllers/dataset.py

@@ -192,8 +192,8 @@ class DatasetInterface( BaseController ):
         data = data = trans.sa_session.query( trans.app.model.HistoryDatasetAssociation ).get( dataset_id )
         if not data:
             raise paste.httpexceptions.HTTPRequestRangeNotSatisfiable( "Invalid reference dataset id: %s." % str( dataset_id ) )
-        roles = trans.get_current_user_roles()
-        if trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+        current_user_roles = trans.get_current_user_roles()
+        if trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
             if data.state == trans.model.Dataset.states.UPLOAD:
                 return trans.show_error_message( "Please wait until this dataset finishes uploading before attempting to view it." )
             
@@ -298,10 +298,10 @@ class DatasetInterface( BaseController ):
         if 'display_url' not in kwd or 'redirect_url' not in kwd:
             return trans.show_error_message( 'Invalid parameters specified for "display at" link, please contact a Galaxy administrator' )
         redirect_url = kwd['redirect_url'] % urllib.quote_plus( kwd['display_url'] )
-        roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         if trans.app.security_agent.dataset_is_public( data.dataset ):
             return trans.response.send_redirect( redirect_url ) # anon access already permitted by rbac
-        if trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+        if trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
             trans.app.host_security_agent.set_dataset_permissions( data, trans.user, site )
             return trans.response.send_redirect( redirect_url )
         else:

lib/galaxy/web/controllers/library.py

@@ -21,13 +21,13 @@ class Library( BaseController ):
         params = util.Params( kwd )
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
-        roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         all_libraries = trans.sa_session.query( trans.app.model.Library ) \
                                         .filter( trans.app.model.Library.table.c.deleted==False ) \
                                         .order_by( trans.app.model.Library.name )
         authorized_libraries = []
         for library in all_libraries:
-            if trans.app.security_agent.library_is_public( library ) or trans.app.security_agent.can_access_library( roles, library ):
+            if trans.app.security_agent.can_access_library( current_user_roles, library ):
                 authorized_libraries.append( library )
         return trans.fill_template( '/library/browse_libraries.mako', 
                                     libraries=authorized_libraries,

lib/galaxy/web/controllers/library_common.py

@@ -101,6 +101,7 @@ class LibraryCommon( BaseController ):
         show_deleted = util.string_as_bool( params.get( 'show_deleted', False ) )
         created_ldda_ids = params.get( 'created_ldda_ids', '' )
         hidden_folder_ids = util.listify( params.get( 'hidden_folder_ids', '' ) )
+        current_user_roles = trans.get_current_user_roles()
         if created_ldda_ids and not msg:
             msg = "%d datasets are uploading in the background to the library '%s' (each is selected).  "  % \
                 ( len( created_ldda_ids.split( ',' ) ), library.name )
@@ -115,6 +116,7 @@ class LibraryCommon( BaseController ):
                                     default_action=params.get( 'default_action', None ),
                                     show_deleted=show_deleted,
                                     comptypes=comptypes,
+                                    current_user_roles=current_user_roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -126,18 +128,14 @@ class LibraryCommon( BaseController ):
         library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
         # See if we have any associated templates
         widgets = library.get_template_widgets( trans )
+        current_user_roles = trans.get_current_user_roles()
         if params.get( 'rename_library_button', False ):
             old_name = library.name
             new_name = util.restore_text( params.name )
             new_description = util.restore_text( params.description )
             if not new_name:
                 msg = 'Enter a valid name'
-                return trans.fill_template( '/library/common/library_info.mako',
-                                            cntrller=cntrller,
-                                            library=library,
-                                            widgets=widgets,
-                                            msg=msg,
-                                            messagetype='error' )
+                messagetype='error'
             else:
                 library.name = new_name
                 library.description = new_description
@@ -157,6 +155,7 @@ class LibraryCommon( BaseController ):
                                     cntrller=cntrller,
                                     library=library,
                                     widgets=widgets,
+                                    current_user_roles=current_user_roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -166,6 +165,7 @@ class LibraryCommon( BaseController ):
         messagetype = params.get( 'messagetype', 'done' )
         library_id = params.get( 'id', None )
         library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+        current_user_roles = trans.get_current_user_roles()
         if params.get( 'update_roles_button', False ):
             # The user clicked the Save button on the 'Associate With Roles' form
             permissions = {}
@@ -183,9 +183,12 @@ class LibraryCommon( BaseController ):
                                                               id=trans.security.encode_id( library.id ),
                                                               msg=util.sanitize_text( msg ),
                                                               messagetype='done' ) )
+        roles = library.get_legitimate_roles( trans )
         return trans.fill_template( '/library/common/library_permissions.mako',
                                     cntrller=cntrller,
                                     library=library,
+                                    current_user_roles=current_user_roles,
+                                    roles=roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -233,52 +236,34 @@ class LibraryCommon( BaseController ):
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
         folder = trans.sa_session.query( trans.app.model.LibraryFolder ).get( trans.security.decode_id( id ) )
-        if cntrller != 'library_admin':
-            roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         # See if we have any associated templates
         widgets = folder.get_template_widgets( trans )
         if params.get( 'rename_folder_button', False ):
-            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, folder ):
+            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, folder ):
                 old_name = folder.name
                 new_name = util.restore_text( params.name )
                 new_description = util.restore_text( params.description )
                 if not new_name:
                     msg = 'Enter a valid name'
-                    return trans.fill_template( "/library/common/folder_info.mako",
-                                                cntrller=cntrller,
-                                                folder=folder,
-                                                library_id=library_id,
-                                                widgets=widgets,
-                                                msg=msg,
-                                                messagetype='error' )
+                    messagetype='error'
                 else:
                     folder.name = new_name
                     folder.description = new_description
                     trans.sa_session.add( folder )
                     trans.sa_session.flush()
                     msg = "Folder '%s' has been renamed to '%s'" % ( old_name, new_name )
-                    return trans.response.send_redirect( web.url_for( controller='library_common',
-                                                                      action='folder_info',
-                                                                      cntrller=cntrller,
-                                                                      id=id,
-                                                                      library_id=library_id,
-                                                                      msg=util.sanitize_text( msg ),
-                                                                      messagetype='done' ) )
+                    messagetype='done'
             else:
                 msg = "You are not authorized to edit this folder"
-                return trans.fill_template( "/library/common/folder_info.mako",
-                                            cntrller=cntrller,
-                                            folder=folder,
-                                            library_id=library_id,
-                                            widgets=widgets,
-                                            msg=msg,
-                                            messagetype='error' )
+                messagetype='error'
         return trans.fill_template( '/library/common/folder_info.mako',
                                     cntrller=cntrller,
                                     folder=folder,
                                     library_id=library_id,
                                     widgets=widgets,
-                                    msg=msg,
+                                    current_user_roles=current_user_roles,
+                                    msg=util.sanitize_text( msg ),
                                     messagetype=messagetype )
     @web.expose
     def folder_permissions( self, trans, cntrller, id, library_id, **kwd ):
@@ -294,36 +279,40 @@ class LibraryCommon( BaseController ):
                                                               id=library_id,
                                                               msg=util.sanitize_text( msg ),
                                                               messagetype='error' ) )
-        if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         if params.get( 'update_roles_button', False ):
             # The user clicked the Save button on the 'Associate With Roles' form
-            if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( roles, folder ):
+            if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( current_user_roles, folder ):
                 permissions = {}
                 for k, v in trans.app.model.Library.permitted_actions.items():
-                    in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( int( x ) ) for x in util.listify( params.get( k + '_in', [] ) ) ]
-                    permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
+                    if k != 'LIBRARY_ACCESS':
+                        # LIBRARY_ACCESS is a special permission set only at the library level
+                        # and it is not inherited.
+                        in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( int( x ) ) for x in util.listify( params.get( k + '_in', [] ) ) ]
+                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
                 trans.app.security_agent.set_all_library_permissions( folder, permissions )
                 trans.sa_session.refresh( folder )
                 msg = 'Permissions updated for folder %s' % folder.name
-                return trans.response.send_redirect( web.url_for( controller='library_common',
-                                                                  action='folder_permissions',
-                                                                  id=trans.security.encode_id( folder.id ),
-                                                                  library_id=library_id,
-                                                                  msg=util.sanitize_text( msg ),
-                                                                  messagetype='done' ) )
+                messagetype='done'
             else:
                 msg = "You are not authorized to manage permissions on this folder"
-                return trans.response.send_redirect( web.url_for( controller='library_common',
-                                                                  action='folder_permissions',
-                                                                  id=trans.security.encode_id( folder.id ),
-                                                                  library_id=library_id,
-                                                                  msg=util.sanitize_text( msg ),
-                                                                  messagetype='error' ) )
+                messagetype = "error"
+            return trans.response.send_redirect( web.url_for( controller='library_common',
+                                                              action='folder_permissions',
+                                                              id=trans.security.encode_id( folder.id ),
+                                                              library_id=library_id,
+                                                              msg=util.sanitize_text( msg ),
+                                                              messagetype=messagetype ) )
+        # If the library is public all roles are legitimate, but if the library is restricted, only those
+        # roles associated with the LIBRARY_ACCESS permission are legitimate.
+        library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+        roles = library.get_legitimate_roles( trans )
         return trans.fill_template( '/library/common/folder_permissions.mako',
                                     cntrller=cntrller,
                                     folder=folder,
                                     library_id=library_id,
+                                    current_user_roles=current_user_roles,
+                                    roles=roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -343,15 +332,14 @@ class LibraryCommon( BaseController ):
         dbkey = params.get( 'dbkey', '?' )
         if isinstance( dbkey, list ):
             dbkey = dbkey[0]
-        if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         file_formats = [ dtype_name for dtype_name, dtype_value in trans.app.datatypes_registry.datatypes_by_extension.iteritems() if dtype_value.allow_datatype_change ]
         file_formats.sort()
         # See if we have any associated templates
         widgets = ldda.get_template_widgets( trans )
         if params.get( 'change', False ):
             # The user clicked the Save button on the 'Change data type' form
-            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda ):
+            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda ):
                 if ldda.datatype.allow_datatype_change and trans.app.datatypes_registry.get_datatype_by_extension( params.datatype ).allow_datatype_change:
                     trans.app.datatypes_registry.change_datatype( ldda, params.datatype )
                     trans.sa_session.flush()
@@ -369,11 +357,12 @@ class LibraryCommon( BaseController ):
                                         library_id=library_id,
                                         file_formats=file_formats,
                                         widgets=widgets,
+                                        current_user_roles=current_user_roles,
                                         msg=msg,
                                         messagetype=messagetype )
         elif params.get( 'save', False ):
             # The user clicked the Save button on the 'Edit Attributes' form
-            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda ):
+            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda ):
                 old_name = ldda.name
                 new_name = util.restore_text( params.get( 'name', '' ) )
                 new_info = util.restore_text( params.get( 'info', '' ) )
@@ -409,11 +398,12 @@ class LibraryCommon( BaseController ):
                                         library_id=library_id,
                                         file_formats=file_formats,
                                         widgets=widgets,
+                                        current_user_roles=current_user_roles,
                                         msg=msg,
                                         messagetype=messagetype )
         elif params.get( 'detect', False ):
             # The user clicked the Auto-detect button on the 'Edit Attributes' form
-            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda ):
+            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda ):
                 for name, spec in ldda.datatype.metadata_spec.items():
                     # We need to be careful about the attributes we are resetting
                     if name not in [ 'name', 'info', 'dbkey' ]:
@@ -433,9 +423,10 @@ class LibraryCommon( BaseController ):
                                         library_id=library_id,
                                         file_formats=file_formats,
                                         widgets=widgets,
+                                        current_user_roles=current_user_roles,
                                         msg=msg,
                                         messagetype=messagetype )
-        if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda ):
+        if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda ):
             if "dbkey" in ldda.datatype.metadata_spec and not ldda.metadata.dbkey:
                 # Copy dbkey into metadata, for backwards compatability
                 # This looks like it does nothing, but getting the dbkey
@@ -449,6 +440,7 @@ class LibraryCommon( BaseController ):
                                     library_id=library_id,
                                     file_formats=file_formats,
                                     widgets=widgets,
+                                    current_user_roles=current_user_roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -469,6 +461,7 @@ class LibraryCommon( BaseController ):
         library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
         # See if we have any associated templates
         widgets = ldda.get_template_widgets( trans )
+        current_user_roles = trans.get_current_user_roles()
         return trans.fill_template( '/library/common/ldda_info.mako',
                                     cntrller=cntrller,
                                     ldda=ldda,
@@ -483,7 +476,6 @@ class LibraryCommon( BaseController ):
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
         ids = util.listify( id )
-        # Display permission form, permissions will be updated for all lddas simultaneously.
         lddas = []
         for id in [ trans.security.decode_id( id ) for id in ids ]:
             ldda = trans.sa_session.query( trans.app.model.LibraryDatasetDatasetAssociation ).get( id )
@@ -496,63 +488,44 @@ class LibraryCommon( BaseController ):
                                                            msg=util.sanitize_text( msg ),
                                                            messagetype='error' ) )
             lddas.append( ldda )
+        # If the library is public all roles are legitimate, but if the library is restricted, only those
+        # roles associated with the LIBRARY_ACCESS permission are legitimate.
+        library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+        roles = library.get_legitimate_roles( trans )
         if params.get( 'update_roles_button', False ):
-            if cntrller=='library_admin' or ( trans.app.security_agent.can_manage_library_item( roles, ldda ) and \
-                                              trans.app.security_agent.can_manage_dataset( roles, ldda.dataset ) ):
-                permissions = {}
-                accessible = False
-                for k, v in trans.app.model.Dataset.permitted_actions.items():
-                    in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( x ) for x in util.listify( params.get( k + '_in', [] ) ) ]
-                    # At least 1 user must have every role associated with this dataset, or the dataset is inaccessible
-                    if v == trans.app.security_agent.permitted_actions.DATASET_ACCESS:
-                        if len( in_roles ) > 1:
-                            # Get the set of all users that are being associated with the dataset
-                            in_roles_set = set()
-                            for role in in_roles:
-                                in_roles_set.add( role )
-                            users_set = set()
-                            for role in in_roles:
-                                for ura in role.users:
-                                    users_set.add( ura.user )
-                            # Make sure that at least 1 user has every role being associated with the dataset
-                            for user in users_set:
-                                user_roles_set = set()
-                                for ura in user.roles:
-                                    user_roles_set.add( ura.role )
-                                if in_roles_set.issubset( user_roles_set ):
-                                    accessible = True
-                                    break
-                        else:
-                            accessible = True
-                    if not accessible and v == trans.app.security_agent.permitted_actions.DATASET_ACCESS:
-                        # Don't set the permissions for DATASET_ACCESS if inaccessbile, but set all other permissions
-                        # TODO: keep access permissions as they originally were, rather than automatically making public
-                        permissions[ trans.app.security_agent.get_action( v.action ) ] = []
-                    else:
-                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
+            current_user_roles = trans.get_current_user_roles()
+            if cntrller=='library_admin' or ( trans.app.security_agent.can_manage_library_item( current_user_roles, ldda ) and \
+                                              trans.app.security_agent.can_manage_dataset( current_user_roles, ldda.dataset ) ):
+                permissions, in_roles, error, msg = \
+                    trans.app.security_agent.check_library_dataset_access( trans, trans.app.security.decode_id( library_id ), **kwd )
                 for ldda in lddas:
-                    # Set the DATASET permissions on the Dataset
+                    # Set the DATASET permissions on the Dataset.
+                    if error == trans.app.security_agent.IN_ACCESSIBLE:
+                        # If the check_library_dataset_access() returned a "in_accessible" error, then we keep the original role
+                        # associations for the DATASET_ACCESS permission on each ldda.
+                        a = trans.app.security_agent.get_action( trans.app.security_agent.permitted_actions.DATASET_ACCESS.action )
+                        permissions[ a ] = ldda.get_access_roles( trans )
                     trans.app.security_agent.set_all_dataset_permissions( ldda.dataset, permissions )
                     trans.sa_session.refresh( ldda.dataset )
+                # Set the LIBRARY permissions on the LibraryDataset
+                # NOTE: the LibraryDataset and LibraryDatasetDatasetAssociation will be set with the same permissions
                 permissions = {}
                 for k, v in trans.app.model.Library.permitted_actions.items():
-                    in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
-                    permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
+                    if k != 'LIBRARY_ACCESS':
+                        # LIBRARY_ACCESS is a special permission set only at the library level and it is not inherited.
+                        in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
+                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
                 for ldda in lddas:
-                    # Set the LIBRARY permissions on the LibraryDataset
-                    # NOTE: the LibraryDataset and LibraryDatasetDatasetAssociation will be set with the same permissions
                     trans.app.security_agent.set_all_library_permissions( ldda.library_dataset, permissions )
                     trans.sa_session.refresh( ldda.library_dataset )
                     # Set the LIBRARY permissions on the LibraryDatasetDatasetAssociation
                     trans.app.security_agent.set_all_library_permissions( ldda, permissions )
                     trans.sa_session.refresh( ldda )
-                if not accessible:
-                    msg = "At least 1 user must have every role associated with accessing these %d datasets. " % len( lddas )
-                    msg += "The roles you attempted to associate for access would make these datasets inaccessible by everyone, "
-                    msg += "so access permissions were not set.  All other permissions were updated for the datasets."
+                if error:
                     messagetype = 'error'
                 else:
-                    msg = "Permissions have been updated on %d datasets" % len( lddas )
+                    msg = 'Permissions have been updated on %d datasets.' % len( lddas )
+                    messagetype= 'done'
             else:
                 msg = "You are not authorized to change the permissions of dataset '%s'" % ldda.name
                 messagetype = 'error'
@@ -560,6 +533,7 @@ class LibraryCommon( BaseController ):
                                         cntrller=cntrller,
                                         lddas=lddas,
                                         library_id=library_id,
+                                        roles=roles,
                                         msg=msg,
                                         messagetype=messagetype )
         if len( ids ) > 1:
@@ -587,10 +561,12 @@ class LibraryCommon( BaseController ):
                                                                id=library_id,
                                                                msg=util.sanitize_text( msg ),
                                                                messagetype='error' ) )
+        # Display permission form, permissions will be updated for all lddas simultaneously.
         return trans.fill_template( "/library/common/ldda_permissions.mako",
                                     cntrller=cntrller,
                                     lddas=lddas,
                                     library_id=library_id,
+                                    roles=roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -623,72 +599,86 @@ class LibraryCommon( BaseController ):
             replace_dataset = None
             upload_option = params.get( 'upload_option', 'upload_file' )
         if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+            current_user_roles = trans.get_current_user_roles()
         if cntrller == 'library_admin' or \
-            ( trans.app.security_agent.can_add_library_item( roles, folder ) or \
-              ( replace_dataset and trans.app.security_agent.can_modify_library_item( roles, replace_dataset ) ) ):
+            ( trans.app.security_agent.can_add_library_item( current_user_roles, folder ) or \
+              ( replace_dataset and trans.app.security_agent.can_modify_library_item( current_user_roles, replace_dataset ) ) ):
             if params.get( 'runtool_btn', False ) or params.get( 'ajax_upload', False ):
-                # See if we have any inherited templates, but do not inherit contents.
-                info_association, inherited = folder.get_info_association( inherited=True )
-                if info_association:
-                    template_id = str( info_association.template.id )
-                    widgets = folder.get_template_widgets( trans, get_contents=False )
-                else:
-                    template_id = 'None'
-                    widgets = []
-                created_outputs = trans.webapp.controllers[ 'library_common' ].upload_dataset( trans,
-                                                                                               cntrller=cntrller,
-                                                                                               library_id=library_id,
-                                                                                               folder_id=folder_id,
-                                                                                               template_id=template_id,
-                                                                                               widgets=widgets,
-                                                                                               replace_dataset=replace_dataset,
-                                                                                               **kwd )
-                if created_outputs:
-                    total_added = len( created_outputs.values() )
-                    ldda_id_list = [ str( v.id ) for v in created_outputs.values() ]
-                    if replace_dataset:
-                        msg = "Added %d dataset versions to the library dataset '%s' in the folder '%s'." % ( total_added, replace_dataset_name, folder.name )
+                # Check to see if the user selected roles to associate with the DATASET_ACCESS permission
+                # on the dataset that would make the dataset in-accessible to everyone.
+                roles = params.get( 'roles', False )
+                error = None
+                if roles:
+                    vars = dict( DATASET_ACCESS_in=roles )
+                    permissions, in_roles, error, msg = \
+                        trans.app.security_agent.check_library_dataset_access( trans, trans.app.security.decode_id( library_id ), **vars )
+                    if error:
+                        if error == trans.app.security_agent.IN_ACCESSIBLE:
+                            msg = "At least 1 user must have every role associated with accessing datasets.  The roles you "
+                            msg += "attempted to associate for access would make the datasets in-accessible by everyone."
+                        messagetype = 'error'
+                if not error:
+                    # See if we have any inherited templates, but do not inherit contents.
+                    info_association, inherited = folder.get_info_association( inherited=True )
+                    if info_association:
+                        template_id = str( info_association.template.id )
+                        widgets = folder.get_template_widgets( trans, get_contents=False )
                     else:
-                        if not folder.parent:
-                            # Libraries have the same name as their root_folder
-                            msg = "Added %d datasets to the library '%s' (each is selected).  " % ( total_added, folder.name )
+                        template_id = 'None'
+                        widgets = []
+                    created_outputs = trans.webapp.controllers[ 'library_common' ].upload_dataset( trans,
+                                                                                                   cntrller=cntrller,
+                                                                                                   library_id=library_id,
+                                                                                                   folder_id=folder_id,
+                                                                                                   template_id=template_id,
+                                                                                                   widgets=widgets,
+                                                                                                   replace_dataset=replace_dataset,
+                                                                                                   **kwd )
+                    if created_outputs:
+                        total_added = len( created_outputs.values() )
+                        ldda_id_list = [ str( v.id ) for v in created_outputs.values() ]
+                        if replace_dataset:
+                            msg = "Added %d dataset versions to the library dataset '%s' in the folder '%s'." % ( total_added, replace_dataset_name, folder.name )
                         else:
-                            msg = "Added %d datasets to the folder '%s' (each is selected).  " % ( total_added, folder.name )
-                        if cntrller == 'library_admin':
-                            msg += "Click the Go button at the bottom of this page to edit the permissions on these datasets if necessary."
-                            messagetype='done'
-                        else:
-                            # Since permissions on all LibraryDatasetDatasetAssociations must be the same at this point, we only need
-                            # to check one of them to see if the current user can manage permissions on them.
-                            check_ldda = trans.sa_session.query( trans.app.model.LibraryDatasetDatasetAssociation ).get( ldda_id_list[0] )
-                            if trans.app.security_agent.can_manage_library_item( roles, check_ldda ):
-                                if replace_dataset:
-                                    default_action = ''
-                                else:
-                                    msg += "Click the Go button at the bottom of this page to edit the permissions on these datasets if necessary."
-                                    default_action = 'manage_permissions'
+                            if not folder.parent:
+                                # Libraries have the same name as their root_folder
+                                msg = "Added %d datasets to the library '%s' (each is selected).  " % ( total_added, folder.name )
                             else:
-                                default_action = 'add'
-                            trans.response.send_redirect( web.url_for( controller='library_common',
-                                                                       action='browse_library',
-                                                                       cntrller=cntrller,
-                                                                       id=library_id,
-                                                                       default_action=default_action,
-                                                                       created_ldda_ids=",".join( ldda_id_list ), 
-                                                                       msg=util.sanitize_text( msg ), 
-                                                                       messagetype='done' ) )
-                    
-                else:
-                    msg = "Upload failed"
-                    messagetype='error'
-                trans.response.send_redirect( web.url_for( controller='library_common',
-                                                           action='browse_library',
-                                                           cntrller=cntrller,
-                                                           id=library_id,
-                                                           created_ldda_ids=",".join( [ str( v.id ) for v in created_outputs.values() ] ),
-                                                           msg=util.sanitize_text( msg ),
-                                                           messagetype=messagetype ) )
+                                msg = "Added %d datasets to the folder '%s' (each is selected).  " % ( total_added, folder.name )
+                            if cntrller == 'library_admin':
+                                msg += "Click the Go button at the bottom of this page to edit the permissions on these datasets if necessary."
+                                messagetype='done'
+                            else:
+                                # Since permissions on all LibraryDatasetDatasetAssociations must be the same at this point, we only need
+                                # to check one of them to see if the current user can manage permissions on them.
+                                check_ldda = trans.sa_session.query( trans.app.model.LibraryDatasetDatasetAssociation ).get( ldda_id_list[0] )
+                                if trans.app.security_agent.can_manage_library_item( current_user_roles, check_ldda ):
+                                    if replace_dataset:
+                                        default_action = ''
+                                    else:
+                                        msg += "Click the Go button at the bottom of this page to edit the permissions on these datasets if necessary."
+                                        default_action = 'manage_permissions'
+                                else:
+                                    default_action = 'add'
+                                trans.response.send_redirect( web.url_for( controller='library_common',
+                                                                           action='browse_library',
+                                                                           cntrller=cntrller,
+                                                                           id=library_id,
+                                                                           default_action=default_action,
+                                                                           created_ldda_ids=",".join( ldda_id_list ), 
+                                                                           msg=util.sanitize_text( msg ), 
+                                                                           messagetype='done' ) )
+                        
+                    else:
+                        msg = "Upload failed"
+                        messagetype='error'
+                    trans.response.send_redirect( web.url_for( controller='library_common',
+                                                               action='browse_library',
+                                                               cntrller=cntrller,
+                                                               id=library_id,
+                                                               created_ldda_ids=",".join( [ str( v.id ) for v in created_outputs.values() ] ),
+                                                               msg=util.sanitize_text( msg ),
+                                                               messagetype=messagetype ) )
         # See if we have any inherited templates, but do not inherit contents.
         widgets = folder.get_template_widgets( trans, get_contents=False )
         upload_option = params.get( 'upload_option', 'upload_file' )
@@ -700,10 +690,11 @@ class LibraryCommon( BaseController ):
             for dbkey, build_name in util.dbnames:
                 yield build_name, dbkey, ( dbkey==last_used_build )
         dbkeys = get_dbkey_options( last_used_build )
-        # Send list of roles to the form so the dataset can be associated with 1 or more of them.
-        roles = trans.sa_session.query( trans.app.model.Role ) \
-                                .filter( trans.app.model.Role.table.c.deleted==False ) \
-                                .order_by( trans.app.model.Role.table.c.name )
+        # Send list of legitimate roles to the form so the dataset can be associated with 1 or more of them.
+        # If the library is public, all active roles are legitimate.  If the library is restricted by the
+        # LIBRARY_ACCESS permission, only those roles associated with that permission are legitimate.
+        library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+        roles = library.get_legitimate_roles( trans )
         # Send the current history to the form to enable importing datasets from history to library
         history = trans.get_history()
         trans.sa_session.refresh( history )
@@ -949,8 +940,8 @@ class LibraryCommon( BaseController ):
                             # Since permissions on all LibraryDatasetDatasetAssociations must be the same at this point, we only need
                             # to check one of them to see if the current user can manage permissions on them.
                             check_ldda = trans.sa_session.query( trans.app.model.LibraryDatasetDatasetAssociation ).get( trans.security.decode_id( ldda_id_list[0] ) )
-                            roles = trans.get_current_user_roles()
-                            if trans.app.security_agent.can_manage_library_item( roles, check_ldda ):
+                            current_user_roles = trans.get_current_user_roles()
+                            if trans.app.security_agent.can_manage_library_item( current_user_roles, check_ldda ):
                                 if replace_dataset:
                                     default_action = ''
                                 else:
@@ -977,10 +968,9 @@ class LibraryCommon( BaseController ):
                     for dbkey, build_name in util.dbnames:
                         yield build_name, dbkey, ( dbkey==last_used_build )
                 dbkeys = get_dbkey_options( last_used_build )
-                # Send list of roles to the form so the dataset can be associated with 1 or more of them.
-                roles = trans.sa_session.query( trans.app.model.Role ) \
-                                        .filter( trans.app.model.Role.table.c.deleted==False ) \
-                                        .order_by( trans.app.model.Role.table.c.name )
+                # Send list of legitimate roles to the form so the dataset can be associated with 1 or more of them.
+                library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+                roles = library.get_legitimate_roles( trans )
                 return trans.fill_template( "/library/common/upload.mako",
                                             upload_option=upload_option,
                                             library_id=library_id,
@@ -1039,10 +1029,9 @@ class LibraryCommon( BaseController ):
                                                               id=library_id,
                                                               msg=util.sanitize_text( msg ),
                                                               messagetype='error' ) )
-        if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         if params.get( 'edit_attributes_button', False ):
-            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, library_dataset ):
+            if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, library_dataset ):
                 if params.get( 'edit_attributes_button', False ):
                     old_name = library_dataset.name
                     new_name = util.restore_text( params.get( 'name', '' ) )
@@ -1064,6 +1053,7 @@ class LibraryCommon( BaseController ):
                                     cntrller=cntrller,
                                     library_dataset=library_dataset,
                                     library_id=library_id,
+                                    current_user_roles=current_user_roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
@@ -1080,15 +1070,17 @@ class LibraryCommon( BaseController ):
                                                               id=library_id,
                                                               msg=util.sanitize_text( msg ),
                                                               messagetype='error' ) )
-        if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         if params.get( 'update_roles_button', False ):
-            if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( roles, library_dataset ):
+            if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( current_user_roles, library_dataset ):
                 # The user clicked the Save button on the 'Associate With Roles' form
                 permissions = {}
                 for k, v in trans.app.model.Library.permitted_actions.items():
-                    in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
-                    permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
+                    if k != 'LIBRARY_ACCESS':
+                        # LIBRARY_ACCESS is a special permission set only at the library level
+                        # and it is not inherited.
+                        in_roles = [ trans.sa_session.query( trans.app.model.Role ).get( x ) for x in util.listify( kwd.get( k + '_in', [] ) ) ]
+                        permissions[ trans.app.security_agent.get_action( v.action ) ] = in_roles
                 # Set the LIBRARY permissions on the LibraryDataset
                 # NOTE: the LibraryDataset and LibraryDatasetDatasetAssociation will be set with the same permissions
                 trans.app.security_agent.set_all_library_permissions( library_dataset, permissions )
@@ -1101,16 +1093,20 @@ class LibraryCommon( BaseController ):
             else:
                 msg = "You are not authorized to managed the permissions of this dataset"
                 messagetype = "error"
+        library = trans.sa_session.query( trans.app.model.Library ).get( trans.security.decode_id( library_id ) )
+        roles = library.get_legitimate_roles( trans )
         return trans.fill_template( '/library/common/library_dataset_permissions.mako',
                                     cntrller=cntrller,
                                     library_dataset=library_dataset,
                                     library_id=library_id,
+                                    roles=roles,
+                                    current_user_roles=current_user_roles,
                                     msg=msg,
                                     messagetype=messagetype )
     @web.expose
     def act_on_multiple_datasets( self, trans, cntrller, library_id, ldda_ids='', **kwd ):
         # This method is used by the select list labeled "Perform action on selected datasets"
-        # on the analysis library browser.
+        # on the analysis library browser
         params = util.Params( kwd )
         msg = util.restore_text( params.get( 'msg', ''  ) )
         messagetype = params.get( 'messagetype', 'done' )
@@ -1204,11 +1200,11 @@ class LibraryCommon( BaseController ):
                                                                   msg=util.sanitize_text( msg ),
                                                                   messagetype='error' ) )
             seen = []
-            roles = trans.get_current_user_roles()
+            current_user_roles = trans.get_current_user_roles()
             for ldda_id in ldda_ids:
                 ldda = trans.sa_session.query( trans.app.model.LibraryDatasetDatasetAssociation ).get( trans.security.decode_id( ldda_id ) )
                 if not ldda \
-                    or not trans.app.security_agent.can_access_dataset( roles, ldda.dataset ) \
+                    or not trans.app.security_agent.can_access_dataset( current_user_roles, ldda.dataset ) \
                     or ldda.dataset.state in [ 'new', 'upload', 'queued', 'running', 'empty', 'discarded' ]:
                     continue
                 path = ""
@@ -1318,7 +1314,7 @@ class LibraryCommon( BaseController ):
                                                        id=response_id,
                                                        library_id=library_id,
                                                        msg=msg,
-                                                       message_type='done' ) )
+                                                       messagetype='done' ) )
         return trans.fill_template( '/library/common/select_info_template.mako',
                                     cntrller=cntrller,
                                     library_item_name=library_item.name,

lib/galaxy/web/controllers/requests.py

@@ -604,11 +604,11 @@ class Requests( BaseController ):
         all_libraries = trans.sa_session.query( trans.app.model.Library ) \
                                         .filter( trans.app.model.Library.table.c.deleted == False ) \
                                         .order_by( trans.app.model.Library.name )
-        roles = trans.get_current_user_roles()
+        current_user_roles = trans.get_current_user_roles()
         actions_to_check = [ trans.app.security_agent.permitted_actions.LIBRARY_ADD ]
         libraries = odict()
         for library in all_libraries:
-            can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( trans.user, roles, library, actions_to_check )
+            can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( trans.user, current_user_roles, library, actions_to_check )
             if can_show:
                 libraries[ library ] = hidden_folder_ids
         # create data library selectbox with refresh on change enabled
@@ -662,7 +662,7 @@ class Requests( BaseController ):
                 folder_list.add_option('Select one', 'none')
             # get all show-able folders for the selected library
             showable_folders = trans.app.security_agent.get_showable_folders( trans.user,
-                                                                              roles, 
+                                                                              current_user_roles, 
                                                                               selected_lib, 
                                                                               actions_to_check, 
                                                                               selected_hidden_folder_ids )

lib/galaxy/web/controllers/root.py

@@ -161,8 +161,8 @@ class RootController( BaseController ):
             except:
                 return "Dataset id '%s' is invalid" %str( id )
         if data:
-            roles = trans.get_current_user_roles()
-            if trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+            current_user_roles = trans.get_current_user_roles()
+            if trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
                 mime = trans.app.datatypes_registry.get_mimetype_by_extension( data.extension.lower() )
                 trans.response.set_content_type(mime)
                 if tofile:
@@ -194,8 +194,8 @@ class RootController( BaseController ):
             if data:
                 child = data.get_child_by_designation( designation )
                 if child:
-                    roles = trans.get_current_user_roles()
-                    if trans.app.security_agent.can_access_dataset( roles, child ):
+                    current_user_roles = trans.get_current_user_roles()
+                    if trans.app.security_agent.can_access_dataset( current_user_roles, child ):
                         return self.display( trans, id=child.id, tofile=tofile, toext=toext )
                     else:
                         return "You are not privileged to access this dataset."
@@ -211,8 +211,8 @@ class RootController( BaseController ):
         if 'authz_method' in kwd:
             authz_method = kwd['authz_method']
         if data:
-            roles = trans.get_current_user_roles()
-            if authz_method == 'rbac' and trans.app.security_agent.can_access_dataset( roles, data ):
+            current_user_roles = trans.get_current_user_roles()
+            if authz_method == 'rbac' and trans.app.security_agent.can_access_dataset( current_user_roles, data ):
                 trans.response.set_content_type( data.get_mime() )
                 trans.log_event( "Formatted dataset id %s for display at %s" % ( str( id ), display_app ) )
                 return data.as_display_type( display_app, **kwd )
@@ -266,8 +266,8 @@ class RootController( BaseController ):
             return trans.show_error_message( "Problem retrieving dataset." )
         if id is not None and data.history.user is not None and data.history.user != trans.user:
             return trans.show_error_message( "This instance of a dataset (%s) in a history does not belong to you." % ( data.id ) )
-        roles = trans.get_current_user_roles()
-        if trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+        current_user_roles = trans.get_current_user_roles()
+        if trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
             if data.state == trans.model.Dataset.states.UPLOAD:
                 return trans.show_error_message( "Please wait until this dataset finishes uploading before attempting to edit its metadata." )
             params = util.Params( kwd, sanitize=False )
@@ -333,7 +333,7 @@ class RootController( BaseController ):
             elif params.update_roles_button:
                 if not trans.user:
                     return trans.show_error_message( "You must be logged in if you want to change permissions." )
-                if trans.app.security_agent.can_manage_dataset( roles, data.dataset ):
+                if trans.app.security_agent.can_manage_dataset( current_user_roles, data.dataset ):
                     permissions = {}
                     for k, v in trans.app.model.Dataset.permitted_actions.items():
                         in_roles = params.get( k + '_in', [] )
@@ -358,7 +358,10 @@ class RootController( BaseController ):
             ldatatypes = [ dtype_name for dtype_name, dtype_value in trans.app.datatypes_registry.datatypes_by_extension.iteritems() if dtype_value.allow_datatype_change ]
             ldatatypes.sort()
             trans.log_event( "Opened edit view on dataset %s" % str(id) )
-            return trans.fill_template( "/dataset/edit_attributes.mako", data=data, datatypes=ldatatypes )
+            return trans.fill_template( "/dataset/edit_attributes.mako",
+                                        data=data,
+                                        datatypes=ldatatypes,
+                                        current_user_roles=current_user_roles )
         else:
             return trans.show_error_message( "You do not have permission to edit this dataset's ( id: %s ) information." % str( id ) )
 

lib/galaxy/web/controllers/tool_runner.py

@@ -140,6 +140,18 @@ class ToolRunner( BaseController ):
         """
         Precreate datasets for asynchronous uploading.
         """
+        roles = kwd.get( 'roles', False )
+        if roles:
+            # The user associated the DATASET_ACCESS permission on the uploaded datasets with 1 or more roles.
+            # We need to ensure that the roles are legitimately derived from the roles associated with the LIBRARY_ACCESS
+            # permission if the library is not public ( this should always be the case since any ill-legitimate roles
+            # were filtered out of the roles displayed on the upload form.  In addition, we need to ensure that the user
+            # did not associated roles that would make the dataset in-accessible by everyone.
+            library_id = trans.app.security.decode_id( kwd.get( 'library_id', '' ) )
+            vars = dict( DATASET_ACCESS_in=roles )
+            permissions, in_roles, error, msg = trans.app.security_agent.check_library_dataset_access( trans, library_id, **vars )
+            if error:
+                return [ 'error', msg ]
         permissions = trans.app.security_agent.history_get_default_permissions( trans.history )
         def create_dataset( name ):
             ud = Bunch( name=name, file_type=None, dbkey=None )

templates/admin/center.mako

@@ -6,7 +6,7 @@
 
 <p>The menu on the left provides the following features</p>
 <ul>
-    <li><strong>Security</strong>
+    <li><strong>Security</strong> - see the <strong>Data Security and Data Libraries</strong> section below for details
         <p/>
         <ul>
             <li>
@@ -16,14 +16,15 @@
             <p/>
             <li>
                 <strong>Manage groups</strong> - provides a view of all groups along with the members of the group and the roles associated with
-                each group (both private and non-private roles).  Non-private roles include a link to a page that allows you to manage the users
-                and groups that are associated with the role.  The page also includes a view of the data library datasets that are associated with the
-                role and the permissions applied to each dataset.
+                each group (both private and non-private roles).  The group names include a link to a page that allows you to manage the users and 
+                roles that are associated with the group.
             </li>
             <p/>
             <li>
                 <strong>Manage roles</strong> - provides a view of all non-private roles along with the role type, and the users and groups that
-                are associated with the role.
+                are associated with the role.  The role names include a link to a page that allows you to manage the users and groups that are associated 
+                with the role.  The page also includes a view of the data library datasets that are associated with the role and the permissions applied 
+                to each dataset.
             </li>
         </ul>
     </li>
@@ -33,8 +34,21 @@
         <ul>
             <li>
                 <strong>Manage data libraries</strong> - Data libraries enable a Galaxy administrator to upload datasets into a data library.  Currently,
-                only administrators can create data libraries, but permission to perform the following functions on the data library can be granted to
-                users (a library item is one of: a data library, a library folder, a library dataset).
+                only administrators can create data libraries.
+                <p/>
+                When a data library is first created, it is considered "public" since it will be displayed in the "Data Libraries" view for any user, even 
+                those that are not logged in.  The Galaxy administrator can restrict access to a data library by associating roles with the data library's 
+                "access library" permission.  This permission will conservatively override the [dataset] "access" permission for the data library's contained 
+                datasets.
+                <p/>
+                For example, if a data library's "access library" permission is associated with Role1 and the data library contains "public" datasets, the 
+                data library will still only be displayed to those users that have Role1.  However, if the data library's "access library" permission is 
+                associated with both Role1 and Role2 and the data library contains datasets whose [dataset] "access" permission is associated with only Role1, 
+                then users that have Role2 will be able to access the library, but will not see those contained datasets whose [dataset] "access" permission 
+                is associated with only Role1.
+                <p/>
+                In addition to the "access library" permission, permission to perform the following functions on the data library (and it's contents) can 
+                be granted to users (a library item is one of: a data library, a library folder, a library dataset).
                 <p/>
                 <ul>
                     <li><strong>add library item</strong> - Role members can add library items to this data library or folder</li>
@@ -42,10 +56,10 @@
                     <li><strong>manage library permissions</strong> - Role members can manage permissions applied to this library item</li>
                 </ul>
                 <p/>
-                The default behavior is for no permissions to be applied to a data library item, but applied permissions are inherited downward, so it is
-                important to set desired permissions on a new data library when it is created.  When this is done, new folders and datasets added to the
-                data library will automatically inherit those permissions.  In the same way, permissions can be applied to a folder, which will be 
-                automatically inherited by all contained datasets and sub-folders.
+                The default behavior is for no permissions to be applied to a data library item, but applied permissions are inherited downward (with the exception
+                of the "access library" permission, which is only available on the data library itself).  Because of this, it is important to set desired permissions 
+                on a new data library when it is created.  When this is done, new folders and datasets added to the data library will automatically inherit those 
+                permissions.  In the same way, permissions can be applied to a folder, which will be automatically inherited by all contained datasets and sub-folders.
                 <p/>
                 The "Data Libraries" menu item allows users to access the datasets in a data library as long as they are not restricted from accessing them.
                 Importing a library dataset into a history will not make a copy of the dataset, but will be a "pointer" to the dataset on disk.  This
@@ -71,7 +85,20 @@
             </li>
         </ul>
     </li>
+    <p/>
+    <li><strong>Forms</strong>
+        <p/>To be completed
+    </li>
+    <p/>
+    <li><strong>Sequencing Requests</strong>
+        <p/>To be completed
+    </li>
+    <p/>
+    <li><strong>Cloud</strong>
+        <p/>To be completed
+    </li>
 </ul>
+<p/>
 <p><strong>Data Security and Data Libraries</strong></p>
 <p/>
 <strong>Security</strong> - Data security in Galaxy is a new feature, so familiarize yourself with the details which can be found 
@@ -120,9 +147,10 @@ in the way Galaxy has behaved in the past.
             <li>
                 <strong>access</strong> - users associated with the role can import this dataset into their history for analysis.
                 <p>
-                    If no roles with the "access" permission are associated with a dataset, the dataset is "public" and may be accessed by
-                    anyone.  Public data library datasets will be accessible to all users (as well as anyone not logged in during a Galaxy session)
-                    from the list of data libraries displayed when the "Data Libraries" menu item is selected.
+                    If no roles with the "access" permission are associated with a dataset, the dataset is "public" and may be accessed by anyone 
+                    that can access the data library in which it is contained.  See the <strong>Manage data libraries</strong> section above for 
+                    details.  Public datasets contained in public data libraries will be accessible to all users (as well as anyone not logged in 
+                    during a Galaxy session) from the list of data libraries displayed when the "Data Libraries" menu item is selected.
                 </p>
                 <p>
                     Associating a dataset with a role that includes the "access" permission restricts the set of users that can access it.  
@@ -139,11 +167,11 @@ in the way Galaxy has behaved in the past.
                 <p>
                     In order for a user to make a dataset private (i.e., only they can access it), they should associate the dataset with
                     their private role (the role identical to their Galaxy user name / email address).  Associating additional roles that
-                    include the "access" permission is not possible, since it would render the dataset inaccessible to everyone.  To make
-                    a dataset private to themselves and one or more other users, the user can create a new role and associate the dataset
-                    with that role, not their "private role".  Galaxy makes this easy by telling the user they are about to share a private
-                    dataset and giving them the option of doing so.  If they respond positively, the sharing role is automatically created
-                    for them.
+                    include the "access" permission is not possible, since it would render the dataset inaccessible to everyone.
+                <p>
+                    To make a dataset private to themselves and one or more other users, the user can create a new role and associate the dataset 
+                    with that role, not their "private role".  Galaxy makes this easy by telling the user they are about to share a private dataset 
+                    and giving them the option of doing so.  If they respond positively, the sharing role is automatically created for them.
                 </p>
                 <p>
                     Private data (data associated with roles that include the "access" permission) must be made public in order to be used
@@ -156,3 +184,4 @@ in the way Galaxy has behaved in the past.
         </ul>
     </li>
 </ul>
+<br/>

templates/base_panels.mako

@@ -95,16 +95,33 @@
                             }
                             // Make a synchronous request to create the datasets first
                             var async_datasets;
+                            var upload_error = false;
                             $.ajax( {
                                 async:      false,
                                 type:       "POST",
                                 url:        "${h.url_for(controller='/tool_runner', action='upload_async_create')}",
                                 data:       $(this).formSerialize(),
                                 dataType:   "json",
-                                success:    function( d, s ) { async_datasets = d.join() }
+                                success:    function(array_obj, status) {
+                                                if (array_obj.length > 0) {
+                                                    if (array_obj[0] == 'error') {
+                                                        upload_error = true;
+                                                        upload_form_error(array_obj[1]);
+                                                    } else {
+                                                        async_datasets = array_obj.join();
+                                                    }
+                                                } else {
+                                                    // ( gvk 1/22/10 ) FIXME: this block is never entered, so there may be a bug somewhere
+                                                    // I've done some debugging like checking to see if array_obj is undefined, but have not
+                                                    // tracked down the behavior that will result in this block being entered.  I believe the
+                                                    // intent was to have this block entered if the upload button is clicked on the upload
+                                                    // form but no file was selected.
+                                                    upload_error = true;
+                                                    upload_form_error( 'No data was entered in the upload form.  You may choose to upload a file, paste some data directly in the data box, or enter URL(s) to fetch data.' );
+                                                }
+                                            }
                             } );
-                            if (async_datasets == '') {
-                                upload_form_error( 'No data was entered in the upload form.  You may choose to upload a file, paste some data directly in the data box, or enter URL(s) to fetch from.' );
+                            if (upload_error == true) {
                                 return false;
                             } else {
                                 $(this).find("input[name=async_datasets]").val( async_datasets );

templates/dataset/edit_attributes.mako

@@ -6,7 +6,6 @@
 <%def name="stylesheets()">
     ${h.css( "base", "autocomplete_tagging" )}
 </%def>
-<% user_roles = trans.get_current_user_roles() %>
 
 <%def name="javascripts()">
     ${parent.javascripts()}
@@ -163,9 +162,9 @@
 </div>
 <p />
 
-%if trans.app.security_agent.can_manage_dataset( user_roles, data.dataset ):
+%if trans.app.security_agent.can_manage_dataset( current_user_roles, data.dataset ):
     <%namespace file="/dataset/security_common.mako" import="render_permission_form" />
-    ${render_permission_form( data.dataset, data.get_display_name(), h.url_for( controller='root', action='edit', id=data.id ), user_roles )}
+    ${render_permission_form( data.dataset, data.get_display_name(), h.url_for( controller='root', action='edit', id=data.id ), current_user_roles )}
 %elif trans.user:
     <div class="toolForm">
         <div class="toolFormTitle">View Permissions</div>

templates/library/common/browse_library.mako

@@ -16,12 +16,11 @@
 
 <%
     if cntrller in [ 'library', 'requests' ]:
-        roles = trans.get_current_user_roles()
-        can_add = trans.app.security_agent.can_add_library_item( roles, library )
+        can_add = trans.app.security_agent.can_add_library_item( current_user_roles, library )
         if can_add:
             info_association, inherited = library.get_info_association()
-        can_modify = trans.app.security_agent.can_modify_library_item( roles, library )
-        can_manage = trans.app.security_agent.can_manage_library_item( roles, library )
+        can_modify = trans.app.security_agent.can_modify_library_item( current_user_roles, library )
+        can_manage = trans.app.security_agent.can_manage_library_item( current_user_roles, library )
     elif cntrller in [ 'library_admin', 'requests_admin' ]:
         info_association, inherited = library.get_info_association()
 
@@ -162,8 +161,8 @@
         if ldda == library_dataset.library_dataset_dataset_association:
             current_version = True
             if cntrller in [ 'library', 'requests' ]:
-                can_modify_library_dataset = trans.app.security_agent.can_modify_library_item( roles, library_dataset )
-                can_manage_library_dataset = trans.app.security_agent.can_manage_library_item( roles, library_dataset )
+                can_modify_library_dataset = trans.app.security_agent.can_modify_library_item( current_user_roles, library_dataset )
+                can_manage_library_dataset = trans.app.security_agent.can_manage_library_item( current_user_roles, library_dataset )
         else:
             current_version = False
         if current_version and ldda.state not in ( 'ok', 'error', 'empty', 'deleted', 'discarded' ):
@@ -191,7 +190,7 @@
                         <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_display_info', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( folder.id ), id=trans.security.encode_id( ldda.id ) )}">View this dataset's information</a>
                     %endif
                     %if cntrller in [ 'library_admin', 'requests_admin' ] or can_manage_library_dataset:
-                        <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_permissions', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( folder.id ), id=trans.security.encode_id( ldda.id ), permissions=True )}">Edit this dataset's permissions</a>
+                        <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_permissions', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( folder.id ), id=trans.security.encode_id( ldda.id ) )}">Edit this dataset's permissions</a>
                     %endif
                     %if cntrller in [ 'library_admin', 'requests_admin' ] or can_modify_library_dataset:
                         <a class="action-button" href="${h.url_for( controller='library_common', action='upload_library_dataset', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( folder.id ), replace_id=trans.security.encode_id( library_dataset.id ) )}">Upload a new version of this dataset</a>
@@ -236,22 +235,22 @@
             return ""
         my_row = None
         if cntrller in [ 'library', 'requests' ]:
-            can_access, folder_ids = trans.app.security_agent.check_folder_contents( user, roles, folder )
+            can_access, folder_ids = trans.app.security_agent.check_folder_contents( trans.user, current_user_roles, folder )
             if not can_access:
                 can_show, folder_ids = \
-                    trans.app.security_agent.show_library_item( user,
-                                                                roles,
+                    trans.app.security_agent.show_library_item( trans.user,
+                                                                current_user_roles,
                                                                 folder,
                                                                 [ trans.app.security_agent.permitted_actions.LIBRARY_ADD,
                                                                   trans.app.security_agent.permitted_actions.LIBRARY_MODIFY,
                                                                   trans.app.security_agent.permitted_actions.LIBRARY_MANAGE ] )
                 if not can_show:
                     return ""
-            can_add = trans.app.security_agent.can_add_library_item( roles, folder )
+            can_add = trans.app.security_agent.can_add_library_item( current_user_roles, folder )
             if can_add:
                 info_association, inherited = folder.get_info_association( restrict=True )
-            can_modify = trans.app.security_agent.can_modify_library_item( roles, folder )
-            can_manage = trans.app.security_agent.can_manage_library_item( roles, folder )
+            can_modify = trans.app.security_agent.can_modify_library_item( current_user_roles, folder )
+            can_manage = trans.app.security_agent.can_manage_library_item( current_user_roles, folder )
         elif cntrller in [ 'library_admin', 'requests_admin' ]:
             info_association, inherited = folder.get_info_association( restrict=True )
     %>
@@ -311,7 +310,7 @@
         %for library_dataset in folder.active_library_datasets:
             <%
                 ldda = library_dataset.library_dataset_dataset_association
-                can_access = trans.app.security_agent.can_access_dataset( roles, ldda.dataset )
+                can_access = trans.app.security_agent.can_access_dataset( current_user_roles, ldda.dataset )
                 selected = created_ldda_ids and str( ldda.id ) in created_ldda_ids
             %>
             %if can_access:

templates/library/common/common.mako

@@ -15,14 +15,14 @@
             library_item_type = 'library_dataset_dataset_association'
             library_item_desc = 'library dataset'
         if cntrller == 'library':
-            roles = trans.get_current_user_roles()
+            current_user_roles = trans.get_current_user_roles()
     %>
     %if widgets:
         <p/>
         <div class="toolForm">
             <div class="toolFormTitle">Other information about ${library_item_desc} ${library_item.name}</div>
             <div class="toolFormBody">
-                %if editable and ( cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, library_item ) ):
+                %if editable and ( cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, library_item ) ):
                     <form name="edit_info" action="${h.url_for( controller='library_common', action='edit_template_info', cntrller=cntrller, library_id=library_id, response_action=response_action, num_widgets=len( widgets ) )}" method="post">
                         <input type="hidden" name="library_item_id" value="${trans.security.encode_id( library_item.id )}"/>
                         <input type="hidden" name="library_item_type" value="${library_item_type}"/>

templates/library/common/folder_info.mako

@@ -2,11 +2,6 @@
 <%namespace file="/message.mako" import="render_msg" />
 <%namespace file="/library/common/common.mako" import="render_template_info" />
 
-<%
-    if cntrller != 'library_admin':
-        roles = trans.get_current_user_roles()
-%>
-
 <br/><br/>
 <ul class="manage-table-actions">
     <li>
@@ -21,7 +16,7 @@
 <div class="toolForm">
     <div class="toolFormTitle">Edit folder name and description</div>
     <div class="toolFormBody">
-        %if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, folder ):
+        %if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, folder ):
             <form name="folder" action="${h.url_for( controller='library_common', action='folder_info', cntrller=cntrller, id=trans.security.encode_id( folder.id ), library_id=library_id )}" method="post" >
                 <div class="form-row">
                     <label>Name:</label>

templates/library/common/folder_permissions.mako

@@ -13,9 +13,7 @@
     ${render_msg( msg, messagetype )}
 %endif
 
-<% roles = trans.get_current_user_roles() %>
-
-%if cntrller=='library_admin' or trans.app.security_agent.can_manage_library_item( roles, folder ):
+%if cntrller in [ 'library_admin', 'requests_admin' ] or trans.app.security_agent.can_manage_library_item( current_user_roles, folder ):
     ## LIBRARY_ACCESS is a special permission that is set only at the library level.
     ${render_permission_form( folder, folder.name, h.url_for( controller='library_common', action='folder_permissions', cntrller=cntrller, id=trans.security.encode_id( folder.id ), library_id=library_id ), roles, do_not_render=[ 'LIBRARY_ACCESS' ] )}
 %endif

templates/library/common/ldda_edit_info.mako

@@ -20,11 +20,6 @@
     ${h.css( "autocomplete_tagging" )}
 </%def>
 
-<%
-    if cntrller == 'library':
-        roles = trans.get_current_user_roles()
-%>
-
 %if ldda == ldda.library_dataset.library_dataset_dataset_association:
     <b><i>This is the latest version of this library dataset</i></b>
 %else:
@@ -54,7 +49,7 @@
     </select>
 </%def>
 
-%if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda.library_dataset ):
+%if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda.library_dataset ):
     <div class="toolForm">
         <div class="toolFormTitle">Edit attributes of ${ldda.name}</div>
         <div class="toolFormBody">

templates/library/common/ldda_info.mako

@@ -8,8 +8,6 @@
         current_version = True
     else:
         current_version = False
-    if cntrller == 'library':
-        roles = trans.get_current_user_roles()
 %>
 
 %if current_version:
@@ -42,15 +40,15 @@
         %if not library.deleted and not ldda.library_dataset.folder.deleted and not ldda.deleted:
             <a id="dataset-${ldda.id}-popup" class="popup-arrow" style="display: none;">&#9660;</a>
             <div popupmenu="dataset-${ldda.id}-popup">
-                %if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda.library_dataset ):
+                %if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda.library_dataset ):
                     <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_edit_info', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( ldda.library_dataset.folder.id ), id=trans.security.encode_id( ldda.id ) )}">Edit this dataset's information</a>
                 %else:
                     <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_display_info', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( ldda.library_dataset.folder.id ), id=trans.security.encode_id( ldda.id ) )}">View this dataset's information</a>
                 %endif
-                %if cntrller=='library_admin' or trans.app.security_agent.can_manage_dataset( roles, ldda.dataset ) and trans.app.security_agent.can_manage_library_item( roles, ldda.library_dataset ):
+                %if cntrller=='library_admin' or trans.app.security_agent.can_manage_dataset( current_user_roles, ldda.dataset ) and trans.app.security_agent.can_manage_library_item( current_user_roles, ldda.library_dataset ):
                     <a class="action-button" href="${h.url_for( controller='library_common', action='ldda_permissions', library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( ldda.library_dataset.folder.id ), id=trans.security.encode_id( ldda.id ) )}">Edit this dataset's permissions</a>
                 %endif
-                %if current_version and ( cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, ldda.library_dataset ) ):
+                %if current_version and ( cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, ldda.library_dataset ) ):
                     <a class="action-button" href="${h.url_for( controller='library_common', action='upload_library_dataset', cntrller=cntrller, library_id=trans.security.encode_id( library.id ), folder_id=trans.security.encode_id( ldda.library_dataset.folder.id ), replace_id=trans.security.encode_id( ldda.library_dataset.id ) )}">Upload a new version of this dataset</a>
                 %endif
                 %if cntrller=='library' and ldda.has_data:

templates/library/common/ldda_permissions.mako

@@ -3,11 +3,6 @@
 <%namespace file="/dataset/security_common.mako" import="render_permission_form" />
 
 <%
-    from galaxy import util
-
-    roles = trans.sa_session.query( trans.app.model.Role ) \
-                            .filter( trans.app.model.Role.table.c.deleted==False ) \
-                            .order_by( trans.app.model.Role.table.c.name )
     if len( lddas ) > 1:
         name_str = '%d selected datasets' % len( lddas )
     else:

templates/library/common/library_dataset_info.mako

@@ -2,11 +2,6 @@
 <%namespace file="/message.mako" import="render_msg" />
 <%namespace file="/library/common/common.mako" import="render_template_info" />
 
-<%
-    if cntrller=='library':
-        roles = trans.get_current_user_roles()
-%>
-
 %if library_dataset == library_dataset.library_dataset_dataset_association.library_dataset:
     <b><i>This is the latest version of this library dataset</i></b>
 %else:
@@ -24,7 +19,7 @@
     ${render_msg( msg, messagetype )}
 %endif
 
-%if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( roles, library_dataset ):
+%if cntrller=='library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, library_dataset ):
     <div class="toolForm">
         <div class="toolFormTitle">Edit attributes of ${library_dataset.name}</div>
         <div class="toolFormBody">

templates/library/common/library_dataset_permissions.mako

@@ -2,11 +2,6 @@
 <%namespace file="/message.mako" import="render_msg" />
 <%namespace file="/dataset/security_common.mako" import="render_permission_form" />>
 
-<%
-    if cntrller == 'library':
-        roles = trans.get_current_user_roles()
-%>
-
 %if library_dataset == library_dataset.library_dataset_dataset_association.library_dataset:
     <b><i>This is the latest version of this library dataset</i></b>
 %else:
@@ -24,12 +19,7 @@
     ${render_msg( msg, messagetype )}
 %endif
 
-%if trans.app.security_agent.can_manage_library_item( user_roles, library_dataset ):
-    <%
-        roles = trans.sa_session.query( trans.app.model.Role ) \
-                                .filter( trans.app.model.Role.table.c.deleted==False ) \
-                                .order_by( trans.app.model.Role.table.c.name )
-    %>
+%if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( current_user_roles, library_dataset ):
     ## LIBRARY_ACCESS is a special permission that is set only at the library level.
     ${render_permission_form( library_dataset, library_dataset.name, h.url_for( controller='library_common', action='library_dataset_permissions', cntrller=cntrller, id=trans.security.encode_id( library_dataset.id ), library_id=library_id ), roles, do_not_render=[ 'LIBRARY_ACCESS' ] )}
 %endif

templates/library/common/library_info.mako

@@ -2,11 +2,6 @@
 <%namespace file="/message.mako" import="render_msg" />
 <%namespace file="/library/common/common.mako" import="render_template_info" />
 
-<%
-    if not trans.user_is_admin():
-        roles = trans.get_current_user_roles()
-%>
-
 <br/><br/>
 <ul class="manage-table-actions">
     <li>
@@ -18,7 +13,7 @@
     ${render_msg( msg, messagetype )}
 %endif
 
-%if cntrller == 'library_admin' or trans.app.security_agent.can_modify_library_item( roles, library ):
+%if cntrller == 'library_admin' or trans.app.security_agent.can_modify_library_item( current_user_roles, library ):
     <div class="toolForm">
         <div class="toolFormTitle">Change library name and description</div>
         <div class="toolFormBody">

templates/library/common/library_permissions.mako

@@ -2,11 +2,6 @@
 <%namespace file="/message.mako" import="render_msg" />
 <%namespace file="/dataset/security_common.mako" import="render_permission_form" />
 
-<%
-    if not trans.user_is_admin():
-        roles = trans.get_current_user_roles()
-%>
-
 <br/><br/>
 <ul class="manage-table-actions">
     <li>
@@ -18,11 +13,6 @@
     ${render_msg( msg, messagetype )}
 %endif
 
-%if trans.user_is_admin or trans.app.security_agent.can_manage_library_item( user_roles, library ):
-    <%
-        roles = trans.sa_session.query( trans.app.model.Role ) \
-                                .filter( trans.app.model.Role.table.c.deleted==False ) \
-                                .order_by( trans.app.model.Role.table.c.name )
-    %>
+%if cntrller == 'library_admin' or trans.app.security_agent.can_manage_library_item( current_user_roles, library ):
     ${render_permission_form( library, library.name, h.url_for( controller='library_common', action='library_permissions', cntrller=cntrller, id=trans.security.encode_id( library.id ), cntrller=cntrller ), roles )}
 %endif

templates/mobile/history/detail.mako

@@ -36,8 +36,8 @@
         
         <div class="secondary">
         ## Body for history items, extra info and actions, data "peek"
-        	<% roles = trans.get_current_user_roles() %>
-            %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+        	<% current_user_roles = trans.get_current_user_roles() %>
+            %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
                 <div>You do not have permission to view this dataset.</div>
             %elif data_state == "queued":
                 <div>Job is waiting to run</div>

templates/mobile/manage_library.mako

@@ -3,13 +3,13 @@
 <%namespace file="/dataset/security_common.mako" import="render_permission_form" />
 <%namespace file="/library/common/common.mako" import="render_template_info" />
 
-<% roles = trans.get_current_user_roles() %>
+<% current_user_roles = trans.get_current_user_roles() %>
 
 %if msg:
     ${render_msg( msg, messagetype )}
 %endif
 
-%if trans.app.security_agent.can_modify_library_item( roles, library ):
+%if trans.app.security_agent.can_modify_library_item( current_user_roles, library ):
     <div class="toolForm">
         <div class="toolFormTitle">Change library name and description</div>
         <div class="toolFormBody">
@@ -49,12 +49,8 @@
         </div>
     </div>
 %endif
-%if trans.app.security_agent.can_manage_library_item( roles, library ):
-    <%
-        roles = trans.sa_session.query( trans.app.model.Role ) \
-                                .filter( trans.app.model.Role.table.c.deleted==False ) \
-                                .order_by( trans.app.model.Role.table.c.name )
-    %>
+%if trans.app.security_agent.can_manage_library_item( current_user_roles, library ):
+    <% roles = library.get_legitimate_roles( trans ) %>
     ${render_permission_form( library, library.name, h.url_for( controller='library_common', cntrller='mobile', action='library_permissions', id=trans.security.encode_id( library.id ) ), roles )}
 %endif
 

templates/root/history_common.mako

@@ -7,9 +7,9 @@
     	    data_state = "queued"
     	else:
     	    data_state = data.state
-    	roles = trans.get_current_user_roles()
+    	current_user_roles = trans.get_current_user_roles()
     %>
-    %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+    %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
         <div class="historyItemWrapper historyItem historyItem-${data_state} historyItem-noPermission" id="historyItem-${data.id}">
     %else:
         <div class="historyItemWrapper historyItem historyItem-${data_state}" id="historyItem-${data.id}">
@@ -49,7 +49,7 @@
         ## Body for history items, extra info and actions, data "peek"
         
         <div id="info${data.id}" class="historyItemBody">
-            %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( roles, data.dataset ):
+            %if not trans.user_is_admin() and not trans.app.security_agent.can_access_dataset( current_user_roles, data.dataset ):
                 <div>You do not have permission to view this dataset.</div>
             %elif data_state == "upload":
                 <div>Dataset is uploading</div>

test/base/twilltestcase.py

@@ -1454,7 +1454,7 @@ class TwillTestCase( unittest.TestCase ):
         tc.fv( "1", "dbkey", dbkey )
         tc.fv( "1", "message", message.replace( '+', ' ' ) )
         for role_id in roles:
-            tc.fv( "1", "roles", role_id ) # form field 7 is the select list named out_groups, note the buttons...
+            tc.fv( "1", "roles", role_id )
         # Add template field contents, if any...
         if template_field_name1:
             tc.fv( "1", template_field_name1, template_field_contents1 )
@@ -1463,7 +1463,6 @@ class TwillTestCase( unittest.TestCase ):
             check_str = "Added 1 datasets to the library '%s' (each is selected)." % folder_name
         else:
             check_str = "Added 1 datasets to the folder '%s' (each is selected)." % folder_name
-        data = self.last_page()
         self.library_wait( library_id )
         self.home()
     def set_library_dataset_permissions( self, cntrller, library_id, folder_id, ldda_id, ldda_name, role_ids_str, permissions_in, permissions_out ):

test/functional/test_security_and_libraries.py

@@ -243,6 +243,14 @@ class TestSecurityAndLibraries( TwillTestCase ):
         global regular_user3
         regular_user3 = sa_session.query( galaxy.model.User ).filter( galaxy.model.User.table.c.email==email ).first()
         assert regular_user3 is not None, 'Problem retrieving user with email "%s" from the database' % email
+        global regular_user3_private_role
+        regular_user3_private_role = None
+        for role in regular_user3.all_roles():
+            if role.name == regular_user3.email and role.description == 'Private Role for %s' % regular_user3.email:
+                regular_user3_private_role = role
+                break
+        if not regular_user3_private_role:
+            raise AssertionError( "Private role not found for user '%s'" % regular_user3.email )
         # Make sure DefaultUserPermissions were created
         if not regular_user3.default_permissions:
             raise AssertionError( 'No DefaultUserPermissions were created for user %s when the admin created the account' % email )
@@ -510,16 +518,32 @@ class TestSecurityAndLibraries( TwillTestCase ):
                                                galaxy.model.Library.table.c.deleted==False ) ) \
                                 .first()
         assert library_one is not None, 'Problem retrieving library named "%s" from the database' % name
-        # Set permissions on the library, sort for later testing
+        # Make sure library_one is public
+        assert 'access library' not in [ a.action for a in library_one.actions ], 'Library %s is not public when first created' % library_one.name
+        # Set permissions on the library, sort for later testing.
         permissions_in = [ k for k, v in galaxy.model.Library.permitted_actions.items() ]
         permissions_out = []
-        # Role one members are: admin_user, regular_user1, regular_user3.  Each of these users will be permitted to
-        # LIBRARY_ADD, LIBRARY_MODIFY, LIBRARY_MANAGE for library items.
+        # Role one members are: admin_user, regular_user1, regular_user3.  Each of these users will be permitted for
+        # LIBRARY_ACCESS, LIBRARY_ADD, LIBRARY_MODIFY, LIBRARY_MANAGE on this library and it's contents.
         self.library_permissions( self.security.encode_id( library_one.id ),
                                   library_one.name,
                                   str( role_one.id ),
                                   permissions_in,
-                                  permissions_out )                                            
+                                  permissions_out )
+        # Make sure the library is accessible by admin_user
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( library_one.name )
+        # Make sure the library is not accessible by regular_user2 since regular_user2 does not have Role1.
+        self.logout()
+        self.login( email=regular_user2.email )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        try:
+            self.check_page_for_string( library_one.name )
+            raise AssertionError, 'Library %s is accessible by %s when it should be restricted' % ( library_one.name, regular_user2.email )
+        except:
+            pass
+        self.logout()
+        self.login( email=admin_user.email )
         # Rename the library
         rename = "Library One's been Renamed"
         redescription = "This is Library One's Re-described"
@@ -593,7 +617,11 @@ class TestSecurityAndLibraries( TwillTestCase ):
     def test_085_add_public_dataset_to_root_folder( self ):
         """Testing adding a public dataset to the root folder, making sure library template is inherited"""
         # Logged in as admin_user
-        actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        #actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        actions = []
+        for k, v in galaxy.model.Library.permitted_actions.items():
+            if k != 'LIBRARY_ACCESS':
+                actions.append( v.action )
         actions.sort()
         message = 'Testing adding a public dataset to the root folder'
         # The form_one template should be inherited to the library dataset upload form.
@@ -625,8 +653,8 @@ class TestSecurityAndLibraries( TwillTestCase ):
                                      .all()
         ldda_permissions = [ lddap_obj.action for lddap_obj in ldda_permissions ]
         ldda_permissions.sort()
-        assert actions == ldda_permissions, "Permissions for ldda id %s not correctly inherited from library %s" \
-                            % ( ldda_one.id, library_one.name )
+        assert actions == ldda_permissions, "Permissions (%s) for ldda id %s not correctly inherited from library %s which has permissions (%s)." \
+                            % ( str( ldda_permissions ), ldda_one.id, library_one.name, str( actions ) )
         # Make sure DatasetPermissions are correct - default is 'manage permissions'
         if len( ldda_one.dataset.actions ) > 1:
             raise AssertionError( '%d DatasetPermissionss were created for dataset id %d when it was created ( should have been 1 )' \
@@ -640,15 +668,56 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.visit_url( "%s/library_common/ldda_edit_info?cntrller=library_admin&library_id=%s&folder_id=%s&id=%s" % \
                         ( self.url, self.security.encode_id( library_one.id ), self.security.encode_id( library_one.root_folder.id ), self.security.encode_id( ldda_one.id ) ) )
         self.check_page_for_string( template_contents )
-        # Make sure other users can access the dataset from the Libraries view
+        # Make sure only users that have Role1 can access library_one even though it now contains a public dataset
         self.logout()
+        # regular_user2 does not have Role1
         self.login( email=regular_user2.email )
-        self.home()
-        self.visit_url( '%s/library_common/browse_library?cntrller=library_admin&id=%s' % ( self.url, self.security.encode_id( library_one.id ) ) )
-        self.check_page_for_string( "1.bed" )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        try:
+            self.check_page_for_string( library_one.name )
+            raise AssertionError, 'Library %s is accessible by %s when it should be restricted' % ( library_one.name, regular_user2.email )
+        except:
+            pass
         self.logout()
         self.login( email=admin_user.email )
         self.home()
+        # Make sure only legitimate roles are displayed on the permissions form for the public dataset.
+        # Legitimate roles are as follows:
+        # 'Role One' since the LIBRARY_ACCESS permission is associated with Role One.  # Role one members are: admin_user, regular_user1, regular_user3.
+        # 'test@bx.psu.edu' ( admin_user's private role ) since admin_user has Role One
+        # 'Role Two' since admin_user has Role Two
+        # 'Role Three' since admin_user has Role Three
+        # 'test1@bx.psu.edu' ( regular_user1's private role ) since regular_user1 has Role One
+        # 'test3@bx.psu.edu' ( regular_user3's private role ) since regular_user3 has Role One
+        library_id = self.security.encode_id( library_one.id )
+        folder_id = self.security.encode_id( library_one.root_folder.id )
+        id = self.security.encode_id( ldda_one.id )
+        self.visit_url( '%s/library_common/ldda_permissions?cntrller=library_admin&library_id=%s&folder_id=%s&id=%s' % \
+                        ( self.url, library_id, folder_id, id ) )
+        self.check_page_for_string( role_one.name )
+        self.check_page_for_string( admin_user_private_role.name )
+        self.check_page_for_string( role_two.name )
+        self.check_page_for_string( role_three.name )
+        self.check_page_for_string( regular_user1_private_role.name )
+        self.check_page_for_string( regular_user3_private_role.name )
+        try:
+            self.check_page_for_string( regular_user2_private_role.name )
+            raise AssertionError, "Role (%s) incorrectly displayed on permission form for library dataset (%s)." % \
+                ( regular_user2_private_role.name, ldda_one.name )
+        except:
+            pass
+        # Even though the dataset is public, only users with Role1 should be able to see the library_one
+        # from the Data Libraries view.  Role one members are: admin_user, regular_user1, regular_user3.
+        self.logout()
+        self.login( email=regular_user1.email )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( library_one.name )
+        self.logout()
+        self.login( email=regular_user2.email )
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( "You are not authorized to access any libraries" )
+        self.logout()
+        self.login( email=admin_user.email )
     def test_090_add_new_folder_to_root_folder( self ):
         """Testing adding a folder to a library root folder"""
         # logged in as admin_user
@@ -696,7 +765,11 @@ class TestSecurityAndLibraries( TwillTestCase ):
         # logged in as admin_user
         name = "Folder One's Subfolder"
         description = "This is the Folder One's subfolder"
-        self.add_folder( 'library_admin', self.security.encode_id( library_one.id ), self.security.encode_id( folder_one.id ), name=name, description=description )
+        self.add_folder( 'library_admin',
+                         self.security.encode_id( library_one.id ),
+                         self.security.encode_id( folder_one.id ),
+                         name=name,
+                         description=description )
         global subfolder_one
         subfolder_one = sa_session.query( galaxy.model.LibraryFolder ) \
                                   .filter( and_( galaxy.model.LibraryFolder.table.c.parent_id==folder_one.id,
@@ -728,6 +801,18 @@ class TestSecurityAndLibraries( TwillTestCase ):
         tc.submit( 'edit_info_button' )
         self.check_page_for_string( 'The information has been updated.' )
         self.check_page_for_string( template_contents )
+        # Make sure no roles except Role1 are displayed on the permissions form for the sub-folder.
+        library_id = self.security.encode_id( library_one.id )
+        id = self.security.encode_id( subfolder_one.id )
+        self.visit_url( '%s/library_common/folder_permissions?cntrller=library_admin&id=%s&library_id=%s' % \
+                        ( self.url, id, library_id ) )
+        self.check_page_for_string( role_one.name )
+        try:
+            self.check_page_for_string( role_two.name )
+            raise AssertionError, "Role (%s) incorrectly displayed on permission form for library dataset (%s)." % \
+                ( role_two.name, ldda_one.name )
+        except:
+            pass
     def test_100_add_2nd_new_folder_to_root_folder( self ):
         """Testing adding a 2nd folder to a library root folder"""
         # logged in as admin_user
@@ -762,7 +847,11 @@ class TestSecurityAndLibraries( TwillTestCase ):
     def test_105_add_public_dataset_to_root_folders_2nd_subfolder( self ):
         """Testing adding a public dataset to the root folder's 2nd sub-folder"""
         # Logged in as admin_user
-        actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        #actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        actions = []
+        for k, v in galaxy.model.Library.permitted_actions.items():
+            if k != 'LIBRARY_ACCESS':
+                actions.append( v.action )
         actions.sort()
         message = "Testing adding a public dataset to the folder named %s" % folder_two.name
         # The form_one template should be inherited to the library dataset upload form.
@@ -796,7 +885,11 @@ class TestSecurityAndLibraries( TwillTestCase ):
     def test_110_add_2nd_public_dataset_to_root_folders_2nd_subfolder( self ):
         """Testing adding a 2nd public dataset to the root folder's 2nd sub-folder"""
         # Logged in as admin_user
-        actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        #actions = [ v.action for k, v in galaxy.model.Library.permitted_actions.items() ]
+        actions = []
+        for k, v in galaxy.model.Library.permitted_actions.items():
+            if k != 'LIBRARY_ACCESS':
+                actions.append( v.action )
         actions.sort()
         message = "Testing adding a 2nd public dataset to the folder named %s" % folder_two.name
         # The form_one template should be inherited to the library dataset upload form.
@@ -830,6 +923,9 @@ class TestSecurityAndLibraries( TwillTestCase ):
     def test_115_add_dataset_with_private_role_restriction_to_folder( self ):
         """Testing adding a dataset with a private role restriction to a folder"""
         # Logged in as admin_user
+        #
+        # Keep in mind that # LIBRARY_ACCESS = "Role One" on the whole library
+        #
         # Add a dataset restricted by the following:
         # DATASET_MANAGE_PERMISSIONS = "test@bx.psu.edu" via DefaultUserPermissions
         # DATASET_ACCESS = "regular_user1" private role via this test method
@@ -877,7 +973,20 @@ class TestSecurityAndLibraries( TwillTestCase ):
     def test_120_accessing_dataset_with_private_role_restriction( self ):
         """Testing accessing a dataset with a private role restriction"""
         # Logged in as admin_user
-        # admin_user should not be able to see 2.bed from the analysis view's access libraries
+        #
+        # Keep in mind that # LIBRARY_ACCESS = "Role One" on the whole library
+        # Role one members are: admin_user, regular_user1, regular_user3.  Each of these users will be permitted for
+        # LIBRARY_ACCESS, LIBRARY_ADD, LIBRARY_MODIFY, LIBRARY_MANAGE on this library and it's contents.
+        #
+        # Legitimate roles displayed on the permission form are as follows:
+        # 'Role One' since the LIBRARY_ACCESS permission is associated with Role One.  # Role one members are: admin_user, regular_user1, regular_user3.
+        # 'test@bx.psu.edu' ( admin_user's private role ) since admin_user has Role One
+        # 'Role Two' since admin_user has Role Two
+        # 'Role Three' since admin_user has Role Three
+        # 'test1@bx.psu.edu' ( regular_user1's private role ) since regular_user1 has Role One
+        # 'test3@bx.psu.edu' ( regular_user3's private role ) since regular_user3 has Role One
+        #
+        # admin_user should not be able to see 4.bed from the analysis view's access libraries
         self.home()
         self.visit_url( '%s/library_common/browse_library?cntrller=library&id=%s' % ( self.url, self.security.encode_id( library_one.id ) ) )
         try:
@@ -901,23 +1010,15 @@ class TestSecurityAndLibraries( TwillTestCase ):
         self.check_page_for_string( folder_one.name )
         self.check_page_for_string( '4.bed' )
         self.logout()
-        # regular_user2 should not be able to see 1.bed from the analysis view's access librarys
+        # regular_user2 should not be to see the library since they do not have 
+        # Role One which is associated with the LIBRARY_ACCESS permission
         self.login( email='test2@bx.psu.edu' )
-        try:
-            self.check_page_for_string( folder_one.name )
-            raise AssertionError( '%s can see library folder %s when it contains only datasets restricted by role %s' \
-                                  % ( regular_user2.email, folder_one.name, regular_user1_private_role.description ) )
-        except:
-            pass
-        try:
-            self.check_page_for_string( '4.bed' )
-            raise AssertionError( '%s can see dataset 4.bed in library folder %s when it was restricted by role %s' \
-                                  % ( regular_user2.email, folder_one.name, regular_user1_private_role.description ) )
-        except:
-            pass
+        self.visit_url( '%s/library/browse_libraries' % self.url )
+        self.check_page_for_string( "You are not authorized to access any libraries" )
         self.logout()
-        # regular_user3 should not be able to see 2.bed from the analysis view's access librarys
+        # regular_user3 should not be able to see 4.bed from the analysis view's access librarys
         self.login( email='test3@bx.psu.edu' )
+        self.visit_url( '%s/library_common/browse_library?cntrller=library&id=%s' % ( self.url, self.security.encode_id( library_one.id ) ) )
         try:
             self.check_page_for_string( folder_one.name )
             raise AssertionError( '%s can see library folder %s when it contains only datasets restricted by role %s' \
@@ -929,16 +1030,18 @@ class TestSecurityAndLibraries( TwillTestCase ):
             raise AssertionError( '%s can see dataset 4.bed in library folder %s when it was restricted by role %s' \
                                   % ( regular_user3.email, folder_one.name, regular_user1_private_role.description ) )
         except:
-            pass # This is the behavior we want
+            pass
         self.logout()
         self.login( email=admin_user.email )
         self.home()
     def test_125_change_dataset_access_permission( self ):
         """Testing changing the access permission on a dataset with a private role restriction"""
         # Logged in as admin_user
-        # We need admin_user to be able to access 2.bed
-        permissions_in = [ k for k, v in galaxy.model.Dataset.permitted_actions.items() ] + \
-                         [ k for k, v in galaxy.model.Library.permitted_actions.items() ]
+        # We need admin_user to be able to access 4.bed
+        permissions_in = [ k for k, v in galaxy.model.Dataset.permitted_actions.items() ]
+        for k, v in galaxy.model.Library.permitted_actions.items():
+            if k != 'LIBRARY_ACCESS':
+                permissions_in.append( k )
         permissions_out = []
         role_ids_str = '%s,%s' % ( str( role_one.id ), str( admin_user_private_role.id ) )
         self.set_library_dataset_permissions( 'library_admin',
@@ -960,6 +1063,22 @@ class TestSecurityAndLibraries( TwillTestCase ):
         # Add a dataset restricted by role_two, which is currently associated as follows:
         # groups: group_two
         # users: test@bx.psu.edu, test1@bx.psu.edu via group_two
+        #
+        # We first need to make library_one public
+        permissions_in = []
+        for k, v in galaxy.model.Library.permitted_actions.items():
+            if k != 'LIBRARY_ACCESS':
+                permissions_in.append( k )
+        permissions_out = []
+        # Role one members are: admin_user, regular_user1, regular_user3.  Each of these users will now be permitted for
+        # LIBRARY_ADD, LIBRARY_MODIFY, LIBRARY_MANAGE on this library and it's contents.  The library will be public from
+        # this point on.
+        self.library_permissions( self.security.encode_id( library_one.id ),
+                                  library_one.name,
+                                  str( role_one.id ),
+                                  permissions_in,
+                                  permissions_out )
+        sa_session.refresh( library_one )
         message = 'Testing adding a dataset with a role that is associated with a group and users'
         # The form_one template should be inherited to the library dataset upload form.
         template_contents = "%s contents for %s 5.bed" % ( form_one_field_label, folder_one.name )

test/functional/test_toolbox.py

@@ -69,6 +69,7 @@ class ToolTestCase( TwillTestCase ):
             name, file, sort = output_tuple
             # Get the correct hid
             elem = data_list[ elem_index ]
+            self.assertTrue( elem )
             elem_hid = elem.get( 'hid' )
             elem_index += 1
             self.verify_dataset_correctness( file, hid=elem_hid, maxseconds=testdef.maxseconds, sort=sort )