Implement IMaskinportenClient interface #669

danielskovli · 2024-06-03T12:17:14Z

Description

Implements IMaskinportenClient interface as per proof of concept project.

Related Issue(s)

Implement IMaskinportenClient interface #602

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)

Source POC: https://github.com/danielskovli/maskinporten-authentication-dotnet

src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs

test/Altinn.App.Core.Tests/Features/Maskinporten/Delegates/MaskinportenDelegatingHandlerTest.cs

test/Altinn.App.Core.Tests/Features/Maskinporten/MaskinportenClientTest.cs

test/Altinn.App.Core.Tests/Features/Maskinporten/TestHelpers.cs

src/Altinn.App.Core/Features/Maskinporten/MaskinportenClient.cs

ivarne

Ser bra ut. Noen raske kommentarer (håper de er gode).

WebApplicationFactory

Savner en full test der du bruker WebApplicationFactory til å hente token til en IOptionProvider, for å teste hele løpet, men det blir kanskje for mye mocking av litt mange http klienter til at det egentlig gir verdi. (mulig CustomWebApplicationFactory.SendAsync gjør at du kan teste med flere http klienter og at maskinporten delegating handler kommer i forkant.)

app-lib-dotnet/test/Altinn.App.Api.Tests/CustomWebApplicationFactory.cs

Lines 212 to 215 in 0161134

    
               protected Func<HttpRequestMessage, Task<HttpResponseMessage>> SendAsync { get; set; } = 
        
                   (request) => 
        
                       throw new NotImplementedException("You must set SendAsync in your test when it uses a real http client."); 
        
           }

Tid

Har ikke gjort det selv, men det finnes interfaces for å gjøre det enklere å teste cache expiration med tid (uten Task.Delay som gjør testene veeeldig trege)

https://andrewlock.net/exploring-the-dotnet-8-preview-avoiding-flaky-tests-with-timeprovider-and-itimer/

src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs

src/Altinn.App.Core/EFormidling/Implementation/EformidlingStatusCheckEventHandler2.cs

src/Altinn.App.Core/Features/Maskinporten/Converters/JsonWebKeyConverter.cs

src/Altinn.App.Api/Extensions/HttpClientBuilderExtensions.cs

src/Altinn.App.Core/Features/Maskinporten/MaskinportenClient.cs

test/Altinn.App.Core.Tests/Features/Maskinporten/Models/MaskinportenSettingsTest.cs

…constructor

src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs

Comments on what CS0618 is Co-authored-by: Ivar Nesje <[email protected]>

…/602

martinothamar

👏

I made a small change to UseMaskinportenAuthorization and the related test since I found it easier than to explain, hope that's OK

src/Altinn.App.Api/Extensions/ServiceCollectionExtensions.cs

src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs

src/Altinn.App.Core/Features/Maskinporten/Converters/JsonWebKeyConverter.cs

src/Altinn.App.Core/Features/Maskinporten/Exceptions/MaskinportenException.cs

src/Altinn.App.Core/Features/Maskinporten/Exceptions/MaskinportenTokenExpiredException.cs

src/Altinn.App.Core/Features/Maskinporten/Models/MaskinportenSettings.cs

test/Altinn.App.Api.Tests/Telemetry/TelemetryConfigurationTests.cs

… transparency

…/602

danielskovli · 2024-06-07T06:32:00Z

@ivarne

Savner en full test der du bruker WebApplicationFactory til å hente token til en IOptionProvider, for å teste hele løpet

Jeg synes vi har ganske bra dekning på alle komponentene nå, men det jeg savner selv er vel en faktisk implementasjon med en test-mottaker som kan ta imot en request og verifisere at Authorization: Bearer xxx stemmer.

All annen mocking blir bare ... mer mocking. Ta gjerne en titt på den litt oppdaterte testen her og se om det hjelper noe på magefølelsen.

Tid [...] Har ikke gjort det selv, men det finnes interfaces for å gjøre det enklere å teste cache expiration med tid

Veldig interessert i dette! Skal ta en titt 😄

danielskovli · 2024-06-07T06:33:20Z

@martinothamar

I made a small change to UseMaskinportenAuthorization and the related test since I found it easier than to explain, hope that's OK

Very appreciated and more than okay! 🙏

It does not work with semaphore locking

`JsonConverter` was not respected in all code paths, leading to inconsistencies between systems and versions. Additionally we want to support a base64 encoding of the JWK for ease of runtime configuration (legacy). All of which would have been sweet with a JsonConverter, but alas.

src/Altinn.App.Core/Features/Maskinporten/Converters/JsonWebKeyConverter.cs

test/Altinn.App.Core.Tests/Features/Maskinporten/Models/MaskinportenSettingsTest.cs

src/Altinn.App.Core/Features/Maskinporten/MaskinportenClient.cs

sonarcloud · 2024-07-05T08:33:45Z

Quality Gate passed

Issues
9 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

This reverts commit d6bfccc.

bjorntore · 2024-07-10T15:22:16Z

Setting ignore-for-release since this had to be reverted from main due to a preview dependency, which is not allowed.

There will be a new PR for this later.

Migrates and rewrites core functionality as required

8b5d75a

Source POC: https://github.com/danielskovli/maskinporten-authentication-dotnet

danielskovli linked an issue Jun 3, 2024 that may be closed by this pull request

Implement IMaskinportenClient interface #602

Closed

danielskovli mentioned this pull request Jun 3, 2024

Implement IMaskinportenClient interface #602

Closed

danielskovli added 2 commits June 3, 2024 14:50

WIP tests

105b1e4

WIP tests

a43f1ec

danielskovli self-assigned this Jun 3, 2024

danielskovli added the feature Label Pull requests with new features. Used when generation releasenotes label Jun 3, 2024

danielskovli added 5 commits June 3, 2024 16:37

Fixes pragma typo

22016e1

Moar pragma

76bc12d

Improves testing, moves some extensions

99007f3

Fixes JWK deserializing

f57178e

Fixes doctag warnings

6d436d0

github-advanced-security bot found potential problems Jun 4, 2024

View reviewed changes

danielskovli marked this pull request as ready for review June 4, 2024 13:39

ivarne reviewed Jun 4, 2024

View reviewed changes

danielskovli added 2 commits June 5, 2024 09:38

Avoids passing IServiceProvider to MaskinportenDelegatingHandler …

e218d7f

…constructor

Extracts WebHostBuilderExtensions methods for readability

0c0f601

github-advanced-security bot found potential problems Jun 5, 2024

View reviewed changes

src/Altinn.App.Api/Extensions/WebHostBuilderExtensions.cs Dismissed Show dismissed Hide dismissed

danielskovli and others added 5 commits June 5, 2024 09:52

Apply suggestions from code review

f69695f

Comments on what CS0618 is Co-authored-by: Ivar Nesje <[email protected]>

Makes logger for MaskinportenClient mandatory

959b7ca

Merge branch 'feat/602' of github.com:Altinn/app-lib-dotnet into feat…

59822d3

…/602

Comments on test case specifics

aa19243

Simplify 'UseMaskinportenAuthorization' test

7237c2e

martinothamar reviewed Jun 5, 2024

View reviewed changes

danielskovli added 4 commits June 6, 2024 15:45

Makes at least one scope required for UseMaskinportenAuthorization

1dadb57

Exposes MaskinportenDelegatingHandler.Scopes for better testing and…

f463f98

… transparency

Merge branch 'feat/602' of github.com:Altinn/app-lib-dotnet into feat…

6bb2b2f

…/602

Fixes additional merge conflicts

edfacab

danielskovli added 2 commits June 11, 2024 10:15

Fixes some httpclient/messagehandler dispose issues in tests

e4c5954

Merge branch 'main' into feat/602

ba079b0

danielskovli mentioned this pull request Jun 11, 2024

