Skip to content

Add global functions user permissions 🎨 #7868

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

Merged

Conversation

wvangeit
Copy link
Contributor

@wvangeit wvangeit commented Jun 10, 2025

What do these changes do?

This will allow control of which users/user groups are allowed to read/write(create)/execute functions, function jobs and function job collections, on a global level per product. This is different from a PR where the permissions 'per function object' were introduced.

This could be linked to the frontend in a future PR, by exposing a webserver rest api where the frontend can ask for the global permissions of a user to affect e.g. the appearance of the 'create function' panel.

Related issue/s

How to test

Try to run/read/create a function as a user who doesn't have access to, the appropriate permission denied error should be returned.

Dev-ops

No changes to ENV.
Migration script will introduce a new table that will by default deny all access to function abilities to all users. To enable access for a certain group, add a row to the funcapi_group_api_access_rights table, with in each column a bool enabling/disabling the particular right for that group.

@wvangeit wvangeit added this to the Bazinga! milestone Jun 10, 2025
@wvangeit wvangeit self-assigned this Jun 10, 2025
@wvangeit wvangeit added the a:webserver issue related to the webserver service label Jun 10, 2025
@wvangeit wvangeit added the a:database associated to postgres service and postgres-database package label Jun 10, 2025
@wvangeit wvangeit added the a:apiserver api-server service label Jun 10, 2025
@wvangeit wvangeit requested a review from bisgaard-itis June 10, 2025 17:36
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 85.84071% with 16 lines in your changes missing coverage. Please review.

Project coverage is 87.98%. Comparing base (264dc42) to head (a3892a9).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7868      +/-   ##
==========================================
+ Coverage   87.80%   87.98%   +0.18%     
==========================================
  Files        1791     1753      -38     
  Lines       69717    68606    -1111     
  Branches     1220     1065     -155     
==========================================
- Hits        61214    60366     -848     
+ Misses       8151     7924     -227     
+ Partials      352      316      -36     
Flag Coverage Δ
integrationtests 64.25% <37.50%> (-0.03%) ⬇️
unittests 86.52% <85.84%> (+0.15%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 93.25% <100.00%> (+0.05%) ⬆️
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.18% <100.00%> (+0.02%) ⬆️
pkg_service_integration 69.92% <ø> (ø)
pkg_service_library 72.59% <0.00%> (-0.05%) ⬇️
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.10% <ø> (+0.11%) ⬆️
agent 96.29% <ø> (ø)
api_server 91.84% <100.00%> (+0.08%) ⬆️
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.57% <ø> (-0.23%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (ø)
director_v2 91.12% <ø> (+0.05%) ⬆️
dynamic_scheduler 96.69% <ø> (∅)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.00% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 88.98% <ø> (-0.11%) ⬇️
storage 87.46% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 87.47% <77.08%> (-0.16%) ⬇️

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 264dc42...a3892a9. 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 requested a review from odeimaiz June 10, 2025 17:40
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

Looks sound.
I am wondering a few things here:

  • why are the permissions + access rights only checked on the webserver level and not already in the api-server?
  • what is the concept/difference between permissions and abilities?
  • you are missing some tests on these new exceptions on the api-server side.

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

Some initial questions

@wvangeit
Copy link
Contributor Author

Looks sound. I am wondering a few things here:

* why are the permissions + access rights only checked on the webserver level and not already in the api-server?

* what is the concept/difference between permissions and abilities?

* you are missing some tests on these new exceptions on the api-server side.

Wrt to 1. I just checked on the deepest level which looked like the safest/most efficient. If i do it in the api-server I would also have to pull the permissions out of the db with rpc calls. I consider it better to avoid that and let the repository layer handle all this immediately? One exception is running functions, because there the function is implemented in the api server, for now.
Wrt to 2. See my other comment to you about access rights.
Wrt to 3. Yes, I added a test.

@wvangeit wvangeit requested a review from sanderegg June 13, 2025 09:42
@wvangeit
Copy link
Contributor Author

@mergify queue

@wvangeit wvangeit added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 13, 2025
Copy link
Contributor

mergify bot commented Jun 13, 2025

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • #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
      • 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
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@wvangeit
Copy link
Contributor Author

@mergify update

Copy link
Contributor

mergify bot commented Jun 13, 2025

update

☑️ Nothing to do, the required conditions are not met

  • #commits-behind > 0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position = -1 [📌 update requirement]

@wvangeit
Copy link
Contributor Author

@mergify refresh

Copy link
Contributor

mergify bot commented Jun 13, 2025

refresh

✅ Pull request refreshed

@wvangeit wvangeit enabled auto-merge (squash) June 13, 2025 14:19
@wvangeit
Copy link
Contributor Author

@mergify refresh

Copy link
Contributor

mergify bot commented Jun 13, 2025

refresh

✅ Pull request refreshed

@wvangeit
Copy link
Contributor Author

@mergify queue

Copy link
Contributor

mergify bot commented Jun 13, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • #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
      • 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
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

Copy link

@wvangeit wvangeit modified the milestones: Bazinga!, Engage Jun 13, 2025
@wvangeit
Copy link
Contributor Author

@mergify refresh

Copy link
Contributor

mergify bot commented Jun 13, 2025

refresh

✅ Pull request refreshed

@pcrespov pcrespov disabled auto-merge June 13, 2025 15:35
@pcrespov pcrespov merged commit 23a82ba into ITISFoundation:master Jun 13, 2025
145 of 148 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖-automerge marks PR as ready to be merged for Mergify a:apiserver api-server service a:database associated to postgres service and postgres-database package a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants