Breakout development services & improve naming #427
Conversation
…cate/redundant properties
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #427 +/- ##
==========================================
+ Coverage 75.54% 77.50% +1.95%
==========================================
Files 100 98 -2
Lines 2785 2716 -69
Branches 455 445 -10
==========================================
+ Hits 2104 2105 +1
+ Misses 562 492 -70
Partials 119 119 ☔ View full report in Codecov by Sentry. |
| @@ -4,6 +4,7 @@ | |||
| <TargetFramework>net6.0</TargetFramework> | |||
| <Nullable>enable</Nullable> | |||
| <ImplicitUsings>enable</ImplicitUsings> | |||
| <LangVersion>11</LangVersion> | |||
There was a problem hiding this comment.
Better remove this property from all the projects and update the one in the global Directory.Build.props instead.
There was a problem hiding this comment.
@iamcarbon Agreed, but could be separate PR.
|
|
||
| public byte[] UserHandle { get; set; } | ||
|
|
||
| public string CredType { get; set; } |
There was a problem hiding this comment.
Could rename this to AttestationFormat, see #426.
There was a problem hiding this comment.
Agreed that AttestationFormat is a better name here. Updated.
|
This will definitely complete 3) from #426, nice. |
iamcarbon
changed the title
|
Thanks @iamcarbon and @Regenhardt - I've done a first pass, but will need more time to review this as it's extensive changes. Combined with a busy schedule, it make take some extra time before I'm able to merge. Thanks for the contributions and patience. |
|
Just an update, I will start work on this. |
|
@abergs I think we have two options on CredentialId.
My thinking on keeping Id (and dropping CredentialId) in this PR is explained in #428. The meaning of Id could also be clarified by renaming the type to something ending in Credential. |
| @@ -4,6 +4,7 @@ | |||
| <TargetFramework>net6.0</TargetFramework> | |||
| <Nullable>enable</Nullable> | |||
| <ImplicitUsings>enable</ImplicitUsings> | |||
| <LangVersion>11</LangVersion> | |||
There was a problem hiding this comment.
@iamcarbon Agreed, but could be separate PR.
| @@ -79,7 +79,7 @@ public AuthenticatorData(byte[] rpIdHash, AuthenticatorFlags flags, uint signCou | |||
| /// Backup state is signaled in authenticator data's flags and can change over time. | |||
| /// <see cref="https://w3c.github.io/webauthn/#backup-state"/> | |||
| /// </summary> | |||
| public bool BackupState => _flags.HasFlag(AuthenticatorFlags.BS); | |||
| public bool IsBackedUp => _flags.HasFlag(AuthenticatorFlags.BS); | |||
There was a problem hiding this comment.
I was contemplating if we should always keep using Spec terms on places you added IsBackedUp.
I decided the readability is worth not aligning to spec (as long as we mention BS/BE in XML).
|
@iamcarbon While this PR is accepted as is - It would be nice to change the name of |
|
@abergs All set. |
Moves the DevelopmentInMemoryStore, StoredCredential, and ConformanceMetadataService into a new Fido2.Development project.
Renames a few properties on StoredCredential for clarity.
Removes duplicate properties on StoredCredential (preferring specification name)