Skip to content

Check function run permissions 🎨 #7844

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 32 commits into from
Jun 6, 2025
Merged

Check function run permissions 🎨 #7844

merged 32 commits into from
Jun 6, 2025

Conversation

@wvangeit
Copy link
Contributor

@wvangeit wvangeit commented Jun 6, 2025 •
edited
Loading

What do these changes do?

Before running a function there is now a check to see if the user has execute permissions for the function.

Related issue/s

ITISFoundation/osparc-issues#1897

How to test

Execute a function you don't have execute access to

Dev-ops

No changes

wvangeit and others added 26 commits June 2, 2025 11:16
@wvangeit
Make sure function job was successful before returning cache
d7b8ce8
@wvangeit
Merge branch 'master' into function_cache_check_success
cdc111b
@wvangeit
Merge branch 'master' into function_cache_check_success
d187c42
@wvangeit
Fix return type error
a72507a
@wvangeit
Merge branch 'function_cache_check_success' of github.com:wvangeit/os…
82460f7 
…parc-simcore into function_cache_check_success
@wvangeit
Fix mypy bug
e8da50c
@wvangeit
Merge branch 'master' into function_cache_check_success
9300f6e
@wvangeit
Merge branch 'master' into function_cache_check_success
45cfd1f
@wvangeit
Merge branch 'master' into function_cache_check_success
d26df1e
@wvangeit
Return the created_at data of registered function api objects
6314ffe
@wvangeit
Merge branch 'master' into add_registered_at_to_functions
fdadd8a
@wvangeit
Update openapi specs
5ebc54c
@wvangeit
Add error handlers for functions api
27494dd
@wvangeit
Merge branch 'master' into handle_function_errors_api
8df445f
@wvangeit
Fix import typecheck
f18c72b
@wvangeit
Merge branch 'master' into handle_function_errors_api
6ff8037
@wvangeit
Fix mypy typecheck
12770c5
@wvangeit
Import fastapi for status
6831d81
@wvangeit
Remove fastapi import from function models
520f55d
@wvangeit
Merge branch 'master' into handle_function_errors_api
529f40a
@wvangeit
Merge branch 'handle_function_errors_api' of github.com:wvangeit/ospa…
c2bbc45 
…rc-simcore into handle_function_errors_api
@wvangeit
Merge branch 'master' into handle_function_errors_api
e7e3db4
@wvangeit
Merge branch 'master' into handle_function_errors_api
88d14b5
@mergify
Merge branch 'master' into handle_function_errors_api
240c0a2
@wvangeit
Add check for function execute permission
539669e
@wvangeit
Merge branch 'master' into check_function_run_permissions
98fdf9c
@wvangeit wvangeit added this to the Bazinga! milestone Jun 6, 2025
@wvangeit wvangeit self-assigned this Jun 6, 2025
@wvangeit wvangeit mentioned this pull request Jun 6, 2025
Closed
@codecov
Copy link

codecov bot commented Jun 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.42%. Comparing base (aaf1079) to head (968b6e3).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7844      +/-   ##
==========================================
- Coverage   86.74%   86.42%   -0.32%     
==========================================
  Files        1852     1437     -415     
  Lines       71973    60230   -11743     
  Branches     1218      617     -601     
==========================================
- Hits        62433    52056   -10377     
+ Misses       9198     7975    -1223     
+ Partials      342      199     -143
Flag Coverage Δ
integrationtests 64.28% <57.14%> (+0.03%) ⬆️
unittests 86.12% <71.42%> (-0.37%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library 71.87% <0.00%> (-0.06%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.05% <ø> (-0.06%) ⬇️
agent 96.29% <ø> (ø)
api_server 91.74% <88.88%> (-0.01%) ⬇️
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.07% <ø> (ø)
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.08% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 89.09% <ø> (-0.06%) ⬇️
storage 87.78% <ø> (+0.31%) ⬆️
webclient ∅ <ø> (∅)
webserver 83.72% <100.00%> (-0.01%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaf1079...968b6e3. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wvangeit wvangeit added a:apiserver api-server service a:webserver webserver's codebase. Assigning the area is particularly useful for bugs labels Jun 6, 2025
@wvangeit
Copy link
Contributor Author

wvangeit commented Jun 6, 2025

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2025 •
edited
Loading

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images

matusdrobuliak66
matusdrobuliak66 reviewed Jun 6, 2025
View reviewed changes
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

General question → The lowest-level building blocks of our backend are projects wtih services inside, and they have their own sharing mechanism. Did you also consider situations where you share a function with someone, but they don't have access to the underlying projects or services?

matusdrobuliak66
matusdrobuliak66 approved these changes Jun 6, 2025
View reviewed changes
Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

👍

@wvangeit
Copy link
Contributor Author

wvangeit commented Jun 6, 2025 •
edited
Loading

General question → The lowest-level building blocks of our backend are projects wtih services inside, and they have their own sharing mechanism. Did you also consider situations where you share a function with someone, but they don't have access to the underlying projects or services?

I completely agree that we have to look into it further. This permissions were rather urgent to limit functions to single users, so not all use cases are covered yet (i.e. sharing functions). But tbh, I was kind of hoping the create study job etc would raise their own permission denied errors. But once we have a sharing endpoint, we should indeed throw errors in advance if a target user doesn't have access to the underlying project/services etc.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@matusdrobuliak66 matusdrobuliak66 enabled auto-merge (squash) June 6, 2025 15:38
@wvangeit
Copy link
Contributor Author

wvangeit commented Jun 6, 2025

@mergify refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 6, 2025

refresh

✅ Pull request refreshed

@matusdrobuliak66 matusdrobuliak66 merged commit 360fa2e into ITISFoundation:master Jun 6, 2025
93 of 95 checks passed
@wvangeit wvangeit deleted the check_function_run_permissions branch June 6, 2025 16:08
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 6, 2025
Closed
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
Closed
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GitHK GitHK GitHK approved these changes

@pcrespov pcrespov pcrespov approved these changes

@sanderegg sanderegg sanderegg approved these changes

@JavierGOrdonnez JavierGOrdonnez JavierGOrdonnez approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

Assignees

@wvangeit wvangeit

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:apiserver api-server service a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Milestone

Bazinga!

Development

Successfully merging this pull request may close these issues.

6 participants

@wvangeit @GitHK @pcrespov @sanderegg @JavierGOrdonnez @matusdrobuliak66