[identity] add IsMachine extension and identity:users.read claim type for read-only operation is UserApi - #1003
[identity] add IsMachine extension and identity:users.read claim type for read-only operation is UserApi#1003dkarkanas wants to merge 2 commits into
IsMachine extension and identity:users.read claim type for read-only operation is UserApi#1003Conversation
…type for read-only operation is UserApi
There was a problem hiding this comment.
Pull request overview
This PR introduces a new granular scope (identity:users.read) intended to allow machine-to-machine principals to read users (and logs) with more fine-grained permissions in the Identity Server management APIs.
Changes:
- Added a new sub-scope constant
IdentityEndpoints.SubScopes.UsersRead = "identity:users.read". - Expanded Users API allowed scopes to include
identity:users.read. - Updated
BeUsersReader/BeLogsReaderpolicies to recognize “machine users” via a newClaimsPrincipal.IsMachine()extension.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/Indice.Features.Identity.Server/Manager/UsersApi.cs | Adds UsersRead to the Users API route group allowed scopes. |
| src/Indice.Features.Identity.Server/Manager/IdentityEndpoints.cs | Introduces IdentityEndpoints.SubScopes.UsersRead constant. |
| src/Indice.Features.Identity.Server/Extensions/ServiceCollectionExtensions.cs | Updates authorization policies to allow machine principals under certain scopes. |
| src/Indice.Common/Security/ClaimsPrincipalExtensions.cs | Adds IsMachine() extension method used by authorization policies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| authOptions.AddPolicy(IdentityEndpoints.Policies.BeUsersReader, policy => { | ||
| policy.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme) | ||
| .RequireAuthenticatedUser() | ||
| .RequireAssertion(x => x.User.HasScope(IdentityEndpoints.SubScopes.Users) && x.User.CanReadUsers()); | ||
| .RequireAssertion(x => (x.User.HasScope(IdentityEndpoints.SubScopes.Users) && x.User.CanReadUsers()) || (x.User.HasScope(IdentityEndpoints.SubScopes.UsersRead) && x.User.IsMachine())); | ||
| }); |
There was a problem hiding this comment.
BeUsersReader policy is registered twice in this method. The later AddPolicy(IdentityEndpoints.Policies.BeUsersReader, ...) (lines 304-308) will overwrite the earlier registration (lines 294-298), effectively removing the new UsersRead/IsMachine() logic at runtime. Remove the duplicate policy registration or update it so the final registered policy includes the new assertion.
| group.WithGroupName("identity"); | ||
| // Add security requirements, all incoming requests to this API *must* be authenticated with a valid user. | ||
| var allowedScopes = new[] { options.ApiScope, IdentityEndpoints.SubScopes.Users }.FilterOutNulls().ToArray(); | ||
| var allowedScopes = new[] { options.ApiScope, IdentityEndpoints.SubScopes.Users , IdentityEndpoints.SubScopes.UsersRead }.FilterOutNulls().ToArray(); |
There was a problem hiding this comment.
By adding IdentityEndpoints.SubScopes.UsersRead to the route group's allowedScopes, any endpoint that relies only on the group-level RequireClaim(Scope, allowedScopes) (i.e., does not add BeUsersReader/BeUsersWriter explicitly) becomes callable with the read-only scope. In this group there is at least one non-GET endpoint (POST {userId}/email/confirmation) without an explicit writer policy, so identity:users.read can trigger side effects. Add explicit authorization (likely BeUsersWriter) to that endpoint, or split read vs write endpoints into separate groups with different allowed scopes.
| policy.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme) | ||
| .RequireAuthenticatedUser() | ||
| .RequireAssertion(x => x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && x.User.CanReadUsers()); | ||
| .RequireAssertion(x => (x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && x.User.CanReadUsers()) || (x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && x.User.IsMachine())); |
There was a problem hiding this comment.
The BeLogsReader assertion repeats HasScope(IdentityEndpoints.SubScopes.Logs) in both branches, which makes the intent harder to read. Consider factoring the scope check once (e.g., require the logs scope and then check (CanReadUsers() || IsMachine())) to keep the policy easier to maintain.
| .RequireAssertion(x => (x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && x.User.CanReadUsers()) || (x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && x.User.IsMachine())); | |
| .RequireAssertion(x => x.User.HasScope(IdentityEndpoints.SubScopes.Logs) && (x.User.CanReadUsers() || x.User.IsMachine())); |
| public static bool IsMachine(this ClaimsPrincipal principal) => | ||
| principal.Identity?.IsAuthenticated is true && principal.FindSubjectId() is null; |
There was a problem hiding this comment.
IsMachine uses principal.Identity?.IsAuthenticated, which only reflects the first identity on the principal. If a principal contains multiple identities, this can incorrectly return false even when another identity is authenticated. Consider using principal.Identities.Any(i => i.IsAuthenticated) (or, since callers already use RequireAuthenticatedUser(), dropping the authentication check and only checking the absence of subject id).
| authOptions.AddPolicy(IdentityEndpoints.Policies.BeUsersReader, policy => { | ||
| policy.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme) | ||
| .RequireAuthenticatedUser() | ||
| .RequireAssertion(x => x.User.HasScope(IdentityEndpoints.SubScopes.Users) && x.User.CanReadUsers()); | ||
| .RequireAssertion(x => (x.User.HasScope(IdentityEndpoints.SubScopes.Users) && x.User.CanReadUsers()) || (x.User.HasScope(IdentityEndpoints.SubScopes.UsersRead) && x.User.IsMachine())); | ||
| }); |
There was a problem hiding this comment.
New authorization behavior is introduced here (identity:users.read + IsMachine() for BeUsersReader, and machine access for logs). There are existing identity endpoint tests (e.g., UserApiTests/UserAvatarApiTest) but none appear to cover these new scope/policy combinations. Add tests that assert: (1) a machine principal with identity:users.read can call GET user endpoints, (2) it cannot call write endpoints, and (3) a machine principal with identity:logs can call log-reading endpoints.
This pull request introduces support for a new
users.readscope, enabling machine users to access user and log reading endpoints with more granular permissions. It also updates authorization policies to recognize machine users and allows them to use the new scope. The most important changes are summarized below:Authorization Policy Updates:
BeUsersReaderpolicy to allow access if a principal has the newusers.readscope and is a machine user, in addition to the existing requirements.BeLogsReaderpolicy to allow access if the principal has thelogsscope and is a machine user, in addition to the existing requirements.Scope and API Changes:
UsersReadscope (identity:users.read) to theSubScopesclass for granular user read access.users.readscope, enabling clients with this scope to access user management endpoints.ClaimsPrincipal Extension:
IsMachineextension method toClaimsPrincipalto identify machine users (authenticated principals without a subject id claim).