Skip to content

Draft: 931 feature add role based access control to running workflows #939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

eenblam
Copy link

@eenblam eenblam commented May 8, 2025

Here's my first pass at adding auth for inputstep per #931.

I'd appreciate feedback on my present approach, but I'd also like to address the following loose ends before merging:

  • Adding tests for how @workflow auth interacts with @inputstep auth and resolving any issues identified in the process.
  • Logging step in which an error was encountered if possible (see TODO in current changes)
  • Ensure UI is either disabled or user is alerted before hitting Resume Workflow if possible. Current changes only address the backend authorization.

Copy link

codspeed-hq bot commented May 8, 2025

CodSpeed Performance Report

Merging #939 will not alter performance

Comparing 931-feature-add-role-based-access-control-to-running-workflows (dbd45ed) with main (cbdf950)

Summary

✅ 12 untouched benchmarks

Copy link

codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 72.91667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.86%. Comparing base (f0677d1) to head (dbd45ed).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
orchestrator/api/api_v1/endpoints/processes.py 62.06% 8 Missing and 3 partials ⚠️
orchestrator/services/celery.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #939      +/-   ##
==========================================
+ Coverage   83.49%   83.86%   +0.37%     
==========================================
  Files         205      211       +6     
  Lines       10206    10220      +14     
  Branches     1022     1008      -14     
==========================================
+ Hits         8521     8571      +50     
+ Misses       1414     1382      -32     
+ Partials      271      267       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eenblam eenblam force-pushed the 931-feature-add-role-based-access-control-to-running-workflows branch from e4a5e3a to 6e33048 Compare May 8, 2025 16:28
@eenblam eenblam force-pushed the 931-feature-add-role-based-access-control-to-running-workflows branch from abc0074 to 6a96dae Compare May 21, 2025 00:08
Comment on lines +99 to +100
authorize_callback: Authorizer
retry_auth_callback: Authorizer
Copy link
Author

@eenblam eenblam May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see different arguments for what to name these. For now, I've chosen to not change Alex's naming of authorize_callback in case it's already in use downstream, but I'm open to feedback on how best to name the following callbacks. Each is a function implementing a RBAC policy.

  • authorize_callback: called when a user starts a workflow.
  • resume_auth_callback: called when a user resumes a workflow paused by an inputstep. "resume_authorize_callback" felt a bit unwieldy, but I'm open to it if we want to maintain authorize_callback AND make them match.
  • retry_auth_callback: called when a failed step is retried. Arg to both @workflow and @inputstep.

Ben Elam added 3 commits May 20, 2025 18:09
Unlike new_process, this can be checked immediately in the request handler.

Policy priorities specified via workflow and steps are resolved via get_auth_callbacks.
@eenblam eenblam force-pushed the 931-feature-add-role-based-access-control-to-running-workflows branch from a4faed1 to 3bbe13b Compare May 21, 2025 01:16
@eenblam eenblam force-pushed the 931-feature-add-role-based-access-control-to-running-workflows branch from 7b508e0 to dbd45ed Compare May 23, 2025 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants