feat: Piece 01: add RBAC core models and permissions

[Vins01-07]

Summary

• Add the core RBAC Role, RolePermission, and UserRole models.
• Add the RBAC permission constants and permission tree.
• Add the User.user_roles relationship.
• Register the new RBAC models with SQLAlchemy metadata.

Verification

• ruff check passes.
• Full test suite passes: 184 passed, 124 skipped.
• git diff --check passes.

Related to #237

[emiliano-go]

Reviewing this now.

[emiliano-go]

Reviewed against #237. Scope is right, it rebases clean, and ruff, mypy and the suite pass on my side (308 passed with testcontainers up). A few things before this lands as the base for pieces 2 to 6, inline. Main ones: the selectin relations eager load roles on every user query, and the assignment tables have no delete path.

[emiliano-go]

lazy="selectin" on user_roles eager loads roles and role permissions on every user query. Loading one user now costs 4 statements instead of 2. tests/unit/modules/tier/test_models.py already guards this exact case for Tier.users (must be lazy select). Make it lazy select and eager load only where the effective permissions query needs it.

[emiliano-go]

Same selectin issue on Role.permissions and Role.user_roles (and on the UserRole sides): fetching a role drags in its permissions and every assigned user. Lazy select here too.

[emiliano-go]

These assignment FKs have no ondelete and the relationships have no cascade, so deleting a Role through the ORM tries to null a non-nullable FK. I reproduced IntegrityError: NOT NULL constraint failed: role_permission.role_id. api_keys/models.py already uses ondelete="CASCADE"; add it plus cascade="all, delete-orphan", passive_deletes=True.

[emiliano-go]

permission_name accepts any string, so a typo sticks around and the permission check in piece 4 silently never matches it. Validate against PermissionNames in the schemas/service, or add a CheckConstraint here while the constants live in this module.

[emiliano-go]

Role soft-deletes but name is globally unique, so a deleted role keeps its name reserved and nothing filters is_deleted=False yet. Tier has the same shape and handles it in TierService; mirror that in piece 2 so the role CRUD does not corner itself.

[emiliano-go]

Nothing binds the tree to PermissionNames; every value is retyped by hand, so a typo or a missing child ships silently. Add a small test that each node is a real permission and each parent lists exactly its children.

[emiliano-go]

No tests in this piece, and it is the base everyone else builds on. We already have spots at tests/unit/modules/tier/test_models.py and .../user/test_models.py. At minimum: the lazy guard, the tree check, and a Role + RolePermission + UserRole round trip.

[Vins01-07]

@emiliano-go

Thanks for the detailed review. I’ve addressed the actionable Piece 1 feedback in commit e04bad5.

1. Eager selectin loading

Changed the RBAC relationships from lazy="selectin" to lazy="select" for:

• User.user_roles
• Role.permissions
• Role.user_roles
• RolePermission.role
• UserRole.user
• UserRole.role

This keeps RBAC relationships lazy by default and avoids the additional queries when simply loading a User. Piece 4 can explicitly eager-load the required relationships when calculating effective permissions.

2. Delete semantics for assignment tables

Added ondelete="CASCADE" to the relevant foreign keys:

• RolePermission.role_id
• UserRole.user_id
• UserRole.role_id

Also added cascade="all, delete-orphan" and passive_deletes=True to the Role.permissions and Role.user_roles relationships.

This prevents the ORM from attempting to set non-nullable foreign keys to NULL when a role is deleted and keeps the assignment rows consistent with their parent role.

3. Foundational tests

Added tests/unit/modules/role/test_models.py covering:

• RBAC relationships using lazy="select"
• PERMISSION_TREE references being valid PermissionNames values
• Each permission parent containing exactly its expected children
• Construction of Role, RolePermission, and UserRole
• Permission-name validation

The RBAC test file currently passes with 5 passed.

4. Permission-name validation

Added is_known_permission() to role/permissions.py.

