Skip to content

Commit 4bc83ea

Browse files
FrostyApeOneFrostyApeOne
FrostyApeOne
authored and
FrostyApeOne
committed
Isolating tenant level user permissions
1 parent 87ab2bf commit 4bc83ea

6 files changed

Lines changed: 154 additions & 55 deletions

File tree

‎src/GovUK.Dfe.FlexForms.Application/Applications/Queries/GetApplicationByReferenceQueryHandler.cs‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,8 @@ public async Task<Result<ApplicationDto>> Handle(
4040
if (dto is null)
4141
return Result<ApplicationDto>.NotFound("Application not found");
4242

43-
var templateId = dto.TemplateSchema?.TemplateId ?? Guid.Empty;
44-
if (templateId == Guid.Empty)
45-
return Result<ApplicationDto>.NotFound("Application not found");
46-
4743
if (!await tenantPermissionFilter.ApplicationBelongsToCurrentTenantAsync(
48-
new TemplateId(templateId),
44+
dto.ApplicationId,
4945
cancellationToken))
5046
{
5147
return Result<ApplicationDto>.Forbid("Application does not belong to the current tenant");

‎src/GovUK.Dfe.FlexForms.Application/Services/ITenantPermissionFilter.cs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
using GovUK.Dfe.FlexForms.Domain.Entities;
2-
using GovUK.Dfe.FlexForms.Domain.ValueObjects;
32

43
namespace GovUK.Dfe.FlexForms.Application.Services;
54

@@ -16,9 +15,9 @@ Task<IReadOnlyList<Permission>> FilterToCurrentTenantAsync(
1615
CancellationToken cancellationToken = default);
1716

1817
/// <summary>
19-
/// Returns true when the application's template belongs to the current tenant catalogue.
18+
/// Returns true when the application's template belongs to the current tenant.
2019
/// </summary>
2120
Task<bool> ApplicationBelongsToCurrentTenantAsync(
22-
TemplateId templateId,
21+
Guid applicationId,
2322
CancellationToken cancellationToken = default);
2423
}

‎src/GovUK.Dfe.FlexForms.Application/Services/TenantPermissionFilter.cs‎

Lines changed: 106 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,29 @@
22
using GovUK.Dfe.FlexForms.Domain.Common;
33
using GovUK.Dfe.FlexForms.Domain.Entities;
44
using GovUK.Dfe.FlexForms.Domain.Interfaces.Repositories;
5+
using GovUK.Dfe.FlexForms.Domain.Tenancy;
56
using GovUK.Dfe.FlexForms.Domain.ValueObjects;
67
using Microsoft.EntityFrameworkCore;
7-
using ApplicationId = GovUK.Dfe.FlexForms.Domain.ValueObjects.ApplicationId;
88

99
namespace GovUK.Dfe.FlexForms.Application.Services;
1010

1111
/// <inheritdoc />
1212
public sealed class TenantPermissionFilter(
1313
ITenantTemplateCatalogue tenantTemplateCatalogue,
14-
IApplicationRepository applicationRepository) : ITenantPermissionFilter
14+
IApplicationRepository applicationRepository,
15+
ITenantContextAccessor tenantContextAccessor) : ITenantPermissionFilter
1516
{
1617
/// <inheritdoc />
1718
public async Task<IReadOnlyList<Permission>> FilterToCurrentTenantAsync(
1819
IEnumerable<Permission> permissions,
1920
CancellationToken cancellationToken = default)
2021
{
22+
var currentTenantId = tenantContextAccessor.CurrentTenant?.Id;
23+
if (currentTenantId is null)
24+
return Array.Empty<Permission>();
25+
2126
var tenantTemplateIds = (await tenantTemplateCatalogue.GetTemplateIdsAsync(cancellationToken))
27+
.Select(id => id.Value)
2228
.ToHashSet();
2329

2430
if (tenantTemplateIds.Count == 0)
@@ -28,23 +34,66 @@ public async Task<IReadOnlyList<Permission>> FilterToCurrentTenantAsync(
2834
if (permissionList.Count == 0)
2935
return permissionList;
3036

31-
var applicationTemplateMap = await BuildApplicationTemplateMapAsync(permissionList, cancellationToken);
37+
var applicationOwnership = await BuildApplicationOwnershipMapAsync(
38+
permissionList,
39+
cancellationToken);
3240

3341
return permissionList
34-
.Where(p => BelongsToTenant(p, tenantTemplateIds, applicationTemplateMap))
42+
.Where(p => BelongsToTenant(
43+
p,
44+
currentTenantId.Value,
45+
tenantTemplateIds,
46+
applicationOwnership))
3547
.ToList();
3648
}
3749

3850
/// <inheritdoc />
39-
public Task<bool> ApplicationBelongsToCurrentTenantAsync(
40-
TemplateId templateId,
51+
public async Task<bool> ApplicationBelongsToCurrentTenantAsync(
52+
Guid applicationId,
4153
CancellationToken cancellationToken = default)
42-
=> tenantTemplateCatalogue.ContainsAsync(templateId, cancellationToken);
54+
{
55+
var currentTenantId = tenantContextAccessor.CurrentTenant?.Id;
56+
if (currentTenantId is null || applicationId == Guid.Empty)
57+
return false;
58+
59+
var tenantTemplateIds = (await tenantTemplateCatalogue.GetTemplateIdsAsync(cancellationToken))
60+
.Select(id => id.Value)
61+
.ToHashSet();
62+
63+
if (tenantTemplateIds.Count == 0)
64+
return false;
4365

66+
var ownership = await applicationRepository.Query()
67+
.AsNoTracking()
68+
.Where(a => a.Id != null && a.Id.Value == applicationId)
69+
.Select(a => new
70+
{
71+
TemplateId = a.TemplateVersion!.TemplateId.Value,
72+
TemplateTenantId = a.TemplateVersion!.Template!.TenantId
73+
})
74+
.FirstOrDefaultAsync(cancellationToken);
75+
76+
if (ownership is null)
77+
return false;
78+
79+
return IsTemplateInTenant(
80+
ownership.TemplateId,
81+
ownership.TemplateTenantId,
82+
currentTenantId.Value,
83+
tenantTemplateIds);
84+
}
85+
86+
/// <summary>
87+
/// Application/template grants belong to the current tenant when their template is in the
88+
/// tenant catalogue and, when the template is tenant-owned, the owning tenant matches.
89+
/// This prevents HostMappings overlap from leaking another tenant's application grants
90+
/// into the permissions list.
91+
/// </summary>
4492
internal static bool BelongsToTenant(
4593
Permission permission,
46-
HashSet<TemplateId> tenantTemplateIds,
47-
IReadOnlyDictionary<Guid, TemplateId> applicationTemplateMap)
94+
Guid currentTenantId,
95+
HashSet<Guid> tenantTemplateIds,
96+
IReadOnlyDictionary<Guid, ApplicationOwnership> applicationOwnership)
4897
{
4998
switch (permission.ResourceType)
5099
{
@@ -53,21 +102,24 @@ internal static bool BelongsToTenant(
53102
return tenantTemplateIds.Count > 0;
54103

55104
return Guid.TryParse(permission.ResourceKey, out var templateGuid)
56-
&& tenantTemplateIds.Contains(new TemplateId(templateGuid));
105+
&& IsTemplateInTenant(templateGuid, owningTenantId: null, currentTenantId, tenantTemplateIds);
57106

58107
case ResourceType.Application:
59108
case ResourceType.ApplicationFiles:
60109
if (IsAnyKey(permission.ResourceKey))
61110
return tenantTemplateIds.Count > 0;
62111

63-
if (permission.Application?.TemplateVersion?.TemplateId is { } loadedTemplateId)
64-
return tenantTemplateIds.Contains(loadedTemplateId);
112+
if (!TryResolveApplicationId(permission, out var applicationGuid))
113+
return false;
65114

66-
if (!Guid.TryParse(permission.ResourceKey, out var applicationGuid))
115+
if (!applicationOwnership.TryGetValue(applicationGuid, out var ownership))
67116
return false;
68117

69-
return applicationTemplateMap.TryGetValue(applicationGuid, out var mappedTemplateId)
70-
&& tenantTemplateIds.Contains(mappedTemplateId);
118+
return IsTemplateInTenant(
119+
ownership.TemplateId,
120+
ownership.TemplateTenantId,
121+
currentTenantId,
122+
tenantTemplateIds);
71123

72124
case ResourceType.User:
73125
case ResourceType.Notifications:
@@ -76,37 +128,66 @@ internal static bool BelongsToTenant(
76128
}
77129
}
78130

79-
private async Task<IReadOnlyDictionary<Guid, TemplateId>> BuildApplicationTemplateMapAsync(
131+
internal static bool IsTemplateInTenant(
132+
Guid templateId,
133+
Guid? owningTenantId,
134+
Guid currentTenantId,
135+
HashSet<Guid> tenantTemplateIds)
136+
{
137+
if (!tenantTemplateIds.Contains(templateId))
138+
return false;
139+
140+
// Tenant-owned templates are authoritative: never treat another tenant's owned
141+
// template as belonging here just because it also appears in HostMappings.
142+
if (owningTenantId is Guid owner && owner != currentTenantId)
143+
return false;
144+
145+
return true;
146+
}
147+
148+
private async Task<IReadOnlyDictionary<Guid, ApplicationOwnership>> BuildApplicationOwnershipMapAsync(
80149
IReadOnlyCollection<Permission> permissions,
81150
CancellationToken cancellationToken)
82151
{
83-
var applicationIds = permissions
152+
var applicationGuids = permissions
84153
.Where(p => p.ResourceType is ResourceType.Application or ResourceType.ApplicationFiles)
85-
.Where(p => !IsAnyKey(p.ResourceKey))
86-
.Select(p => Guid.TryParse(p.ResourceKey, out var id) ? id : Guid.Empty)
154+
.Select(p => TryResolveApplicationId(p, out var id) ? id : Guid.Empty)
87155
.Where(id => id != Guid.Empty)
88156
.Distinct()
89-
.Select(id => new ApplicationId(id))
90157
.ToList();
91158

92-
if (applicationIds.Count == 0)
93-
return new Dictionary<Guid, TemplateId>();
159+
if (applicationGuids.Count == 0)
160+
return new Dictionary<Guid, ApplicationOwnership>();
94161

95162
var rows = await applicationRepository.Query()
96163
.AsNoTracking()
97-
.Where(a => applicationIds.Contains(a.Id!))
164+
.Where(a => a.Id != null && applicationGuids.Contains(a.Id.Value))
98165
.Select(a => new
99166
{
100167
ApplicationId = a.Id!.Value,
101-
TemplateId = a.TemplateVersion!.TemplateId
168+
TemplateId = a.TemplateVersion!.TemplateId.Value,
169+
TemplateTenantId = a.TemplateVersion!.Template!.TenantId
102170
})
103171
.ToListAsync(cancellationToken);
104172

105173
return rows.ToDictionary(
106174
row => row.ApplicationId,
107-
row => row.TemplateId);
175+
row => new ApplicationOwnership(row.TemplateId, row.TemplateTenantId));
176+
}
177+
178+
private static bool TryResolveApplicationId(Permission permission, out Guid applicationId)
179+
{
180+
if (permission.ApplicationId is not null)
181+
{
182+
applicationId = permission.ApplicationId.Value;
183+
return applicationId != Guid.Empty;
184+
}
185+
186+
return Guid.TryParse(permission.ResourceKey, out applicationId) && applicationId != Guid.Empty;
108187
}
109188

110189
private static bool IsAnyKey(string resourceKey) =>
111190
string.Equals(resourceKey, PermissionConstants.AnyResourceKey, StringComparison.OrdinalIgnoreCase);
191+
192+
internal readonly record struct ApplicationOwnership(Guid TemplateId, Guid? TemplateTenantId);
112193
}

‎src/GovUK.Dfe.FlexForms.Application/Users/Commands/SetUserPermissionsCommandHandler.cs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,9 @@ await userCacheInvalidator.InvalidateForUserAsync(
170170
cancellationToken);
171171

172172
return Result<IReadOnlyCollection<UserPermissionDto>>.Success(
173-
user.Permissions.Select(Map).ToList());
173+
(await tenantPermissionFilter.FilterToCurrentTenantAsync(user.Permissions, cancellationToken))
174+
.Select(Map)
175+
.ToList());
174176
}
175177
catch (InvalidOperationException ex)
176178
{
@@ -207,8 +209,12 @@ await userCacheInvalidator.InvalidateForUserAsync(
207209
return $"Application '{key}' was not found.";
208210

209211
var templateId = application.TemplateVersion?.TemplateId;
212+
var templateTenantId = application.TemplateVersion?.Template?.TenantId;
213+
var currentTenantId = tenantContextAccessor.CurrentTenant?.Id;
210214
if (templateId is null
211-
|| !await tenantTemplateCatalogue.ContainsAsync(templateId, cancellationToken))
215+
|| currentTenantId is null
216+
|| !await tenantTemplateCatalogue.ContainsAsync(templateId, cancellationToken)
217+
|| (templateTenantId is Guid owner && owner != currentTenantId.Value))
212218
{
213219
return $"Application '{key}' does not belong to the current tenant.";
214220
}

‎src/Tests/GovUK.Dfe.FlexForms.Application.Tests/QueryHandlers/Applications/GetApplicationByReferenceQueryHandlerTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ public class GetApplicationByReferenceQueryHandlerTests
2323
private static ITenantPermissionFilter CreateTenantPermissionFilter(bool belongsToTenant = true)
2424
{
2525
var filter = Substitute.For<ITenantPermissionFilter>();
26-
filter.ApplicationBelongsToCurrentTenantAsync(Arg.Any<TemplateId>(), Arg.Any<CancellationToken>())
26+
filter.ApplicationBelongsToCurrentTenantAsync(Arg.Any<Guid>(), Arg.Any<CancellationToken>())
2727
.Returns(belongsToTenant);
2828
return filter;
2929
}

‎src/Tests/GovUK.Dfe.FlexForms.Application.Tests/Services/TenantPermissionFilterTests.cs‎

Lines changed: 36 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,68 +8,85 @@ namespace GovUK.Dfe.FlexForms.Application.Tests.Services;
88

99
public class TenantPermissionFilterTests
1010
{
11-
private readonly TemplateId _tenantTemplateId = new(Guid.NewGuid());
12-
private readonly TemplateId _otherTemplateId = new(Guid.NewGuid());
11+
private readonly Guid _currentTenantId = Guid.NewGuid();
12+
private readonly Guid _otherTenantId = Guid.NewGuid();
13+
private readonly Guid _tenantTemplateId = Guid.NewGuid();
14+
private readonly Guid _otherTemplateId = Guid.NewGuid();
1315
private readonly Guid _tenantApplicationId = Guid.NewGuid();
1416
private readonly Guid _otherApplicationId = Guid.NewGuid();
1517

1618
[Fact]
1719
public void BelongsToTenant_ShouldKeepTenantTemplatePermissions()
1820
{
1921
var userId = new UserId(Guid.NewGuid());
20-
var tenantTemplateIds = new HashSet<TemplateId> { _tenantTemplateId };
21-
var map = new Dictionary<Guid, TemplateId>();
22+
var tenantTemplateIds = new HashSet<Guid> { _tenantTemplateId };
23+
var map = new Dictionary<Guid, TenantPermissionFilter.ApplicationOwnership>();
2224

23-
var tenantPermission = CreatePermission(userId, ResourceType.Template, _tenantTemplateId.Value.ToString(), AccessType.Read);
24-
var otherPermission = CreatePermission(userId, ResourceType.Template, _otherTemplateId.Value.ToString(), AccessType.Read);
25+
var tenantPermission = CreatePermission(userId, ResourceType.Template, _tenantTemplateId.ToString(), AccessType.Read);
26+
var otherPermission = CreatePermission(userId, ResourceType.Template, _otherTemplateId.ToString(), AccessType.Read);
2527

26-
Assert.True(TenantPermissionFilter.BelongsToTenant(tenantPermission, tenantTemplateIds, map));
27-
Assert.False(TenantPermissionFilter.BelongsToTenant(otherPermission, tenantTemplateIds, map));
28+
Assert.True(TenantPermissionFilter.BelongsToTenant(tenantPermission, _currentTenantId, tenantTemplateIds, map));
29+
Assert.False(TenantPermissionFilter.BelongsToTenant(otherPermission, _currentTenantId, tenantTemplateIds, map));
2830
}
2931

3032
[Fact]
31-
public void BelongsToTenant_ShouldKeepOnlyTenantApplicationPermissions()
33+
public void BelongsToTenant_ShouldKeepOnlyCurrentTenantOwnedApplicationPermissions()
3234
{
3335
var userId = new UserId(Guid.NewGuid());
34-
var tenantTemplateIds = new HashSet<TemplateId> { _tenantTemplateId };
35-
var map = new Dictionary<Guid, TemplateId>
36+
// Both templates appear in HostMappings/catalogue (overlap), but ownership differs.
37+
var tenantTemplateIds = new HashSet<Guid> { _tenantTemplateId, _otherTemplateId };
38+
var map = new Dictionary<Guid, TenantPermissionFilter.ApplicationOwnership>
3639
{
37-
[_tenantApplicationId] = _tenantTemplateId,
38-
[_otherApplicationId] = _otherTemplateId
40+
[_tenantApplicationId] = new(_tenantTemplateId, _currentTenantId),
41+
[_otherApplicationId] = new(_otherTemplateId, _otherTenantId)
3942
};
4043

4144
var tenantPermission = CreatePermission(userId, ResourceType.Application, _tenantApplicationId.ToString(), AccessType.Read);
4245
var otherPermission = CreatePermission(userId, ResourceType.Application, _otherApplicationId.ToString(), AccessType.Read);
4346

44-
Assert.True(TenantPermissionFilter.BelongsToTenant(tenantPermission, tenantTemplateIds, map));
45-
Assert.False(TenantPermissionFilter.BelongsToTenant(otherPermission, tenantTemplateIds, map));
47+
Assert.True(TenantPermissionFilter.BelongsToTenant(tenantPermission, _currentTenantId, tenantTemplateIds, map));
48+
Assert.False(TenantPermissionFilter.BelongsToTenant(otherPermission, _currentTenantId, tenantTemplateIds, map));
4649
}
4750

4851
[Fact]
4952
public void BelongsToTenant_ShouldRejectUnknownApplicationPermissions()
5053
{
5154
var userId = new UserId(Guid.NewGuid());
52-
var tenantTemplateIds = new HashSet<TemplateId> { _tenantTemplateId };
55+
var tenantTemplateIds = new HashSet<Guid> { _tenantTemplateId };
5356
var unknownApplicationId = Guid.NewGuid();
5457
var permission = CreatePermission(userId, ResourceType.Application, unknownApplicationId.ToString(), AccessType.Read);
5558

5659
Assert.False(TenantPermissionFilter.BelongsToTenant(
5760
permission,
61+
_currentTenantId,
5862
tenantTemplateIds,
59-
new Dictionary<Guid, TemplateId>()));
63+
new Dictionary<Guid, TenantPermissionFilter.ApplicationOwnership>()));
6064
}
6165

6266
[Fact]
6367
public void BelongsToTenant_ShouldKeepTenantWideAnyPermission()
6468
{
6569
var userId = new UserId(Guid.NewGuid());
66-
var tenantTemplateIds = new HashSet<TemplateId> { _tenantTemplateId };
70+
var tenantTemplateIds = new HashSet<Guid> { _tenantTemplateId };
6771
var permission = CreatePermission(userId, ResourceType.Application, PermissionConstants.AnyResourceKey, AccessType.Read);
6872

6973
Assert.True(TenantPermissionFilter.BelongsToTenant(
7074
permission,
75+
_currentTenantId,
7176
tenantTemplateIds,
72-
new Dictionary<Guid, TemplateId>()));
77+
new Dictionary<Guid, TenantPermissionFilter.ApplicationOwnership>()));
78+
}
79+
80+
[Fact]
81+
public void IsTemplateInTenant_ShouldRejectOtherTenantOwnedTemplateEvenWhenInCatalogue()
82+
{
83+
var tenantTemplateIds = new HashSet<Guid> { _otherTemplateId };
84+
85+
Assert.False(TenantPermissionFilter.IsTemplateInTenant(
86+
_otherTemplateId,
87+
_otherTenantId,
88+
_currentTenantId,
89+
tenantTemplateIds));
7390
}
7491

7592
private static Permission CreatePermission(

0 commit comments

Comments
 (0)