Skip to content

feat(aspnetcore): add group-role mapping extension for authorization policies#143

Draft
vaernion wants to merge 3 commits intomainfrom
feat/aspnet-group-roles
Draft

feat(aspnetcore): add group-role mapping extension for authorization policies#143
vaernion wants to merge 3 commits intomainfrom
feat/aspnet-group-roles

Conversation

@vaernion
Copy link
Copy Markdown
Contributor

@vaernion vaernion commented Jan 20, 2026

Example usage:

            services.AddGroupRoleMappings(
                new Dictionary<string, IEnumerable<string>>()
                {
                    {
                        MasseutsendelseAuthorization.ReaderPolicy,
                        // senders also have read access
                        [.. senderGroupIds, .. readerGroupIds]
                    },
                    { MasseutsendelseAuthorization.SenderPolicy, senderGroupIds },
                }
            );

NB: Client credential tokens (m2m) are not handled yet. We can add a path for checking for role access_as_application, but what should the API be?

    public static void AddGroupRoleMappings(
        this IServiceCollection services,
        IReadOnlyDictionary<string, IEnumerable<string>> roleGroupMappings,
        string m2mAppRole, // "access_as_application",
        string groupClaimType = "groups",
        string rolesClaimType = "roles"
    )

Copilot AI review requested due to automatic review settings January 20, 2026 17:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new authorization extension method AddGroupRoleMappings that enables mapping group IDs from JWT claims to authorization policies, allowing role-based authorization when only group claims are available in tokens.

Changes:

  • Added AddGroupRoleMappings extension method to configure authorization policies based on group-to-role mappings
  • Implemented comprehensive test coverage for various authorization scenarios
  • Updated package version to 2.3.0-alpha.1 and documented the new feature in CHANGELOG

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
AuthorizationExtensions.cs Implements the core extension method for group-role mapping with case-insensitive group ID comparison
AuthorizationExtensionsTests.cs Provides comprehensive test coverage for authentication, group membership, and custom claim type scenarios
CHANGELOG.md Documents the new HasAnyAllowedGroup extension method in version 2.3.0
AT.Common.AspNetCore.Publish.csproj Updates package version to 2.3.0-alpha.1 for the new feature release

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread AspNetCore/AT.Common.AspNetCore.Publish/Extensions/AuthorizationExtensions.cs Outdated
Comment thread AspNetCore/AT.Common.AspNetCore.Publish/CHANGELOG.md Outdated
@vaernion vaernion requested review from at-bgp and mkilgus-at January 20, 2026 17:13
foreach (var mapping in roleGroupMappings)
{
var allowed = (mapping.Value ?? Array.Empty<string>()).ToHashSet(
StringComparer.OrdinalIgnoreCase
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Jeg tror denne kan skjule/skape usynlige feil.

Hvis det er to groups "reader" og "Reader", så blir bare den første brukt (siden de er equal under string compare med StringComparer.OrdinalIgnoreCase)

jeg ville heller latt det være case sensitive.

Hva enn vi velger så må vi sørge for at det passer i HasAnyAllowedGroup()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Det er fordi gruppene her ikke er navn som "Reader", men objekt-ids i Entra. De er GUID's som ikke er case sensitive.

F.eks. 52d10c33-1ee7-4215-b433-f719b42d00f4 eller 52D10C33-1EE7-4215-b433-f719b42d00f4 fungerer likt ved kall til Entra API (testet med client credential request).

Copy link
Copy Markdown
Contributor Author

@vaernion vaernion Jan 23, 2026

Choose a reason for hiding this comment

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

Vi kan fjerne det uansett for å forenkle koden da. HashSet er neppe nødvendig også siden det er snakk om korte lister med grupper, men Copilot maste om det.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aha, da foreslår jeg at vi fjerner HasSet og legger på StringComparer.OrdinalIgnoreCase på det .Contains-kallet i HasAnyAllowedGroup()

@mkilgus-at
Copy link
Copy Markdown
Collaborator

Ide:
Kan vi utvide den her PR med å eksponere en egen authorization extension i tillegg?
Da kunne vi flytte AuthenticationConfiguration også hit, og kanskje rename/forbedre hele config objektet.
Målet er å flytte det her:
image
Slik at vi får noko ala

services.AddEntraAuthentication(config);
services.AddEntraGroupRoleMappings(config);

@sonarqubecloud
Copy link
Copy Markdown

@mkilgus-at
Copy link
Copy Markdown
Collaborator

@vaernion ka er status her? har du lyst til å fortsette på den? Ble åpenbart merge konflikter etter @at-bgp sine endringer i v2.5.0, antar at vi kunne godt ta en gjennomgang hvis du trenger hjelp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants