Skip to content

Commit

Permalink
fix: Simplify subject attribute matching (#1348)
Browse files Browse the repository at this point in the history 
## Description

This simplifies the subject attribute mapping to only use a single
attribute, as multiple subject attributes are explicitly disallowed by
the PDP (eg. for system users). In fact, for external use, only a single
subject attribute is ever required (eg. sending the authlvl attribute is
not supported, as it is the PEPs responsibility to enforce any
obligations returned from the PDP).

In addition, support for allowing pure Maskinporten tokens (using only
consumer-claims) has been removed, as this is not officially supported
in the Altinn Authorization model; only userid/pid/systemuserid will
cause the PDP to resolve roles/access packages in order to match policy
rules, so the only way sending organization numbers as subject claims is
if the policy itself contains hard coded organization numbers, which is
discouraged (should access lists for that).

Note that urn:altinn:org (ie serviceowner acronym claim types) are left
out, as authenticated service owners should not use the end user APIs
(this would potentially leak information that we only want to make
available to the end users).

## Related Issue(s)

See previous PR (#1340) and [slack
thread](https://digdir.slack.com/archives/C079ZFUSFMW/p1729772275391209).

## Verification

- [x] **Your** code builds clean without any errors or warnings
- [x] Manual testing done (required)
- [x] Relevant automated test added (if you find this hard, leave it and
we'll help out)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **New Features**
- Enhanced error handling with the introduction of an
`UnreachableException` for invalid user types.
	- Streamlined attribute selection logic for improved performance.

- **Bug Fixes**
- Updated claims structure in tests to reflect recent changes, ensuring
accurate validation.

- **Tests**
	- Added a new test for exception handling.
- Renamed and consolidated existing tests for clarity and
maintainability.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
@elsand
elsand authored Oct 30, 2024
1 parent 169b043 commit 55159b7
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 82 deletions.
90 changes: 52 additions & 38 deletions src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
using Altinn.Authorization.ABAC.Xacml.JsonProfile;
using System.Diagnostics;
using Altinn.Authorization.ABAC.Xacml.JsonProfile;
using System.Security.Claims;

using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Externals.AltinnAuthorization;
using Digdir.Domain.Dialogporten.Domain.Parties;
Expand All @@ -10,20 +12,29 @@ namespace Digdir.Domain.Dialogporten.Infrastructure.Altinn.Authorization;
internal static class DecisionRequestHelper
{
private const string SubjectId = "s1";
private const string AltinnUrnNsPrefix = "urn:altinn:";

private const string PidClaimType = "pid";
private const string ConsumerClaimType = "consumer";
private const string UserIdClaimType = "urn:altinn:userid";
private const string RarAuthorizationDetailsClaimType = "authorization_details";

private const string AttributeIdAction = "urn:oasis:names:tc:xacml:1.0:action:action-id";
private const string AttributeIdResource = "urn:altinn:resource";
private const string AttributeIdResourceInstance = "urn:altinn:resourceinstance";
private const string AltinnAutorizationDetailsClaim = "authorization_details";
private const string AttributeIdSubResource = "urn:altinn:subresource";

private const string AttributeIdOrg = "urn:altinn:org";
private const string AttributeIdApp = "urn:altinn:app";
private const string AttributeIdSystemUser = "urn:altinn:systemuser:uuid";
private const string AttributeIdAppInstance = "urn:altinn:instance-id";

private const string AttributeIdUserId = "urn:altinn:userid";
private const string AttributeIdPerson = "urn:altinn:person:identifier-no";
private const string AttributeIdSystemUser = "urn:altinn:systemuser:uuid";

// The order of these attribute types is important as we want to prioritize the most specific claim types.
private static readonly List<string> PrioritizedClaimTypes = [AttributeIdUserId, AttributeIdPerson, AttributeIdSystemUser];

private const string ReservedResourcePrefixForApps = "app_";
private const string AttributeIdAppInstance = "urn:altinn:instance-id";
private const string AttributeIdSubResource = "urn:altinn:subresource";

private const string PermitResponse = "Permit";

public static XacmlJsonRequestRoot CreateDialogDetailsRequest(DialogDetailsAuthorizationRequest request)
Expand Down Expand Up @@ -71,39 +82,42 @@ public static DialogDetailsAuthorizationResult CreateDialogDetailsResponse(List<
};
}

private static List<XacmlJsonCategory> CreateAccessSubjectCategory(IEnumerable<Claim> claims)
{
var attributes = claims
.Select(x => x switch
private static List<XacmlJsonCategory> CreateAccessSubjectCategory(IEnumerable<Claim> claims) =>
// The PDP expects for the most part only a single subject attribute, and will even fail the request
// for some types (e.g. the urn:altinn:systemuser:uuid) if there are multiple subject attributes (for
// security reasons). We therefore need to filter out the relevant attributes and only include those,
// which in essence is the pid and the system user uuid. In addition, we also utilize urn:altinn:userid
// if present instead of the pid as a simple optimization as this offloads the PDP from having to look up
// the user id from the pid. See PrioritizedClaimTypes for the order of prioritization.
claims.Select(claim => claim.Type switch
{
UserIdClaimType => new XacmlJsonCategory
{
{ Type: PidClaimType } => new XacmlJsonAttribute { AttributeId = NorwegianPersonIdentifier.Prefix, Value = x.Value },
{ Type: var type } when type.StartsWith(AltinnUrnNsPrefix, StringComparison.Ordinal) => new() { AttributeId = type, Value = x.Value },
{ Type: ConsumerClaimType } when x.TryGetOrganizationNumber(out var organizationNumber) => new() { AttributeId = NorwegianOrganizationIdentifier.Prefix, Value = organizationNumber },
{ Type: AltinnAutorizationDetailsClaim } => new() { AttributeId = AttributeIdSystemUser, Value = GetSystemUserId(x) },
_ => null
})
.Where(x => x is not null)
.Cast<XacmlJsonAttribute>()
.ToList();

// If we're authorizing a person (i.e. ID-porten token), we are not interested in the consumer-claim (organization number)
// as that is not relevant for the authorization decision (it's just the organization owning the OAuth client).
// The same goes if urn:altinn:userid is present, which might be present if using a legacy enterprise user token
if (attributes.Any(x => x.AttributeId == NorwegianPersonIdentifier.Prefix) ||
attributes.Any(x => x.AttributeId == AttributeIdUserId))
Id = SubjectId,
Attribute = [new() { AttributeId = AttributeIdUserId, Value = claim.Value }]
},
PidClaimType => new XacmlJsonCategory
{
Id = SubjectId,
Attribute = [new() { AttributeId = AttributeIdPerson, Value = claim.Value }]
},
RarAuthorizationDetailsClaimType when new ClaimsPrincipal(new ClaimsIdentity(new[] { claim })).TryGetSystemUserId(out var systemUserId) => new XacmlJsonCategory
{
Id = SubjectId,
Attribute =
[
new XacmlJsonAttribute { AttributeId = AttributeIdSystemUser, Value = systemUserId }
]
},
_ => null
})
.Where(x => x != null)
.MinBy(x => PrioritizedClaimTypes.IndexOf(x!.Attribute[0].AttributeId)) switch
{
attributes.RemoveAll(x => x.AttributeId == NorwegianOrganizationIdentifier.Prefix);
}

return [new() { Id = SubjectId, Attribute = attributes }];
}

private static string GetSystemUserId(Claim claim)
{
var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity([claim]));
claimsPrincipal.TryGetSystemUserId(out var systemUserId);
return systemUserId!;
}
{ } validCategory => new List<XacmlJsonCategory> { validCategory },
_ => throw new UnreachableException(
"Unable to find a suitable subject attribute for the authorization request. Having a known user type should be enforced during authentication (see UserTypeValidationMiddleware)."),
};

