Skip to content

Add internal user authz#224

Open
petersbingham wants to merge 1 commit into
openkcm:mainfrom
petersbingham:feature/internal-user-authz
Open

Add internal user authz#224
petersbingham wants to merge 1 commit into
openkcm:mainfrom
petersbingham:feature/internal-user-authz

Conversation

@petersbingham
Copy link
Copy Markdown
Contributor

@petersbingham petersbingham commented Apr 2, 2026

What this PR does / why we need it:


Special notes for your reviewer:


Release note:


Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced role-based authorization framework for internal operations, including dedicated roles for task processing, tenant provisioning, and workflow management.
  • Bug Fixes

    • Fixed authorization bypass where system operations previously bypassed access control checks.
  • Chores

    • Refactored authorization enforcement across async tasks, managers, and API controllers to use the new role-based system.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 2, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67b59964-a868-4023-aca4-6dba34f52065

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

This pull request introduces a comprehensive refactoring of the authorization system to distinguish between business users and internal system roles. Key changes include: splitting the single Role type into BusinessRole and InternalRole constants; refactoring authorization handlers to support separate business-user and internal-user authorization paths; separating API-level and repository-level authorization loaders; updating context injection to use BusinessSource and InternalSource markers; replacing system-user hardcoded checks with role-based authorization validation; adding ProcessTenantsInBatchWithOptions to control batch processing failure modes; and updating constructors across managers, tasks, and controllers to wire authorization dependencies. The auditor's fallback behavior for missing client context changes from using constants.SystemUser to uuid.Max. Throughout, business operations now explicitly inject business client data, and internal/automated operations inject internal role data for authorization checks.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@petersbingham petersbingham marked this pull request as ready for review April 13, 2026 11:51
Comment thread internal/authz/repo_internal_policies.go Outdated
@petersbingham petersbingham force-pushed the feature/internal-user-authz branch from c3180e4 to 8d31f20 Compare April 17, 2026 15:20
Comment thread internal/authz/repo_base.go
Copy link
Copy Markdown
Contributor

@minh-nghia minh-nghia left a comment

Choose a reason for hiding this comment

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

Will also need to cover event reconciler, mainly these functions:
resolveTasks, confirmJob, jobDoneFunc, jobFailedFunc, jobCanceledFunc

Comment thread utils/context/context.go
Comment thread internal/middleware/clientdata.go
@anettlippert
Copy link
Copy Markdown
Contributor

@coderabbitai /summarize

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 27, 2026

@anettlippert: I'll regenerate the summary for this PR right away!

✅ Actions performed

Summary regeneration triggered.

Comment thread cmd/tenant-manager-cli/commands/commands.go
Comment thread cmd/task-worker/main.go
r := sql.NewRepository(dbCon)

sis, err := manager.NewSystemInformationManager(r, svcRegistry, &cfg.ContextModels.System)
sis, err := manager.NewSystemInformationManager(r, nil, svcRegistry, &cfg.ContextModels.System)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here, maybe we can have two functions: NewSystemInformationManager NewSystemInformationManagerWithoutAuthz so it is more clear

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This also applies to the NewSystemManager. I think the problem is more that this really should have the loader and not nil, so there is basically missing authz here, which I've fixed. Thanks for pointing this out.

There are still instances in the test code where this is nil. Based on previous comment by @jmpTeixeira02 the preference is to avoid lists of specifically named functions (although I'll leave Joao to comment if the context is different here) and also 1) now that this only affects test code I don't think worth introducing another function for this; 2) other parameters are set to nil in the test code (eg eventsFactory for newSystemManager).

Comment thread internal/auditor/auditor.go
Comment thread internal/authz/authorization_data.go
@petersbingham petersbingham force-pushed the feature/internal-user-authz branch from 8d31f20 to e0b45c8 Compare April 30, 2026 15:12
@petersbingham petersbingham force-pushed the feature/internal-user-authz branch from e0b45c8 to 611cf38 Compare April 30, 2026 15:14
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.

3 participants