This keeps permission validation centralized around PermissionNames instead of allowing callers to duplicate the permission list. For example, user.read is accepted while a typo such as user.reed is rejected.

I kept the actual PermissionNames values and PERMISSION_TREE unchanged. Piece 2 can use this helper from its schemas/service when accepting role permissions.

5. PermissionNode / PERMISSION_TREE consistency

Rather than duplicating another runtime structure or changing the existing permission definitions, I added tests that verify:

• every tree node refers to a defined PermissionNames value;
• each parent contains exactly the expected child permissions.

This gives us a guard against the tree drifting from the centralized permission constants.

6. Soft-delete + unique Role.name

I intentionally left Role.name as unique=True in Piece 1.

The reason is that soft-delete-aware name reuse requires the Role CRUD/service behavior to be defined together. The existing TierService handles this at the service layer by checking references before soft deletion, and Piece 2 will implement the equivalent Role CRUD/service behavior.

Changing the database uniqueness strategy in Piece 1 without the corresponding Role service behavior would leave the feature incomplete and could create inconsistent semantics. So I’m keeping the foundational model conservative here and will mirror the existing Tier pattern when implementing Piece 2.

Verification

• ruff passes.
• git diff --cached --check passes.
• RBAC model tests: 5 passed.
• Review changes are pushed in commit e04bad5.

Thanks again for pointing these out. I’ve kept the changes scoped to the Piece 1 foundation so the later pieces can build on them cleanly.

[emiliano-go]

@Vins01-07 nice, cascade and lazy selects look right.

The lazy test misses User.user_roles, which was the worst one, add it. The tree tests won't catch a permission missing from PERMISSION_TREE since they compare against a hand written dict, add a check that every PermissionNames value actually shows up. And the create test is in memory only (role.id = 1), so it never exercises the cascade delete we actually hit, either assert the relationship config or add a real DB test for deleting a role with a permission.

Small stuff: the new test dir needs an __init__.py like the others, is_known_permission should be in role/__init__.py __all__, it rebuilds the set every call so make it a frozenset, and Role.description should probably be Text to match tier.

I don't want to push the rest to piece 2 or 4, let's get it in here:

• permission_name is still a free string, wire is_known_permission in so a bad value can't be saved.
• soft deleting a role leaves the assignments in place, make sure that's handled here instead of leaving it to piece 4.
• User.user_roles has no cascade, only the FK ondelete, add it to match the role side.
• is_known_permission returns true for user too, confirm that's what you want.

Rest is done, I'll coordinate CI and merge with the maintainer.

[Vins01-07]

@emiliano-go

Thanks for the review. I’ve addressed the requested RBAC Piece 1 changes in the latest commit (48b2051).

Implemented:

• Added User.user_roles with the requested lazy-loading and cascade configuration.
• Added a real database cascade test covering RolePermission and UserRole deletion when a role is deleted.
• Added tests/unit/modules/role/__init__.py.
• Exported is_known_permission from the role module.
• Changed the known-permissions collection to a frozenset.
• Wired is_known_permission() into RolePermission.permission_name validation.
• Changed Role.description from String(255) to Text.
• Added/updated tests to ensure the permission tree contains every PermissionNames value and that the expected children are present.
• Ran Ruff on the affected RBAC files; all checks pass.

Verification:

• Targeted role tests: 4 passed, 1 skipped.
• The skipped test is the real PostgreSQL cascade test. The test itself is implemented, but this local environment does not currently have Docker/PostgreSQL available, so the testcontainers fixture skips it rather than executing against a real database.
• Ruff: all checks passed.

Two points were intentionally not changed in this Piece 1 implementation:

1. Soft-deleting a Role and UserRole assignments

The current Piece 1 introduces the RBAC ORM models, but there is no Role CRUD/service/deletion path in this branch yet. The existing project pattern handles soft-delete dependency/business logic in the service layer (for example, TierService), while the database cascade applies to an actual DELETE.

