Skip to content

Add composite roles for AND permission checks - #954

Closed
Chehak7 wants to merge 1 commit into
utopia-php:mainfrom
Chehak7:feat-13317-and-permissions
Closed

Add composite roles for AND permission checks#954
Chehak7 wants to merge 1 commit into
utopia-php:mainfrom
Chehak7:feat-13317-and-permissions

Conversation

@Chehak7

@Chehak7 Chehak7 commented Sep 4, 2026

Copy link
Copy Markdown

Summary

  • add a canonical Role::allOf() representation for two required roles
  • preserve composite roles while parsing, aggregating, and extracting permissions
  • validate each component role and keep adapter matching exact
  • cover unit behavior and the full adapter matrix

This is the database-layer prerequisite for appwrite/appwrite#13317.

Testing

All checks were run inside Docker:

  • unit suite: 465 tests, 2,383 assertions
  • composite permission adapter test: 16 tests, 80 assertions
  • PHPStan level 7: no errors

The complete adapter suite was also started, but the MySQL service stopped responding at 4% with SQLSTATE[HY000]: General error: 2006 MySQL server has gone away; the issue-specific test had already passed against all 16 adapter configurations.

Summary by CodeRabbit

  • New Features

    • Added support for composite roles that require multiple roles together.
    • Composite roles can be created, parsed, serialized, and inspected.
    • Permissions now support and validate multiple roles within a composite role.
  • Bug Fixes

    • Permission matching now requires exact role expressions, preventing partial matches.
  • Tests

    • Added coverage for composite role creation, parsing, validation, aggregation, and permission matching.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 52af24a6-f728-41bd-b0bd-22aaaaabee51

📥 Commits

Reviewing files that changed from the base of the PR and between debef42 and 76d7fab.

📒 Files selected for processing (9)
  • src/Database/Document.php
  • src/Database/Helpers/Permission.php
  • src/Database/Helpers/Role.php
  • src/Database/Validator/Permissions.php
  • tests/e2e/Adapter/Scopes/PermissionTests.php
  • tests/unit/DocumentTest.php
  • tests/unit/PermissionTest.php
  • tests/unit/RoleTest.php
  • tests/unit/Validator/PermissionsTest.php

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The permission model now supports allOf roles composed of two distinct roles. Composite roles serialize and parse consistently, preserve role objects through permission operations, validate each member role, and match only exact composite permissions.

Changes

Composite permission roles

Layer / File(s) Summary
Composite role representation
src/Database/Helpers/Role.php, tests/unit/RoleTest.php
Role supports validated allOf combinations, recursive parsing, serialization, and access to constituent roles.
Permission parsing and validation
src/Database/Helpers/Permission.php, src/Database/Validator/Permissions.php, src/Database/Document.php, tests/unit/PermissionTest.php, tests/unit/Validator/PermissionsTest.php, tests/unit/DocumentTest.php
Permissions preserve Role objects, parse composite role strings, validate every constituent role, aggregate composite roles, and extract exact permission values.
Exact role matching
tests/e2e/Adapter/Scopes/PermissionTests.php
End-to-end coverage verifies that individual roles and mismatched composites do not satisfy an exact composite permission.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 76d7f

This change adds two-role all-of permissions so access is granted only for the exact composite role. The covered construction, validation, serialization, extraction, aggregation, and authorization paths leave no current merge-blocking risk.

Suggested reviewers: abnegate

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 9 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding composite roles to support AND permission checks.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@greptile-apps

greptile-apps Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces canonical two-role composites for representing AND permission checks.

  • Adds Role::allOf() construction, deterministic serialization, and parsing.
  • Preserves composite roles through permission parsing and aggregate expansion.
  • Validates each constituent role before permissions are stored.
  • Correctly extracts nested composite role text from document permissions.
  • Adds unit and adapter-level coverage for construction, parsing, validation, aggregation, and exact authorization matching.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness, security, or repository-rule violations identified.

Composite roles remain canonical across construction, parsing, validation, document extraction, aggregate expansion, and the intentionally exact authorization matching model.

Important Files Changed

Filename Overview
src/Database/Helpers/Role.php Adds deterministic construction, parsing, and serialization for exactly two distinct, non-nested composite roles.
src/Database/Helpers/Permission.php Preserves Role objects—including composites—through permission parsing, construction, and aggregate expansion.
src/Database/Validator/Permissions.php Extends permission validation to validate every constituent of a composite role.
src/Database/Document.php Extracts permission payloads using the exact permission grammar, preserving nested composite parentheses.
tests/e2e/Adapter/Scopes/PermissionTests.php Verifies exact composite-role authorization behavior across adapter-backed document reads.
tests/unit/RoleTest.php Covers canonical ordering, round-trip parsing, and composite arity validation.
tests/unit/PermissionTest.php Covers composite permission round trips and aggregate write expansion.
tests/unit/Validator/PermissionsTest.php Covers acceptance of valid composites and rejection of invalid constituent roles.

Reviews (1): Last reviewed commit: "feat: add composite permission roles (#1..." | Re-trigger Greptile

@Chehak7

Chehak7 commented Sep 4, 2026

Copy link
Copy Markdown
Author

Closing this because I am withdrawing the dependency fork and will not proceed with this implementation.

@Chehak7 Chehak7 closed this Sep 4, 2026
@Chehak7
Chehak7 deleted the feat-13317-and-permissions branch September 4, 2026 17:53
@Chehak7

Chehak7 commented Sep 4, 2026

Copy link
Copy Markdown
Author

The dependency fork and branch have been restored. GitHub could not reopen this PR after the fork was deleted and recreated, so the work now continues in #955.

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