Skip to content

Make Origins readonly - #393

Merged
abergs merged 1 commit into
passwordless-lib:masterfrom
Regenhardt:feature/readonly-origins
Jun 26, 2023
Merged

abergs merged 1 commit into
passwordless-lib:masterfrom
Regenhardt:feature/readonly-origins

Conversation

@Regenhardt

Copy link
Copy Markdown
Contributor

Closes #392

@abergs
abergs self-requested a review May 17, 2023 20:48
@abergs abergs self-assigned this May 17, 2023
Comment thread Src/Fido2.Models/Fido2Configuration.cs Outdated
@Regenhardt
Regenhardt force-pushed the feature/readonly-origins branch from 82c68c2 to cf00991 Compare May 17, 2023 21:00
Comment thread Src/Fido2.Models/Fido2Configuration.cs Outdated
@codecov-commenter

codecov-commenter commented May 17, 2023

Copy link
Copy Markdown

Codecov Report

Merging #393 (4fd115c) into master (a33848c) will increase coverage by 0.05%.
The diff coverage is 25.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
+ Coverage   76.80%   76.86%   +0.05%     
==========================================
  Files          89       89              
  Lines        2531     2524       -7     
  Branches      427      426       -1     
==========================================
- Hits         1944     1940       -4     
+ Misses        466      463       -3     
  Partials      121      121              
Impacted Files Coverage Δ
Src/Fido2/AuthenticatorAssertionResponse.cs 50.84% <ø> (ø)
Src/Fido2/AuthenticatorResponse.cs 57.14% <ø> (ø)
Src/Fido2.Models/Fido2Configuration.cs 69.23% <25.00%> (+2.56%) ⬆️

@abergs abergs added the breaking change Indicate that something is a breaking change label May 19, 2023
@abergs

abergs commented May 19, 2023

Copy link
Copy Markdown
Collaborator

This solution to the #392 will cause a breaking change. That is fine since we will drop a v4 with other breaking changes.

However, looking through this code, I'm wondering if we should take the opportunity clean up some of this mess.

  • Drop the Origin property
  • Make FullyQualifiedOrigins both readonly and IReadOnlySet

That way we can just set it once in Origins setter.

Any objections @aseigler?

@aseigler

Copy link
Copy Markdown
Collaborator

No objections, makes sense to me.

@Regenhardt

Copy link
Copy Markdown
Contributor Author

Like this? Although FullyQualifiedOrigins might aswell be a { get; private set; } property now. Should I additionally document the breaking change somewhere?

@abergs

abergs commented May 22, 2023

Copy link
Copy Markdown
Collaborator

@Regenhardt It will be documented automatically in the Release notes.

I will do a final review and merge as soon as I have a moment. Traveling this week.

@abergs

abergs commented Jun 21, 2023

Copy link
Copy Markdown
Collaborator

@Regenhardt This now has a bit of conflicts because we brought some older PRs in. Would you mind rebasing and sorting it out and we're pretty good to go?

@Regenhardt
Regenhardt force-pushed the feature/readonly-origins branch from 4fd115c to 7faba0e Compare June 22, 2023 09:59
@Regenhardt

Copy link
Copy Markdown
Contributor Author

Rebased it

@Regenhardt

Copy link
Copy Markdown
Contributor Author

Not sure what's going on, it looks like dotnet restore did nothing for 7 minutes and then timed out? Any idea what to do? Can the runner have had a problem so we just re-run the pipeline?

@Regenhardt Regenhardt mentioned this pull request Jun 23, 2023
@abergs

abergs commented Jun 26, 2023

Copy link
Copy Markdown
Collaborator

Re-ran all tests, seems good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Indicate that something is a breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

adding a origin to Origins directly should throw a error

4 participants