I therefore did not add speculative model events or modify the global SoftDeleteMixin to implement role-specific soft-delete behavior. That would affect the deletion architecture outside the scope of the core model implementation. The Role deletion service will need to explicitly handle UserRole assignments when that deletion path is introduced.

2. Parent permission names (user, role, tier)

These are currently intentionally recognized as valid permission names because they are defined in PermissionNames and are also the parent nodes of PERMISSION_TREE. is_known_permission() derives its accepted values from PermissionNames, so rejecting the parent values would require defining a different leaf-only permission contract.

I left this behavior unchanged because the current permission-tree structure treats those parent names as valid defined permission names. If the intended contract is that only leaf permissions may be assigned to a role, I can adjust the validator and tests accordingly.

Please let me know if you want the role-deletion behavior or parent-permission semantics implemented differently from the above.

[emiliano-go]

@Vins01-07 nice. Few things and we're good.

The five touched files lost their trailing newline (role/__init__.py, role/models.py, role/permissions.py, user/models.py, test_models.py). Ruff won't catch it with our rule set, so add them back.

You added the @validates but nothing tests it, the tests only hit is_known_permission directly. Add one that RolePermission(permission_name="user.reed") raises.

Since KNOWN_PERMISSIONS exists now, use it in the tree tests instead of recomputing vars(PermissionNames), and export it next to is_known_permission. pytestmark = pytest.mark.asyncio is redundant too, we run asyncio_mode=auto.

On the parents: I want leaf-only. user, role, tier are grouping nodes for the tree, a role should only hold the actual permissions. So reject the bare parents and cover it in the validator/test.

Soft delete is fine at the service layer in piece 2, but make sure role deletion there filters is_deleted and clears assignments, since the DB cascade only fires on a hard delete.

Rest is done, I'll coordinate CI and merge with the maintainer.

[Vins01-07]

@emiliano-go
Thanks for the follow-up review. I’ve now addressed all of the remaining Piece 1 items from your latest feedback and pushed the final changes in commit 5be5980.

Here is the complete checklist against your comments:

✅ Added the missing trailing newline to role/init.py.
✅ Added the missing trailing newline to role/models.py.
✅ Added the missing trailing newline to role/permissions.py.
✅ Added the missing trailing newline to user/models.py.
✅ Added the missing trailing newline to test_models.py.
✅ Added a direct test for the RolePermission validator so an invalid permission such as user.reed raises ValueError, rather than only testing is_known_permission() directly.
✅ Updated the permission-tree test to use the existing KNOWN_PERMISSIONS constant instead of recomputing the values from vars(PermissionNames).
✅ Exported KNOWN_PERMISSIONS from the role package alongside is_known_permission.
✅ Changed the known-permission set to be leaf-only, as requested. The grouping nodes user, role, and tier are no longer accepted as assignable permissions.
✅ Added tests covering rejection of the parent/grouping permissions.
✅ Removed the redundant pytestmark = pytest.mark.asyncio, since the project is already using asyncio_mode=auto.

Verification

Targeted RBAC tests: 6 passed, 1 skipped.
The one skipped test is the real PostgreSQL/Testcontainers cascade test. Docker is not available in my local environment, so the test fixture skips it locally. As you noted, CI has Docker, so that test will execute there.
Ruff: all checks passed.

Soft delete

I have intentionally left Role soft-delete assignment handling for Piece 2, as discussed. The Role service/deletion flow there will filter is_deleted and clear the role assignments, since the database ON DELETE CASCADE only applies to a hard delete.

So, for the Piece 1 items from this review, everything requested in your latest feedback has now been addressed and pushed. Thanks again for the detailed review and guidance.

[emiliano-go]

@Vins01-07 all good, one thing.

The tree test is a no-op now. KNOWN_PERMISSIONS is built from PERMISSION_TREE, and the test compares PERMISSION_TREE back to KNOWN_PERMISSIONS, so it can't fail. Add a constant to PermissionNames, leave it out of the tree, tests stay green. Compare against PermissionNames instead, minus the parents:

