Skip to content

Unobsolete GET instance copy endpoint, copy based on Authenticated #1228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Unobsolete GET instance copy endpoint, copy based on Authenticated #1228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/Altinn.App.Api/Controllers/InstancesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Altinn.App.Core.Constants;
using Altinn.App.Core.Extensions;
using Altinn.App.Core.Features;
using Altinn.App.Core.Features.Auth;
using Altinn.App.Core.Helpers;
using Altinn.App.Core.Helpers.Serialization;
using Altinn.App.Core.Internal.App;
Expand Down Expand Up @@ -70,7 +71,7 @@ public class InstancesController : ControllerBase
private readonly ModelSerializationService _serializationService;
private readonly InternalPatchService _patchService;
private readonly InstanceDataUnitOfWorkInitializer _instanceDataUnitOfWorkInitializer;

private readonly IAuthenticationContext _authenticationContext;
private const long RequestSizeLimit = 2000 * 1024 * 1024;

/// <summary>
Expand All @@ -93,7 +94,8 @@ public InstancesController(
IHostEnvironment env,
ModelSerializationService serializationService,
InternalPatchService patchService,
IServiceProvider serviceProvider
IServiceProvider serviceProvider,
IAuthenticationContext authenticationContext
)
{
_logger = logger;
Expand All @@ -114,6 +116,7 @@ IServiceProvider serviceProvider
_serializationService = serializationService;
_patchService = patchService;
_instanceDataUnitOfWorkInitializer = serviceProvider.GetRequiredService<InstanceDataUnitOfWorkInitializer>();
_authenticationContext = authenticationContext;
}