Add documentation for new Maskinporten integration #681

Open

4 tasks

Remove refetchBeforeExpiry

136991f

It does not work with semaphore locking

ivarne force-pushed the feat/602 branch from 253eac7 to 136991f Compare June 12, 2024 06:38

danielskovli added 2 commits June 12, 2024 10:16

Merge remote-tracking branch 'origin/feat/602' into feat/602

02e7e96

github-advanced-security bot found potential problems Jun 12, 2024

View reviewed changes

danielskovli added 3 commits June 28, 2024 11:28

Merge branch 'main' into feat/602

b98a40d

Implements HybridCache, removes RefreshCache

5916cbd

Fixes tests

620e6b3

github-advanced-security bot found potential problems Jul 3, 2024

View reviewed changes

src/Altinn.App.Core/Features/Maskinporten/MaskinportenClient.cs Fixed Show fixed Hide fixed

ivarne reviewed Jul 3, 2024

View reviewed changes

src/Altinn.App.Core/Features/Maskinporten/MaskinportenClient.cs Outdated Show resolved Hide resolved

danielskovli added 4 commits July 3, 2024 13:46

Fixes more tests

9b6824a

Adds a sprinkle of salt

ea6c421

Merge remote-tracking branch 'origin/main' into feat/602

08790be

Merge branch 'main' into feat/602

964dc78

danielskovli merged commit d6bfccc into main Jul 5, 2024
10 checks passed

danielskovli deleted the feat/602 branch July 5, 2024 08:35

bjorntore added a commit that referenced this pull request Jul 10, 2024

Revert "Implement IMaskinportenClient interface (#669)"

84ff668

This reverts commit d6bfccc.

bjorntore mentioned this pull request Jul 10, 2024

Revert "Implement IMaskinportenClient interface (#669)" #713

Merged

danielskovli restored the feat/602 branch July 10, 2024 15:01

danielskovli linked an issue Jul 10, 2024 that may be closed by this pull request

MaskinportenClient implementation cannot use HybridCache because it is currently in preview #714

Closed

danielskovli mentioned this pull request Jul 10, 2024

MaskinportenClient implementation cannot use HybridCache because it is currently in preview #714

Closed

bjorntore added a commit that referenced this pull request Jul 10, 2024

Revert "Implement IMaskinportenClient interface (#669)" (#713)

506fb06

This reverts commit d6bfccc.

bjorntore added ignore-for-release and removed feature Label Pull requests with new features. Used when generation releasenotes labels Jul 10, 2024

danielskovli mentioned this pull request Jul 10, 2024

Implement IMaskinportenClient interface #715

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IMaskinportenClient interface #669

Implement IMaskinportenClient interface #669

danielskovli commented Jun 3, 2024 •

edited

Loading

ivarne left a comment

martinothamar left a comment

danielskovli commented Jun 7, 2024 •

edited

Loading

danielskovli commented Jun 7, 2024

sonarcloud bot commented Jul 5, 2024

bjorntore commented Jul 10, 2024

	protected Func<HttpRequestMessage, Task<HttpResponseMessage>> SendAsync { get; set; } =
	(request) =>
	throw new NotImplementedException("You must set SendAsync in your test when it uses a real http client.");
	}

Implement IMaskinportenClient interface #669

Implement IMaskinportenClient interface #669

Conversation

danielskovli commented Jun 3, 2024 • edited Loading

Description

Related Issue(s)

Verification

Documentation

ivarne left a comment

Choose a reason for hiding this comment

WebApplicationFactory

Tid

martinothamar left a comment

Choose a reason for hiding this comment

danielskovli commented Jun 7, 2024 • edited Loading

danielskovli commented Jun 7, 2024

sonarcloud bot commented Jul 5, 2024

Quality Gate passed

bjorntore commented Jul 10, 2024

danielskovli commented Jun 3, 2024 •

edited

Loading

danielskovli commented Jun 7, 2024 •

edited

Loading