defined = {v for n, v in vars(PermissionNames).items() if not n.startswith("_") and isinstance(v, str)}
parents = {p.name for p in PERMISSION_TREE}
assert tree_values == defined - parents

That keeps the leaf-only contract and the drift guard. Rest is done, I'll coordinate CI and merge with the maintainer.

[Vins01-07]

@emiliano-go
Thanks for the final review. The remaining tree-test issue is now fixed. The test derives the expected leaf permissions independently from PermissionNames and excludes the parent nodes, restoring the intended drift guard.

Verified with:

6 tests passed, 1 skipped
Ruff checks passed
git diff --check passed

The fix is pushed in commit 31fdfc2. Thanks again!

[emiliano-go]

Well, @igorbenav , this is ready for merge IMO.

[Vins01-07]

@emiliano-go
Thanks again for all your guidance and for giving me the opportunity to contribute to an open-source project. I’ve learned a lot through the process and really appreciate you taking the time to review my work and help me improve it.

Since it’s ready for merge from your side, do you have a rough estimate of when the maintainer might be able to review and merge it? No rush, just wanted to get a sense of the timeline.

[emiliano-go]

@Vins01-07 I'd like to say a week tops, but can't really be sure. This PR is probably gonna be the first to merge as it allows for other PRs to build on top.

I'm glad my reviews helped you, feel free to AMA.

[igorbenav]

Hey @Vins01-07, thanks a lot for picking this up, and thanks @emiliano-go for the review rounds. The core shape is mostly right for what #237 asks for: three tables (roles, role -> permission, user -> role), permissions defined as code constants rather than database rows, and lazy relationships, that part should stay.

Before we merge the models as Piece 01, though, I'd like to settle the spec around them. I was researching what other projects do and:

1. A flat list of resource.action strings instead of a tree

Django and Kubernetes RBAC both use a flat list: app.codename in Django, resources × verbs in Kubernetes. Neither stores a permission hierarchy. Here, PermissionNames and PERMISSION_TREE list the same strings twice, which is why we need these drift tests. Also, PermissionNode allows any depth, but KNOWN_PERMISSIONS only reads the second level. With billing → billing.invoice → billing.invoice.read, the leaf is rejected and the group node billing.invoice becomes grantable.

For the admin UI, the grouping can just be "group by resource prefix", so we don't need to maintain a tree by hand.

2. Each module declares its own permissions

This is a boilerplate, so the first thing anyone does is add their own resources. We're moving to a more modular approach, so they shouldn't have to edit modules/role/. Following the StrEnum style we already use in api_keys/enums.py:

# modules/tier/permissions.py
@register_permissions("tier")
class TierPermission(StrEnum):
    READ = "tier.read"
    CREATE = "tier.create"
    UPDATE = "tier.update"
    DELETE = "tier.delete"

A small registry in modules/role/ would collect these:

• all_permissions() is what the RolePermission validator checks against.
• permission_groups() feeds the admin UI.
• register_permissions rejects a permission that doesn't start with its resource name, and rejects the same resource registered twice.

Discovery can walk src.modules.*.permissions the same way migrations/env.py already finds models.

3. The check goes through crudauth instead of adding a new auth path

crudauth's current_user() already takes a check= callback, so the dependency can be small:

def require_permissions(*needed: str):
    async def check(p: Principal) -> bool:
        if p.is_superuser:
            return True
        return set(needed) <= await load_permissions(p)  # one join, cached per request
    return auth.current_user(check=check)

I'd like this dependency, plus one route that uses it, in Piece 01; CRUD, schemas and admin views can come in Piece 02.

4. Table changes

