Admin permission surface: what a user may do, and why#36
Merged
Conversation
…her it holds An administrator asking why someone can reach a document has had no way to be told. Check answers yes or no, and ListObjects answers what, but neither says through which relationship, so the admin surface could only show a tick. Add RelationshipExpansionPort over OpenFGA Expand. It returns the userset tree for one object and permission in provider-neutral form: direct tuples, computed rewrites, the tuple-to-userset hop that carries inheritance, and the set operations composing them. Nothing may grant access on the strength of it — enforcement stays with the check ports, and this only describes an answer already obtained. Malformed provider responses become an indeterminate result rather than an exception, matching the other adapters, and the unconfigured stand-in answers indeterminate so a missing OpenFGA still fails closed. Also add can_curate_graph as knowledge_curator or administrator. The POC grants no curator tuple and the administrator satisfies it through the or branch; separating the two roles later is deleting that clause, with no Java change and no tuple rewrite. The two demo employees were both plain members of the same organization and unit, so every verdict came out identical and there was nothing to explain. Minh becomes the unit's manager and its managers become reviewers of the sales space, which is what his TEAM_LEAD record already claimed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reading an expansion is not the same as explaining one. AccessExplanationService turns the userset tree into the single derivation that reached this principal, following usersets recursively and stopping at the first branch a union satisfies — which is how the engine resolves it, so reporting every branch that happens to hold would suggest the answer depended on all of them. A refusal instead names the branches that were evaluated. The verdict comes from the relationship port directly rather than through EffectiveAuthorizationService. That service collapses an unanswered check into a denial, which is right for enforcement and wrong to show an administrator: "no relationship grants this" and "the engine did not answer" need different actions, and only one is a permission problem. Nothing here grants access, so keeping the third outcome costs no safety, and the organization guard is kept. AccessState therefore has three values. An expired mirrored ACL is UNKNOWN even when the tuples still allow, because those tuples are a copy of a decision Slack or Drive owns and past its validity nobody can say the copy is current. RelationName and UsersetRef carry the nouns a walk descends into. PermissionKey still admits only can_*, which is what keeps a noun out of an enforcement call. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…administrators The admin surface reported role, activation, and a mapped-principal count, none of which is an authorization decision. Two endpoints answer what a user may actually do: the organization permissions the model defines, and whether one principal holds one permission on one resource with the derivation that decided it. Nothing is persisted. A permission is computed from relationships when asked, so storing a copy would be a second source of truth that drifts; each response carries the instant it was evaluated and the authorization model it resolved against. The explainable resource types are an allowlist rather than anything the caller names, and the permission list is written out rather than discovered, so adding one to the model stays a deliberate decision to show it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A role is tuple data, so granting one is a single tuple and revoking it a single deletion — no deploy, no migration. That is why type role exists in the model, and it is the whole of what an administrator may change about access. AdministrativeTupleScope refuses any write whose object is not an organization or a role. Document permissions arrive from Slack and Drive through convergence and are mirrored, not authored; a second writer would let the two diverge, after which nobody could answer who may read a document. The refusal is by object type rather than by reading the authority column, because it has to hold for an object that does not exist yet and for one whose row cannot be read. Listing roles pages the store because OpenFGA has no call for it, and reports when it stopped rather than presenting a truncated list as the whole set. Revoking your own role is refused. Unlike the existing self-edit guard on user role, there would be nobody left who could undo it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The users list reported role, sign-in linkage, and a mapped-principal count, none of which is an authorization decision — and the role control it offers changes a column that no guard reads. Each name now links to a permission profile that asks the engine. AccessVerdict is the atom every surface renders through, so none of them can drop the authority, generation, and sync age that make a verdict safe to act on. A mirrored permission is labelled a copy of a decision the source system owns, not its current state. An allow renders as a rail, not a tree: the engine short-circuits a union, so exactly one derivation is decisive and branches would imply the answer depended on all of them. A refusal renders as a flat list of what was tried, keeping an explicit deny visually apart from a missing relationship because only one of them can be fixed by granting something. Roles are the one editable block. Document permissions arrive from the connected source and say so on screen. users.tsx becomes users.index.tsx: with a sibling detail route, the old file was a parent whose component rendered no Outlet, so the detail page changed the address and then rendered the list. No new dependency — the layout is fixed, so a graph library would solve a problem this does not have. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughAdds authorization explanation models and OpenFGA expansion support, administrator APIs for permissions and roles, role tuple administration, and a web interface for inspecting permissions and managing role assignments. ChangesAuthorization explanation and role administration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
The permission profile was only reachable by clicking a name in the users list, so from the navigation it looked like nothing had shipped. Access check is now a Permissions entry of its own, asking for a user, a permission and one resource. The check card moves into AccessInspector and both surfaces use it, so the profile page and the standalone screen cannot drift into answering the same question two ways. It still refuses to be a corpus listing: the scope is always a user and one named resource. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java`:
- Around line 128-137: Add an explicit null check for request.permission() in
the validation sequence of AdminPermissionController, before calling
PermissionKey.of(...), and throw the same clean IllegalArgumentException style
used for resourceType and resourceId. Preserve the existing explanation flow for
non-null permissions.
- Around line 99-117: Move the OpenFGA calls in permissions() and explain()
outside the `@Transactional`(readOnly = true) boundary. Keep the transactional
portion limited to requireUserInOrganization and other database reads, then
invoke explanations.effectivePermissions, ClientCheckRequest.compute(), and
ClientExpandRequest.compute() after that transaction has completed while
preserving the existing responses and authorization checks.
In
`@apps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.java`:
- Around line 22-26: Update the accessExplanationService bean factory to accept
a Spring-managed Clock parameter and pass it to AccessExplanationService instead
of constructing Clock.systemUTC() internally. Preserve the existing relationship
and expansion dependencies while enabling test or application clock overrides.
In `@core/src/main/java/com/orgmemory/core/authorization/AccessExplanation.java`:
- Around line 25-34: Update the AccessExplanation constructor to enforce
complete ALLOWED explanations: require non-null provenance and reject an ALLOWED
state when path is empty, while preserving the existing restriction against
paths on non-ALLOWED states.
In
`@core/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java`:
- Around line 163-246: Change cycle tracking in firstThrough so visited remains
scoped to the current recursive path: after each userset reference finishes
expansion and walk, remove its key from visited before continuing or returning,
including unresolved and successful branches. Preserve cycle prevention while
allowing the same reference to be retried from later Union or nested paths with
a fresh depth budget; update the walk/firstThrough interaction only as needed.
In `@core/src/main/java/com/orgmemory/core/authorization/AclProvenance.java`:
- Around line 32-40: Update AclProvenance.source so SOURCE provenance is marked
expired when validUntil is absent or when now is at or after validUntil; require
now only when evaluating the timestamp, preserving the existing constructor and
provenance fields.
In `@integrations/authorization-openfga/src/test/openfga/store.fga.yaml`:
- Around line 130-145: Add ListObjects assertions for can_curate_graph in the
“Curation is its own permission” test model, covering the relevant positive and
negative users and ensuring only organizations they can curate are returned.
Preserve the existing Check assertions and include negative ListObjects coverage
to prevent unauthorized organization metadata from appearing.
In `@web/src/features/admin/components/admin-user-permissions-page.tsx`:
- Around line 274-279: Add an accessible label for the resourceId input in the
admin permissions form, using a visible label or an appropriate aria-label while
preserving the existing value, onChange, placeholder, and styling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f239abf-f2af-41ec-9e5b-49c50a1d2aea
⛔ Files ignored due to path filters (4)
contracts/openapi.jsonis excluded by!contracts/openapi.jsondocs/increments/active/2026-07-24-admin-permission-surface/design.mdis excluded by!docs/**docs/increments/active/2026-07-24-admin-permission-surface/plan.mdis excluded by!docs/**integrations/authorization-openfga/src/main/openfga/local-demo-tuples.csvis excluded by!**/*.csv
📒 Files selected for processing (32)
apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminRoleController.javaapps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.javacore/src/main/java/com/orgmemory/core/authorization/AccessBlock.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanation.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.javacore/src/main/java/com/orgmemory/core/authorization/AccessState.javacore/src/main/java/com/orgmemory/core/authorization/AccessStep.javacore/src/main/java/com/orgmemory/core/authorization/AclProvenance.javacore/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.javacore/src/main/java/com/orgmemory/core/authorization/ExpansionNode.javacore/src/main/java/com/orgmemory/core/authorization/RelationName.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.javacore/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.javacore/src/main/java/com/orgmemory/core/authorization/UsersetRef.javacore/src/test/java/com/orgmemory/core/authorization/AccessExplanationServiceTests.javacore/src/test/java/com/orgmemory/core/authorization/RoleAdministrationServiceTests.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/openfga/model.fgaintegrations/authorization-openfga/src/test/openfga/store.fga.yamlweb/src/features/admin/admin-queries.tsweb/src/features/admin/components/access-path.tsxweb/src/features/admin/components/access-verdict.tsxweb/src/features/admin/components/admin-user-permissions-page.tsxweb/src/features/admin/components/admin-users-page.tsxweb/src/routeTree.gen.tsweb/src/routes/admin/users.$userId.tsxweb/src/routes/admin/users.index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Backend · Java 25
🧰 Additional context used
📓 Path-based instructions (10)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Before changing unfamiliar Spring Boot 4, Spring Modulith 2, Spring AI 2, Gradle, React, Vite, Tailwind, or TypeScript APIs, consult Context7/current official documentation and the projectorgmemory-*verification skills.
Readdocs/guidelines/agent-safety.mdbefore retrieval, AI, MCP, permission, upload, graph, or export work.
Never commit.envfiles, provider keys, tokens, or customer data.
Run the relevant gates fromdocs/guidelines/testing-harness.md; use a terminating clean test as the context gate, and do not treatbootRunas verification.
Current behavior belongs in architecture/specs only after it exists in code; intent belongs in vision, roadmap, or an active increment, and repository state must not be duplicated across documents.
Files:
web/src/routes/admin/users.index.tsxintegrations/authorization-openfga/src/test/openfga/store.fga.yamlintegrations/authorization-openfga/src/main/openfga/model.fgacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.javaweb/src/features/admin/components/admin-users-page.tsxweb/src/routes/admin/users.$userId.tsxcore/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanation.javacore/src/main/java/com/orgmemory/core/authorization/AccessState.javacore/src/main/java/com/orgmemory/core/authorization/RelationName.javacore/src/main/java/com/orgmemory/core/authorization/AccessStep.javacore/src/main/java/com/orgmemory/core/authorization/AccessBlock.javaweb/src/features/admin/components/access-path.tsxcore/src/test/java/com/orgmemory/core/authorization/RoleAdministrationServiceTests.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.javacore/src/main/java/com/orgmemory/core/authorization/AclProvenance.javaweb/src/features/admin/components/access-verdict.tsxcore/src/main/java/com/orgmemory/core/authorization/ExpansionNode.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.javacore/src/main/java/com/orgmemory/core/authorization/UsersetRef.javaweb/src/features/admin/components/admin-user-permissions-page.tsxcore/src/test/java/com/orgmemory/core/authorization/AccessExplanationServiceTests.javaapps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.javaweb/src/features/admin/admin-queries.tsapps/api/src/main/java/com/orgmemory/api/admin/AdminRoleController.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.javacore/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.javaweb/src/routeTree.gen.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For frontend files, use Oxlint, TypeScript typecheck, the production build, and browser tests when the UI flow matters.
Files:
web/src/routes/admin/users.index.tsxweb/src/features/admin/components/admin-users-page.tsxweb/src/routes/admin/users.$userId.tsxweb/src/features/admin/components/access-path.tsxweb/src/features/admin/components/access-verdict.tsxweb/src/features/admin/components/admin-user-permissions-page.tsxweb/src/features/admin/admin-queries.tsweb/src/routeTree.gen.ts
**/*.{ts,tsx,js,jsx,json,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not run JetBrains IDE inspection on TypeScript, TSX, or web configuration files.
Files:
web/src/routes/admin/users.index.tsxweb/src/features/admin/components/admin-users-page.tsxweb/src/routes/admin/users.$userId.tsxweb/src/features/admin/components/access-path.tsxweb/src/features/admin/components/access-verdict.tsxweb/src/features/admin/components/admin-user-permissions-page.tsxweb/src/features/admin/admin-queries.tsweb/src/routeTree.gen.ts
web/src/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
web/src/**/*.{ts,tsx}: OAuth access and refresh tokens must never enter browser JavaScript or
browser storage. Use the HttpOnly BFF session, CSRF-protected mutations,
generated Hey API data clients, accessible states, and both light and
dark themes. Handwritten transport is reserved for documented protocol
flows such as navigation redirects and streaming.
Files:
web/src/routes/admin/users.index.tsxweb/src/features/admin/components/admin-users-page.tsxweb/src/routes/admin/users.$userId.tsxweb/src/features/admin/components/access-path.tsxweb/src/features/admin/components/access-verdict.tsxweb/src/features/admin/components/admin-user-permissions-page.tsxweb/src/features/admin/admin-queries.tsweb/src/routeTree.gen.ts
**/*.{yml,yaml,properties}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
ddl-auto=validatein application configuration.
Files:
integrations/authorization-openfga/src/test/openfga/store.fga.yaml
integrations/authorization-openfga/**/*
⚙️ CodeRabbit configuration file
integrations/authorization-openfga/**/*: Source-native ACL is a hard ceiling. Flag any parent, organization, role,
or wildcard relation that can broaden source access. Require negative
Check and ListObjects coverage for every new permission path.
Files:
integrations/authorization-openfga/src/test/openfga/store.fga.yamlintegrations/authorization-openfga/src/main/openfga/model.fgaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.java
**/*.{java,sql}
📄 CodeRabbit inference engine (CLAUDE.md)
Pair JPA schema changes with a Flyway migration.
Files:
core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.javacore/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanation.javacore/src/main/java/com/orgmemory/core/authorization/AccessState.javacore/src/main/java/com/orgmemory/core/authorization/RelationName.javacore/src/main/java/com/orgmemory/core/authorization/AccessStep.javacore/src/main/java/com/orgmemory/core/authorization/AccessBlock.javacore/src/test/java/com/orgmemory/core/authorization/RoleAdministrationServiceTests.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.javacore/src/main/java/com/orgmemory/core/authorization/AclProvenance.javacore/src/main/java/com/orgmemory/core/authorization/ExpansionNode.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.javacore/src/main/java/com/orgmemory/core/authorization/UsersetRef.javacore/src/test/java/com/orgmemory/core/authorization/AccessExplanationServiceTests.javaapps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminRoleController.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.javacore/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java
**/*.java
📄 CodeRabbit inference engine (CLAUDE.md)
JetBrains IDE inspection is a Java-backend gate only.
Files:
core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.javacore/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanation.javacore/src/main/java/com/orgmemory/core/authorization/AccessState.javacore/src/main/java/com/orgmemory/core/authorization/RelationName.javacore/src/main/java/com/orgmemory/core/authorization/AccessStep.javacore/src/main/java/com/orgmemory/core/authorization/AccessBlock.javacore/src/test/java/com/orgmemory/core/authorization/RoleAdministrationServiceTests.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.javacore/src/main/java/com/orgmemory/core/authorization/AclProvenance.javacore/src/main/java/com/orgmemory/core/authorization/ExpansionNode.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.javacore/src/main/java/com/orgmemory/core/authorization/UsersetRef.javacore/src/test/java/com/orgmemory/core/authorization/AccessExplanationServiceTests.javaapps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminRoleController.javaintegrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.javacore/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java
core/src/main/java/com/orgmemory/core/{authorization,knowledge,permission}/**/*.java
⚙️ CodeRabbit configuration file
core/src/main/java/com/orgmemory/core/{authorization,knowledge,permission}/**/*.java: Treat PostgreSQL ACL evidence as canonical and OpenFGA as the relationship
authorization decision point. Authorization must fail closed. Filtering
must happen before ranking, LIMIT, graph traversal, answer generation,
export, and citation rendering. Flag metadata or timing leak paths.
Files:
core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.javacore/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanation.javacore/src/main/java/com/orgmemory/core/authorization/AccessState.javacore/src/main/java/com/orgmemory/core/authorization/RelationName.javacore/src/main/java/com/orgmemory/core/authorization/AccessStep.javacore/src/main/java/com/orgmemory/core/authorization/AccessBlock.javacore/src/main/java/com/orgmemory/core/authorization/AclProvenance.javacore/src/main/java/com/orgmemory/core/authorization/ExpansionNode.javacore/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.javacore/src/main/java/com/orgmemory/core/authorization/UsersetRef.javacore/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.javacore/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java
apps/api/src/main/java/**/*.java
⚙️ CodeRabbit configuration file
apps/api/src/main/java/**/*.java: Enforce the browser-BFF and resource-server boundaries. Authentication
must resolve an active internal actor through the explicit issuer and
subject binding. Reject identity, tenant, roles, or permissions supplied
by request payloads, JWT email, or untrusted JWT role claims.
Files:
apps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminRoleController.javaapps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java
🧠 Learnings (1)
📚 Learning: 2026-07-23T03:36:09.053Z
Learnt from: kl3inIT
Repo: kl3inIT/OrgMemory PR: 16
File: web/src/features/admin/components/admin-mappings-page.tsx:190-211
Timestamp: 2026-07-23T03:36:09.053Z
Learning: In the OrgMemory admin UI, preserve the principal display ordering returned by the backend instead of re-sorting on the client. The server-owned order (e.g., unmapped views first, then deterministic ordering by source-system, connection, kind, and external-key as defined in SourcePrincipalAdminService#listPrincipals) must be used as-is to avoid mismatches with the server’s intended admin mappings/principals list.
Applied to files:
web/src/features/admin/components/admin-users-page.tsxweb/src/features/admin/components/access-path.tsxweb/src/features/admin/components/access-verdict.tsxweb/src/features/admin/components/admin-user-permissions-page.tsx
🪛 PMD (7.26.0)
integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.java
[Low] 56-61: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 3 arguments but found 4
(InvalidLogMessageFormat (Error Prone))
core/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.java
[Medium] 63-63: UnusedAssignment (Best Practices): The initializer for variable 'policyVersion' is never used (overwritten on line 70)
(UnusedAssignment (Best Practices))
🔇 Additional comments (29)
apps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.java (1)
3-10: LGTM!Also applies to: 28-32
web/src/features/admin/admin-queries.ts (1)
16-19: LGTM!Also applies to: 33-49, 134-139
web/src/features/admin/components/access-path.tsx (1)
1-126: LGTM!web/src/features/admin/components/access-verdict.tsx (1)
1-79: LGTM!web/src/features/admin/components/admin-user-permissions-page.tsx (2)
1-249: LGTM!Also applies to: 280-326
251-258: 🎯 Functional CorrectnessNo change needed.
can_view,can_publish, andcan_editare defined relations in the OpenFGA model for the selected resource types.web/src/features/admin/components/admin-users-page.tsx (1)
241-247: LGTM!web/src/routeTree.gen.ts (1)
21-24: LGTM!Also applies to: 70-74, 82-91, 113-117, 127-132, 145-150, 164-168, 178-183, 195-200, 276-276, 290-303, 339-343, 352-356
web/src/routes/admin/users.$userId.tsx (1)
1-15: LGTM!web/src/routes/admin/users.index.tsx (1)
5-5: LGTM!apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java (1)
124-140: 🔒 Security & PrivacyNo change needed for
resourceIdorganization scoping.
AccessExplanationService.decide()rejects anyResourceRefwhoseorganizationIddiffers from the actor-supplied organization before invoking OpenFGA, so an administrator cannot use this path to probe resources belonging to another tenant.core/src/main/java/com/orgmemory/core/authorization/AccessBlock.java (1)
12-39: LGTM!core/src/main/java/com/orgmemory/core/authorization/AccessState.java (1)
11-20: LGTM!core/src/main/java/com/orgmemory/core/authorization/AccessStep.java (1)
8-34: LGTM!integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.java (1)
49-54: LGTM!integrations/authorization-openfga/src/main/openfga/model.fga (1)
23-31: LGTM!core/src/main/java/com/orgmemory/core/authorization/RoleAdministrationService.java (1)
59-115: LGTM!core/src/main/java/com/orgmemory/core/authorization/AdministrativeTupleScope.java (1)
20-38: LGTM!core/src/test/java/com/orgmemory/core/authorization/RoleAdministrationServiceTests.java (1)
85-87: 📐 Maintainability & Code QualityNo change needed.
coreinherits the Spring-library plugin, which delegates to the Java convention that declares Java 25, soList.getFirst()is supported.core/src/main/java/com/orgmemory/core/authorization/ExpansionNode.java (1)
1-95: LGTM!core/src/main/java/com/orgmemory/core/authorization/RelationName.java (1)
14-32: LGTM!core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.java (1)
11-14: LGTM!core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionQuery.java (1)
14-24: LGTM!core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionResult.java (1)
5-50: LGTM!core/src/main/java/com/orgmemory/core/authorization/UsersetRef.java (1)
14-46: LGTM!core/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java (1)
55-154: LGTM!Also applies to: 248-292
core/src/test/java/com/orgmemory/core/authorization/AccessExplanationServiceTests.java (1)
18-189: LGTM!integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.java (1)
41-135: LGTM!integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/UnavailableAuthorizationConfiguration.java (1)
79-86: LGTM!
Comment on lines
+99
to
+117
| @Transactional(readOnly = true) | ||
| EffectivePermissionResponse permissions(@PathVariable UUID userId, Authentication authentication) { | ||
| CurrentActor actor = guard.requireAdministrator(authentication); | ||
| AppUser user = requireUserInOrganization(userId, actor); | ||
| var states = explanations.effectivePermissions( | ||
| actor.organizationId(), | ||
| PrincipalRef.user(user.getId()), | ||
| ORGANIZATION_PERMISSIONS, | ||
| ResourceRef.of(actor.organizationId(), "organization", actor.organizationId())); | ||
| return new EffectivePermissionResponse( | ||
| user.getId(), | ||
| ORGANIZATION_PERMISSIONS.stream() | ||
| .collect(Collectors.toMap( | ||
| PermissionKey::value, | ||
| permission -> states.getOrDefault(permission, AccessState.UNKNOWN), | ||
| (first, second) -> first, | ||
| LinkedHashMap::new)), | ||
| Instant.now()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm effectivePermissions/explain make network calls to OpenFGA per invocation
rg -nP -A10 'effectivePermissions\(' core/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java
rg -nP -A10 'public .*explain\(' core/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.javaRepository: kl3inIT/OrgMemory
Length of output: 2163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate files"
fd -a 'AdminPermissionController|AccessExplanationService|RelationshipExpansionPort|RelationshipAuthorizationPort|PermissionKey' . | sed 's#^\./##'
echo
echo "AdminPermissionController outline"
ast-grep outline apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java --view expanded || true
echo
echo "AccessExplanationService relevant section"
sed -n '1,220p' core/src/main/java/com/orgmemory/core/authorization/AccessExplanationService.java
echo
echo "Controller permission methods"
sed -n '1,170p' apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java
echo
echo "Search permission counts"
rg -n "ORGANIZATION_PERMISSIONS|enum\s+PermissionKey|class PermissionKey|interface PermissionKey" .Repository: kl3inIT/OrgMemory
Length of output: 19764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Ports and implementations"
fd -a 'OpenFGA|Fga|fga|openfga|OpenFga|OpenFGA' . | sed 's#^\./##'
fd -a 'RelationshipExpansionPort|RelationshipAuthorizationPort|AuthorizationPort|AuthorizationClient' . | sed 's#^\./##'
echo
echo "RelationshipExpansionPort"
cat -n core/src/main/java/com/orgmemory/core/authorization/RelationshipExpansionPort.java
echo
echo "RelationshipAuthorizationPort"
cat -n core/src/main/java/com/orgmemory/core/authorization/RelationshipAuthorizationPort.java
echo
echo "Search executeNetwork/expand/check/fga/expand"
rg -n -C3 "interface RelationshipExpansionPort|class .*Relationship.*|expand\(|check\(|executeNetwork|OpenFGA|openfga|client" core apps/api -g '*.java'Repository: kl3inIT/OrgMemory
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "OpenFGA adapters"
for f in integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipAuthorizationAdapter.java integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaRelationshipExpansionAdapter.java integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga/OpenFgaAuthorizationConfiguration.java; do
echo "===== $f ====="
sed -n '1,260p' "$f"
done
echo
echo "Check for transaction boundaries around OpenFGA network calls"
rg -n -C3 "`@Transactional`|openfgaClient|check|expand|AuthorizationClient|ReadAssertions|RelationAssertion|ExecuteNetwork|executeNetwork" integrations/authorization-openfga/src/main/java/com/orgmemory/integrations/authorization/openfga core src -g '*.java'Repository: kl3inIT/OrgMemory
Length of output: 50374
Move OpenFGA checks/expansion outside the @Transactional boundary.
permissions() performs 7 sequential ClientCheckRequest.compute() calls under @Transactional(readOnly = true), and explain() performs a ClientCheckRequest.compute() plus ClientExpandRequest.compute() under the same transaction. Because the OpenFGA SDK calls use blocking .get(...) and production apps wire a real OpenFgaClient, the DB transaction can hold a connection open during multiple external round trips and with retries/latency increase the chance of connection-pool exhaustion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/main/java/com/orgmemory/api/admin/AdminPermissionController.java`
around lines 99 - 117, Move the OpenFGA calls in permissions() and explain()
outside the `@Transactional`(readOnly = true) boundary. Keep the transactional
portion limited to requireUserInOrganization and other database reads, then
invoke explanations.effectivePermissions, ClientCheckRequest.compute(), and
ClientExpandRequest.compute() after that transaction has completed while
preserving the existing responses and authorization checks.
Comment on lines
+22
to
+26
| @Bean | ||
| AccessExplanationService accessExplanationService( | ||
| RelationshipAuthorizationPort relationships, RelationshipExpansionPort expansion) { | ||
| return new AccessExplanationService(relationships, expansion, Clock.systemUTC()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for an existing Clock bean definition pattern in the codebase
rg -nP -B2 -A2 '\bClock\b' --type=java -g '*Configuration.java'Repository: kl3inIT/OrgMemory
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files named or containing AuthorizationConfiguration or Clock:"
fd -i 'AuthorizationConfiguration|Config|Security.*Config' . || true
rg -n "class AuthorizationConfiguration|interface AuthorizationConfiguration|AccessExplanationService|evaluatedAt|provenance|Clock|`@Bean`" --type=java -S apps/api/src/main/java docs || true
target="apps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.java"
if [ -f "$target" ]; then
echo "--- $target outline ---"
ast-grep outline "$target" || true
echo "--- $target contents ---"
cat -n "$target"
fi
echo "--- AccessExplanationService candidates ---"
fd -i 'AccessExplanationService' . -x sh -c 'echo "### $1"; cat -n "$1"' sh {}Repository: kl3inIT/OrgMemory
Length of output: 33795
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Java Clock occurrences outside comments/strings ---"
python3 - <<'PY'
import os, re
for root, dirs, files in os.walk('.'):
if any(part.startswith('.') or part in ('node_modules','build','dist') for part in root.split(os.sep)):
continue
for f in files:
if f.endswith(('.java','.kt','.java')):
path=os.path.join(root,f)
try:
s=open(path,encoding='utf-8',errors='ignore').read()
except Exception:
continue
# strip string literals/lonely line comments for a conservative match
c=re.sub(r'//.*','',s,flags=re.S)
c=re.sub(r'"""[\s\S]*?"""','',c,flags=re.S)
c=re.sub(r'"/?/[\s\S]*?"/?/','',c,flags='S')
c=re.sub(r'"(?:\\.|[^"\\])*"','',c)
for i,line in enumerate(s.splitlines(),1):
if 'Clock' in line:
print(f'{path}:{i}:{line.strip()}')
PY
echo "--- Spring configuration bean conventions ---"
rg -n "`@Bean`|BeanPostProcessor|`@Profile`|Clock.systemUTC|Clock.fixed|Clock\u003c\>|InstantClock" --type=java -S .Repository: kl3inIT/OrgMemory
Length of output: 483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Java Clock occurrences tracked, outside comments/strings ---"
git ls-files '*.java' | while IFS= read -r path; do
python3 - <<PY "$path"
import sys,re
path=sys.argv[1]
s=open(path,encoding='utf-8',errors='ignore').read()
c=re.sub(r'//.*','',s,flags=re.S)
c=re.sub(r'"""[\s\S]*?"""','',c,flags=re.S)
c=re.sub(r'"/?/[\s\S]*?"/?/','',c,flags='S')
c=re.sub(r'"(?:\\.|[^"\\])*"','',c)
for i,line in enumerate(s.splitlines(),1):
if 'Clock' in line and 'Clock' in c.splitlines()[i-1]:
print(f'{path}:{i}:{line.strip()}')
PY
done
echo "--- Spring configuration bean conventions ---"
rg -n '`@Bean`|BeanPostProcessor|`@Profile`|Clock\.systemUTC|Clock\.fixed|Clock<|InstantClock' --type=java -S .
echo "--- Security/AI/Connect config files around relevant beans ---"
for f in $(fd -i 'SecurityConfig.java|AuthorizationConfiguration.java|ProductionSecurityConfiguration.java|AssistantConfiguration.java|ProductionConfigurationGuard.java|.*Configuration.java' apps/core integrations --max-depth 3); do
echo "### $f"
rg -n '`@Bean`|Clock|Instant' "$f" -C 2 || true
done
echo "--- Test classes and resources mentioning Clock/fixed time ---"
rg -n 'Clock|fixed\(|Instant\.parse|evaluatedAt|provenance' --type=java -S apps/core apps/api integrations || trueRepository: kl3inIT/OrgMemory
Length of output: 622
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- Clock occurrences in tracked Java files ---"
git ls-files '*.java' | xargs -r rg -n '\bClock\b' -S
echo "--- Code references to Clock.systemUTC/Clock.fixed/InstantClock ---"
git ls-files '*.java' | xargs -r rg -n 'Clock\.systemUTC|Clock\.fixed|InstantClock' -S
echo "--- `@Configuration/`@Bean conventions in config/test classes ---"
git ls-files '*.java' | xargs -r rg -n '`@Configuration`|`@Component`|`@Bean`|Clock\.systemUTC|Clock\.fixed' -S | head -200Repository: kl3inIT/OrgMemory
Length of output: 20643
Inject Clock instead of hardcoding Clock.systemUTC().
AccessExplanationService uses the provided clock for evaluatedAt/provenance freshness, and unit tests already demonstrate fixed-time Clock injection. A Spring Clock bean override here would avoid hardcoding real-time behavior for this bean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/api/src/main/java/com/orgmemory/api/security/AuthorizationConfiguration.java`
around lines 22 - 26, Update the accessExplanationService bean factory to accept
a Spring-managed Clock parameter and pass it to AccessExplanationService instead
of constructing Clock.systemUTC() internally. Preserve the existing relationship
and expansion dependencies while enabling test or application clock overrides.
Comment on lines
+25
to
+34
| public AccessExplanation { | ||
| Objects.requireNonNull(state, "state"); | ||
| reasonCode = requireText(reasonCode, "reasonCode"); | ||
| path = List.copyOf(Objects.requireNonNull(path, "path")); | ||
| blockedBy = List.copyOf(Objects.requireNonNull(blockedBy, "blockedBy")); | ||
| policyVersion = requireText(policyVersion, "policyVersion"); | ||
| Objects.requireNonNull(evaluatedAt, "evaluatedAt"); | ||
| if (state != AccessState.ALLOWED && !path.isEmpty()) { | ||
| throw new IllegalArgumentException("Only a granted decision carries a derivation path"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce complete allowed explanations.
provenance may be null, and ALLOWED may have an empty path. That permits an affirmative verdict with neither its authority/freshness nor its derivation.
Proposed fix
path = List.copyOf(Objects.requireNonNull(path, "path"));
blockedBy = List.copyOf(Objects.requireNonNull(blockedBy, "blockedBy"));
+ Objects.requireNonNull(provenance, "provenance");
policyVersion = requireText(policyVersion, "policyVersion");
Objects.requireNonNull(evaluatedAt, "evaluatedAt");
+ if (state == AccessState.ALLOWED && path.isEmpty()) {
+ throw new IllegalArgumentException("A granted decision requires a derivation path");
+ }
if (state != AccessState.ALLOWED && !path.isEmpty()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public AccessExplanation { | |
| Objects.requireNonNull(state, "state"); | |
| reasonCode = requireText(reasonCode, "reasonCode"); | |
| path = List.copyOf(Objects.requireNonNull(path, "path")); | |
| blockedBy = List.copyOf(Objects.requireNonNull(blockedBy, "blockedBy")); | |
| policyVersion = requireText(policyVersion, "policyVersion"); | |
| Objects.requireNonNull(evaluatedAt, "evaluatedAt"); | |
| if (state != AccessState.ALLOWED && !path.isEmpty()) { | |
| throw new IllegalArgumentException("Only a granted decision carries a derivation path"); | |
| } | |
| public AccessExplanation { | |
| Objects.requireNonNull(state, "state"); | |
| reasonCode = requireText(reasonCode, "reasonCode"); | |
| path = List.copyOf(Objects.requireNonNull(path, "path")); | |
| blockedBy = List.copyOf(Objects.requireNonNull(blockedBy, "blockedBy")); | |
| Objects.requireNonNull(provenance, "provenance"); | |
| policyVersion = requireText(policyVersion, "policyVersion"); | |
| Objects.requireNonNull(evaluatedAt, "evaluatedAt"); | |
| if (state == AccessState.ALLOWED && path.isEmpty()) { | |
| throw new IllegalArgumentException("A granted decision requires a derivation path"); | |
| } | |
| if (state != AccessState.ALLOWED && !path.isEmpty()) { | |
| throw new IllegalArgumentException("Only a granted decision carries a derivation path"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/orgmemory/core/authorization/AccessExplanation.java`
around lines 25 - 34, Update the AccessExplanation constructor to enforce
complete ALLOWED explanations: require non-null provenance and reject an ALLOWED
state when path is empty, while preserving the existing restriction against
paths on non-ALLOWED states.
Comment on lines
+32
to
+40
| public static AclProvenance source( | ||
| String origin, long generation, Instant capturedAt, Instant validUntil, Instant now) { | ||
| return new AclProvenance( | ||
| "SOURCE", | ||
| origin, | ||
| generation, | ||
| capturedAt, | ||
| validUntil, | ||
| validUntil != null && Objects.requireNonNull(now, "now").isAfter(validUntil)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when mirrored ACL validity is absent or exhausted.
A SOURCE provenance with validUntil == null returns expired=false, so incomplete mirror evidence can appear current; the exact expiry instant is also accepted. This conflicts with AccessState.UNKNOWN for incomplete ACLs.
Proposed fix
public static AclProvenance source(
String origin, long generation, Instant capturedAt, Instant validUntil, Instant now) {
+ Instant evaluatedAt = Objects.requireNonNull(now, "now");
+ boolean incomplete = capturedAt == null || validUntil == null;
return new AclProvenance(
"SOURCE",
origin,
generation,
capturedAt,
validUntil,
- validUntil != null && Objects.requireNonNull(now, "now").isAfter(validUntil));
+ incomplete || !evaluatedAt.isBefore(validUntil));
}As per path instructions, “Authorization must fail closed.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static AclProvenance source( | |
| String origin, long generation, Instant capturedAt, Instant validUntil, Instant now) { | |
| return new AclProvenance( | |
| "SOURCE", | |
| origin, | |
| generation, | |
| capturedAt, | |
| validUntil, | |
| validUntil != null && Objects.requireNonNull(now, "now").isAfter(validUntil)); | |
| public static AclProvenance source( | |
| String origin, long generation, Instant capturedAt, Instant validUntil, Instant now) { | |
| Instant evaluatedAt = Objects.requireNonNull(now, "now"); | |
| boolean incomplete = capturedAt == null || validUntil == null; | |
| return new AclProvenance( | |
| "SOURCE", | |
| origin, | |
| generation, | |
| capturedAt, | |
| validUntil, | |
| incomplete || !evaluatedAt.isBefore(validUntil)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/main/java/com/orgmemory/core/authorization/AclProvenance.java`
around lines 32 - 40, Update AclProvenance.source so SOURCE provenance is marked
expired when validUntil is absent or when now is at or after validUntil; require
now only when evaluating the timestamp, preserving the existing constructor and
provenance fields.
Source: Path instructions
Comment on lines
+274
to
+279
| <input | ||
| value={resourceId} | ||
| onChange={(event) => setResourceId(event.target.value)} | ||
| placeholder="Resource id" | ||
| className="h-9 min-w-[300px] flex-1 rounded-md border bg-transparent px-3 text-sm shadow-xs outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50" | ||
| /> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Resource id input lacks an accessible label.
The <input> only has a placeholder, with no <label> or aria-label. Screen reader users won't get a persistent field name once text is entered.
♿ Proposed fix
<input
+ aria-label="Resource id"
value={resourceId}
onChange={(event) => setResourceId(event.target.value)}
placeholder="Resource id"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <input | |
| value={resourceId} | |
| onChange={(event) => setResourceId(event.target.value)} | |
| placeholder="Resource id" | |
| className="h-9 min-w-[300px] flex-1 rounded-md border bg-transparent px-3 text-sm shadow-xs outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50" | |
| /> | |
| <input | |
| aria-label="Resource id" | |
| value={resourceId} | |
| onChange={(event) => setResourceId(event.target.value)} | |
| placeholder="Resource id" | |
| className="h-9 min-w-[300px] flex-1 rounded-md border bg-transparent px-3 text-sm shadow-xs outline-none focus-visible:ring-[3px] focus-visible:ring-ring/50" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/features/admin/components/admin-user-permissions-page.tsx` around
lines 274 - 279, Add an accessible label for the resourceId input in the admin
permissions form, using a visible label or an appropriate aria-label while
preserving the existing value, onChange, placeholder, and styling behavior.
`visited` was a running set shared across every union branch and never unwound. A userset abandoned deep in one branch — or simply one whose expansion timed out, since the key was added before the read — was then skipped in every later branch, so a grant reachable by a shorter route came back "path unavailable". Unwinding on the way out still breaks cycles: a repeat is only reachable while the first visit is on the stack. A mirrored ACL with no recorded validity now counts as expired. Absent validity is missing evidence, not licence to treat the copy as current, and it is exactly what UNKNOWN exists for. The boundary instant expires with it. An explanation must carry provenance. Every construction already passed one but nothing said so, and a verdict without it cannot be acted on — the reader has no way to tell a decision OrgMemory makes from a copy of one Slack or Drive owns. Also: refuse a blank permission with a message rather than a NullPointerException, and assert ListObjects for can_curate_graph, because a correct Check still leaks if the listing over-returns. Reported by CodeRabbit on #36. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n-in Authenticating at the identity provider has never been enough to reach OrgMemory: an external_identities row must bind the OIDC subject to an app user, and nothing wrote one. Every onboarding was a manual INSERT, and the admin surface could not tell "never expected" from "expected but not linked" — both read as Not linked. Meanwhile the 32 Keycloak accounts with no app user were invisible entirely, so a refused sign-in left nothing on screen to diagnose. An invitation is the missing record: an address an administrator expects, and the role it arrives as. The first sign-in matching an open invitation creates the user and writes the binding. This widens nothing — a sign-in with no invitation is refused exactly as before — and the binding is still to (issuer, subject). The address only chooses which invitation applies; it never becomes the identity. Ambiguity fails closed. Two organizations expecting the same address is not resolvable from a sign-in alone, so nothing is provisioned rather than placing somebody in the wrong tenant. An address that already has an account is linked rather than duplicated. Invitations are their own list on the users screen, not a status on a user row, because an invitation has no user behind it. Accepted and revoked rows are kept: knowing an address was invited, by whom, and what became of it is the point. Onyx solves this by auto-creating on first login against an invited-email list in a key-value store. The gate is worth copying; the store is not — this keeps the (issuer, subject) ledger with its foreign keys and audit columns. Also filters the inspector's permissions by resource type. Offering every permission against every type invited questions the engine cannot answer, and "may they administer the organization, on a knowledge space" came back UNKNOWN, which reads as "we could not tell" when the question was never meaningful. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
Author
|
Đã xử lý phản hồi review và bổ sung onboarding, tất cả trên PR này. Sửa theo CodeRabbitHai cái là lỗi thật:
Ba cái nhỏ: Không sửa: góp ý về số lần gọi mạng mỗi lần mở trang. Đó là đánh đổi có chủ ý và đã ghi trong code — quyền được tính khi hỏi, không lưu; cache lại chính là nguồn sự thật thứ hai mà thiết kế này tránh. Bảy check trên một tập cố định là chi phí có chặn trên. Bổ sung: onboarding bằng lời mờiKiểm dữ liệu thật cho thấy
Onyx giải bài này bằng auto-create dựa trên danh sách email trong KV store. Cái cổng đáng học; cái kho thì không — ở đây giữ nguyên sổ Chạy thậtMời
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The admin surface reported
role,active,signInLinked, and a mapped-principal count — none of which is an authorization decision. Worse, the role control it offers writesapp_users.role, while every guard asks OpenFGA, so that control grants nothing. An administrator had no way to see what a user could actually do.What this adds
RelationshipExpansionPortover OpenFGAExpand— the one new port. It returns the userset tree for an object and relation in provider-neutral form: direct tuples, computed rewrites, the tuple-to-userset hop that carries inheritance, and the set operations composing them. Nothing grants access on the strength of it; enforcement stays with the check ports.AccessExplanationServiceturns that tree into the single derivation that reached a principal, following usersets recursively and stopping at the first branch a union satisfies — which is how the engine resolves it. A refusal instead names the branches that were evaluated.Two read endpoints and three write endpoints. Effective organization permissions, an access explanation, and role assignment. Roles are tuple data, so granting one is a tuple and revoking it a deletion — no deploy, no migration.
A permission profile at
/admin/users/$userIdwith the three shared components the design settles on:AccessVerdict,AccessPath,AccessDenied.Three decisions worth reviewing
A verdict has three states, not two.
ALLOWED,DENIED,UNKNOWN. Most document ACLs are mirrored from Slack or Drive under a 23-hour validity, and an expired mirror is not a denial. Keeping the third state meant checkingRelationshipAuthorizationPortdirectly rather than throughEffectiveAuthorizationService, which collapses an unanswered check into a denial — right for enforcement, wrong to show an administrator, since "no relationship grants this" and "the engine did not answer" need different actions. Nothing on this path grants access, so preserving the outcome costs no safety.Administrators may not write against content the source system owns.
AdministrativeTupleScoperefuses any write whose object is not anorganizationor arole, tested against aknowledge_assettarget and against a role name trying to smuggle one. A second writer would let OrgMemory and Drive diverge, after which nobody could answer who may read a document.can_curate_graph: knowledge_curator or administrator. Additive; no existing tuple changes meaning. For the POC nobody holdsknowledge_curatorand the administrator satisfies it through theorbranch, so separating the roles later is deleting that clause — no Java change, no migration.knowledge_curatoris an assignable capability noun in the same family asknowledge_readerandknowledge_reviewer, not a business role, so it does not cross the "business roles are tuple data" convention.Verification
.\gradlew.bat --no-daemon clean testgreen.fga model test6/6 tests, 23 checks.pnpm -C web typecheckandbuildgreen, no dependency added.Browser run against the live stack (Keycloak, OpenFGA, Postgres, API, Vite):
can_curate_graphAllowed with noknowledge_curatortuple present — theor administratorbranch, live;can_publishon space…802Allowed, renderingcan_publish→reviewer→organizational_unit …2222 · manager;The demo fixture changed to make that possible: Linh and Minh were both plain members of the same organization and unit, so every verdict came out identical and there was nothing to explain. Minh becomes the unit's manager and its managers become reviewers of the sales space — what his
TEAM_LEADrecord already claimed.Deliberately not in this PR
GET /users/{id}/containers. Reachingacl_authority, generation, and validity means joiningknowledge_assetsthroughsource_objectsandraw_source_objectstosource_acl_snapshots, which cannot be honestly verified without a live database.AclProvenanceis already a parameter ofexplainand rides every response, so it is wiring rather than redesign. Until it lands provenance isORGMEMORY, and consequently the mirrored-verdict rendering is covered by unit tests rather than a browser run.PermissionAuditServicelives incore.permission, whichcore.authorizationmust not depend on without inverting the module direction Modulith enforces. It belongs in the API layer; left out rather than bent.app_users.rolecontrol — it still reads as a grant while granting nothing.Design and plan, including the ten decisions and the enterprise references they came from, are in
docs/increments/active/2026-07-24-admin-permission-surface/.🤖 Generated with Claude Code
Summary by CodeRabbit