Add Client resource type and scopes to authorization schema

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Avoid UOE on removal events, filter type resource
What’s good: New Clients resource type and V2 evaluator integrate with AdminPermissionsSchema coherently; permission checks reuse shared guards and leverage root.evaluatePermission consistently.
Review Status: ❌ Requires changes
Overall Priority: Critical

Issues (Critical & High only)

Severity	Issue	Why it matters
Critical	Correctness — UnsupportedOperationException in setPermissionsEnabled br...	…/permissions/ClientPermissionsV2.java This method throws UnsupportedOperationException in V2 but is still invoked by the AdminPermissions event listener on ClientRemovedEvent, causing a runtime crash when ADMIN_FINE_GRAINED_AUTHZ is enabled. Implement a no-op to preserve event flow and avoid crashes while keeping V2 semantics.

Showing top 1 issues. Critical: 1, High: 0. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: V2 methods that are invoked by existing infrastructure should not throw UnsupportedOperationException; prefer no-ops or delegation to maintain parity and prevent crashes. Also filter out the type-level resource in getClientsWithPermission to avoid returning a non-client entry.
• Testing: Add a test that removes a client while ADMIN_FINE_GRAINED_AUTHZ is enabled to ensure no exceptions are thrown by the event listener (setPermissionsEnabled path). Also include an assertion for getClientsWithPermission that the returned set contains only actual client IDs and not the type-level resource.
• Documentation: Document the intended behavior of permissions toggling in V2 (e.g., setPermissionsEnabled/isPermissionsEnabled as no-ops) and clarify that getClientsWithPermission returns client IDs (not clientIds) and excludes the type-level 'Clients' resource.
• Compatibility: Wrapping the event listener under ADMIN_FINE_GRAINED_AUTHZ changes behavior; ensure environments relying on automatic cleanup when the feature is disabled are unaffected. Keeping V2 methods as no-ops maintains backward compatibility with existing event flows.
• Performance: Minor: both hasPermission methods create a List for a single scope (Arrays.asList(scope)); a direct equals comparison would avoid small allocations, but impact is negligible.
• Security: Authorization checks appear consistent with schema scopes and admin roles; no obvious escalation paths. Ensure the 'all-clients' fallback in hasPermission(ClientModel, String) cannot accidentally grant permissions when no policies exist (current policy check guards this).
• Open questions: Is MgmtPermissionsV2 guaranteed to never call setPermissionsEnabled/isPermissionsEnabled in V2 flows? If not, can we make these methods no-ops to preserve event listener behavior without breaking V2 semantics?

Confidence: 2/5 — Not ready to merge (1 critical · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant ProviderEventManager
    participant AdminPermissions
    participant MgmtPermissionsV2
    participant ClientPermissionsV2
    ProviderEventManager->>AdminPermissions: onEvent(ClientRemovedEvent)
    AdminPermissions->>MgmtPermissionsV2: management(...).clients()
    MgmtPermissionsV2-->>AdminPermissions: ClientPermissionsV2
    AdminPermissions->>ClientPermissionsV2: setPermissionsEnabled(client, false)
    alt V2 method throws UOE
        ClientPermissionsV2-->>AdminPermissions: UnsupportedOperationException
    end

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

🛑 Critical: This method throws UnsupportedOperationException in V2 but is still invoked by the AdminPermissions event listener on ClientRemovedEvent, causing a runtime crash when ADMIN_FINE_GRAINED_AUTHZ is enabled. Implement a no-op to preserve event flow and avoid crashes while keeping V2 semantics.

Suggested change

	public void setPermissionsEnabled(ClientModel client, boolean enable) {
	public void setPermissionsEnabled(ClientModel client, boolean enable) {
	// No-op in V2: permissions toggling is managed by policies, not per-entity flags
	}

[codoki-pr-intelligence]

🔷 Medium: getClientsWithPermission likely includes the type-level 'Clients' resource, returning a non-client name (e.g., "Clients") alongside actual client IDs. Filter out the type-level resource to return only concrete clients.

[codoki-pr-intelligence]

💡 Low: This helper is unused in V2 and increases maintenance burden; either remove it or integrate it where token-bound client authorization is evaluated.

Suggested change

	private EvaluationContext getEvaluationContext(ClientModel authorizedClient, AccessToken token) {
	/* Remove unused method to reduce maintenance surface in V2 */