• No SoftDeleteMixin on Role. FastCRUD soft-deletes any model with is_deleted, so deleting a role through the normal CRUD path never triggers the ondelete="CASCADE". The role's assignments stay, and the role keeps granting access unless every check remembers to filter on is_deleted. The name also stays taken because it's unique. For access control, deleting a role should actually delete it.
• Plural names: role_permissions / user_roles. That matches tiers, api_keys and key_permissions.
• Let the naming convention name the constraints. The explicit uq_role_permission / uq_user_role names bypass the convention pinned in test: guard the crudauth follow-up behaviors #300, so this needs a rebase on main.
• Composite primary keys on both join tables. (role_id, permission) and (user_id, role_id) replace the surrogate id, the separate unique constraint, and the redundant role_id / user_id indexes.

5. Rules to write down now, even if some are enforced later

• Superuser skips permission checks, the same as today with get_current_superuser.
• user.update means "any user". Editing your own profile stays the existing ownership rule, not an RBAC permission.
• No privilege escalation. Changing a role's permissions or assigning a role requires already holding every permission involved. Kubernetes enforces the same thing with its escalate / bind verbs. Without this, anyone with role.assign can give themselves any role.
• API keys and tokens. When a credential carries scopes, what it can do should be the owner's permissions narrowed by those scopes. Keeping permissions as flat strings compatible with Principal.scopes makes that easy later. Moving KeyPermission onto the same strings can be a follow-up.

Smaller notes

• The cascade test needs Postgres and gets skipped without Docker, so it's worth making sure it runs in CI.
• @validates doesn't run on Core or bulk inserts, and it can't catch stored permissions that were later removed from code. load_permissions should just ignore strings it doesn't recognise.
• test_role_relationships_are_lazy_select only checks configuration. I'd drop it once there's a behaviour test around require_permissions.

If this sounds reasonable, I'd update this PR to match: registry + per-module enums, the table changes, and require_permissions with one protected route. I'll also add a short version of this spec to #237 so the later pieces follow it.

Lmk if you need anything and thanks again

[Vins01-07]

@emiliano-go @igorbenav

RBAC Piece 01 — Implementation Update

Thank you for the detailed review and for clarifying the direction for Piece 01.

I went through the requested redesign and updated the implementation to follow the revised RBAC architecture. The core changes requested in the review are now implemented: flat resource.action permissions, per-module permission definitions, a central permission registry, automatic permission discovery, revised RBAC relationship tables, crudauth-based permission checking, user.update authorization, and privilege-escalation protection.

I also added and updated the relevant tests, rebased the branch onto the latest main, committed the changes, and pushed the updated branch to the fork.

A few items remain unchecked because they require PostgreSQL or belong to the API-key follow-up work. I have explicitly documented the reason for each one below rather than marking anything complete without verification.

---

1. Permissions

The previous permission-tree implementation has been removed and replaced with a flat resource.action permission model.

Each module now owns its permissions through its own enum:

• modules/tier/permissions.py
• modules/user/permissions.py
• modules/role/permissions.py

For example:

@register_permissions("tier")
class TierPermission(StrEnum):
    READ = "tier.read"
    CREATE = "tier.create"
    UPDATE = "tier.update"
    DELETE = "tier.delete"

A central registry in modules/role/ now provides:

• register_permissions()
• all_permissions()
• permission_groups()
• is_known_permission()

The registry validates that:

• the same resource cannot be registered twice
• every permission starts with the declared resource prefix

Permission modules are automatically discovered by walking src.modules.*.permissions.

This removes the previous duplication between the permission tree and permission constants and gives each module ownership of its own permission definitions.

Checklist

• Per-module permission enums
• Central permission registry
• register_permissions()
• all_permissions()
• permission_groups()
• Duplicate-resource protection
• Resource-prefix validation
• Automatic permission discovery
• RolePermission registry validation
• Old permission-tree implementation removed
• Verify all permissions register successfully
• Registry tests
• Model permission tests

---

2. Models / Database

The RBAC database model was updated to match the requested design.

Role no longer uses SoftDeleteMixin. This is intentional because a deleted role should not remain capable of granting permissions through existing assignments.

