Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Sep 4, 2025

What do these changes do?

This pull request introduces group-based permissions for product support groups.

The main changes include adding a new decorator to check permissions via both user roles and/or group membership, updating user and group service APIs to use a stricter UserNameID type, and improving how pre-registered user information is retrieved.

Access Control Enhancements

  • Added group_or_role_permission_required decorator to enable permission checks via both user roles and/or group membership, specifically supporting product support groups (PRODUCT_SUPPORT_GROUP). This allows users to access endpoints if they have the required permission either through their role or by being in a designated group. [1] [2] [3] [4]
  • Defined NAMED_GROUP_PERMISSIONS mapping for group-based permissions, starting with support for PRODUCT_SUPPORT_GROUP.
    • Permissions: read access to user accounts and product details
      • product.details.*
      • admin.users.read
    • Endpoints:
      • GET /products/{product_name}ProductGet
      • GET /admin/user-accountsPage[UserAccountGet]
      • GET /admin/user-accounts:search?email=glob*UserAccountGet
        • by email glob
        • by primary group id
        • by user name (glob)

User and Group Service API Refactoring

  • Introduced and enforced the stricter UserNameID type (annotated string) throughout user and group service APIs, replacing the previous IDStr alias for improved validation and clarity. [1] [2] [3] [4] [5] [6] [7] [8]
  • Added new is_user_in_group service and repository functions to check group membership by user and group IDs, supporting new permission logic. [1] [2] [3] [4]

Product Support Group Utilities

  • Added is_user_in_product_support_group utility to check if a user belongs to the support group for the current product, enabling group-based permission checks for product-related endpoints. [1] [2] [3]

Related issue/s

How to test

cd services/web/server
make install-dev
pytest -v tests/unit/**/test*user*.py

Dev-ops

None

@pcrespov pcrespov added this to the Cheops milestone Sep 4, 2025
@pcrespov pcrespov self-assigned this Sep 4, 2025
@codecov
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

❌ Patch coverage is 84.42623% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.26%. Comparing base (bbb7767) to head (6baa179).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8313      +/-   ##
==========================================
+ Coverage   79.47%   88.26%   +8.79%     
==========================================
  Files        1561     1526      -35     
  Lines       61010    63191    +2181     
  Branches      801      499     -302     
