Skip to content

Commit 34ee24e

Browse files
ramonsmitsramonsmits
committed
♻️ Replace wildcard role-permission expansion with explicit lists
Roles are explicit permission-constant lists built from four additive groups: Read (16 views), ReadConfiguration (licensing/notifications/ redirects/throughput views), Operate (message triage, housekeeping deletes, endpoints/connections manage), Configure (licensing/ notifications/redirects/throughput manage + test). Reader = Read + ReadConfiguration, Writer = Read + Operate, Admin = everything. All pattern parsing/expansion machinery is removed. The sets match the include/exclude patterns of #5569 exactly: writer holds endpoints:manage and connections:manage but has no access to the licensing/notifications/redirects/throughput areas (not even :view), so reader is intentionally not a subset of writer. Guard tests break the build when a new permission constant is not assigned to a role, enforce reader/writer ⊂ admin, and pin writer's configuration-area exclusions.
1 parent 3825fc2 commit 34ee24e

10 files changed

Lines changed: 165 additions & 134 deletions

File tree

‎src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public async Task Should_accept_requests_with_valid_bearer_token()
126126
_ = await Define<Context>()
127127
.Done(async ctx =>
128128
{
129-
// The "reader" role grants every *:*:view permission, including error:messages:view
129+
// The "reader" role grants every :view permission, including error:messages:view
130130
// required by /api/errors. Without a role-bearing claim the request would be 403.
131131
var validToken = mockOidcServer.GenerateToken(
132132
additionalClaims: new[] { new Claim("roles", "reader") });

‎src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_my_routes_are_requested.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public async Task Writer_can_retry()
8383
var routes = await GetRoutes(RolePermissions.Writer);
8484

8585
Assert.That(routes.Any(r => r.Method == "POST" && r.UrlTemplate.EndsWith("/retry")), Is.True,
86-
"writer holds every permission, so retry routes are allowed");
86+
"writer holds the operate permissions, so retry routes are allowed");
8787
}
8888

8989
async Task<List<RouteManifestEntry>> GetRoutes(string role)

‎src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public async Task Should_accept_requests_with_valid_bearer_token()
9494
_ = await Define<Context>()
9595
.Done(async ctx =>
9696
{
97-
// The "reader" role grants every *:*:view permission, including audit:message:view
97+
// The "reader" role grants every :view permission, including audit:message:view
9898
// required by /api/messages. Without a role-bearing claim the request would be 403.
9999
var validToken = mockOidcServer.GenerateToken(
100100
additionalClaims: new[] { new Claim("roles", "reader") });

‎src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ protected override Task HandleRequirementAsync(
4242
var roles = context.User.FindAll(ClaimTypes.Role).Select(claim => claim.Value).ToArray();
4343
var permission = requirement.Permission;
4444

45-
if (RolePermissions.IsGranted(roles, permission))
45+
if (IsGranted(roles, permission))
4646
{
4747
auditLog.Decision(
4848
subjectId,
@@ -71,4 +71,17 @@ protected override Task HandleRequirementAsync(
7171
// Leave the requirement unmet → the framework forbids (403).
7272
return Task.CompletedTask;
7373
}
74+
75+
static bool IsGranted(string[] roles, string permission)
76+
{
77+
foreach (var role in roles)
78+
{
79+
if (RolePermissions.Roles.TryGetValue(role, out var granted) && granted.Contains(permission))
80+
{
81+
return true;
82+
}
83+
}
84+
85+
return false;
86+
}
7487
}

‎src/ServiceControl.Infrastructure.Tests/Auth/EffectivePermissionsTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public void Rbac_enabled_returns_the_union_of_role_permissions()
3131

3232
var result = EffectivePermissions.ForUser(PrincipalWithRoles(RolePermissions.Reader), settings);
3333

34-
Assert.That(result, Is.EquivalentTo(RolePermissions.GetPermissions(RolePermissions.Reader)));
34+
Assert.That(result, Is.EquivalentTo(RolePermissions.Roles[RolePermissions.Reader]));
3535
}
3636

3737
[Test]

‎src/ServiceControl.Infrastructure.Tests/Auth/RolePermissionsTests.cs‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
#nullable enable
2+
namespace ServiceControl.Infrastructure.Tests.Auth;
3+
4+
using System.Linq;
5+
using NUnit.Framework;
6+
using ServiceControl.Infrastructure.Auth;
7+
8+
[TestFixture]
9+
class RolePermissionsTests
10+
{
11+
[Test]
12+
public void Every_permission_is_assigned_to_a_role()
13+
{
14+
var unassigned = Permissions.All
15+
.Except(RolePermissions.Roles[RolePermissions.Admin])
16+
.Order()
17+
.ToArray();
18+
19+
Assert.That(unassigned, Is.Empty,
20+
$"Every permission constant must be assigned to a role group in RolePermissions. Unassigned: [{string.Join(", ", unassigned)}]");
21+
}
22+
23+
[Test]
24+
public void Admin_is_a_proper_superset_of_the_other_roles()
25+
{
26+
var reader = RolePermissions.Roles[RolePermissions.Reader];
27+
var writer = RolePermissions.Roles[RolePermissions.Writer];
28+
var admin = RolePermissions.Roles[RolePermissions.Admin];
29+
30+
using (Assert.EnterMultipleScope())
31+
{
32+
Assert.That(reader.IsProperSubsetOf(admin), Is.True, "reader must be a proper subset of admin");
33+
Assert.That(writer.IsProperSubsetOf(admin), Is.True, "writer must be a proper subset of admin");
34+
}
35+
}
36+
37+
[Test]
38+
public void Writer_has_no_access_to_configuration_areas()
39+
{
40+
var writer = RolePermissions.Roles[RolePermissions.Writer];
41+
42+
string[] configurationAreaPermissions =
43+
[
44+
Permissions.ErrorLicensingView,
45+
Permissions.ErrorLicensingManage,
46+
Permissions.ErrorNotificationsView,
47+
Permissions.ErrorNotificationsManage,
48+
Permissions.ErrorNotificationsTest,
49+
Permissions.ErrorRedirectsView,
50+
Permissions.ErrorRedirectsManage,
51+
Permissions.ErrorThroughputView,
52+
Permissions.ErrorThroughputManage,
53+
];
54+
55+
var granted = configurationAreaPermissions.Where(writer.Contains).ToArray();
56+
57+
Assert.That(granted, Is.Empty,
58+
$"Writer must not hold licensing/notifications/redirects/throughput permissions. Granted: [{string.Join(", ", granted)}]");
59+
}
60+
61+
[Test]
62+
public void Role_sets_contain_only_known_permissions()
63+
{
64+
foreach (var (role, granted) in RolePermissions.Roles)
65+
{
66+
var unknown = granted.Except(Permissions.All).Order().ToArray();
67+
68+
Assert.That(unknown, Is.Empty,
69+
$"Role '{role}' grants permissions that are not declared on Permissions: [{string.Join(", ", unknown)}]");
70+
}
71+
}
72+
}

‎src/ServiceControl.Infrastructure/Auth/EffectivePermissions.cs‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
#nullable enable
22
namespace ServiceControl.Infrastructure.Auth;
33

4+
using System;
45
using System.Collections.Generic;
5-
using System.Linq;
66
using System.Security.Claims;
77

88
/// <summary>
99
/// The set of permissions a principal effectively holds, computed per request. Mirrors the inputs the
1010
/// enforcement handler uses: when role-based authorization is enabled, the union of the permissions
11-
/// granted by the principal's <see cref="ClaimTypes.Role"/> claims (via <see cref="RolePermissions"/>);
11+
/// granted by the principal's <see cref="ClaimTypes.Role"/> claims (via <see cref="RolePermissions.Roles"/>);
1212
/// when it is disabled the platform runs allow-all, so every known permission is held.
1313
/// </summary>
1414
public static class EffectivePermissions
@@ -20,7 +20,15 @@ public static IReadOnlySet<string> ForUser(ClaimsPrincipal user, OpenIdConnectSe
2020
return Permissions.All;
2121
}
2222

23-
var roles = user.FindAll(ClaimTypes.Role).Select(claim => claim.Value);
24-
return RolePermissions.GetPermissions(roles);
23+
var permissions = new HashSet<string>(StringComparer.Ordinal);
24+
foreach (var claim in user.FindAll(ClaimTypes.Role))
25+
{
26+
if (RolePermissions.Roles.TryGetValue(claim.Value, out var granted))
27+
{
28+
permissions.UnionWith(granted);
29+
}
30+
}
31+
32+
return permissions;
2533
}
2634
}

‎src/ServiceControl.Infrastructure/Auth/Permissions.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public static class Permissions
8585
/// <inheritdoc cref="ErrorThroughputView"/>
8686
public const string ErrorThroughputManage = "error:throughput:manage";
8787

88-
/// <summary>Platform connections area — viewing and managing broker/platform connection settings.</summary>
88+
/// <summary>Platform connections area — viewing and managing platform connection settings.</summary>
8989
public const string ErrorConnectionsView = "error:connections:view";
9090
/// <inheritdoc cref="ErrorConnectionsView"/>
9191
public const string ErrorConnectionsManage = "error:connections:manage";

‎src/ServiceControl.Infrastructure/Auth/RolePermissions.cs‎

Lines changed: 61 additions & 123 deletions
Original file line numberDiff line numberDiff line change
@@ -4,136 +4,74 @@ namespace ServiceControl.Infrastructure.Auth;
44
using System;
55
using System.Collections.Frozen;
66
using System.Collections.Generic;
7-
using System.Linq;
87

9-
/// <summary>
10-
/// Role → permission policy. Two roles:
11-
/// <list type="bullet">
12-
/// <item><c>reader</c> — granted every <c>*:*:view</c> permission (read-only access).</item>
13-
/// <item><c>writer</c> — granted every permission (<c>*:*:*</c>).</item>
14-
/// </list>
15-
/// The wildcard patterns (<c>*</c> is a colon-segment wildcard) are the source of truth, but they are
16-
/// <b>expanded once</b> at type initialization against <see cref="Permissions.All"/> into a concrete,
17-
/// immutable <see cref="FrozenSet{T}"/> of granted permissions per role. As a result both
18-
/// <see cref="IsGranted"/> and <see cref="GetPermissions(string)"/> are O(1) hash lookups with no
19-
/// per-call pattern matching or allocation.
20-
/// </summary>
218
public static class RolePermissions
229
{
23-
/// <summary>Read-only role: every <c>*:*:view</c> permission.</summary>
2410
public const string Reader = "reader";
25-
26-
/// <summary>Full-access role: every permission.</summary>
2711
public const string Writer = "writer";
28-
29-
/// <summary>
30-
/// Platform-administrator role: read-only on everything, plus full management of the configuration /
31-
/// admin-area resources (licensing, notifications, retry redirects, throughput, connections) — but
32-
/// <b>not</b> the message-triage write actions (retry/edit/archive/restore).
33-
/// </summary>
3412
public const string Admin = "admin";
3513

36-
// Source of truth: the wildcard pattern(s) each role grants.
37-
static readonly Dictionary<string, string[]> RolePatterns = new(StringComparer.OrdinalIgnoreCase)
38-
{
39-
[Reader] = ["*:*:view"],
40-
[Writer] = ["*:*:*"],
41-
[Admin] =
42-
[
43-
"*:*:view",
44-
"error:licensing:*",
45-
"error:notifications:*",
46-
"error:redirects:*",
47-
"error:throughput:*",
48-
"error:connections:*",
49-
],
50-
};
51-
52-
// Expanded once against the full permission catalogue: role -> concrete granted permissions.
53-
static readonly FrozenDictionary<string, FrozenSet<string>> PermissionsByRole = Expand();
54-
55-
/// <summary>
56-
/// Returns <see langword="true"/> if any of the supplied <paramref name="roles"/> grants the
57-
/// requested <paramref name="permission"/>. O(1) per role — a frozen-set membership test.
58-
/// </summary>
59-
public static bool IsGranted(IEnumerable<string> roles, string permission)
60-
{
61-
foreach (var role in roles)
62-
{
63-
if (PermissionsByRole.TryGetValue(role, out var granted) && granted.Contains(permission))
64-
{
65-
return true;
66-
}
67-
}
68-
69-
return false;
70-
}
71-
72-
/// <summary>
73-
/// The complete set of permissions granted to a single role (empty if the role is unknown).
74-
/// O(1) and allocation-free — returns the precomputed frozen set.
75-
/// </summary>
76-
public static IReadOnlySet<string> GetPermissions(string role) =>
77-
PermissionsByRole.TryGetValue(role, out var granted) ? granted : FrozenSet<string>.Empty;
78-
79-
/// <summary>
80-
/// The union of permissions granted across several <paramref name="roles"/>. Allocation-free for the
81-
/// common single-role case; only the multi-role union allocates.
82-
/// </summary>
83-
public static IReadOnlySet<string> GetPermissions(IEnumerable<string> roles)
84-
{
85-
var list = roles as IReadOnlyList<string> ?? roles.ToList();
86-
if (list.Count <= 1)
87-
{
88-
return list.Count == 0 ? FrozenSet<string>.Empty : GetPermissions(list[0]);
89-
}
90-
91-
var union = new HashSet<string>(StringComparer.Ordinal);
92-
foreach (var role in list)
93-
{
94-
if (PermissionsByRole.TryGetValue(role, out var granted))
95-
{
96-
union.UnionWith(granted);
97-
}
98-
}
99-
100-
return union;
101-
}
102-
103-
static FrozenDictionary<string, FrozenSet<string>> Expand()
104-
{
105-
var expanded = new Dictionary<string, FrozenSet<string>>(StringComparer.OrdinalIgnoreCase);
106-
107-
foreach (var (role, patterns) in RolePatterns)
108-
{
109-
expanded[role] = Permissions.All
110-
.Where(permission => patterns.Any(pattern => Matches(pattern, permission)))
111-
.ToFrozenSet(StringComparer.Ordinal);
112-
}
113-
114-
return expanded.ToFrozenDictionary(StringComparer.OrdinalIgnoreCase);
115-
}
116-
117-
/// <summary>Matches a colon-delimited permission against a pattern where <c>*</c> is a segment wildcard.</summary>
118-
static bool Matches(string pattern, string permission)
119-
{
120-
var patternSegments = pattern.Split(':');
121-
var permissionSegments = permission.Split(':');
122-
123-
if (patternSegments.Length != permissionSegments.Length)
124-
{
125-
return false;
126-
}
127-
128-
for (var i = 0; i < patternSegments.Length; i++)
14+
static readonly string[] Read =
15+
[
16+
Permissions.ErrorMessagesView,
17+
Permissions.ErrorRecoverabilityGroupsView,
18+
Permissions.ErrorEndpointsView,
19+
Permissions.ErrorHeartbeatsView,
20+
Permissions.ErrorCustomChecksView,
21+
Permissions.ErrorSagasView,
22+
Permissions.ErrorEventLogView,
23+
Permissions.ErrorQueuesView,
24+
Permissions.ErrorConnectionsView,
25+
Permissions.AuditMessageView,
26+
Permissions.AuditConnectionView,
27+
Permissions.AuditEndpointView,
28+
Permissions.AuditSagaView,
29+
Permissions.MonitoringEndpointView,
30+
Permissions.MonitoringConnectionView,
31+
Permissions.MonitoringLicenseView,
32+
];
33+
34+
static readonly string[] ReadConfiguration =
35+
[
36+
Permissions.ErrorLicensingView,
37+
Permissions.ErrorNotificationsView,
38+
Permissions.ErrorRedirectsView,
39+
Permissions.ErrorThroughputView,
40+
];
41+
42+
static readonly string[] Operate =
43+
[
44+
Permissions.ErrorMessagesRetry,
45+
Permissions.ErrorMessagesArchive,
46+
Permissions.ErrorMessagesUnarchive,
47+
Permissions.ErrorMessagesEdit,
48+
Permissions.ErrorRecoverabilityGroupsRetry,
49+
Permissions.ErrorRecoverabilityGroupsArchive,
50+
Permissions.ErrorRecoverabilityGroupsUnarchive,
51+
Permissions.ErrorEndpointsManage,
52+
Permissions.ErrorEndpointsDelete,
53+
Permissions.ErrorCustomChecksDelete,
54+
Permissions.ErrorQueuesDelete,
55+
Permissions.ErrorConnectionsManage,
56+
Permissions.MonitoringEndpointDelete,
57+
];
58+
59+
static readonly string[] Configure =
60+
[
61+
Permissions.ErrorLicensingManage,
62+
Permissions.ErrorNotificationsManage,
63+
Permissions.ErrorNotificationsTest,
64+
Permissions.ErrorRedirectsManage,
65+
Permissions.ErrorThroughputManage,
66+
];
67+
68+
public static readonly FrozenDictionary<string, FrozenSet<string>> Roles =
69+
new Dictionary<string, FrozenSet<string>>(StringComparer.OrdinalIgnoreCase)
12970
{
130-
if (patternSegments[i] != "*"
131-
&& !string.Equals(patternSegments[i], permissionSegments[i], StringComparison.OrdinalIgnoreCase))
132-
{
133-
return false;
134-
}
135-
}
71+
[Reader] = ToSet([.. Read, .. ReadConfiguration]),
72+
[Writer] = ToSet([.. Read, .. Operate]),
73+
[Admin] = ToSet([.. Read, .. ReadConfiguration, .. Operate, .. Configure]),
74+
}.ToFrozenDictionary(StringComparer.OrdinalIgnoreCase);
13675

137-
return true;
138-
}
76+
static FrozenSet<string> ToSet(string[] permissions) => permissions.ToFrozenSet(StringComparer.Ordinal);
13977
}

‎src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public async Task Should_accept_requests_with_valid_bearer_token()
9494
_ = await Define<Context>()
9595
.Done(async ctx =>
9696
{
97-
// The "reader" role grants every *:*:view permission, including
97+
// The "reader" role grants every :view permission, including
9898
// monitoring:endpoint:view required by /monitored-endpoints. Without a
9999
// role-bearing claim the request would be 403.
100100
var validToken = mockOidcServer.GenerateToken(

0 commit comments

Comments
 (0)