/// <summary>
Expand Down Expand Up @@ -597,7 +600,8 @@ await _processEngine.HandleEventsAndUpdateStorage(

/// <summary>
/// This method handles the copy endpoint for when a user wants to create a copy of an existing instance.
/// The endpoint will primarily be accessed directly by a user clicking the copy button for an archived instance.
/// The endpoint will primarily be accessed directly by a user clicking the copy button for an archived instance
/// from the message box in the Altinn 2 portal/Altinn 3 arbeidsflate.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// from the message box in the Altinn 2 portal/Altinn 3 arbeidsflate.
/// from the message box in the Altinn 2 portal/Altinn 3 arbeidsflate.
/// WARNING: The endpoint will be removed in an updated version of the app packages as soon as we have an
/// alternative where dialogporten/arbeidsflate where it doesn't need to rely on the user cookie propagating
/// through a GET request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's discussions going on about the future of Altinn authentication (where/how to store session info, not doing token exchange etc etc) but there is nothing concrete yet, so I don't think it's appropriate to make any assertions on what we'll do at that point. Whatever it is, it has to be planned and agreed upon with the Arbeidsflate team

Copy link
Member

Choose a reason for hiding this comment

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

The important part is that the endpoint might be removed without warning when arbeidsflate has an alternative and stops using it for apps newer than a specific version (or however we decide to do the transition)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is your point that this is only meant to be consumed by Arbeidsflate? And that we don't care about potential breakage for any other possible consumer? If so I agree that we should document that someplace, but I don't think it belongs in an XML doc comment that doesn't go anywhere

/// </summary>
/// <param name="org">Unique identifier of the organisation responsible for the app</param>
/// <param name="app">Application identifier which is unique within an organisation</param>
Expand All @@ -607,9 +611,13 @@ await _processEngine.HandleEventsAndUpdateStorage(
/// <remarks>
/// The endpoint will return a redirect to the new instance if the copy operation was successful.
/// </remarks>
[Obsolete("This endpoint will be removed in a future release of the app template packages.")]
[ApiExplorerSettings(IgnoreApi = true)]
[Authorize]
// The URL contains "legacy", but it is not really legacy.
// Originally it was thought of as tech debt to do mutation like this in a GET endpoint,
// but after further consideration, it was decided to keep it as is.
// A related topic is the fact that Altinn tokens are "global" and not scoped to a specific app.
// Since it now would be a breaking change to rename or remove, it still has "legacy" as part of the name.
[HttpGet("/{org}/{app}/legacy/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/copy")]
[ProducesResponseType(typeof(Instance), StatusCodes.Status200OK)]
[ProducesResponseType(StatusCodes.Status400BadRequest)]
Expand All @@ -621,9 +629,9 @@ [FromRoute] Guid instanceGuid
)
{
// This endpoint should be used exclusively by end users. Ideally from a browser as a request after clicking
// a button in the message box, but for now we simply just exclude app owner(s).
string? orgClaim = User.GetOrg();
if (orgClaim is not null)
// a button in the message box.
var auth = _authenticationContext.Current;
if (auth is not Authenticated.User and not Authenticated.SelfIdentifiedUser)
{
return Forbid();
}
Expand Down
6 changes: 6 additions & 0 deletions src/Altinn.App.Core/Features/Auth/Authenticated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,11 @@ public sealed class SelfIdentifiedUser : Authenticated
/// </summary>
public string AuthenticationMethod { get; }

/// <summary>
/// True if the user was authenticated through the Altinn portal
/// </summary>
public bool InAltinnPortal { get; }

private Details? _extra;
private readonly Func<int, Task<UserProfile?>> _getUserProfile;
private readonly ApplicationMetadata _appMetadata;
Expand All @@ -355,6 +360,7 @@ ref ParseContext context
AuthenticationMethod = authenticationMethod;
// Since they are self-identified, they are always 0
AuthenticationLevel = 0;
InAltinnPortal = context.IsInAltinnPortal;
_getUserProfile = context.GetUserProfile;
_appMetadata = context.AppMetadata;
}
Expand Down
8 changes: 7 additions & 1 deletion test/Altinn.App.Api.Tests/Controllers/InstancesControllerFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Altinn.App.Api.Helpers.Patch;
using Altinn.App.Core.Configuration;
using Altinn.App.Core.Features;
using Altinn.App.Core.Features.Auth;
using Altinn.App.Core.Helpers.Serialization;
using Altinn.App.Core.Internal.App;
using Altinn.App.Core.Internal.AppModel;
Expand Down Expand Up @@ -55,7 +56,7 @@ public void VerifyNoOtherCalls(

public void Dispose() => (ServiceProvider as IDisposable)?.Dispose();

internal static InstancesControllerFixture Create()
internal static InstancesControllerFixture Create(Authenticated? auth = null)
{
var services = new ServiceCollection();
services.AddLogging(logging => logging.AddProvider(NullLoggerProvider.Instance));
Expand Down Expand Up @@ -86,6 +87,11 @@ internal static InstancesControllerFixture Create()
services.AddTransient<ModelSerializationService>();
services.AddTransient<InstanceDataUnitOfWorkInitializer>();

Mock<IAuthenticationContext> authenticationContextMock = new();
services.AddSingleton(authenticationContextMock.Object);
if (auth is not null)
authenticationContextMock.Setup(m => m.Current).Returns(auth);

services.AddTransient(sp =>
{
var controller = ActivatorUtilities.CreateInstance<InstancesController>(sp);
Expand Down
78 changes: 39 additions & 39 deletions test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
public async Task CopyInstance_CopyInstanceNotDefined_ReturnsBadRequest()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
var auth = TestAuthentication.GetUserAuthentication(userPartyId: 343234);
using var fixture = InstancesControllerFixture.Create(auth);
ApplicationMetadata application = new("ttd/copy-instance") { };
fixture.Mock<IAppMetadata>().Setup(a => a.GetApplicationMetadata()).ReturnsAsync(application);

Expand All @@ -45,7 +46,8 @@
public async Task CopyInstance_CopyInstanceNotEnabled_ReturnsBadRequest()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
var auth = TestAuthentication.GetUserAuthentication(userPartyId: 343234);
using var fixture = InstancesControllerFixture.Create(auth);
const string Org = "ttd";
const string AppName = "copy-instance";
fixture
Expand All @@ -70,7 +72,8 @@
public async Task CopyInstance_AsAppOwner_ReturnsForbidResult()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
var auth = TestAuthentication.GetServiceOwnerAuthentication(org: "ttd");
using var fixture = InstancesControllerFixture.Create(auth);
fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
Expand All @@ -91,13 +94,14 @@
public async Task CopyInstance_AsUnauthorized_ReturnsForbidden()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
var auth = TestAuthentication.GetUserAuthentication(userPartyId: 65434312);
using var fixture = InstancesControllerFixture.Create(auth);
fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(1337));
.Returns(TestAuthentication.GetUserPrincipal(65434312));
const string Org = "ttd";
const string AppName = "copy-instance";
fixture
.Mock<IAppMetadata>()
.Setup(a => a.GetApplicationMetadata())
Expand Down Expand Up @@ -125,7 +129,6 @@
public async Task CopyInstance_InstanceNotArchived_ReturnsBadRequest()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
int instanceOwnerPartyId = 343234;
Expand All @@ -135,11 +138,13 @@
Id = $"{instanceOwnerPartyId}/{instanceGuid}",
Status = new InstanceStatus() { IsArchived = false },
};

var auth = TestAuthentication.GetUserAuthentication(userPartyId: instanceOwnerPartyId);
using var fixture = InstancesControllerFixture.Create(auth);
fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(1337, instanceOwnerPartyId));
.Returns(TestAuthentication.GetUserPrincipal(partyId: instanceOwnerPartyId));

fixture
.Mock<IAppMetadata>()
.Setup(a => a.GetApplicationMetadata())
Expand Down Expand Up @@ -172,11 +177,12 @@
public async Task CopyInstance_InstanceDoesNotExists_ReturnsBadRequest()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
int instanceOwnerPartyId = 343234;
Guid instanceGuid = Guid.NewGuid();
var auth = TestAuthentication.GetUserAuthentication(userPartyId: instanceOwnerPartyId);
using var fixture = InstancesControllerFixture.Create(auth);

// Storage returns Forbidden if the given instance id is wrong.
PlatformHttpException platformHttpException = await PlatformHttpException.CreateAsync(
Expand All @@ -186,7 +192,7 @@
fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(1337, instanceOwnerPartyId));
.Returns(TestAuthentication.GetUserPrincipal(partyId: instanceOwnerPartyId));
fixture
.Mock<IAppMetadata>()
.Setup(a => a.GetApplicationMetadata())
Expand Down Expand Up @@ -219,11 +225,12 @@
public async Task CopyInstance_PlatformReturnsError_ThrowsException()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
int instanceOwnerPartyId = 343234;
Guid instanceGuid = Guid.NewGuid();
var auth = TestAuthentication.GetUserAuthentication(userPartyId: instanceOwnerPartyId);
using var fixture = InstancesControllerFixture.Create(auth);

// Simulate a BadGateway respons from Platform
PlatformHttpException platformHttpException = await PlatformHttpException.CreateAsync(
Expand All @@ -233,7 +240,7 @@
fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(1337, instanceOwnerPartyId));
.Returns(TestAuthentication.GetUserPrincipal(partyId: instanceOwnerPartyId));
fixture
.Mock<IAppMetadata>()
.Setup(a => a.GetApplicationMetadata())
Expand All @@ -247,22 +254,13 @@
.Setup(i => i.GetInstance(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<int>(), It.IsAny<Guid>()))
.ThrowsAsync(platformHttpException);

PlatformHttpException? actual = null;

// Act
try
{
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>();
await controller.CopyInstance("ttd", "copy-instance", instanceOwnerPartyId, instanceGuid);
}
catch (PlatformHttpException phe)
{
actual = phe;
}
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>();
var actual = await Assert.ThrowsAsync<PlatformHttpException>(
async () => await controller.CopyInstance("ttd", "copy-instance", instanceOwnerPartyId, instanceGuid)
);
Comment on lines +259 to +261

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning test

This assignment to
actual
Loading
is useless, since its value is never read.

Copilot Autofix

AI 2 months ago

To fix the problem, we need to remove the unnecessary assignment to the variable actual. The Assert.ThrowsAsync method call should be retained to ensure that the expected exception is thrown, but the result of this call does not need to be assigned to a variable.

  • Remove the assignment to the variable actual on line 259.
  • Retain the Assert.ThrowsAsync method call to ensure the test still verifies that the PlatformHttpException is thrown.
Suggested changeset 1
test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs b/test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
--- a/test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
+++ b/test/Altinn.App.Api.Tests/Controllers/InstancesController_CopyInstanceTests.cs
@@ -258,3 +258,3 @@
         var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>();
-        var actual = await Assert.ThrowsAsync<PlatformHttpException>(
+        await Assert.ThrowsAsync<PlatformHttpException>(
             async () => await controller.CopyInstance("ttd", "copy-instance", instanceOwnerPartyId, instanceGuid)
EOF
@@ -258,3 +258,3 @@
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>();
var actual = await Assert.ThrowsAsync<PlatformHttpException>(
await Assert.ThrowsAsync<PlatformHttpException>(
async () => await controller.CopyInstance("ttd", "copy-instance", instanceOwnerPartyId, instanceGuid)
Copilot is powered by AI and may make mistakes. Always verify output.

// Assert
Assert.NotNull(actual);

fixture.Mock<IAppMetadata>().VerifyAll();
fixture.Mock<IPDP>().VerifyAll();
fixture.Mock<IInstanceClient>().VerifyAll();
Expand All @@ -273,7 +271,6 @@
public async Task CopyInstance_InstantiationValidationFails_ReturnsForbidden()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
int instanceOwnerPartyId = 343234;
Expand All @@ -284,11 +281,13 @@
Status = new InstanceStatus() { IsArchived = true },
};
InstantiationValidationResult? instantiationValidationResult = new() { Valid = false };
var auth = TestAuthentication.GetUserAuthentication(userPartyId: instanceOwnerPartyId);
using var fixture = InstancesControllerFixture.Create(auth);

fixture
.Mock<HttpContext>()
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(1337, instanceOwnerPartyId));
.Returns(TestAuthentication.GetUserPrincipal(partyId: instanceOwnerPartyId));
fixture
.Mock<IAppMetadata>()
.Setup(a => a.GetApplicationMetadata())
Expand Down Expand Up @@ -327,18 +326,17 @@
public async Task CopyInstance_EverythingIsFine_ReturnsRedirect()
{
// Arrange
using var fixture = InstancesControllerFixture.Create();
const string Org = "ttd";
const string AppName = "copy-instance";
const int InstanceOwnerPartyId = 343234;
const int instanceOwnerPartyId = 343234;
Guid instanceGuid = Guid.NewGuid();
Guid dataGuid = Guid.NewGuid();
const string dataTypeId = "data_type_1";
Instance instance = new()
{
Id = $"{InstanceOwnerPartyId}/{instanceGuid}",
Id = $"{instanceOwnerPartyId}/{instanceGuid}",
AppId = $"{Org}/{AppName}",
InstanceOwner = new InstanceOwner() { PartyId = InstanceOwnerPartyId.ToString() },
InstanceOwner = new InstanceOwner() { PartyId = instanceOwnerPartyId.ToString() },
Status = new InstanceStatus() { IsArchived = true },
Process = new ProcessState() { CurrentTask = new ProcessElementInfo() { ElementId = "First" } },
Data = new List<DataElement>
Expand All @@ -347,11 +345,13 @@
},
};
InstantiationValidationResult? instantiationValidationResult = new() { Valid = true };
var auth = TestAuthentication.GetUserAuthentication(userPartyId: instanceOwnerPartyId);
using var fixture = InstancesControllerFixture.Create(auth);

fixture
.Mock<HttpContext>()
.Setup(hc => hc.User)
.Returns(TestAuthentication.GetUserPrincipal(1337, InstanceOwnerPartyId));
.Setup(httpContext => httpContext.User)
.Returns(TestAuthentication.GetUserPrincipal(partyId: instanceOwnerPartyId));
fixture.Mock<HttpContext>().Setup(hc => hc.Request).Returns(Mock.Of<HttpRequest>());
fixture
.Mock<IAppMetadata>()
Expand Down Expand Up @@ -392,7 +392,7 @@
);
fixture
.Mock<IDataClient>()
.Setup(p => p.GetFormData(instanceGuid, It.IsAny<Type?>()!, Org, AppName, InstanceOwnerPartyId, dataGuid))
.Setup(p => p.GetFormData(instanceGuid, It.IsAny<Type?>()!, Org, AppName, instanceOwnerPartyId, dataGuid))
.ReturnsAsync(new { test = "test" });
fixture
.Mock<IDataClient>()
Expand All @@ -403,20 +403,20 @@
It.IsAny<Type?>()!,
Org,
AppName,
InstanceOwnerPartyId,
instanceOwnerPartyId,
dataTypeId
)
)
.ReturnsAsync(new DataElement());

// Act
var controller = fixture.ServiceProvider.GetRequiredService<InstancesController>();
ActionResult actual = await controller.CopyInstance(Org, AppName, InstanceOwnerPartyId, instanceGuid);
ActionResult actual = await controller.CopyInstance(Org, AppName, instanceOwnerPartyId, instanceGuid);

// Assert
Assert.IsType<RedirectResult>(actual);
RedirectResult objectResult = (RedirectResult)actual;
Assert.Contains($"/#/instance/{InstanceOwnerPartyId}/", objectResult.Url);
Assert.Contains($"/#/instance/{instanceOwnerPartyId}/", objectResult.Url);

fixture.Mock<IAppMetadata>().VerifyAll();
fixture.Mock<IPDP>().VerifyAll();
Expand Down
1 change: 1 addition & 0 deletions ...s/Auth/AuthenticatedTests.Can_Parse_Real_Tokens_type=SelfIdentifiedUser_ffv+.verified.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
UserId: 1428813,
PartyId: 53328660,
AuthenticationMethod: SelfIdentified,
InAltinnPortal: true,
TokenIssuer: Altinn,
TokenIsExchanged: false,
Scopes: altinn:portal/enduser,
Expand Down
Loading