The join tables are now:

• role_permissions
• user_roles

The join tables use composite primary keys:

• role_permissions: (role_id, permission)
• user_roles: (user_id, role_id)

Surrogate IDs and redundant indexes were removed.

The explicit constraint names were also removed so the project's SQLAlchemy naming convention can generate the names.

Checklist

• Remove SoftDeleteMixin from Role
• Rename join tables
• Composite PKs
• Remove surrogate IDs
• Remove explicit constraint names
• Verify redundant indexes
• Migration
• PostgreSQL cascade verification

Why the last two remain unchecked

PostgreSQL is not available in the current environment.

I verified the database port directly:

localhost:5432
TcpTestSucceeded: False

Because PostgreSQL was unavailable, I could not honestly run and verify the migration or the database-level cascade behavior.

I therefore left both items unchecked rather than claiming database behavior that was not actually tested.

---

3. Permission Checking

Permission checking now goes through crudauth using:

require_permissions(*needed)

which uses:

auth.current_user(check=check)

The implementation includes:

• superuser bypass
• registered-permission validation
• effective permission loading
• requiring all requested permissions
• ignoring unrecognized stored permissions

A protected route was also added using the new permission dependency.

Checklist

• load_permissions()
• Ignore unrecognized permissions
• Superuser bypass
• require_permissions(*needed)
• crudauth current_user(check=...)
• One protected route
• FastAPI dependency wiring
• Authorization behavior tests
• Request-local permission caching

Why request-local permission caching remains unchecked

load_permissions() supports request-state caching when a Request is available.

However, after checking the actual crudauth API, the callback supplied to:

auth.current_user(check=check)

receives the Principal, not the FastAPI Request.

Because of that API boundary, request-local caching cannot currently be guaranteed inside the require_permissions() callback without changing the authentication integration or introducing a separate authorization path.

I therefore left this unchecked rather than claiming that the requested caching behavior is fully implemented.

---

4. Security

The existing self-profile ownership rule has been preserved while adding the requested user.update permission behavior.

The resulting rules are:

• superusers can update any user
• users with user.update can update any user
• regular users without user.update can update only their own profile

Privilege-escalation protection was also added.

A principal cannot delegate permissions they do not already possess.

This applies to:

• role permission delegation
• role assignment

When assigning a role, the permissions contained in that role are checked against the assigner's effective permissions.

Superusers bypass these restrictions.

Checklist

• user.update applies to any user
• Preserve self-profile ownership rule
• Prevent privilege escalation
• Role-permission escalation checks
• Role-assignment escalation checks
• API-key scope narrowing

Why API-key scope narrowing remains unchecked

The existing API-key implementation still uses its separate legacy resource/action permission model.

Moving API-key scopes onto the new flat resource.action permission system would therefore be a separate authorization migration.

I left this as follow-up work rather than mixing another permission-system migration into Piece 01.

---

5. Tests / Verification

The focused RBAC tests are passing.

Results

• Registry tests: 3 passed
• Role model tests: 5 passed, 1 skipped
• Authorization dependency tests: 18 passed
• User service tests: 4 passed
• Combined RBAC/user tests: 22 passed
• Escalation tests: passing

Checklist

• Registry tests — 3 passed
• Model tests — 5 passed, 1 skipped
• Authorization dependency tests — 18 passed
• User service tests — 4 passed
• Combined RBAC/user tests — 22 passed
• Escalation tests
• PostgreSQL cascade test
• Rebase on latest main
• Full verification
• Commit/push

---

6. Full Test Suite

The complete test suite was also executed.

Result:

369 collected
237 passed
130 skipped
2 failed
4 warnings

The two failures are:

tests/integration/auth/test_oauth.py::test_google_is_told_to_return_to_the_route_that_serves_the_callback

tests/integration/auth/test_oauth.py::test_the_auth_paths_keep_their_existing_contract