==========================================
+ Hits        48487    55776    +7289     
+ Misses      12311     7286    -5025     
+ Partials      212      129      -83     
Flag Coverage Δ
integrationtests 64.02% <35.84%> (-0.07%) ⬇️
unittests 88.62% <84.42%> (+11.29%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 93.08% <66.66%> (-0.07%) ⬇️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.15% <ø> (+0.11%) ⬆️
agent 93.53% <ø> (ø)
api_server 91.91% <100.00%> (∅)
autoscaling 95.77% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 92.15% <ø> (+0.33%) ⬆️
datcore_adapter 97.94% <ø> (ø)
director 75.81% <ø> (ø)
director_v2 78.08% <ø> (-0.02%) ⬇️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.39% <ø> (+8.51%) ⬆️
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.18% <ø> (-0.11%) ⬇️
storage 86.28% <ø> (∅)
webclient ∅ <ø> (∅)
webserver 88.00% <86.79%> (+21.90%) ⬆️

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 bbb7767...6baa179. 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.

@mergify
Copy link
Contributor

mergify bot commented Sep 4, 2025

🧪 CI Insights

Here's what we observed from your CI run for 6baa179.

❌ Job Failures

Pipeline Job Health on master Retries 🔍 CI Insights 📄 Logs
CI unit-tests Broken 0 View View

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI system-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@pcrespov pcrespov force-pushed the is340/support-center-new-role branch from 40a7ec5 to d8258b1 Compare September 5, 2025 09:49
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Sep 5, 2025
@pcrespov pcrespov changed the title WIP:✨ Is340/New PRODUCT_SUPPORT role has access to admin.account permissions WIP:✨ Support Center: Is340/New PRODUCT_SUPPORT role has access to admin.account permissions Sep 5, 2025
@pcrespov pcrespov changed the title WIP:✨ Support Center: Is340/New PRODUCT_SUPPORT role has access to admin.account permissions ✨ Support Center: Add PRODUCT_SUPPORT Role with Read Access to User Accounts and Product Details Sep 5, 2025
@pcrespov pcrespov marked this pull request as ready for review September 5, 2025 12:58

This comment was marked as outdated.

@pcrespov pcrespov added the 🤖-do-not-merge (optional) blocks Mergify from merging the PR label Sep 5, 2025
@pcrespov
Copy link
Member Author

pcrespov commented Sep 5, 2025

After discussing with @odeimaiz, we decided to remove the PRODUCT_SUPPORT role and instead adopt a group-based access strategy.

  • A support group is a STANDARD group that is explicitly marked as support for one or more products (in the products table).
  • Each product can have only one support group.
  • A user in a support group will:
    • inherit all the usual access rights granted to that organization’s resources (like any STANDARD group), and
    • gain extended read access to all user and product information.
  • The administrator/s of the support group can add/remove other users to the support group

In the UI, support groups should be clearly distinguished: they are not just resource-sharing organizations, but also provide extended access.

  • This **distinction SHOULD be automatically reflected front-end with e.g.
    • Fixed title
    • Fixed Description (e.g. similar to above)
    • special icon/color etc
  • A warning message with the extended access rights above should be prompt to the administrator when a new user is added/removed.

@pcrespov pcrespov force-pushed the is340/support-center-new-role branch from 7948ec9 to eb68d9b Compare September 8, 2025 09:16
@pcrespov pcrespov changed the title ✨ Support Center: Add PRODUCT_SUPPORT Role with Read Access to User Accounts and Product Details ✨ Support Center: Gives Read Access to Support Users to User Accounts and Product Details Sep 8, 2025
@pcrespov pcrespov requested a review from Copilot September 8, 2025 09:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new PRODUCT_SUPPORT user role that provides read-only access to user accounts and product details for support teams. The implementation extends the existing permission system to support both role-based and group-based authorization.

  • Adds PRODUCT_SUPPORT role with specific read permissions for user accounts and product details
  • Implements hybrid authorization allowing access via either user role OR group membership
  • Updates REST endpoints to use the new permission decorator for flexible access control

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
services/web/server/src/simcore_service_webserver/security/decorators.py Adds new group_or_role_permission_required decorator for hybrid authorization
services/web/server/src/simcore_service_webserver/security/_authz_access_roles.py Defines permissions for PRODUCT_SUPPORT role and introduces group-based permissions mapping
services/web/server/src/simcore_service_webserver/users/_controller/rest/accounts_rest.py Updates user account endpoints to use hybrid authorization decorator
services/web/server/src/simcore_service_webserver/products/_controller/rest.py Updates product details endpoint to use hybrid authorization decorator
services/web/server/src/simcore_service_webserver/users/_accounts_repository.py Adds username lookup for account reviewers instead of just user IDs
packages/models-library/src/models_library/users.py Refactors UserNameID type definition to use proper string constraints
packages/models-library/src/models_library/api_schemas_webserver/users.py Updates UserAccountGet model to use UserNameID for reviewer fields
services/web/server/tests/unit/with_dbs/03/invitations/test_users_accounts_rest_registration.py Updates tests to expect reviewer names instead of IDs and adjusts role filtering logic

@pcrespov pcrespov requested a review from sanderegg September 8, 2025 09:59
@pcrespov pcrespov marked this pull request as draft September 8, 2025 10:00
@pcrespov pcrespov changed the title ✨ Support Center: Gives Read Access to Support Users to User Accounts and Product Details WIP: ✨ Support Center: Gives Read Access to Support Users to User Accounts and Product Details Sep 8, 2025
@pcrespov pcrespov force-pushed the is340/support-center-new-role branch from 978661d to 24f7dbc Compare September 8, 2025 13:25
@pcrespov pcrespov changed the title WIP: ✨ Support Center: Gives Read Access to Support Users to User Accounts and Product Details ✨ Support Center: Gives Read Access to Support Users to User Accounts and Product Details Sep 8, 2025
@pcrespov pcrespov force-pushed the is340/support-center-new-role branch from ef8f1ab to fa404d3 Compare September 9, 2025 07:29
@pcrespov
Copy link
Member Author

pcrespov commented Sep 9, 2025

What is then the new query parameter I need to pass in the search? GET /admin/user-accounts:search?

@odeimaiz SEE

/v0/admin/user-accounts:search:
get:
tags:
- users
- admin
summary: Search User Accounts
operationId: search_user_accounts
parameters:
- name: email
in: query
required: false
schema:
anyOf:
- type: string
minLength: 3
maxLength: 200
pattern: ^[^%]*$
- type: 'null'
title: Email
- name: primary_group_id
in: query
required: false
schema:
anyOf:
- type: integer
exclusiveMinimum: true
minimum: 0
- type: 'null'
title: Primary Group Id
- name: user_name
in: query
required: false
schema:
anyOf:
- type: string
minLength: 3
maxLength: 200
pattern: ^[^%]*$
- type: 'null'
title: User Name
responses:
'200':
description: Successful Response
content:
application/json:
schema:
$ref: '#/components/schemas/Envelope_list_UserAccountGet__'

@odeimaiz
Copy link
Member

odeimaiz commented Sep 9, 2025

What is then the new query parameter I need to pass in the search? GET /admin/user-accounts:search?

@odeimaiz SEE

/v0/admin/user-accounts:search:
get:
tags:
- users
- admin
summary: Search User Accounts
operationId: search_user_accounts
parameters:
- name: email
in: query
required: false
schema:
anyOf:
- type: string
minLength: 3
maxLength: 200
pattern: ^[^%]*$
- type: 'null'
title: Email
- name: primary_group_id
in: query
required: false
schema:
anyOf:
- type: integer
exclusiveMinimum: true
minimum: 0
- type: 'null'
title: Primary Group Id
- name: user_name
in: query
required: false
schema:
anyOf:
- type: string
minLength: 3
maxLength: 200
pattern: ^[^%]*$
- type: 'null'
title: User Name
responses:
'200':
description: Successful Response
content:
application/json:
schema:
$ref: '#/components/schemas/Envelope_list_UserAccountGet__'

@pcrespov I CAN SEE IT NOW, THX

@pcrespov pcrespov enabled auto-merge (squash) September 9, 2025 07:44
@pcrespov
Copy link
Member Author

pcrespov commented Sep 9, 2025

@mergify queue

@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Sep 9, 2025
@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2025

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • any of: [🛡 GitHub branch protection]
        • check-neutral = unit-tests
        • check-skipped = unit-tests
        • check-success = unit-tests
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=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 = 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
      • any of: [🛡 GitHub branch protection]
        • check-success = SonarCloud Code Analysis
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@pcrespov pcrespov disabled auto-merge September 9, 2025 10:19
@pcrespov pcrespov merged commit 3f7c322 into ITISFoundation:master Sep 9, 2025
3 of 4 checks passed
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 9, 2025

@pcrespov pcrespov deleted the is340/support-center-new-role branch September 9, 2025 11:19
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:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants