Initial `IAuthenticationContext` abstraction #880

martinothamar · 2024-11-04T07:39:43Z

Description

Initial proposal for #763

Related Issue(s)

Authorization scenarios and mechanisms analysis #763
Instansiering med nivå-0 bruker feiler pga. manglende partyId #652
PDF generation fails during process/next when using org token as opposed to user token (frontend) #908
Handle missing profile when the logged in client in not a user (e.g. service owner) app-frontend-react#2889

Verification

Your code builds clean without any errors or warnings
Manual testing done (required)
Relevant automated test added (if you find this hard, leave it and we'll help out)
All tests run green

Documentation

User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

src/Altinn.App.Core/Internal/Auth/IAuthenticationContext.cs

src/Altinn.App.Api/Controllers/AuthorizationController.cs

test/Altinn.App.Core.Tests/Features/Correspondence/CorrespondenceClientTests.cs

@@ -334,7 +334,8 @@
        var mockHttpClientFactory = fixture.HttpClientFactoryMock;
        var mockMaskinportenClient = fixture.MaskinportenClientMock;
        var mockHttpClient = new Mock<HttpClient>();
-        var altinnTokenResponse = PrincipalUtil.GetOrgToken("ttd");
+        var correspondencePayload = PayloadFactory.Send(authorisation: CorrespondenceAuthorisation.Maskinporten);


ivarne

Fin opprydding i mange av testene og autorisasjon :).

Har kommentert litt ved gjennomlesning

ivarne · 2025-01-09T10:49:14Z

