Skip to content

[identity] add IsMachine extension and identity:users.read claim type for read-only operation is UserApi #1003

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dkarkanas wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

[identity] add IsMachine extension and identity:users.read claim type for read-only operation is UserApi #1003

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f14b771
[identity] add `IsMachine` extension and `identity:users.read` claim…
dkarkanas Apr 20, 2026
2f8a278
Merge branch 'develop' into feature/identity-user-read-claims
travlos Apr 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/Indice.Common/Security/ClaimsPrincipalExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,12 @@ public static bool IsSystemClient(this ClaimsPrincipal principal) {
return isSystem ?? false;
}

/// <summary>Checks if the current principal is a machine user. A machine user is a principal that is authenticated but does not have a subject id claim.</summary>
/// <param name="principal">The current principal.</param>
/// <returns></returns>
public static bool IsMachine(this ClaimsPrincipal principal) =>
principal.Identity?.IsAuthenticated is true && principal.FindSubjectId() is null;
Comment on lines +83 to +84

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.

/// <summary>Checks if the current principal is a system admin.</summary>
/// <param name="principal">The current principal.</param>
public static bool IsAdmin(this ClaimsPrincipal principal) => FindFirstValue<bool>(principal, BasicClaimTypes.Admin) ?? principal.HasRoleClaim("Administrator");
Expand Down
4 changes: 2 additions & 2 deletions src/Indice.Features.Identity.Server/Extensions/ServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ public static IExtendedIdentityServerBuilder AddExtendedEndpoints(this IExtended
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()));
});
Comment on lines 294 to 298

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines 294 to 298

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
authOptions.AddPolicy(IdentityEndpoints.Policies.BeUsersWriter, policy => {
policy.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme)
Expand Down Expand Up @@ -334,7 +334,7 @@ public static IExtendedIdentityServerBuilder AddExtendedEndpoints(this IExtended
authOptions.AddPolicy(IdentityEndpoints.Policies.BeLogsReader, policy => {
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()));

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
.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()));

Copilot uses AI. Check for mistakes.
});
authOptions.AddPolicy(IdentityEndpoints.Policies.BeLogsWriter, policy => {
policy.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme)
Expand Down
2 changes: 2 additions & 0 deletions src/Indice.Features.Identity.Server/Manager/IdentityEndpoints.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public static partial class SubScopes
public const string Clients = "identity:clients";
/// <summary>A scope that allows managing users on IdentityServer.</summary>
public const string Users = "identity:users";
/// <summary>A scope that allows reading users on IdentityServer.</summary>
public const string UsersRead = "identity:users.read";
/// <summary>A scope that allows using the totp endpoints on IdentityServer.</summary>
public const string Totp = "identity:totp";
/// <summary>A scope that allows reading the secret for a user device.</summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Indice.Features.Identity.Server/Manager/UsersApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public static RouteGroupBuilder MapManageUsers(this IdentityServerEndpointRouteB
group.WithTags("Users");
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();

Copilot AI Apr 20, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
group.RequireAuthorization(policy => policy
.RequireAuthenticatedUser()
.AddAuthenticationSchemes(IdentityEndpoints.AuthenticationScheme)
Expand Down
Loading