private static List<XacmlJsonCategory> CreateActionCategories(
List<AltinnAction> altinnActions, out Dictionary<string, string> actionIdByName)
Expand Down
73 changes: 29 additions & 44 deletions tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Security.Claims;
using System.Diagnostics;
using System.Security.Claims;
using Altinn.Authorization.ABAC.Xacml.JsonProfile;
using Digdir.Domain.Dialogporten.Application.Common.Authorization;
using Digdir.Domain.Dialogporten.Application.Externals.AltinnAuthorization;
Expand All @@ -10,8 +11,6 @@ namespace Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests;

public class DecisionRequestHelperTests
{
private const string ConsumerClaimValue = /*lang=json,strict*/ "{\"authority\":\"iso6523-actorid-upis\",\"ID\":\"0192:991825827\"}";

private const string AuthorizationDetailsClaimValue = /*lang=json,strict*/"[{\"type\":\"urn:altinn:systemuser\",\"systemuser_id\":[\"unique_systemuser_id\"]}]";

[Fact]
Expand All @@ -20,10 +19,9 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequest()
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("pid", "12345678901"),

// This should not be copied as subject claim since there's a "pid"-claim
("consumer", ConsumerClaimValue)
("authorization_details", AuthorizationDetailsClaimValue),
("pid", "12345678901")
),
$"{NorwegianOrganizationIdentifier.PrefixWithSeparator}713330310");
var dialogId = request.DialogId;
Expand All @@ -42,9 +40,8 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequest()
// Check AccessSubject attributes
var accessSubject = result.Request.AccessSubject.First();
Assert.Equal("s1", accessSubject.Id);
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:foo" && a.Value == "bar");
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:person:identifier-no" && a.Value == "12345678901");
Assert.DoesNotContain(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:organization:identifier-no");
Assert.Single(accessSubject.Attribute);

// Check Action attributes.
var actionIdsByName = new Dictionary<string, string>();
Expand Down Expand Up @@ -79,15 +76,14 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequest()
}

