From 27eb8e210401aba711d36ee19132c163bc797e12 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 11:44:26 -0500 Subject: [PATCH 1/9] Rearrange to separate security logic from query-building --- lib/galaxy/managers/jobs.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 3f115f3c1cfa..b34b76601114 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -103,9 +103,14 @@ def __init__(self, app: StructuredApp): def index_query(self, trans, payload: JobIndexQueryPayload): is_admin = trans.user_is_admin user_details = payload.user_details - decoded_user_id = payload.user_id + if not is_admin: + if user_details: + raise AdminRequiredException("Only admins can index the jobs with user details enabled") + if decoded_user_id is not None and decoded_user_id != trans.user.id: + raise AdminRequiredException("Only admins can index the jobs of others") + if is_admin: if decoded_user_id is not None: query = trans.sa_session.query(model.Job).filter(model.Job.user_id == decoded_user_id) @@ -113,12 +118,7 @@ def index_query(self, trans, payload: JobIndexQueryPayload): query = trans.sa_session.query(model.Job) if user_details: query = query.outerjoin(model.Job.user) - else: - if user_details: - raise AdminRequiredException("Only admins can index the jobs with user details enabled") - if decoded_user_id is not None and decoded_user_id != trans.user.id: - raise AdminRequiredException("Only admins can index the jobs of others") query = trans.sa_session.query(model.Job).filter(model.Job.user_id == trans.user.id) def build_and_apply_filters(query, objects, filter_func): From 0e6e176346da6d6034f15a64b4c47b053b3e48ad Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 12:11:05 -0500 Subject: [PATCH 2/9] Use model class names in query-building w/o prefixes --- lib/galaxy/managers/jobs.py | 60 ++++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index b34b76601114..aa1a92955bba 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -41,9 +41,13 @@ from galaxy.managers.hdas import HDAManager from galaxy.managers.lddas import LDDAManager from galaxy.model import ( + ImplicitCollectionJobsJobAssociation, Job, JobParameter, User, + Workflow, + WorkflowInvocation, + WorkflowInvocationStep, YIELD_PER_ROWS, ) from galaxy.model.base import transaction @@ -113,13 +117,13 @@ def index_query(self, trans, payload: JobIndexQueryPayload): if is_admin: if decoded_user_id is not None: - query = trans.sa_session.query(model.Job).filter(model.Job.user_id == decoded_user_id) + query = trans.sa_session.query(Job).filter(Job.user_id == decoded_user_id) else: - query = trans.sa_session.query(model.Job) + query = trans.sa_session.query(Job) if user_details: - query = query.outerjoin(model.Job.user) + query = query.outerjoin(Job.user) else: - query = trans.sa_session.query(model.Job).filter(model.Job.user_id == trans.user.id) + query = trans.sa_session.query(Job).filter(Job.user_id == trans.user.id) def build_and_apply_filters(query, objects, filter_func): if objects is not None: @@ -132,38 +136,38 @@ def build_and_apply_filters(query, objects, filter_func): query = query.filter(or_(*t)) return query - query = build_and_apply_filters(query, payload.states, lambda s: model.Job.state == s) - query = build_and_apply_filters(query, payload.tool_ids, lambda t: model.Job.tool_id == t) - query = build_and_apply_filters(query, payload.tool_ids_like, lambda t: model.Job.tool_id.like(t)) - query = build_and_apply_filters(query, payload.date_range_min, lambda dmin: model.Job.update_time >= dmin) - query = build_and_apply_filters(query, payload.date_range_max, lambda dmax: model.Job.update_time <= dmax) + query = build_and_apply_filters(query, payload.states, lambda s: Job.state == s) + query = build_and_apply_filters(query, payload.tool_ids, lambda t: Job.tool_id == t) + query = build_and_apply_filters(query, payload.tool_ids_like, lambda t: Job.tool_id.like(t)) + query = build_and_apply_filters(query, payload.date_range_min, lambda dmin: Job.update_time >= dmin) + query = build_and_apply_filters(query, payload.date_range_max, lambda dmax: Job.update_time <= dmax) history_id = payload.history_id workflow_id = payload.workflow_id invocation_id = payload.invocation_id if history_id is not None: - query = query.filter(model.Job.history_id == history_id) + query = query.filter(Job.history_id == history_id) if workflow_id or invocation_id: if workflow_id is not None: wfi_step = ( - trans.sa_session.query(model.WorkflowInvocationStep) - .join(model.WorkflowInvocation) - .join(model.Workflow) + trans.sa_session.query(WorkflowInvocationStep) + .join(WorkflowInvocation) + .join(Workflow) .filter( - model.Workflow.stored_workflow_id == workflow_id, + Workflow.stored_workflow_id == workflow_id, ) .subquery() ) elif invocation_id is not None: wfi_step = ( - trans.sa_session.query(model.WorkflowInvocationStep) - .filter(model.WorkflowInvocationStep.workflow_invocation_id == invocation_id) + trans.sa_session.query(WorkflowInvocationStep) + .filter(WorkflowInvocationStep.workflow_invocation_id == invocation_id) .subquery() ) query1 = query.join(wfi_step) - query2 = query.join(model.ImplicitCollectionJobsJobAssociation).join( + query2 = query.join(ImplicitCollectionJobsJobAssociation).join( wfi_step, - model.ImplicitCollectionJobsJobAssociation.implicit_collection_jobs_id + ImplicitCollectionJobsJobAssociation.implicit_collection_jobs_id == wfi_step.c.implicit_collection_jobs_id, ) query = query1.union(query2) @@ -196,26 +200,26 @@ def build_and_apply_filters(query, objects, filter_func): if isinstance(term, FilteredTerm): key = term.filter if key == "user": - query = query.filter(text_column_filter(model.User.email, term)) + query = query.filter(text_column_filter(User.email, term)) elif key == "tool": - query = query.filter(text_column_filter(model.Job.tool_id, term)) + query = query.filter(text_column_filter(Job.tool_id, term)) elif key == "handler": - query = query.filter(text_column_filter(model.Job.handler, term)) + query = query.filter(text_column_filter(Job.handler, term)) elif key == "runner": - query = query.filter(text_column_filter(model.Job.job_runner_name, term)) + query = query.filter(text_column_filter(Job.job_runner_name, term)) elif isinstance(term, RawTextTerm): - columns = [model.Job.tool_id] + columns = [Job.tool_id] if user_details: - columns.append(model.User.email) + columns.append(User.email) if is_admin: - columns.append(model.Job.handler) - columns.append(model.Job.job_runner_name) + columns.append(Job.handler) + columns.append(Job.job_runner_name) query = query.filter(raw_text_column_filter(columns, term)) if payload.order_by == JobIndexSortByEnum.create_time: - order_by = model.Job.create_time.desc() + order_by = Job.create_time.desc() else: - order_by = model.Job.update_time.desc() + order_by = Job.update_time.desc() query = query.order_by(order_by) query = query.offset(payload.offset) From b1e4bdc389dfa374afbc5bc19747199655198be2 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 13:29:33 -0500 Subject: [PATCH 3/9] Group local variables in one place --- lib/galaxy/managers/jobs.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index aa1a92955bba..00f3e85033ac 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -108,6 +108,11 @@ def index_query(self, trans, payload: JobIndexQueryPayload): is_admin = trans.user_is_admin user_details = payload.user_details decoded_user_id = payload.user_id + history_id = payload.history_id + workflow_id = payload.workflow_id + invocation_id = payload.invocation_id + search = payload.search + order_by = payload.order_by if not is_admin: if user_details: @@ -142,9 +147,6 @@ def build_and_apply_filters(query, objects, filter_func): query = build_and_apply_filters(query, payload.date_range_min, lambda dmin: Job.update_time >= dmin) query = build_and_apply_filters(query, payload.date_range_max, lambda dmax: Job.update_time <= dmax) - history_id = payload.history_id - workflow_id = payload.workflow_id - invocation_id = payload.invocation_id if history_id is not None: query = query.filter(Job.history_id == history_id) if workflow_id or invocation_id: @@ -172,7 +174,6 @@ def build_and_apply_filters(query, objects, filter_func): ) query = query1.union(query2) - search = payload.search if search: search_filters = { "tool": "tool", @@ -216,11 +217,11 @@ def build_and_apply_filters(query, objects, filter_func): columns.append(Job.job_runner_name) query = query.filter(raw_text_column_filter(columns, term)) - if payload.order_by == JobIndexSortByEnum.create_time: - order_by = Job.create_time.desc() + if order_by == JobIndexSortByEnum.create_time: + _order_by = Job.create_time.desc() else: - order_by = Job.update_time.desc() - query = query.order_by(order_by) + _order_by = Job.update_time.desc() + query = query.order_by(_order_by) query = query.offset(payload.offset) query = query.limit(payload.limit) From d253562fe213674424ef7d71e36032c49877fcc0 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 13:42:27 -0500 Subject: [PATCH 4/9] Convert Query to Select --- lib/galaxy/managers/jobs.py | 72 +++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 00f3e85033ac..f956ee39a4e6 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -120,59 +120,53 @@ def index_query(self, trans, payload: JobIndexQueryPayload): if decoded_user_id is not None and decoded_user_id != trans.user.id: raise AdminRequiredException("Only admins can index the jobs of others") + stmt = select(Job) + if is_admin: if decoded_user_id is not None: - query = trans.sa_session.query(Job).filter(Job.user_id == decoded_user_id) - else: - query = trans.sa_session.query(Job) + stmt = stmt.where(Job.user_id == decoded_user_id) if user_details: - query = query.outerjoin(Job.user) + stmt = stmt.outerjoin(Job.user) else: - query = trans.sa_session.query(Job).filter(Job.user_id == trans.user.id) + stmt = stmt.where(Job.user_id == trans.user.id) - def build_and_apply_filters(query, objects, filter_func): + def build_and_apply_filters(stmt, objects, filter_func): if objects is not None: if isinstance(objects, (str, date, datetime)): - query = query.filter(filter_func(objects)) + stmt = stmt.where(filter_func(objects)) elif isinstance(objects, list): t = [] for obj in objects: t.append(filter_func(obj)) - query = query.filter(or_(*t)) - return query + stmt = stmt.where(or_(*t)) + return stmt - query = build_and_apply_filters(query, payload.states, lambda s: Job.state == s) - query = build_and_apply_filters(query, payload.tool_ids, lambda t: Job.tool_id == t) - query = build_and_apply_filters(query, payload.tool_ids_like, lambda t: Job.tool_id.like(t)) - query = build_and_apply_filters(query, payload.date_range_min, lambda dmin: Job.update_time >= dmin) - query = build_and_apply_filters(query, payload.date_range_max, lambda dmax: Job.update_time <= dmax) + stmt = build_and_apply_filters(stmt, payload.states, lambda s: model.Job.state == s) + stmt = build_and_apply_filters(stmt, payload.tool_ids, lambda t: model.Job.tool_id == t) + stmt = build_and_apply_filters(stmt, payload.tool_ids_like, lambda t: model.Job.tool_id.like(t)) + stmt = build_and_apply_filters(stmt, payload.date_range_min, lambda dmin: model.Job.update_time >= dmin) + stmt = build_and_apply_filters(stmt, payload.date_range_max, lambda dmax: model.Job.update_time <= dmax) if history_id is not None: - query = query.filter(Job.history_id == history_id) + stmt = stmt.where(Job.history_id == history_id) + if workflow_id or invocation_id: + wfi_step = select(WorkflowInvocationStep) if workflow_id is not None: wfi_step = ( - trans.sa_session.query(WorkflowInvocationStep) - .join(WorkflowInvocation) - .join(Workflow) - .filter( - Workflow.stored_workflow_id == workflow_id, - ) - .subquery() + wfi_step.join(WorkflowInvocation).join(Workflow).where(Workflow.stored_workflow_id == workflow_id) ) elif invocation_id is not None: - wfi_step = ( - trans.sa_session.query(WorkflowInvocationStep) - .filter(WorkflowInvocationStep.workflow_invocation_id == invocation_id) - .subquery() - ) - query1 = query.join(wfi_step) - query2 = query.join(ImplicitCollectionJobsJobAssociation).join( + wfi_step = wfi_step.where(WorkflowInvocationStep.workflow_invocation_id == invocation_id) + wfi_step = wfi_step.subquery() + + stmt1 = stmt.join(wfi_step) + stmt2 = stmt.join(ImplicitCollectionJobsJobAssociation).join( wfi_step, ImplicitCollectionJobsJobAssociation.implicit_collection_jobs_id == wfi_step.c.implicit_collection_jobs_id, ) - query = query1.union(query2) + stmt = stmt1.union(stmt2) if search: search_filters = { @@ -201,13 +195,13 @@ def build_and_apply_filters(query, objects, filter_func): if isinstance(term, FilteredTerm): key = term.filter if key == "user": - query = query.filter(text_column_filter(User.email, term)) + stmt = stmt.where(text_column_filter(User.email, term)) elif key == "tool": - query = query.filter(text_column_filter(Job.tool_id, term)) + stmt = stmt.where(text_column_filter(Job.tool_id, term)) elif key == "handler": - query = query.filter(text_column_filter(Job.handler, term)) + stmt = stmt.where(text_column_filter(Job.handler, term)) elif key == "runner": - query = query.filter(text_column_filter(Job.job_runner_name, term)) + stmt = stmt.where(text_column_filter(Job.job_runner_name, term)) elif isinstance(term, RawTextTerm): columns = [Job.tool_id] if user_details: @@ -215,17 +209,17 @@ def build_and_apply_filters(query, objects, filter_func): if is_admin: columns.append(Job.handler) columns.append(Job.job_runner_name) - query = query.filter(raw_text_column_filter(columns, term)) + stmt = stmt.filter(raw_text_column_filter(columns, term)) if order_by == JobIndexSortByEnum.create_time: _order_by = Job.create_time.desc() else: _order_by = Job.update_time.desc() - query = query.order_by(_order_by) + stmt = stmt.order_by(_order_by) - query = query.offset(payload.offset) - query = query.limit(payload.limit) - return query + stmt = stmt.offset(payload.offset) + stmt = stmt.limit(payload.limit) + return trans.sa_session.scalars(stmt) def job_lock(self) -> JobLock: return JobLock(active=self.app.job_manager.job_lock) From 0ccfe05b4c31efd21e64ea5fcc9f5304f7496406 Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 15:21:04 -0500 Subject: [PATCH 5/9] Ensure the result of executing the select statement is models Calling union() on a statement creates a CompoundSelect object; executing it will return tuples of column values, not models. To ensure the method always returns models (regardless of whether the union is called or not), we use aliased() to map the Job model to the CompoundSelect. --- lib/galaxy/managers/jobs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index f956ee39a4e6..88109531347e 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -166,7 +166,8 @@ def build_and_apply_filters(stmt, objects, filter_func): ImplicitCollectionJobsJobAssociation.implicit_collection_jobs_id == wfi_step.c.implicit_collection_jobs_id, ) - stmt = stmt1.union(stmt2) + # Ensure the result is models, not tuples + stmt = select(aliased(Job, stmt1.union(stmt2).subquery())) if search: search_filters = { From a0dd45ad002465adf95c52af693640e48062d02d Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 16:00:40 -0500 Subject: [PATCH 6/9] Ensure UNION + ORDER BY works correctly in sqlite --- lib/galaxy/managers/jobs.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 88109531347e..52c69960ba1d 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -150,6 +150,7 @@ def build_and_apply_filters(stmt, objects, filter_func): if history_id is not None: stmt = stmt.where(Job.history_id == history_id) + order_by_columns = Job if workflow_id or invocation_id: wfi_step = select(WorkflowInvocationStep) if workflow_id is not None: @@ -167,7 +168,11 @@ def build_and_apply_filters(stmt, objects, filter_func): == wfi_step.c.implicit_collection_jobs_id, ) # Ensure the result is models, not tuples - stmt = select(aliased(Job, stmt1.union(stmt2).subquery())) + sq = stmt1.union(stmt2).subquery() + # SQLite won't recognize Job.foo as a valid column for the ORDER BY clause due to the UNION clause, so we'll use the subquery `columns` collection. + # Ref: https://github.com/galaxyproject/galaxy/pull/16852#issuecomment-1804676322 + order_by_columns = sq.c + stmt = select(aliased(Job, sq)) if search: search_filters = { @@ -213,9 +218,9 @@ def build_and_apply_filters(stmt, objects, filter_func): stmt = stmt.filter(raw_text_column_filter(columns, term)) if order_by == JobIndexSortByEnum.create_time: - _order_by = Job.create_time.desc() + _order_by = order_by_columns.create_time.desc() else: - _order_by = Job.update_time.desc() + _order_by = order_by_columns.update_time.desc() stmt = stmt.order_by(_order_by) stmt = stmt.offset(payload.offset) From 22934827ad4ca7144cd9d5cfb95fa7bb87f97b1a Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 16:14:57 -0500 Subject: [PATCH 7/9] Reorganize method for improved readability (I think factoring out these complex chunks of logic into local functions makes it easier to follow the main query-building logic.) --- lib/galaxy/managers/jobs.py | 73 ++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 34 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 52c69960ba1d..26006afeb664 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -114,22 +114,6 @@ def index_query(self, trans, payload: JobIndexQueryPayload): search = payload.search order_by = payload.order_by - if not is_admin: - if user_details: - raise AdminRequiredException("Only admins can index the jobs with user details enabled") - if decoded_user_id is not None and decoded_user_id != trans.user.id: - raise AdminRequiredException("Only admins can index the jobs of others") - - stmt = select(Job) - - if is_admin: - if decoded_user_id is not None: - stmt = stmt.where(Job.user_id == decoded_user_id) - if user_details: - stmt = stmt.outerjoin(Job.user) - else: - stmt = stmt.where(Job.user_id == trans.user.id) - def build_and_apply_filters(stmt, objects, filter_func): if objects is not None: if isinstance(objects, (str, date, datetime)): @@ -141,17 +125,7 @@ def build_and_apply_filters(stmt, objects, filter_func): stmt = stmt.where(or_(*t)) return stmt - stmt = build_and_apply_filters(stmt, payload.states, lambda s: model.Job.state == s) - stmt = build_and_apply_filters(stmt, payload.tool_ids, lambda t: model.Job.tool_id == t) - stmt = build_and_apply_filters(stmt, payload.tool_ids_like, lambda t: model.Job.tool_id.like(t)) - stmt = build_and_apply_filters(stmt, payload.date_range_min, lambda dmin: model.Job.update_time >= dmin) - stmt = build_and_apply_filters(stmt, payload.date_range_max, lambda dmax: model.Job.update_time <= dmax) - - if history_id is not None: - stmt = stmt.where(Job.history_id == history_id) - - order_by_columns = Job - if workflow_id or invocation_id: + def add_workflow_jobs(): wfi_step = select(WorkflowInvocationStep) if workflow_id is not None: wfi_step = ( @@ -169,12 +143,11 @@ def build_and_apply_filters(stmt, objects, filter_func): ) # Ensure the result is models, not tuples sq = stmt1.union(stmt2).subquery() - # SQLite won't recognize Job.foo as a valid column for the ORDER BY clause due to the UNION clause, so we'll use the subquery `columns` collection. + # SQLite won't recognize Job.foo as a valid column for the ORDER BY clause due to the UNION clause, so we'll use the subquery `columns` collection (`sq.c`). # Ref: https://github.com/galaxyproject/galaxy/pull/16852#issuecomment-1804676322 - order_by_columns = sq.c - stmt = select(aliased(Job, sq)) + return select(aliased(Job, sq)), sq.c - if search: + def add_search_criteria(stmt): search_filters = { "tool": "tool", "t": "tool", @@ -216,12 +189,44 @@ def build_and_apply_filters(stmt, objects, filter_func): columns.append(Job.handler) columns.append(Job.job_runner_name) stmt = stmt.filter(raw_text_column_filter(columns, term)) + return stmt + + if not is_admin: + if user_details: + raise AdminRequiredException("Only admins can index the jobs with user details enabled") + if decoded_user_id is not None and decoded_user_id != trans.user.id: + raise AdminRequiredException("Only admins can index the jobs of others") + + stmt = select(Job) + + if is_admin: + if decoded_user_id is not None: + stmt = stmt.where(Job.user_id == decoded_user_id) + if user_details: + stmt = stmt.outerjoin(Job.user) + else: + stmt = stmt.where(Job.user_id == trans.user.id) + + stmt = build_and_apply_filters(stmt, payload.states, lambda s: model.Job.state == s) + stmt = build_and_apply_filters(stmt, payload.tool_ids, lambda t: model.Job.tool_id == t) + stmt = build_and_apply_filters(stmt, payload.tool_ids_like, lambda t: model.Job.tool_id.like(t)) + stmt = build_and_apply_filters(stmt, payload.date_range_min, lambda dmin: model.Job.update_time >= dmin) + stmt = build_and_apply_filters(stmt, payload.date_range_max, lambda dmax: model.Job.update_time <= dmax) + + if history_id is not None: + stmt = stmt.where(Job.history_id == history_id) + + order_by_columns = Job + if workflow_id or invocation_id: + stmt, order_by_columns = add_workflow_jobs() + + if search: + stmt = add_search_criteria(stmt) if order_by == JobIndexSortByEnum.create_time: - _order_by = order_by_columns.create_time.desc() + stmt = stmt.order_by(order_by_columns.create_time.desc()) else: - _order_by = order_by_columns.update_time.desc() - stmt = stmt.order_by(_order_by) + stmt = stmt.order_by(order_by_columns.update_time.desc()) stmt = stmt.offset(payload.offset) stmt = stmt.limit(payload.limit) From a4df9223a08ff4b007b49562cc6e3162c4ca8f6c Mon Sep 17 00:00:00 2001 From: John Davis Date: Mon, 13 Nov 2023 16:48:25 -0500 Subject: [PATCH 8/9] Add assert to help mypy: `search` is guaranteed to be not null --- lib/galaxy/managers/jobs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 26006afeb664..3e2513b566f0 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -169,6 +169,7 @@ def add_search_criteria(stmt): "h": "handler", } ) + assert search parsed_search = parse_filters_structured(search, search_filters) for term in parsed_search.terms: if isinstance(term, FilteredTerm): From 01e627fdd5cbc5dda10050c1a59ab08151c5c82e Mon Sep 17 00:00:00 2001 From: John Davis Date: Tue, 14 Nov 2023 09:26:22 -0500 Subject: [PATCH 9/9] Move access permissions checks to service; add typing --- lib/galaxy/managers/jobs.py | 10 ++------ lib/galaxy/webapps/galaxy/services/jobs.py | 27 +++++++++++++++++----- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/lib/galaxy/managers/jobs.py b/lib/galaxy/managers/jobs.py index 3e2513b566f0..fcb09f1b9a84 100644 --- a/lib/galaxy/managers/jobs.py +++ b/lib/galaxy/managers/jobs.py @@ -6,6 +6,7 @@ datetime, ) +import sqlalchemy from boltons.iterutils import remap from pydantic import ( BaseModel, @@ -27,7 +28,6 @@ from galaxy import model from galaxy.exceptions import ( - AdminRequiredException, ItemAccessibilityException, ObjectNotFound, RequestParameterInvalidException, @@ -104,7 +104,7 @@ def __init__(self, app: StructuredApp): self.app = app self.dataset_manager = DatasetManager(app) - def index_query(self, trans, payload: JobIndexQueryPayload): + def index_query(self, trans, payload: JobIndexQueryPayload) -> sqlalchemy.engine.Result: is_admin = trans.user_is_admin user_details = payload.user_details decoded_user_id = payload.user_id @@ -192,12 +192,6 @@ def add_search_criteria(stmt): stmt = stmt.filter(raw_text_column_filter(columns, term)) return stmt - if not is_admin: - if user_details: - raise AdminRequiredException("Only admins can index the jobs with user details enabled") - if decoded_user_id is not None and decoded_user_id != trans.user.id: - raise AdminRequiredException("Only admins can index the jobs of others") - stmt = select(Job) if is_admin: diff --git a/lib/galaxy/webapps/galaxy/services/jobs.py b/lib/galaxy/webapps/galaxy/services/jobs.py index 98ac5bbb8f9e..5a92d629603a 100644 --- a/lib/galaxy/webapps/galaxy/services/jobs.py +++ b/lib/galaxy/webapps/galaxy/services/jobs.py @@ -2,6 +2,7 @@ from typing import ( Any, Dict, + Optional, ) from galaxy import ( @@ -59,15 +60,18 @@ def index( ): security = trans.security is_admin = trans.user_is_admin - if payload.view == JobIndexViewEnum.admin_job_list: + view = payload.view + if view == JobIndexViewEnum.admin_job_list: payload.user_details = True user_details = payload.user_details - if payload.view == JobIndexViewEnum.admin_job_list and not is_admin: - raise exceptions.AdminRequiredException("Only admins can use the admin_job_list view") - query = self.job_manager.index_query(trans, payload) + decoded_user_id = payload.user_id + + if not is_admin: + self._check_nonadmin_access(view, user_details, decoded_user_id, trans.user.id) + + jobs = self.job_manager.index_query(trans, payload) out = [] - view = payload.view - for job in query.yield_per(model.YIELD_PER_ROWS): + for job in jobs.yield_per(model.YIELD_PER_ROWS): job_dict = job.to_dict(view, system_details=is_admin) j = security.encode_all_ids(job_dict, True) if view == JobIndexViewEnum.admin_job_list: @@ -77,3 +81,14 @@ def index( out.append(j) return out + + def _check_nonadmin_access( + self, view: str, user_details: bool, decoded_user_id: Optional[DecodedDatabaseIdField], trans_user_id: int + ): + """Verify admin-only resources are not being accessed.""" + if view == JobIndexViewEnum.admin_job_list: + raise exceptions.AdminRequiredException("Only admins can use the admin_job_list view") + if user_details: + raise exceptions.AdminRequiredException("Only admins can index the jobs with user details enabled") + if decoded_user_id is not None and decoded_user_id != trans_user_id: + raise exceptions.AdminRequiredException("Only admins can index the jobs of others")