Skip to content
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

feat(permissions): system user support for permission check v2 #9460

Closed
wants to merge 59 commits into from

Conversation

kkrime
Copy link
Contributor

@kkrime kkrime commented Mar 4, 2025

Which Problems Are Solved

For permissoin check v2, we currently check the human users permissions from the DB, however for system users (from the config, which would not be in the DB), their permissions are not in the DB, but are in a config file like defaults.yaml.

  • The current check involves all users (human, machine) in the DB and excludes only system users
  • It's redundant to mention twice that system users are not in the DB.

How the Problems Are Solved

  • Can you please expand a bit on your implementation? For example:
    • what is the precedence of the permissions being applied.
    • which role-permission configuration is used for the system users (should be runtime, but currently is DB, see my other comment)
    • Mention details such as "roles are loaded once per request and cached", which you did in a separate comment.

The way the permissions work are based on the following:
https://zitadel.slack.com/archives/C087ADF8LRX/p1742207808062949?thread_ts=1742206770.965909&cid=C087ADF8LRX

Additional Changes

Important to note from this point onwards, System Users permissions will be picked up from SystemAuthz (added to default.yaml) NOT InternalAuthz

Copy link

vercel bot commented Mar 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs 🛑 Canceled (Inspect) Mar 25, 2025 6:00pm

Copy link

github-actions bot commented Mar 4, 2025

Thanks for your contribution @kkrime! 🎉

Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:

  • I am happy with the code
  • Documentations and examples are up-to-date
  • Logical behavior changes are tested automatically
  • No debug or dead code
  • My code has no repetitions
  • The PR title adheres to the conventional commit format
  • The example texts in the PR description are replaced.
  • If there are any open TODOs or follow-ups, they are described in issues and link to this PR
  • If there are deviations from a user stories acceptance criteria or design, they are agreed upon with the PO and documented.

Sorry, something went wrong.

Copy link

codecov bot commented Mar 4, 2025

Codecov Report

Attention: Patch coverage is 74.13793% with 30 lines in your changes missing coverage. Please review.

Project coverage is 63.36%. Comparing base (febf410) to head (a9fcb3e).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
cmd/setup/52.go 53.84% 4 Missing and 2 partials ⚠️
internal/integration/system.go 53.84% 4 Missing and 2 partials ⚠️
internal/api/authz/authorization.go 87.50% 3 Missing and 1 partial ⚠️
internal/query/user.go 20.00% 3 Missing and 1 partial ⚠️
internal/query/permission.go 72.72% 2 Missing and 1 partial ⚠️
cmd/mirror/projections.go 0.00% 2 Missing ⚠️
cmd/setup/setup.go 50.00% 2 Missing ⚠️
cmd/start/start.go 85.71% 1 Missing ⚠️
internal/api/http/middleware/auth_interceptor.go 87.50% 1 Missing ⚠️
internal/id/sonyflake.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9460       +/-   ##
===========================================
+ Coverage   39.00%   63.36%   +24.35%     
===========================================
  Files        1547     1627       +80     
  Lines      148805   152867     +4062     
===========================================
+ Hits        58044    96859    +38815     
+ Misses      86219    51144    -35075     
- Partials     4542     4864      +322     
Flag Coverage Δ
core-integration-tests-postgres 39.03% <76.23%> (+0.02%) ⬆️
core-unit-tests 46.25% <40.47%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kkrime kkrime force-pushed the syste-users-permissions branch from b144835 to 2947c07 Compare March 4, 2025 14:59
@kkrime kkrime force-pushed the syste-users-permissions branch from a4648ad to 601327f Compare March 4, 2025 15:16
… system user support for permission check v2
Iraq Jaber added 2 commits March 5, 2025 15:11
…ddeding system user support for permission check v2
…ons): Addeding system user support for permission check v2
…ermissions): Addeding system user support for permission check v2
… feat(permissions): Addeding system user support for permission check v2
… fixup! feat(permissions): Addeding system user support for permission check v2
… fixup! fixup! feat(permissions): Addeding system user support for permission check v2
@kkrime kkrime marked this pull request as ready for review March 6, 2025 16:28
… fixup! fixup! fixup! feat(permissions): Addeding system user support for permission check v2
@@ -30,7 +30,7 @@ services:

db:
restart: 'always'
image: 'cockroachdb/cockroach:latest-v24.3'
image: 'cockroachdb/cockroach:latest'
command: 'start-single-node --insecure --http-addr :9090'
Copy link
Contributor Author

@kkrime kkrime Mar 25, 2025

Choose a reason for hiding this comment

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

@livio-a I need to make this change otherwise it starts complaining about a unsupported feture in my sql scripts in the e2e browser pipeline

Copy link
Member

Choose a reason for hiding this comment

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

if it doesn't work with cockroach, we need to make sure it's only released as part of v3 (target branch for that is v3.x)

… Merge branch 'main' into syste-users-permissions
@kkrime kkrime changed the base branch from main to v3.x March 25, 2025 19:48
@kkrime kkrime changed the base branch from v3.x to main March 25, 2025 19:48
@muhlemmer muhlemmer disabled auto-merge March 26, 2025 14:16
@muhlemmer
Copy link
Collaborator

Replaced by #9640

@muhlemmer muhlemmer closed this Mar 26, 2025
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.

Handle system users for database permissions
3 participants