[Fact]
public void CreateDialogDetailsRequestShouldReturnCorrectRequestForLegacyEnterpriseUsers()
public void CreateDialogDetailsRequestShouldReturnCorrectRequestForExchangedTokens()
{
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("urn:altinn:userid", "5678901"),

// This should not be copied as subject claim since there's a "urn:altinn:user-id"-claim
("consumer", ConsumerClaimValue)
("pid", "12345678901"),
("urn:altinn:userid", "5678901")
),
$"{NorwegianOrganizationIdentifier.PrefixWithSeparator}713330310");

Expand All @@ -98,7 +94,7 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequestForLegacyEnterpr
var accessSubject = result.Request.AccessSubject.First();
Assert.Equal("s1", accessSubject.Id);
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:userid" && a.Value == "5678901");
Assert.DoesNotContain(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:organization:identifier-no");
Assert.Single(accessSubject.Attribute);
}

[Fact]
Expand Down Expand Up @@ -151,33 +147,8 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequestForSystemUser()

var accessSubject = result.Request.AccessSubject.First();
Assert.Equal("s1", accessSubject.Id);
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:foo" && a.Value == "bar");
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:systemuser:uuid" && a.Value == "unique_systemuser_id");
}

[Fact]
public void CreateDialogDetailsRequestShouldReturnCorrectRequestForConsumerOrgAndPersonParty()
{
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
// Should be copied as subject claim since there's not a "pid"-claim
("consumer", ConsumerClaimValue)
),
$"{NorwegianPersonIdentifier.PrefixWithSeparator}16073422888");

// Act
var result = DecisionRequestHelper.CreateDialogDetailsRequest(request);

// Assert
// Check that we have the organizationnumber
var accessSubject = result.Request.AccessSubject.First();
Assert.Contains(accessSubject.Attribute, a => a.AttributeId == "urn:altinn:organization:identifier-no" && a.Value == "991825827");

// Check that we have the ssn attribute as resource owner
var resource1 = result.Request.Resource.FirstOrDefault(r => r.Id == "r1");
Assert.NotNull(resource1);
Assert.Contains(resource1.Attribute, a => a.AttributeId == "urn:altinn:person:identifier-no" && a.Value == "16073422888");
Assert.Single(accessSubject.Attribute);
}

[Fact]
Expand All @@ -186,7 +157,7 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequestForOverriddenRes
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("consumer", ConsumerClaimValue)
("pid", "12345678901")
),
$"{NorwegianPersonIdentifier.PrefixWithSeparator}16073422888");

Expand All @@ -211,7 +182,7 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequestForOverriddenRes
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("consumer", ConsumerClaimValue)
("pid", "12345678901")
),
$"{NorwegianPersonIdentifier.PrefixWithSeparator}16073422888");

Expand All @@ -238,7 +209,7 @@ public void CreateDialogDetailsRequestShouldReturnCorrectRequestForFullyQualifie
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("consumer", ConsumerClaimValue)
("pid", "12345678901")
),
$"{NorwegianPersonIdentifier.PrefixWithSeparator}16073422888");

Expand All @@ -262,8 +233,7 @@ public void CreateDialogDetailsResponseShouldReturnCorrectResponse()
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
// Should be copied as subject claim since there's not a "pid"-claim
("consumer", ConsumerClaimValue)
("pid", "12345678901")
),
$"{NorwegianPersonIdentifier.PrefixWithSeparator}12345678901");

Expand All @@ -287,6 +257,21 @@ public void CreateDialogDetailsResponseShouldReturnCorrectResponse()
Assert.DoesNotContain(new AltinnAction("failaction", Constants.MainResource), response.AuthorizedAltinnActions);
}

[Fact]
public void CreateDetailsRequestShouldThrowUnreachableExceptionIfNoValidUserType()
{
// Arrange
var request = CreateDialogDetailsAuthorizationRequest(
GetAsClaims(
("consumer", "somevalue")
),
$"{NorwegianOrganizationIdentifier.PrefixWithSeparator}713330310");

// Act / assert
Assert.Throws<UnreachableException>(() => DecisionRequestHelper.CreateDialogDetailsRequest(request));
}


private static DialogDetailsAuthorizationRequest CreateDialogDetailsAuthorizationRequest(List<Claim> principalClaims, string party, bool isApp = false)
{
var allClaims = new List<Claim>
Expand Down

0 comments on commit 55159b7

Please sign in to comment.