details: http://www.bx.psu.edu/hg/galaxy/rev/a2d69372d699 changeset: 2657:a2d69372d699 user: Greg Von Kuster <greg@bx.psu.edu> date: Tue Sep 01 14:20:49 2009 -0400 description: More performance improvements in checking security on library folders from the Data Libraries view. Fix a bug in the Data Libraries view where empty folders were rendered when they contained no accessible datasets. Clean up requests code that determined the libraries that should be rendered in the select list on the request form. 6 file(s) affected in this change: lib/galaxy/security/__init__.py lib/galaxy/web/controllers/library.py lib/galaxy/web/controllers/requests.py lib/galaxy/web/controllers/requests_admin.py templates/library/browse_libraries.mako templates/library/browse_library.mako diffs (521 lines): diff -r d7a780065c91 -r a2d69372d699 lib/galaxy/security/__init__.py --- a/lib/galaxy/security/__init__.py Sat Aug 29 12:08:35 2009 -0400 +++ b/lib/galaxy/security/__init__.py Tue Sep 01 14:20:49 2009 -0400 @@ -101,7 +101,7 @@ if action == self.permitted_actions.DATASET_ACCESS and action.action not in [ dp.action for dp in dataset.actions ]: # anons only get access, and only if there are no roles required for the access action # Other actions (or if the dataset has roles defined for the access action) fall through - # to the false below + # to the false below for anons return True elif action.action not in [ dp.action for dp in dataset.actions ]: if action.model == 'restrict': @@ -119,28 +119,34 @@ # The user is missing at least one required role return False def allow_library_item_action( self, user, roles, action, library_item ): + """ + Method for checking a permission for the current user to perform a + specific library action on a library item, which must be one of: + Library, LibraryFolder, LibraryDataset, LibraryDatasetDatasetAssociation + """ if user is None: # All permissions are granted, so non-users cannot have permissions return False - if action.model == 'grant': - # Check to see if user has access to any of the roles - allowed_role_assocs = [] - for item_class, permission_class in self.library_item_assocs: - if isinstance( library_item, item_class ): - if permission_class == self.model.LibraryPermissions: - allowed_role_assocs = permission_class.filter_by( action=action.action, library_id=library_item.id ).all() - elif permission_class == self.model.LibraryFolderPermissions: - allowed_role_assocs = permission_class.filter_by( action=action.action, library_folder_id=library_item.id ).all() - elif permission_class == self.model.LibraryDatasetPermissions: - allowed_role_assocs = permission_class.filter_by( action=action.action, library_dataset_id=library_item.id ).all() - elif permission_class == self.model.LibraryDatasetDatasetAssociationPermissions: - allowed_role_assocs = permission_class.filter_by( action=action.action, library_dataset_dataset_association_id=library_item.id ).all() - for allowed_role_assoc in allowed_role_assocs: - if allowed_role_assoc.role in roles: - return True - return False - else: - raise 'Unimplemented model (%s) specified for action (%s)' % ( action.model, action.action ) + # Check to see if user has access to any of the roles + allowed_role_assocs = [] + for item_class, permission_class in self.library_item_assocs: + if isinstance( library_item, item_class ): + if permission_class == self.model.LibraryPermissions: + allowed_role_assocs = permission_class.filter_by( action=action.action, + library=library_item ).all() + elif permission_class == self.model.LibraryFolderPermissions: + allowed_role_assocs = permission_class.filter_by( action=action.action, + folder=library_item ).all() + elif permission_class == self.model.LibraryDatasetPermissions: + allowed_role_assocs = permission_class.filter_by( action=action.action, + library_dataset=library_item ).all() + elif permission_class == self.model.LibraryDatasetDatasetAssociationPermissions: + allowed_role_assocs = permission_class.filter_by( action=action.action, + library_dataset_dataset_association=library_item ).all() + for allowed_role_assoc in allowed_role_assocs: + if allowed_role_assoc.role in roles: + return True + return False def get_item_action( self, action, item ): # item must be one of: Dataset, Library, LibraryFolder, LibraryDataset, LibraryDatasetDatasetAssociation for permission in item.actions: @@ -291,7 +297,10 @@ permissions[ action ] = [ dhp.role ] return permissions def set_all_dataset_permissions( self, dataset, permissions={} ): - # Set new permissions on a dataset, eliminating all current permissions + """ + Set new permissions on a dataset, eliminating all current permissions + 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() @@ -305,7 +314,10 @@ for dp in [ self.model.DatasetPermissions( action, dataset, role ) for role in roles ]: dp.flush() def set_dataset_permission( self, dataset, permission={} ): - # Set a specific permission on a dataset, leaving all other current permissions on the dataset alone + """ + 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(): @@ -331,8 +343,11 @@ dp.delete() dp.flush() def get_dataset_permissions( self, dataset ): - if not isinstance( dataset, self.model.Dataset ): - dataset = dataset.dataset + """ + Return a dictionary containing the actions and associated roles on dataset. + The dictionary looks like: { Action : [ Role, Role ] } + dataset must be an instance of Dataset() + """ permissions = {} for dp in dataset.actions: action = self.get_action( dp.action ) @@ -423,18 +438,29 @@ else: raise 'Invalid class (%s) specified for target_library_item (%s)' % \ ( target_library_item.__class__, target_library_item.__class__.__name__ ) - def show_library_item( self, user, roles, library_item ): - if self.allow_action( user, roles, self.permitted_actions.LIBRARY_MODIFY, library_item=library_item ) or \ - self.allow_action( user, roles, self.permitted_actions.LIBRARY_MANAGE, library_item=library_item ) or \ - self.allow_action( user, roles, self.permitted_actions.LIBRARY_ADD, library_item=library_item ): - return True + def show_library_item( self, user, roles, library_item, actions_to_check, hidden_folder_ids='' ): + """ + This method must be sent an instance of Library() or LibraryFolder(). Recursive execution produces a + comma-separated string of folder ids whose folders do NOT meet the criteria for showing. Along with + the string, True is returned if the current user has permission to perform any 1 of actions_to_check + on library_item. Otherwise, cycle through all sub-folders in library_item until one is found that meets + this criteria, if it exists. + """ + for action in actions_to_check: + if self.allow_library_item_action( user, roles, action, library_item ): + return True, hidden_folder_ids if isinstance( library_item, self.model.Library ): - return self.show_library_item( user, roles, library_item.root_folder ) - elif isinstance( library_item, self.model.LibraryFolder ): - for folder in library_item.folders: - if self.show_library_item( user, roles, folder ): - return True - return False + return self.show_library_item( user, roles, library_item.root_folder, actions_to_check, hidden_folder_ids=hidden_folder_ids ) + if isinstance( library_item, self.model.LibraryFolder ): + for folder in library_item.active_folders: + can_show, hidden_folder_ids = self.show_library_item( user, roles, folder, actions_to_check, hidden_folder_ids=hidden_folder_ids ) + if can_show: + return True, hidden_folder_ids + if hidden_folder_ids: + hidden_folder_ids = '%s,%d' % ( hidden_folder_ids, folder.id ) + else: + hidden_folder_ids = '%d' % folder.id + return False, hidden_folder_ids def set_entity_user_associations( self, users=[], roles=[], groups=[], delete_existing_assocs=True ): for user in users: if delete_existing_assocs: @@ -482,10 +508,12 @@ if 'role' in kwd: return self.model.GroupRoleAssociation.filter_by( role_id = kwd['role'].id, group_id = kwd['group'].id ).first() raise 'No valid method of associating provided components: %s' % kwd - def check_folder_contents( self, user, roles, folder ): + def check_folder_contents( self, user, roles, folder, hidden_folder_ids='' ): """ - Return true if there are any datasets under 'folder' that are public or that the - user has access permission on. + This method must always be sent an instance of LibraryFolder(). Recursive execution produces a + comma-separated string of folder ids whose folders do NOT meet the criteria for showing. Along + with the string, True is returned if the current user has permission to access folder. Otherwise, + cycle through all sub-folders in folder until one is found that meets this criteria, if it exists. """ action = self.permitted_actions.DATASET_ACCESS.action lddas = self.sa_session.query( self.model.LibraryDatasetDatasetAssociation ) \ @@ -498,14 +526,19 @@ ldda_access = self.get_item_action( action, ldda.dataset ) if ldda_access is None: # Dataset is public - return True + return True, hidden_folder_ids if ldda_access.role in roles: # The current user has access permission on the dataset - return True + return True, hidden_folder_ids for sub_folder in folder.active_folders: - if self.check_folder_contents( user, roles, sub_folder ): - return True - return False + can_access, hidden_folder_ids = self.check_folder_contents( user, roles, sub_folder, hidden_folder_ids=hidden_folder_ids ) + if can_access: + return True, hidden_folder_ids + if hidden_folder_ids: + hidden_folder_ids = '%s,%d' % ( hidden_folder_ids, sub_folder.id ) + else: + hidden_folder_ids = '%d' % sub_folder.id + return False, hidden_folder_ids class HostAgent( RBACAgent ): """ diff -r d7a780065c91 -r a2d69372d699 lib/galaxy/web/controllers/library.py --- a/lib/galaxy/web/controllers/library.py Sat Aug 29 12:08:35 2009 -0400 +++ b/lib/galaxy/web/controllers/library.py Tue Sep 01 14:20:49 2009 -0400 @@ -65,23 +65,24 @@ user, roles = trans.get_user_and_roles() all_libraries = trans.app.model.Library.filter( trans.app.model.Library.table.c.deleted==False ) \ .order_by( trans.app.model.Library.name ).all() - authorized_libraries = [] + library_actions = [ trans.app.security_agent.permitted_actions.LIBRARY_ADD, + trans.app.security_agent.permitted_actions.LIBRARY_MODIFY, + trans.app.security_agent.permitted_actions.LIBRARY_MANAGE ] + # The authorized_libraries dictionary looks like: { library : '1,2' }, library : '3' } + # Its keys are the libraries that should be displayed for the current user and whose values are a + # string of comma-separated folder ids, of the associated folders the should NOT be displayed. + # The folders that should not be displayed may not be a complete list, but it is ultimately passed + # to the browse_library() method and the browse_library.mako template to keep from re-checking the + # same folders when the library is rendered. + authorized_libraries = {} for library in all_libraries: - if trans.app.security_agent.allow_action( user, - roles, - trans.app.security_agent.permitted_actions.LIBRARY_ADD, - library_item=library ) or \ - trans.app.security_agent.allow_action( user, - roles, - trans.app.security_agent.permitted_actions.LIBRARY_MODIFY, - library_item=library ) or \ - trans.app.security_agent.allow_action( user, - roles, - trans.app.security_agent.permitted_actions.LIBRARY_MANAGE, - library_item=library ) or \ - trans.app.security_agent.check_folder_contents( user, roles, library.root_folder ) or \ - trans.app.security_agent.show_library_item( user, roles, library ): - authorized_libraries.append( library ) + can_access, hidden_folder_ids = trans.app.security_agent.check_folder_contents( user, roles, library.root_folder ) + if can_access: + authorized_libraries[ library ] = hidden_folder_ids + else: + can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( user, roles, library, library_actions ) + if can_show: + authorized_libraries[ library ] = hidden_folder_ids return trans.fill_template( '/library/browse_libraries.mako', libraries=authorized_libraries, default_action=params.get( 'default_action', None ), @@ -94,6 +95,7 @@ messagetype = params.get( 'messagetype', 'done' ) id = params.get( 'id', None ) if not id: + # To handle bots msg = "You must specify a library id." return trans.response.send_redirect( web.url_for( controller='library', action='browse_libraries', @@ -102,6 +104,7 @@ messagetype='error' ) ) library = library=trans.app.model.Library.get( id ) if not library: + # To handle bots msg = "Invalid library id ( %s )." return trans.response.send_redirect( web.url_for( controller='library', action='browse_libraries', @@ -109,9 +112,11 @@ msg=util.sanitize_text( msg ), messagetype='error' ) ) created_ldda_ids = params.get( 'created_ldda_ids', '' ) + hidden_folder_ids = util.listify( util.restore_text( params.get( 'hidden_folder_ids', '' ) ) ) return trans.fill_template( '/library/browse_library.mako', library=trans.app.model.Library.get( id ), created_ldda_ids=created_ldda_ids, + hidden_folder_ids=hidden_folder_ids, default_action=params.get( 'default_action', None ), comptypes=comptypes, msg=msg, @@ -1197,25 +1202,3 @@ edit_info=True, msg=util.sanitize_text( msg ), messagetype='done' ) ) - -def get_authorized_libs( trans, user ): - # TODO: this is a mis-named function - the name should reflect the authorization policy - # If user is not authenticated, this method should not even be called. Also, it looks - # like all that is using this is the new request stuff, so it should be placed there. - if not user: - return [] - roles = user.all_roles() - all_libraries = trans.app.model.Library.filter( trans.app.model.Library.table.c.deleted == False ) \ - .order_by( trans.app.model.Library.name ).all() - authorized_libraries = [] - for library in all_libraries: - if trans.app.security_agent.allow_action( user, - roles, - trans.app.security_agent.permitted_actions.LIBRARY_ADD, - library_item=library ) \ - or trans.app.security_agent.allow_action( user, - roles, - trans.app.security_agent.permitted_actions.LIBRARY_MODIFY, - library_item=library ): - authorized_libraries.append( library ) - return authorized_libraries diff -r d7a780065c91 -r a2d69372d699 lib/galaxy/web/controllers/requests.py --- a/lib/galaxy/web/controllers/requests.py Sat Aug 29 12:08:35 2009 -0400 +++ b/lib/galaxy/web/controllers/requests.py Tue Sep 01 14:20:49 2009 -0400 @@ -9,8 +9,6 @@ from datetime import datetime, timedelta from cgi import escape, FieldStorage from galaxy.web.controllers.forms import get_form_widgets -from galaxy.web.controllers.library import get_authorized_libs - log = logging.getLogger( __name__ ) @@ -432,8 +430,6 @@ return self.__show_request_form(trans, **kwd) elif params.get('refresh', False) == 'true': return self.__show_request_form(trans, **kwd) - - def __show_request_form(self, trans, **kwd): params = util.Params( kwd ) msg = util.restore_text( params.get( 'msg', '' ) ) @@ -460,7 +456,25 @@ helptext='(Optional)')) # libraries selectbox - libraries = get_authorized_libs(trans, trans.user) + all_libraries = trans.app.model.Library.filter( trans.app.model.Library.table.c.deleted == False ) \ + .order_by( trans.app.model.Library.name ).all() + user, roles = trans.get_user_and_roles() + actions_to_check = [ trans.app.security_agent.permitted_actions.LIBRARY_ADD ] + # The libraries dictionary looks like: { library : '1,2' }, library : '3' } + # Its keys are the libraries that should be displayed for the current user and whose values are a + # string of comma-separated folder ids, of the associated folders the should NOT be displayed. + # The folders that should not be displayed may not be a complete list, but it is ultimately passed + # to the calling method to keep from re-checking the same folders when the library / folder + # select lists are rendered. + # + # TODO: RC, when you add the folders select list to your request form, take advantage of the hidden_folder_ids + # so that you do not need to check those same folders yet again when populating the select list. + # + libraries = {} + for library in all_libraries: + can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( user, roles, library, actions_to_check ) + if can_show: + libraries[ library ] = hidden_folder_ids libui = self.__library_ui(libraries, **kwd) widgets = widgets + libui widgets = widgets + get_form_widgets(trans, request_type.request_form, contents=[], **kwd) @@ -470,12 +484,10 @@ widgets=widgets, msg=msg, messagetype=messagetype) - def __library_ui(self, libraries, request=None, **kwd): params = util.Params( kwd ) lib_id = params.get( 'library_id', 'none' ) - lib_list = SelectField('library_id', refresh_on_change=True, - refresh_on_change_values=['new']) + lib_list = SelectField( 'library_id', refresh_on_change=True, refresh_on_change_values=['new'] ) if request and lib_id == 'none': if request.library: lib_id = str(request.library.id) @@ -483,7 +495,7 @@ lib_list.add_option('Select one', 'none', selected=True) else: lib_list.add_option('Select one', 'none') - for lib in libraries: + for lib, hidden_folder_ids in libraries.items(): if str(lib.id) == lib_id: lib_list.add_option(lib.name, lib.id, selected=True) else: @@ -653,9 +665,27 @@ widgets.append(dict(label='Description', widget=TextField('desc', 40, desc), helptext='(Optional)')) - # libraries selectbox - libraries = get_authorized_libs(trans, trans.user) + all_libraries = trans.app.model.Library.filter( trans.app.model.Library.table.c.deleted == False ) \ + .order_by( trans.app.model.Library.name ).all() + user, roles = trans.get_user_and_roles() + actions_to_check = [ trans.app.security_agent.permitted_actions.LIBRARY_ADD ] + # The libraries dictionary looks like: + # { library : '1,2' }, library : '3' } + # Its keys are the libraries that should be displayed for the current user and whose values are a + # string of comma-separated folder ids, of the associated folders the should NOT be displayed. + # The folders that should not be displayed may not be a complete list, but it is ultimately passed + # to the calling method to keep from re-checking the same folders when the library / folder + # select lists are rendered. + # + # TODO: RC, when you add the folders select list to your request form, take advantage of the hidden_folder_ids + # so that you do not need to check those same folders yet again when populating the select list. + # + libraries = {} + for library in all_libraries: + can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( user, roles, library, actions_to_check ) + if can_show: + libraries[ library ] = hidden_folder_ids libui = self.__library_ui(libraries, request, **kwd) widgets = widgets + libui widgets = widgets + get_form_widgets(trans, request.type.request_form, request.values.content, **kwd) diff -r d7a780065c91 -r a2d69372d699 lib/galaxy/web/controllers/requests_admin.py --- a/lib/galaxy/web/controllers/requests_admin.py Sat Aug 29 12:08:35 2009 -0400 +++ b/lib/galaxy/web/controllers/requests_admin.py Tue Sep 01 14:20:49 2009 -0400 @@ -8,7 +8,6 @@ from galaxy.web.form_builder import * from datetime import datetime, timedelta from galaxy.web.controllers.forms import get_form_widgets -from galaxy.web.controllers.library import get_authorized_libs log = logging.getLogger( __name__ ) @@ -709,14 +708,32 @@ return select_user def __library_ui(self, trans, user, request=None, **kwd): + """ + Return a list of libraries for which user has the permission + to perform the LIBRARY_ADD action on any of it's folders + """ params = util.Params( kwd ) lib_id = params.get( 'library_id', 'none' ) - if not user: - libraries = trans.app.model.Library.filter(trans.app.model.Library.table.c.deleted == False).order_by(trans.app.model.Library.name).all() - else: - libraries = get_authorized_libs(trans, user) - lib_list = SelectField('library_id', refresh_on_change=True, - refresh_on_change_values=['new']) + all_libraries = trans.app.model.Library.filter( trans.app.model.Library.table.c.deleted == False ) \ + .order_by( trans.app.model.Library.name ).all() + roles = user.all_roles() + actions_to_check = [ trans.app.security_agent.permitted_actions.LIBRARY_ADD ] + # The libraries dictionary looks like: { library : '1,2' }, library : '3' } + # Its keys are the libraries that should be displayed for the current user and whose values are a + # string of comma-separated folder ids, of the associated folders the should NOT be displayed. + # The folders that should not be displayed may not be a complete list, but it is ultimately passed + # to the calling method to keep from re-checking the same folders when the library / folder + # select lists are rendered. + # + # TODO: RC, when you add the folders select list to your request form, take advantage of the hidden_folder_ids + # so that you do not need to check those same folders yet again when populating the select list. + # + libraries = {} + for library in all_libraries: + can_show, hidden_folder_ids = trans.app.security_agent.show_library_item( user, roles, library, actions_to_check ) + if can_show: + libraries[ library ] = hidden_folder_ids + lib_list = SelectField( 'library_id', refresh_on_change=True, refresh_on_change_values=['new'] ) if request and lib_id == 'none': if request.library: lib_id = str(request.library.id) @@ -724,7 +741,7 @@ lib_list.add_option('Select one', 'none', selected=True) else: lib_list.add_option('Select one', 'none') - for lib in libraries: + for lib, hidden_folder_ids in libraries.items(): if str(lib.id) == lib_id: lib_list.add_option(lib.name, lib.id, selected=True) else: diff -r d7a780065c91 -r a2d69372d699 templates/library/browse_libraries.mako --- a/templates/library/browse_libraries.mako Sat Aug 29 12:08:35 2009 -0400 +++ b/templates/library/browse_libraries.mako Tue Sep 01 14:20:49 2009 -0400 @@ -20,9 +20,9 @@ </tr> </thead> <tbody> - %for library in libraries: + %for library, hidden_folder_ids in libraries.items(): <tr class="libraryRow libraryOrFolderRow" id="libraryRow"> - <td><a href="${h.url_for( controller='library', action='browse_library', id=library.id )}">${library.name}</a></td> + <td><a href="${h.url_for( controller='library', action='browse_library', id=library.id, hidden_folder_ids=hidden_folder_ids )}">${library.name}</a></td> <td><i>${library.description}</i></td> </tr> %endfor diff -r d7a780065c91 -r a2d69372d699 templates/library/browse_library.mako --- a/templates/library/browse_library.mako Sat Aug 29 12:08:35 2009 -0400 +++ b/templates/library/browse_library.mako Tue Sep 01 14:20:49 2009 -0400 @@ -3,7 +3,6 @@ <% from galaxy import util from time import strftime - user, roles = trans.get_user_and_roles() %> @@ -142,19 +141,21 @@ %> </%def> -<%def name="render_folder( folder, folder_pad, created_ldda_ids, library_id, parent=None, row_counter=None )"> +<%def name="render_folder( folder, folder_pad, created_ldda_ids, library_id, hidden_folder_ids, parent=None, row_counter=None )"> <% - def show_folder(): - ## TODO: instead of calling check_folder_contents(), which we've already done prior to getting here, - ## add a new method that will itself call check_folder_contents() and build a list of accessible folders - ## for each library - this should improve performance dor large libraries where the current user can only - ## access a small number of folders. - if trans.app.security_agent.check_folder_contents( user, roles, folder ) or \ - trans.app.security_agent.show_library_item( user, roles, folder ): - return True - return False - if not show_folder: + if str( folder.id ) in hidden_folder_ids: return "" + can_access, folder_ids = trans.app.security_agent.check_folder_contents( user, roles, folder ) + if not can_access: + can_show, folder_ids = \ + trans.app.security_agent.show_library_item( 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 "" root_folder = not folder.parent if root_folder: pad = folder_pad @@ -213,7 +214,7 @@ %> %endif %for child_folder in name_sorted( folder.active_folders ): - ${render_folder( child_folder, pad, created_ldda_ids, library_id, my_row, row_counter )} + ${render_folder( child_folder, pad, created_ldda_ids, library_id, hidden_folder_ids, my_row, row_counter )} %endfor %for library_dataset in name_sorted( folder.active_library_datasets ): <% @@ -272,7 +273,7 @@ </thead> </tr> <% row_counter = RowCounter() %> - ${render_folder( library.root_folder, 0, created_ldda_ids, library.id, None, row_counter )} + ${render_folder( library.root_folder, 0, created_ldda_ids, library.id, hidden_folder_ids, None, row_counter )} <tfoot> <tr> <td colspan="4" style="padding-left: 42px;">