Skip to content

feat(domain): add permission_grant validation and principal existence check (BDMS-1534) - #1039

Open
likithabommasani21 wants to merge 1 commit into
stagingfrom
BDMS-1534-validation-for-permission-grant-input
Open

likithabommasani21 wants to merge 1 commit into
stagingfrom
BDMS-1534-validation-for-permission-grant-input

Conversation

@likithabommasani21

Copy link
Copy Markdown
Contributor

BDMS-1534: Add permission_grant validation and principal existence check

Why

This PR addresses the following problem / context:

  • permission_grant (BDMS-1532) needs validation before a grant is persisted: that capability/scope_type/principal_type are real values, and that the principal being granted access actually exists where that's checkable.

How

Implementation summary: the following was changed / added / removed:

  • domain/permission_grant.py: pure functions validate_capability, validate_scope_type, validate_principal_type -- check a value against core/enums.py (lexicon-backed), no database.
  • services/permission_grant_helper.py: principal_exists(), the one DB-dependent check. Only api_key has a local table to check against; user/principal_group pass through unchecked today.
  • tests/test_permission_grant.py: 10 tests covering both the domain validators and principal_exists.

Author Checklist

  • Changes are limited to the intended scope.
  • Tests were added or updated where appropriate.
  • Relevant tests and lint checks pass.
  • No secrets, credentials, or sensitive data were publicly exposed.
  • [N/A] Documentation was updated where necessary.

Reviewer Checklist

  • validate_capability/validate_scope_type/validate_principal_type correctly reject invalid values and accept all current lexicon terms.
  • principal_exists handles a non-numeric api_key id without raising (returns False).
  • Agree that user/principal_group passing through unchecked is acceptable for now, given no local table exists for either.
  • domain/permission_grant.py stays free of database/service imports.

Notes

Any special considerations, known limitations, risks, workarounds, or follow-up work to note?

  • principal_exists currently assumes Authentik is the sole owner of user/principal_group membership, and that no local table for either exists in our database -- so there's nothing to check them against, and they pass through unchecked.
  • Posted a comment on ADR6 to confirm this assumption, since some of its wording could be read as Ocotillo owning group membership instead. Once that's resolved: if Ocotillo ends up with a real local table for groups, add an existence check here the same way api_key has one; if not, no change needed.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Coverage

✅ 81.81% total — gate is 75%.

Coverage for the Python files changed in this PR
Name Stmts Miss Cover Missing
domain/permission_grant.py 13 0 100%
services/permission_grant_helper.py 10 0 100%
TOTAL 23 0 100%

This branch has not been deployed

No deployments
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.

1 participant