Both failures occur because the tests iterate over app.routes and encounter FastAPI's _IncludedRouter, which does not have a .path attribute.

These failures are outside the RBAC changes. The RBAC-specific tests continue to pass.

I did not modify the OAuth implementation just to make this RBAC PR pass.

---

7. Delivery

The branch was rebased onto the latest upstream main.

The completed implementation was committed as:

e72206a feat: add core RBAC permission checks

The rebased branch was successfully pushed to the fork.

The working tree is clean after the push.

Checklist

• Rebase on latest main
• Commit changes
• Push changes to fork
• Verify working tree is clean

---

Final Checklist

Permissions

• Per-module permission enums
• Central permission registry
• register_permissions()
• all_permissions()
• permission_groups()
• Duplicate-resource protection
• Resource-prefix validation
• Automatic permission discovery
• RolePermission registry validation
• Old permission-tree implementation removed
• Verify all permissions register successfully
• Registry tests
• Model permission tests

Models / Database

• Remove SoftDeleteMixin from Role
• Rename join tables
• Composite PKs
• Remove surrogate IDs
• Remove explicit constraint names
• Verify redundant indexes
• Migration — blocked because PostgreSQL is unavailable
• PostgreSQL cascade verification — blocked because PostgreSQL is unavailable

Permission Checking

• load_permissions()
• Ignore unrecognized permissions
• Superuser bypass
• require_permissions(*needed)
• crudauth current_user(check=...)
• One protected route
• FastAPI dependency wiring
• Authorization behavior tests
• Request-local permission caching — not claimed because the crudauth callback receives only Principal, not Request

Security

• user.update applies to any user
• Preserve self-profile ownership rule
• Prevent privilege escalation
• Role-permission escalation checks
• Role-assignment escalation checks
• API-key scope narrowing — left as follow-up because the existing API-key permission model is separate

Tests / Delivery

• Registry tests — 3 passed
• Model tests — 5 passed, 1 skipped
• Authorization dependency tests — 18 passed
• User service tests — 4 passed
• Combined RBAC/user tests — 22 passed
• Escalation tests
• PostgreSQL cascade test — blocked because PostgreSQL is unavailable
• Rebase on latest main
• Full verification
• Commit/push
• Working tree clean

---

Summary

The core RBAC Piece 01 requested in the review is now implemented.

The implementation now contains:

• flat resource.action permissions
• module-owned permission definitions
• central permission registration and discovery
• registry-backed validation
• revised composite-key RBAC relationships
• non-soft-deletable roles
• crudauth-based permission enforcement
• superuser bypass
• user.update authorization
• preserved self-profile ownership
• role/permission delegation protection
• role-assignment escalation protection
• focused test coverage

The remaining unchecked items are explicitly documented:

1. PostgreSQL migration and cascade verification are blocked because PostgreSQL is unavailable in the current environment.
2. Request-local permission caching is not claimed because the current crudauth callback API does not provide the Request.
3. API-key scope migration is separate follow-up work.
4. The full suite has two OAuth integration failures unrelated to this RBAC implementation.

Thank you again for the detailed review and for clarifying the intended architecture. The feedback helped make the permission model substantially cleaner, less duplicated, and easier to extend while keeping Piece 01 focused on the core RBAC implementation.

[igorbenav]

I pushed a few things.

A user.update holder could take over a superuser account. PATCH /users/{username} accepted any target once the requester held user.update, and the public UserUpdate schema accepted google_id, github_id, oauth_provider, email_verified and oauth_updated_at. crudauth's OAuth callback resolves a login by provider id first and then by verified provider email, so setting either field on someone else's row hands you their session through "Sign in with Google". Three changes:

• The OAuth and verification fields moved to a new UserAdminUpdate, which is what the admin panel's edit form now builds from. The public schema keeps name, username, email and profile_image_url, and extra="forbid" turns the rest into a 422. This part predates your PR — any user could already set their own email_verified — but user.update turned it into a cross-user issue.
• A non-superuser holding user.update may now only edit accounts weaker than their own: not a superuser, and holding no permission the requester lacks.
• Only a superuser may change another user's email address, for the takeover reason above. A user changing their own email is unaffected.