src/Altinn.App.Api/Controllers/AuthenticationController.cs

    {
        _authenticationClient = authenticationClient;
        _settings = settings.Value;
+        _authenticationContext = serviceProvider.GetRequiredService<IAuthenticationContext>();


Could you add a comment about why you inject IServiceProvider instead of IAuthenticationContext?

in code or just here? It's because the interface is internal for now

ivarne · 2025-01-09T10:51:54Z

src/Altinn.App.Api/Controllers/AuthenticationController.cs

+    /// <returns>Party id for selected party. If invalid, partyId for logged in user is returned.</returns>
+    [Authorize]
+    [ResponseCache(Duration = 0, Location = ResponseCacheLocation.None, NoStore = true)]
+    [HttpGet("{org}/{app}/api/[controller]/current")]


Why /[controller]/ instead of authentication (with a lower case a)

You also need to somehow annotate for swagger to document the possible response(s).

I might remove or comment this out. I was orginally intending to replace 3 separate frontend requests with this endpoint, but put that on the backburner as it seemed difficult to refactor the frontend at the time. Wether a union is a good idea I'm not sure. I did have the start of a TS implementation that used the discriminator to figure which type it was, and a .NET client would be fine with it, but yeah it's not the most common thing

ivarne · 2025-01-09T10:57:31Z

src/Altinn.App.Api/Controllers/AuthenticationController.cs

+    [JsonDerivedType(typeof(OrgResponse), typeDiscriminator: "Org")]
+    [JsonDerivedType(typeof(ServiceOwnerResponse), typeDiscriminator: "ServiceOwner")]
+    [JsonDerivedType(typeof(SystemUserResponse), typeDiscriminator: "SystemUser")]
+    private abstract record CurrentAuthenticationBaseResponse { }


I'm not sure I like the idea of exposing a discriminated union in the api. Does OpenAPI generation support this nicely?

At least we need some docs on the properties

ivarne · 2025-01-09T11:01:17Z

src/Altinn.App.Api/Controllers/AuthenticationController.cs

+    {
+        public required UserProfile Profile { get; init; }
+
+        public required Party Party { get; init; }


Is this the party of the authenticated user or the party he currently intends to represent as specified by the AltinnPartyId cookie (which is kind of useless because it will be wrong for one of the tabs if you have multiple instances for multiple reportees open at the same time )?

ivarne · 2025-01-09T11:11:18Z

src/Altinn.App.Core/Internal/Auth/IAuthenticationContext.cs

+    /// This means that the user has authenticated with a username/password, which can happen using
+    /// * Altinn "self registered users"
+    /// * IDporten through Ansattporten ("low"), MinID self registered eID
+    /// These have limited access to Altinn and can only represent themselves.


Is this correct? At least if you come through Ansattporten, you likely represent the org you're employed at?

I think you end up as a user, and not with authentication_level = 0? I'll check...

ivarne · 2025-01-09T11:16:58Z

src/Altinn.App.Api/Infrastructure/Middleware/AuthenticationContextMiddleware.cs

+    {
+        var authenticationContext = (AuthenticationContext)
+            context.RequestServices.GetRequiredService<IAuthenticationContext>();
+        authenticationContext.ResolveCurrent(context);


Why do you need a middleware here? Don't we already have a middleware that sets IHttpContextAccessor.Current?

I don't remember if there is a good reason anymore, I'll try to retrieve it lazily instead, without middleware

ivarne · 2025-01-09T11:26:44Z

src/Altinn.App.Core/Internal/Auth/IAuthenticationContext.cs

This file has too many classes, records and interfaces. Would be nice to split it up.

…obust

src/Altinn.App.Core/Features/Auth/AuthenticationInfo.cs

…d party everywhere

src/Altinn.App.Core/Features/Auth/AuthenticationInfo.cs

HauklandJ

lgtm

src/Altinn.App.Core/Features/Auth/Authenticated.cs

+
+            var lookupPartyTask =
+                SelectedPartyId == userProfile.PartyId
+                    ? Task.FromResult((Party?)userProfile.Party)


sonarqubecloud · 2025-01-19T08:50:50Z

Quality Gate failed

Failed conditions
6.5% Coverage on New Code (required ≥ 65%)
C Reliability Rating on New Code (required ≥ A)
1.14% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

martinothamar added the feature Label Pull requests with new features. Used when generation releasenotes label Nov 4, 2024

martinothamar self-assigned this Nov 4, 2024

martinothamar mentioned this pull request Nov 4, 2024

Authorization scenarios and mechanisms analysis #763

Closed

martinothamar force-pushed the feat/authn-abstraction branch from 3935f27 to 9a48939 Compare November 8, 2024 09:16

martinothamar changed the title ~~Initial IClientContext abstraction~~ Initial IAuthenticationContext abstraction Nov 8, 2024

martinothamar force-pushed the feat/authn-abstraction branch from 9a48939 to a1452cd Compare November 18, 2024 14:22

martinothamar mentioned this pull request Nov 28, 2024

Populate UserContext.SocialSecurityNumber in UserHelper #930

Merged

5 tasks

martinothamar force-pushed the feat/authn-abstraction branch from 8b7e8aa to 3cb8eab Compare November 29, 2024 07:34

github-advanced-security bot found potential problems Dec 11, 2024

View reviewed changes

src/Altinn.App.Core/Internal/Auth/IAuthenticationContext.cs Fixed Show fixed Hide fixed

src/Altinn.App.Core/Internal/Auth/IAuthenticationContext.cs Fixed Show fixed Hide fixed

martinothamar force-pushed the feat/authn-abstraction branch from d49f6d4 to 029fd81 Compare December 12, 2024 08:11

martinothamar force-pushed the feat/authn-abstraction branch from a1ab771 to cfa4c3a Compare January 8, 2025 07:20

github-advanced-security bot found potential problems Jan 8, 2025

View reviewed changes

martinothamar marked this pull request as ready for review January 9, 2025 08:09

martinothamar mentioned this pull request Jan 9, 2025

Handle missing profile when the logged in client in not a user (e.g. service owner) Altinn/app-frontend-react#2889

Merged

19 tasks

martinothamar force-pushed the feat/authn-abstraction branch from 9973fa2 to 55a75b9 Compare January 9, 2025 09:29

martinothamar mentioned this pull request Jan 9, 2025

Add TTD org in register test data Altinn/app-localtest#133

Open

5 tasks

ivarne reviewed Jan 9, 2025

View reviewed changes

martinothamar and others added 13 commits January 9, 2025 23:33

Initial IClientContext abstraction

ced8b1b

Rename to 'AuthenticationContext', make IAuthenticationContext more r…

089c504

…obust

one more authenticated type

fa2c85b

update

5910637

tmp

a863ff3

Update GetCurrentParty to use new abstraction

25f41e2

Update parties controller

0ea085c

Middleware to resolve authenticationcontext

ede5542

Fix tests, refactor PrincipalUtil slightly

c760698

Testing systemusers, selfidentified users

bffc619

systemuser fixes

4dfe163

fix policy

6df1ea2

bugs after rebase

585129b

martinothamar added 6 commits January 9, 2025 23:33

Extend current auth

d0d46c7

Update naming for selected partyh

8a28197

Handle self identified users

506a2c6

Remove unused endpoint

84863d6

Remove middleware, create AuthenticationInfo lazily

2790c76

Some reorg, split IAuthenticationContext.cs into multiple files

45863cc

martinothamar force-pushed the feat/authn-abstraction branch from df48e10 to 45863cc Compare January 9, 2025 22:34

github-advanced-security bot found potential problems Jan 9, 2025

View reviewed changes

src/Altinn.App.Core/Features/Auth/AuthenticationInfo.cs Fixed Show fixed Hide fixed

Attempt to simplify party representation in user, reportee -> selecte…

3de6594

…d party everywhere

github-advanced-security bot found potential problems Jan 9, 2025

View reviewed changes

src/Altinn.App.Core/Features/Auth/AuthenticationInfo.cs Fixed Show fixed Hide fixed

HauklandJ approved these changes Jan 10, 2025

View reviewed changes

martinothamar added 9 commits January 14, 2025 10:23

Merge branch 'main' into feat/authn-abstraction

eb37bc3

internal

ec2c071

Merge branch 'main' into feat/authn-abstraction

8e4ccae

Remove unused code

e805c55

unused var

e666fe1

Merge branch 'main' into feat/authn-abstraction

3b3ec02

Make IAuthenticationContext public after all

a2a54e9

Merge branch 'main' into feat/authn-abstraction

c0e9ad5

AuthenticationInfo -> Authenticated

6246158

github-advanced-security bot found potential problems Jan 19, 2025

View reviewed changes

src/Altinn.App.Core/Features/Auth/Authenticated.cs

var lookupPartyTask =

SelectedPartyId == userProfile.PartyId

? Task.FromResult((Party?)userProfile.Party)

Check warning

Code scanning / CodeQL

Cast to same type Warning

This cast is redundant because the expression already has type Party.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial `IAuthenticationContext` abstraction #880

Initial `IAuthenticationContext` abstraction #880

martinothamar commented Nov 4, 2024 •

edited

Loading

ivarne left a comment

ivarne Jan 9, 2025

martinothamar Jan 9, 2025

ivarne Jan 9, 2025

ivarne Jan 9, 2025

martinothamar Jan 9, 2025

ivarne Jan 9, 2025

ivarne Jan 9, 2025

ivarne Jan 9, 2025

martinothamar Jan 9, 2025

ivarne Jan 9, 2025

martinothamar Jan 9, 2025

ivarne Jan 9, 2025

martinothamar Jan 9, 2025

HauklandJ left a comment

sonarqubecloud bot commented Jan 19, 2025

Initial IAuthenticationContext abstraction #880

Are you sure you want to change the base?

Initial IAuthenticationContext abstraction #880

Conversation

martinothamar commented Nov 4, 2024 • edited Loading

Description

Related Issue(s)

Verification

Documentation

ivarne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HauklandJ left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 19, 2025

Quality Gate failed

Initial `IAuthenticationContext` abstraction #880

Initial `IAuthenticationContext` abstraction #880

martinothamar commented Nov 4, 2024 •

edited

Loading