From ca26c3d5aff4d92192bfef744348c94b6e41ea7e Mon Sep 17 00:00:00 2001 From: John Chilton Date: Fri, 24 Aug 2018 12:27:57 -0400 Subject: [PATCH] Optimize permission loading and checking collection parameters. We were checking permissions twice it seems like to me, only do it once. Avoid loading dataset permissions for each individual dataset in a collection - work with summary sets of role ids. --- lib/galaxy/dataset_collections/matching.py | 10 +++ lib/galaxy/model/__init__.py | 38 ++++++++++ lib/galaxy/security/__init__.py | 39 +++++++++- lib/galaxy/tools/__init__.py | 3 +- lib/galaxy/tools/actions/__init__.py | 80 ++++++++++++-------- lib/galaxy/tools/actions/model_operations.py | 10 +-- lib/galaxy/tools/execute.py | 6 +- 7 files changed, 144 insertions(+), 42 deletions(-) diff --git a/lib/galaxy/dataset_collections/matching.py b/lib/galaxy/dataset_collections/matching.py index aa62b626a9f..c696a4d04ba 100644 --- a/lib/galaxy/dataset_collections/matching.py +++ b/lib/galaxy/dataset_collections/matching.py @@ -45,6 +45,7 @@ class MatchingCollections(object): self.unlinked_structures = [] self.collections = {} self.subcollection_types = {} + self.action_tuples = {} def __attempt_add_to_linked_match(self, input_name, hdca, collection_type_description, subcollection_type): structure = get_structure(hdca, collection_type_description, leaf_subcollection_type=subcollection_type) @@ -76,6 +77,15 @@ class MatchingCollections(object): effective_structure = effective_structure.multiply(linked_structure) return None if effective_structure.is_leaf else effective_structure + def map_over_action_tuples(self, input_name): + if input_name not in self.action_tuples: + collection_instance = self.collections[input_name] + self.action_tuples[input_name] = collection_instance.collection.dataset_action_tuples + return self.action_tuples[input_name] + + def is_mapped_over(self, input_name): + return input_name in self.collections + @staticmethod def for_collections(collections_to_match, collection_type_descriptions): if not collections_to_match.has_collections(): diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py index fec37f9999e..f58314f02da 100644 --- a/lib/galaxy/model/__init__.py +++ b/lib/galaxy/model/__init__.py @@ -3359,6 +3359,44 @@ class DatasetCollection(Dictifiable, UsesAnnotations): return all(e.child_collection.populated for e in self.elements) return top_level_populated + @property + def dataset_action_tuples(self): + if not hasattr(self, '_dataset_action_tuples'): + db_session = object_session(self) + + dc = alias(DatasetCollection.table) + de = alias(DatasetCollectionElement.table) + hda = alias(HistoryDatasetAssociation.table) + dataset = alias(Dataset.table) + dataset_permission = alias(DatasetPermissions.table) + + select_from = dc.outerjoin(de, de.c.dataset_collection_id == dc.c.id) + + depth_collection_type = self.collection_type + while ":" in depth_collection_type: + child_collection = alias(DatasetCollection.table) + child_collection_element = alias(DatasetCollectionElement.table) + select_from = select_from.outerjoin(child_collection, child_collection.c.id == de.c.child_collection_id) + select_from = select_from.outerjoin(child_collection_element, child_collection_element.c.dataset_collection_id == child_collection.c.id) + + de = child_collection_element + depth_collection_type = depth_collection_type.split(":", 1)[1] + + select_from = select_from.outerjoin(hda, hda.c.id == de.c.hda_id).outerjoin(dataset, hda.c.dataset_id == dataset.c.id) + select_from = select_from.outerjoin(dataset_permission, dataset.c.id == dataset_permission.c.dataset_id) + + select_stmt = select([dataset_permission.c.action, dataset_permission.c.role_id]).select_from(select_from).where(dc.c.id == self.id).distinct() + + _dataset_action_tuples = [] + for _dataset_action_tuple in db_session.execute(select_stmt).fetchall(): + if _dataset_action_tuple[0] is None: + continue + _dataset_action_tuples.append(_dataset_action_tuple) + + self._dataset_action_tuples = _dataset_action_tuples + + return self._dataset_action_tuples + @property def waiting_for_elements(self): top_level_waiting = self.populated_state == DatasetCollection.populated_states.NEW diff --git a/lib/galaxy/security/__init__.py b/lib/galaxy/security/__init__.py index 3d3fe09c208..6bcb9b79371 100644 --- a/lib/galaxy/security/__init__.py +++ b/lib/galaxy/security/__init__.py @@ -9,6 +9,7 @@ from datetime import datetime, timedelta from sqlalchemy import and_, false, not_, or_ from sqlalchemy.orm import eagerload_all +import galaxy.model from galaxy.util import listify from galaxy.util.bunch import Bunch @@ -607,6 +608,17 @@ class GalaxyRBACAgent(RBACAgent): retval = self.dataset_is_public(dataset) or self.allow_action(user_roles, self.permitted_actions.DATASET_ACCESS, dataset) return retval + def can_access_datasets(self, user_roles, action_tuples): + user_role_ids = [galaxy.model.cached_id(r) for r in user_roles] + + # For DATASET_ACCESS, user must have ALL associated roles + for action, user_role_id in action_tuples: + if action == self.permitted_actions.DATASET_ACCESS.action: + if user_role_id not in user_role_ids: + return False + + return True + def can_manage_dataset(self, roles, dataset): return self.allow_action(roles, self.permitted_actions.DATASET_MANAGE_PERMISSIONS, dataset) @@ -717,6 +729,27 @@ class GalaxyRBACAgent(RBACAgent): perms[action].extend([_ for _ in roles if _ not in perms[action]]) return perms + def guess_derived_permissions(self, all_input_permissions): + """Returns a dict of { action : [ role_id, role_id, ... ] } for the output dataset based upon input dataset permissions. + + all_input_permissions should be of the form {action_name: set(role_ids)} + """ + perms = {} + for action_name, role_ids in all_input_permissions.items(): + if not role_ids: + continue + action = self.get_action(action_name) + if action not in perms.keys(): + perms[action] = list(role_ids) + else: + if action.model == 'grant': + # intersect existing roles with new roles + perms[action] = [_ for _ in role_ids if _ in perms[action]] + elif action.model == 'restrict': + # join existing roles with new roles + perms[action].extend([_ for _ in role_ids if _ not in perms[action]]) + return perms + def associate_components(self, **kwd): if 'user' in kwd: if 'group' in kwd: @@ -922,7 +955,11 @@ class GalaxyRBACAgent(RBACAgent): if isinstance(action, Action): action = action.action for role in roles: - dp = self.model.DatasetPermissions(action, dataset, role_id=role.id) + if hasattr(role, "id"): + role_id = role.id + else: + role_id = role + dp = self.model.DatasetPermissions(action, dataset, role_id=role_id) self.sa_session.add(dp) flush_needed = True if flush_needed and flush: diff --git a/lib/galaxy/tools/__init__.py b/lib/galaxy/tools/__init__.py index 93d8a0dab8b..9580a6e85fb 100755 --- a/lib/galaxy/tools/__init__.py +++ b/lib/galaxy/tools/__init__.py @@ -1418,7 +1418,7 @@ class Tool(Dictifiable): output_collections=execution_tracker.output_collections, implicit_collections=execution_tracker.implicit_collections) - def handle_single_execution(self, trans, rerun_remap_job_id, execution_slice, history, execution_cache=None, completed_job=None): + def handle_single_execution(self, trans, rerun_remap_job_id, execution_slice, history, execution_cache=None, completed_job=None, collection_info=None): """ Return a pair with whether execution is successful as well as either resulting output data or an error message indicating the problem. @@ -1432,6 +1432,7 @@ class Tool(Dictifiable): execution_cache=execution_cache, dataset_collection_elements=execution_slice.dataset_collection_elements, completed_job=completed_job, + collection_info=collection_info, ) except webob.exc.HTTPFound as e: # if it's a webob redirect exception, pass it up the stack diff --git a/lib/galaxy/tools/actions/__init__.py b/lib/galaxy/tools/actions/__init__.py index 631cc5cb5a0..16b5d2d02ec 100644 --- a/lib/galaxy/tools/actions/__init__.py +++ b/lib/galaxy/tools/actions/__init__.py @@ -58,7 +58,7 @@ class ToolAction(object): class DefaultToolAction(object): """Default tool action is to run an external command""" - def _collect_input_datasets(self, tool, param_values, trans, history, current_user_roles=None): + def _collect_input_datasets(self, tool, param_values, trans, history, current_user_roles=None, dataset_collection_elements=None, collection_info=None): """ Collect any dataset inputs from incoming. Returns a mapping from parameter name to Dataset instance for each tool parameter that is @@ -67,6 +67,12 @@ class DefaultToolAction(object): if current_user_roles is None: current_user_roles = trans.get_current_user_roles() input_datasets = odict() + all_permissions = {} + + def record_permission(action, role_id): + if action not in all_permissions: + all_permissions[action] = set() + all_permissions[action].add(role_id) def visitor(input, value, prefix, parent=None, **kwargs): @@ -85,8 +91,22 @@ class DefaultToolAction(object): else: data = data.get_converted_dataset(trans, target_ext, target_context=parent, history=history) - if not trans.app.security_agent.can_access_dataset(current_user_roles, data.dataset): - raise Exception("User does not have permission to use a dataset (%s) provided for input." % data.id) + input_name = prefix + input.name + # Checked security of whole collection all at once if mapping over this input, else + # fetch dataset details for this input from the database. + if collection_info and collection_info.is_mapped_over(input_name): + action_tuples = collection_info.map_over_action_tuples(input_name) + if not trans.app.security_agent.can_access_datasets(current_user_roles, action_tuples): + raise Exception("User does not have permission to use a dataset provided for input.") + for action, role_id in action_tuples: + record_permission(action, role_id) + else: + if not trans.app.security_agent.can_access_dataset(current_user_roles, data.dataset): + raise Exception("User does not have permission to use a dataset (%s) provided for input." % data.id) + permissions = trans.app.security_agent.get_permissions(data.dataset) + for action, roles in permissions.items(): + for role in roles: + record_permission(action.action, model.cached_id(role)) return data if isinstance(input, DataToolParameter): if isinstance(value, list): @@ -137,25 +157,28 @@ class DefaultToolAction(object): if not value: return - dataset_instances = [] + collection = None if hasattr(value, 'child_collection'): # if we are mapping a collection over a tool, we only require the child_collection - dataset_instances = value.child_collection.dataset_instances + collection = value.child_collection else: # else the tool takes a collection as input so we need everything - dataset_instances = value.collection.dataset_instances + collection = value.collection - for i, v in enumerate(dataset_instances): - data = v - if not trans.app.security_agent.can_access_dataset(current_user_roles, data.dataset): - raise Exception("User does not have permission to use a dataset (%s) provided for input." % data.id) + action_tuples = collection.dataset_action_tuples + if not trans.app.security_agent.can_access_datasets(current_user_roles, action_tuples): + raise Exception("User does not have permission to use a dataset provided for input.") + for action, role_id in action_tuples: + record_permission(action, role_id) + + for i, v in enumerate(collection.dataset_instances): # Skipping implicit conversion stuff for now, revisit at # some point and figure out if implicitly converting a # dataset collection makes senese. - input_datasets[prefix + input.name + str(i + 1)] = data + input_datasets[prefix + input.name + str(i + 1)] = v tool.visit_inputs(param_values, visitor) - return input_datasets + return input_datasets, all_permissions def collect_input_dataset_collections(self, tool, param_values): def append_to_key(the_dict, key, value): @@ -198,7 +221,7 @@ class DefaultToolAction(object): def _check_access(self, tool, trans): assert tool.allow_user_access(trans.user), "User (%s) is not allowed to access this tool." % (trans.user) - def _collect_inputs(self, tool, trans, incoming, history, current_user_roles): + def _collect_inputs(self, tool, trans, incoming, history, current_user_roles, collection_info): """ Collect history as well as input datasets and collections. """ app = trans.app # Set history. @@ -211,7 +234,7 @@ class DefaultToolAction(object): # input datasets can process these normally. inp_dataset_collections = self.collect_input_dataset_collections(tool, incoming) # Collect any input datasets from the incoming parameters - inp_data = self._collect_input_datasets(tool, incoming, trans, history=history, current_user_roles=current_user_roles) + inp_data, all_permissions = self._collect_input_datasets(tool, incoming, trans, history=history, current_user_roles=current_user_roles, collection_info=collection_info) # grap tags from incoming HDAs preserved_tags = {} @@ -230,10 +253,9 @@ class DefaultToolAction(object): if hasattr(collection, "tags"): for tag in collection.auto_propagated_tags: preserved_tags[tag.value] = tag + return history, inp_data, inp_dataset_collections, preserved_tags, all_permissions - return history, inp_data, inp_dataset_collections, preserved_tags - - def execute(self, tool, trans, incoming=None, return_job=False, set_output_hid=True, history=None, job_params=None, rerun_remap_job_id=None, execution_cache=None, dataset_collection_elements=None, completed_job=None): + def execute(self, tool, trans, incoming=None, return_job=False, set_output_hid=True, history=None, job_params=None, rerun_remap_job_id=None, execution_cache=None, dataset_collection_elements=None, completed_job=None, collection_info=None): """ Executes a tool, creating job and tool outputs, associating them, and submitting the job to the job queue. If history is not specified, use @@ -245,8 +267,7 @@ class DefaultToolAction(object): if execution_cache is None: execution_cache = ToolExecutionCache(trans) current_user_roles = execution_cache.current_user_roles - history, inp_data, inp_dataset_collections, preserved_tags = self._collect_inputs(tool, trans, incoming, history, current_user_roles) - + history, inp_data, inp_dataset_collections, preserved_tags, all_permissions = self._collect_inputs(tool, trans, incoming, history, current_user_roles, collection_info) # Build name for output datasets based on tool name and input names on_text = self._get_on_text(inp_data) @@ -286,7 +307,7 @@ class DefaultToolAction(object): # Determine output dataset permission/roles list existing_datasets = [inp for inp in inp_data.values() if inp] if existing_datasets: - output_permissions = app.security_agent.guess_derived_permissions_for_datasets(existing_datasets) + output_permissions = app.security_agent.guess_derived_permissions(all_permissions) else: # No valid inputs, we will use history defaults output_permissions = app.security_agent.history_get_default_permissions(history) @@ -492,7 +513,7 @@ class DefaultToolAction(object): job_setup_timer = ExecutionTimer() # Create the job object job, galaxy_session = self._new_job_for_session(trans, tool, history) - self._record_inputs(trans, tool, job, incoming, inp_data, inp_dataset_collections, current_user_roles) + self._record_inputs(trans, tool, job, incoming, inp_data, inp_dataset_collections) self._record_outputs(job, out_data, output_collections) job.object_store_id = object_store_populator.object_store_id if job_params: @@ -652,7 +673,7 @@ class DefaultToolAction(object): job.tool_version = "1.0.0" return job, galaxy_session - def _record_inputs(self, trans, tool, job, incoming, inp_data, inp_dataset_collections, current_user_roles): + def _record_inputs(self, trans, tool, job, incoming, inp_data, inp_dataset_collections): # FIXME: Don't need all of incoming here, just the defined parameters # from the tool. We need to deal with tools that pass all post # parameters to the command as a special case. @@ -692,7 +713,7 @@ class DefaultToolAction(object): for name, value in tool.params_to_strings(incoming, trans.app).items(): job.add_parameter(name, value) - self._check_input_data_access(trans, job, inp_data, current_user_roles) + self._record_input_datasets(trans, job, inp_data) def _record_outputs(self, job, out_data, output_collections): out_collections = output_collections.out_collections @@ -705,17 +726,10 @@ class DefaultToolAction(object): job.add_output_dataset_collection(name, dataset_collection_instance) dataset_collection_instance.job = job - def _check_input_data_access(self, trans, job, inp_data, current_user_roles): - access_timer = ExecutionTimer() + def _record_input_datasets(self, trans, job, inp_data): for name, dataset in inp_data.items(): - if dataset: - if not trans.app.security_agent.can_access_dataset(current_user_roles, dataset.dataset): - raise Exception("User does not have permission to use a dataset (%s) provided for input." % dataset.id) - job.add_input_dataset(name, dataset=dataset) - else: - job.add_input_dataset(name, None) - job_str = job.log_str() - log.info("Verified access to datasets for %s %s" % (job_str, access_timer)) + # TODO: figure out why can't pass dataset_id here. + job.add_input_dataset(name, dataset=dataset) def get_output_name(self, output, dataset, tool, on_text, trans, incoming, history, params, job_params): if output.label: diff --git a/lib/galaxy/tools/actions/model_operations.py b/lib/galaxy/tools/actions/model_operations.py index 427afd834b8..ed0fc6e9d1b 100644 --- a/lib/galaxy/tools/actions/model_operations.py +++ b/lib/galaxy/tools/actions/model_operations.py @@ -12,21 +12,21 @@ log = logging.getLogger(__name__) class ModelOperationToolAction(DefaultToolAction): - def check_inputs_ready(self, tool, trans, incoming, history, execution_cache=None): + def check_inputs_ready(self, tool, trans, incoming, history, execution_cache=None, collection_info=None): if execution_cache is None: execution_cache = ToolExecutionCache(trans) current_user_roles = execution_cache.current_user_roles - history, inp_data, inp_dataset_collections, _ = self._collect_inputs(tool, trans, incoming, history, current_user_roles) + history, inp_data, inp_dataset_collections, _, _ = self._collect_inputs(tool, trans, incoming, history, current_user_roles, collection_info) tool.check_inputs_ready(inp_data, inp_dataset_collections) - def execute(self, tool, trans, incoming={}, set_output_hid=False, overwrite=True, history=None, job_params=None, execution_cache=None, **kwargs): + def execute(self, tool, trans, incoming={}, set_output_hid=False, overwrite=True, history=None, job_params=None, execution_cache=None, collection_info=None, **kwargs): if execution_cache is None: execution_cache = ToolExecutionCache(trans) current_user_roles = execution_cache.current_user_roles - history, inp_data, inp_dataset_collections, preserved_tags = self._collect_inputs(tool, trans, incoming, history, current_user_roles) + history, inp_data, inp_dataset_collections, preserved_tags, all_permissions = self._collect_inputs(tool, trans, incoming, history, current_user_roles, collection_info) # Build name for output datasets based on tool name and input names on_text = self._get_on_text(inp_data) @@ -55,7 +55,7 @@ class ModelOperationToolAction(DefaultToolAction): # job, galaxy_session = self._new_job_for_session(trans, tool, history) self._produce_outputs(trans, tool, out_data, output_collections, incoming=incoming, history=history, tags=preserved_tags) - self._record_inputs(trans, tool, job, incoming, inp_data, inp_dataset_collections, current_user_roles) + self._record_inputs(trans, tool, job, incoming, inp_data, inp_dataset_collections) self._record_outputs(job, out_data, output_collections) job.state = job.states.OK trans.sa_session.add(job) diff --git a/lib/galaxy/tools/execute.py b/lib/galaxy/tools/execute.py index 5863fc439cd..ac4d2482a66 100644 --- a/lib/galaxy/tools/execute.py +++ b/lib/galaxy/tools/execute.py @@ -64,7 +64,7 @@ def execute(trans, tool, mapping_params, history, rerun_remap_job_id=None, colle # Only workflow invocation code gets to set this, ignore user supplied # values or rerun parameters. del params['__workflow_resource_params__'] - job, result = tool.handle_single_execution(trans, rerun_remap_job_id, execution_slice, history, execution_cache, completed_job) + job, result = tool.handle_single_execution(trans, rerun_remap_job_id, execution_slice, history, execution_cache, completed_job, collection_info) if job: message = EXECUTION_SUCCESS_MESSAGE % (tool.id, job.id, job_timer) log.debug(message) @@ -80,7 +80,9 @@ def execute(trans, tool, mapping_params, history, rerun_remap_job_id=None, colle tool, trans, params, - history + history, + execution_cache=execution_cache, + collection_info=collection_info, ) execution_tracker.ensure_implicit_collections_populated(history, mapping_params.param_template)