Skip to content

Commit

Permalink
fix: Use service resource org, allow admin-scope to fetch/update dial…
Browse files Browse the repository at this point in the history
…ogs (#1529)

## Description

This implements a proper handling of serviceprovider.admin-scope, where
the "org"-value for the actual service resource is used instead of
always being "digdir".

This also maintains the possibility for the admin-scope-wielder to
access and update the dialog afterwards. The search-endpoint is however
not changed (will only display actually owned dialogs, and requiring
search-scope)

## Related Issue(s)

- #1409

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

## Documentation

- [ ] Documentation is updated (either in `docs`-directory, Altinnpedia
or a separate linked PR in
[altinn-studio-docs.](https://github.com/Altinn/altinn-studio-docs), if
applicable)


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

## Release Notes

- **New Features**
- Added a new property `OwnOrgShortName` to enhance resource
information.
- Introduced conditional filtering in various query handlers to improve
access control based on user roles.
- Expanded testing coverage for service owners with admin capabilities.

- **Bug Fixes**
- Improved error handling for missing organization information in dialog
creation.

- **Documentation**
- Updated test setup to reflect changes in dependencies for dialog
creation tests.

- **Chores**
- Modified API call in tests to retrieve a larger number of dialog
items.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
elsand authored Nov 26, 2024
1 parent cc87c63 commit 25277b5
Show file tree
Hide file tree
Showing 17 changed files with 56 additions and 35 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ public sealed record ServiceResourceInformation
{
public string ResourceType { get; }
public string OwnerOrgNumber { get; }
public string OwnOrgShortName { get; }
public string ResourceId { get; }

public ServiceResourceInformation(string resourceId, string resourceType, string ownerOrgNumber)
public ServiceResourceInformation(string resourceId, string resourceType, string ownerOrgNumber, string ownOrgShortName)
{
ResourceId = resourceId.ToLowerInvariant();
ResourceType = resourceType.ToLowerInvariant();
OwnerOrgNumber = ownerOrgNumber.ToLowerInvariant();
OwnOrgShortName = ownOrgShortName.ToLowerInvariant();
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -43,7 +44,7 @@ public async Task<GetActivityResult> Handle(GetActivityQuery request,
.Include(x => x.Activities.Where(x => x.Id == request.ActivityId))
.ThenInclude(x => x.Description!.Localizations)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -37,7 +38,7 @@ public async Task<SearchActivityResult> Handle(SearchActivityQuery request, Canc
var dialog = await _db.Dialogs
.Include(x => x.Activities)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -44,7 +45,7 @@ public async Task<GetSeenLogResult> Handle(GetSeenLogQuery request,
.Include(x => x.SeenLog.Where(x => x.Id == request.SeenLogId))
.ThenInclude(x => x.SeenBy)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -42,7 +43,7 @@ public async Task<SearchSeenLogResult> Handle(SearchSeenLogQuery request, Cancel
.Include(x => x.SeenLog)
.ThenInclude(x => x.SeenBy)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -50,7 +51,7 @@ public async Task<GetTransmissionResult> Handle(GetTransmissionQuery request,
.Include(x => x.Transmissions)
.ThenInclude(x => x.Sender)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -47,7 +48,7 @@ public async Task<SearchTransmissionResult> Handle(SearchTransmissionQuery reque
.Include(x => x.Transmissions)
.ThenInclude(x => x.Sender)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId,
cancellationToken: cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal sealed class CreateDialogCommandHandler : IRequestHandler<CreateDialogC
private readonly IMapper _mapper;
private readonly IUnitOfWork _unitOfWork;
private readonly IDomainContext _domainContext;
private readonly IUserOrganizationRegistry _userOrganizationRegistry;
private readonly IResourceRegistry _resourceRegistry;
private readonly IServiceResourceAuthorizer _serviceResourceAuthorizer;
private readonly IUser _user;

Expand All @@ -40,16 +40,16 @@ public CreateDialogCommandHandler(
IMapper mapper,
IUnitOfWork unitOfWork,
IDomainContext domainContext,
IUserOrganizationRegistry userOrganizationRegistry,
IResourceRegistry resourceRegistry,
IServiceResourceAuthorizer serviceResourceAuthorizer)
{
_user = user ?? throw new ArgumentNullException(nameof(user));
_db = db ?? throw new ArgumentNullException(nameof(db));
_mapper = mapper ?? throw new ArgumentNullException(nameof(mapper));
_unitOfWork = unitOfWork ?? throw new ArgumentNullException(nameof(unitOfWork));
_domainContext = domainContext ?? throw new ArgumentNullException(nameof(domainContext));
_userOrganizationRegistry = userOrganizationRegistry ?? throw new ArgumentNullException(nameof(userOrganizationRegistry));
_serviceResourceAuthorizer = serviceResourceAuthorizer;
_resourceRegistry = resourceRegistry ?? throw new ArgumentNullException(nameof(resourceRegistry));
_serviceResourceAuthorizer = serviceResourceAuthorizer ?? throw new ArgumentNullException(nameof(serviceResourceAuthorizer));
}

public async Task<CreateDialogResult> Handle(CreateDialogCommand request, CancellationToken cancellationToken)
Expand All @@ -63,12 +63,17 @@ public async Task<CreateDialogResult> Handle(CreateDialogCommand request, Cancel
return forbiddenResult;
}

dialog.Org = await _userOrganizationRegistry.GetCurrentUserOrgShortName(cancellationToken) ?? string.Empty;
if (string.IsNullOrWhiteSpace(dialog.Org))
var serviceResourceInformation = await _resourceRegistry.GetResourceInformation(dialog.ServiceResource, cancellationToken);
if (serviceResourceInformation is null)
{
_domainContext.AddError(new DomainFailure(nameof(DialogEntity.Org),
"Cannot find service owner organization shortname for current user. Please ensure that you are logged in as a service owner."));
"Cannot find service owner organization shortname for referenced service resource."));
}
else
{
dialog.Org = serviceResourceInformation.OwnOrgShortName;
}

CreateDialogEndUserContext(request, dialog);
await EnsureNoExistingUserDefinedIds(dialog, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Diagnostics;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -42,7 +43,7 @@ public async Task<DeleteDialogResult> Handle(DeleteDialogCommand request, Cancel

var dialog = await _db.Dialogs
.Include(x => x.Activities)
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.Id, cancellationToken);

if (dialog is null)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Diagnostics;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Domain.Dialogs.Entities;
Expand Down Expand Up @@ -42,7 +43,7 @@ public async Task<PurgeDialogResult> Handle(PurgeDialogCommand request, Cancella
var dialog = await _db.Dialogs
.Include(x => x.Attachments)
.Include(x => x.Activities)
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.IgnoreQueryFilters()
.FirstOrDefaultAsync(x => x.Id == request.DialogId, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public async Task<UpdateDialogResult> Handle(UpdateDialogCommand request, Cancel
.Include(x => x.Transmissions)
.Include(x => x.DialogEndUserContext)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.Id, cancellationToken);

if (dialog is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using AutoMapper;
using Digdir.Domain.Dialogporten.Application.Common;
using Digdir.Domain.Dialogporten.Application.Common.Authorization;
using Digdir.Domain.Dialogporten.Application.Common.Extensions;
using Digdir.Domain.Dialogporten.Application.Common.ReturnTypes;
using Digdir.Domain.Dialogporten.Application.Externals;
using Digdir.Domain.Dialogporten.Application.Externals.AltinnAuthorization;
Expand Down Expand Up @@ -85,7 +86,7 @@ public async Task<GetDialogResult> Handle(GetDialogQuery request, CancellationTo
.ThenInclude(x => x.SeenBy)
.Include(x => x.DialogEndUserContext)
.IgnoreQueryFilters()
.Where(x => resourceIds.Contains(x.ServiceResource))
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))
.FirstOrDefaultAsync(x => x.Id == request.DialogId, cancellationToken);

if (dialog is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ internal sealed class LocalDevelopmentResourceRegistry : IResourceRegistry
{
private const string LocalResourceType = "LocalResourceType";
private const string LocalOrgId = "742859274";
private const string LocalOrgShortName = "ttd";
private static readonly HashSet<ServiceResourceInformation> CachedResourceIds = new(new ServiceResourceInformationEqualityComparer());
private readonly IDialogDbContext _db;

Expand All @@ -26,7 +27,7 @@ public async Task<IReadOnlyCollection<ServiceResourceInformation>> GetResourceIn

foreach (var id in newIds)
{
CachedResourceIds.Add(new ServiceResourceInformation(id, LocalResourceType, orgNumber));
CachedResourceIds.Add(new ServiceResourceInformation(id, LocalResourceType, orgNumber, LocalOrgShortName));
}

return CachedResourceIds;
Expand All @@ -35,7 +36,7 @@ public async Task<IReadOnlyCollection<ServiceResourceInformation>> GetResourceIn
public Task<ServiceResourceInformation?> GetResourceInformation(string serviceResourceId, CancellationToken cancellationToken)
{
return Task.FromResult<ServiceResourceInformation?>(
new ServiceResourceInformation(serviceResourceId, LocalResourceType, LocalOrgId));
new ServiceResourceInformation(serviceResourceId, LocalResourceType, LocalOrgId, LocalOrgShortName));
}

[SuppressMessage("Performance", "CA1822:Mark members as static")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ ResourceTypeAltinnApp or
.Select(x => new ServiceResourceInformation(
$"{Constants.ServiceResourcePrefix}{x.Identifier}",
x.ResourceType,
x.HasCompetentAuthority.Organization!))
x.HasCompetentAuthority.Organization!,
x.HasCompetentAuthority.OrgCode))
.ToArray();
},
token: cancellationToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ public async Task CreateDialogCommand_Should_Return_Forbidden_When_Scope_Is_Miss

var unitOfWorkSub = Substitute.For<IUnitOfWork>();
var domainContextSub = Substitute.For<IDomainContext>();
var userResourceRegistrySub = Substitute.For<IUserResourceRegistry>();
var userOrganizationRegistrySub = Substitute.For<IUserOrganizationRegistry>();
var resourceRegistrySub = Substitute.For<IResourceRegistry>();
var serviceAuthorizationSub = Substitute.For<IServiceResourceAuthorizer>();
var userSub = Substitute.For<IUser>();

Expand All @@ -37,13 +36,13 @@ public async Task CreateDialogCommand_Should_Return_Forbidden_When_Scope_Is_Miss
.AuthorizeServiceResources(Arg.Any<DialogEntity>(), Arg.Any<CancellationToken>())
.Returns(new Forbidden());

userResourceRegistrySub
.CurrentUserIsOwner(createCommand.ServiceResource, Arg.Any<CancellationToken>())
.Returns(true);
resourceRegistrySub
.GetResourceInformation(createCommand.ServiceResource, Arg.Any<CancellationToken>())
.Returns(new ServiceResourceInformation(createCommand.ServiceResource, "foo", "912345678", "ttd"));

var commandHandler = new CreateDialogCommandHandler(userSub, dialogDbContextSub,
mapper, unitOfWorkSub, domainContextSub,
userOrganizationRegistrySub, serviceAuthorizationSub);
resourceRegistrySub, serviceAuthorizationSub);

// Act
var result = await commandHandler.Handle(createCommand, CancellationToken.None);
Expand All @@ -65,8 +64,7 @@ public async Task CreateDialogCommand_Should_Return_Forbidden_When_User_Is_Not_O

var unitOfWorkSub = Substitute.For<IUnitOfWork>();
var domainContextSub = Substitute.For<IDomainContext>();
var userResourceRegistrySub = Substitute.For<IUserResourceRegistry>();
var userOrganizationRegistrySub = Substitute.For<IUserOrganizationRegistry>();
var resourceRegistrySub = Substitute.For<IResourceRegistry>();
var serviceAuthorizationSub = Substitute.For<IServiceResourceAuthorizer>();
var userSub = Substitute.For<IUser>();
var createCommand = DialogGenerator.GenerateSimpleFakeDialog();
Expand All @@ -75,13 +73,13 @@ public async Task CreateDialogCommand_Should_Return_Forbidden_When_User_Is_Not_O
.AuthorizeServiceResources(Arg.Any<DialogEntity>(), Arg.Any<CancellationToken>())
.Returns(new Forbidden());

userResourceRegistrySub
.CurrentUserIsOwner(createCommand.ServiceResource, Arg.Any<CancellationToken>())
.Returns(false);
resourceRegistrySub
.GetResourceInformation(createCommand.ServiceResource, Arg.Any<CancellationToken>())
.Returns(new ServiceResourceInformation(createCommand.ServiceResource, "foo", "912345678", "ttd"));

var commandHandler = new CreateDialogCommandHandler(userSub, dialogDbContextSub,
mapper, unitOfWorkSub, domainContextSub,
userOrganizationRegistrySub, serviceAuthorizationSub);
resourceRegistrySub, serviceAuthorizationSub);

// Act
var result = await commandHandler.Handle(createCommand, CancellationToken.None);
Expand Down
2 changes: 1 addition & 1 deletion tests/k6/common/sentinel.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default function () {
let continuationToken = "";
let dialogIdsToPurge = [];
do {
let r = getSO('dialogs/?Search=' + sentinelValue + continuationToken, null, tokenOptions);
let r = getSO('dialogs/?Limit=1000&Search=' + sentinelValue + continuationToken, null, tokenOptions);
expectStatusFor(r).to.equal(200);
expect(r, 'response').to.have.validJsonBody();
let response = r.json();
Expand Down
7 changes: 6 additions & 1 deletion tests/k6/tests/serviceowner/authorization.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export default function () {

let validSo = null;
let invalidSo = { orgName: "other", orgNo: "310778737" };
let validSoAdmin = { orgName: "other", orgNo: "310778737", scopes: "digdir:dialogporten.serviceprovider digdir:dialogporten.serviceprovider.admin" };

let dialog = dialogToInsert();
let transmissionId = null; // known after successful insert
Expand All @@ -18,12 +19,16 @@ export default function () {

let permutations = [
[ true, validSo ],
[ false, invalidSo ]
[ false, invalidSo ],
[ true, validSoAdmin ]
];

permutations.forEach(([shouldSucceed, tokenOptions]) => {
let logPrefix = shouldSucceed ? "Allow" : "Deny";
let logSuffix = shouldSucceed ? "valid serviceowner" : "invalid serviceowner"
if (tokenOptions && tokenOptions["scopes"] && tokenOptions["scopes"].indexOf("admin") > -1) {
logSuffix += " (admin)";
}

describe(`${logPrefix} dialog create as ${logSuffix}`, () => {
let r = postSO('dialogs', dialog, null, tokenOptions);
Expand Down

0 comments on commit 25277b5

Please sign in to comment.