user.update also leaked into the other actions. check_update_permission was the shared ownership check behind delete, rate limits and tier, so holding user.update would have granted those too as soon as any of them passed permissions in. The ownership check is back to what it was on main, and profile update has its own path.

The permission lookup opened its own local_session(). That's a second connection per check, it can't see the request's uncommitted writes, and it ignores the test database override — a real login hitting GET /users/ returned 500 against the test DB. The request.state cache never ran either, since neither caller passed a request. load_permissions(db, user_id, ...) now takes the session, get_current_permissions is a normal dependency, and FastAPI's per-request dependency cache is the caching layer, so one request resolves permissions once however many guards and handlers ask. There's a test that fails if that caching goes away.

require_permissions built a fresh closure per call on top of auth.current_user(check=...), so the routes had no dependency the test fixtures could override, which is what broke the 7 existing user tests. It's now a plain dependency over the named get_current_principal; authentication still goes entirely through crudauth. The auth_client fixtures override get_current_principal alongside get_current_user, and the tests that assert RBAC behaviour sign in for real instead.

The join tables couldn't be built from ids: RolePermission.role_id, UserRole.user_id and UserRole.role_id were init=False, so UserRole(user_id=1, role_id=1) raised TypeError. That's what the cascade test was doing, and it only looked green because it was skipping. Those columns are constructible now, which Piece 02's "assign a role" needs anyway.

Registry validation was accepting things the column or a check can't handle: nested names like billing.invoice.read, a dotted resource, an empty action, uppercase and spaces, names longer than the String(100) column, and an empty enum. All rejected now, with the length sharing one constant with the column. all_permissions() also kept rebuilding its frozenset on every call — including on every RolePermission construction — so it's maintained as the registry grows instead.

Smaller things: the escalation helpers now agree on how they treat an unregistered name (never delegable, ignored when stored, no ValueError reaching a route) and their docstrings say they check escalation only, so Piece 02's routes still have to require role.assign and role.update; the enum in role/permissions.py is RolePermissionName so it no longer shares a name with the model; and the docstrings and line wrapping that got swept up in auth/dependencies.py, user/routes.py and user/service.py are back to main's, so the diff is only RBAC.

Tests: 407 passing. The new ones sign in for real and cover the role granting user.read (200), no role (403), superuser without a role (200), each takeover guard with the target row checked afterwards, the 422 on every OAuth field, the email rule in both directions, one permission lookup per request, and a stored permission that's no longer registered granting nothing. Nine mutations of the fixes were each checked against the suite; eight are caught and the ninth turned out to be a no-op (FastAPI writes to its dependency cache even with use_cache=False), with a real cache regression caught instead.

Two things left for Piece 02: role deletion has to clear assignments and filter is_deleted itself, since the DB cascade only fires on a hard delete; and a soft-deleted user keeps their user_roles rows, which isn't a hole today because crudauth gates on is_active, but it's a decision to make when the role service lands.

Thanks again for the work, and for sticking with the review rounds. For the person taking the next one, please wait a couple of days because I'm refactoring some things in the codebase.

[Vins01-07]

@igorbenav @emiliano-go

Thank you so much for giving me the opportunity to contribute to the project and for guiding me through the review process. I really appreciate the time you took to review my work and the feedback you shared throughout the process. This was my first open-source contribution, so getting the chance to work on Piece 01 and seeing it merged into the project means a lot to me. 🙌

For Piece 02, my friend will be taking that one forward. Whenever the codebase refactor is complete and Piece 02 is ready to be worked on, please mention/tag me again so I can make sure we're aligned and help coordinate with him.

Thanks again for the opportunity and for being so welcoming to new contributors! 🚀