Skip to content
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.

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

Implement IMaskinportenClient interface #669

Merged
merged 54 commits into from
Jul 5, 2024
Merged

Implement IMaskinportenClient interface #669

merged 54 commits into from
Jul 5, 2024

Conversation

danielskovli
Copy link
Contributor

@danielskovli danielskovli commented Jun 3, 2024

Description

Implements IMaskinportenClient interface as per proof of concept project.

Related Issue(s)

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)

@danielskovli danielskovli linked an issue Jun 3, 2024 that may be closed by this pull request
@danielskovli danielskovli self-assigned this Jun 3, 2024
@danielskovli danielskovli added the feature Label Pull requests with new features. Used when generation releasenotes label Jun 3, 2024
@danielskovli danielskovli marked this pull request as ready for review June 4, 2024 13:39
Copy link
Member

@ivarne ivarne left a comment

Choose a reason for hiding this comment

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

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

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/

Copy link
Contributor

@martinothamar martinothamar left a comment

Choose a reason for hiding this comment

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

👏

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

@danielskovli
Copy link
Contributor Author

danielskovli commented Jun 7, 2024

@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
Copy link
Contributor Author

@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.
Copy link

sonarcloud bot commented Jul 5, 2024

@danielskovli danielskovli merged commit d6bfccc into main Jul 5, 2024
10 checks passed
@danielskovli danielskovli deleted the feat/602 branch July 5, 2024 08:35
bjorntore added a commit that referenced this pull request Jul 10, 2024
@danielskovli danielskovli restored the feat/602 branch July 10, 2024 15:01
bjorntore added a commit that referenced this pull request Jul 10, 2024
@bjorntore bjorntore added ignore-for-release and removed feature Label Pull requests with new features. Used when generation releasenotes labels Jul 10, 2024
@bjorntore
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
5 participants