Conversation
|
We will need to add the new scopes to Auth0. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1010 +/- ##
==========================================
+ Coverage 71.82% 71.99% +0.17%
==========================================
Files 374 386 +12
Lines 20278 20860 +582
Branches 2686 2777 +91
==========================================
+ Hits 14564 15018 +454
- Misses 4647 4733 +86
- Partials 1067 1109 +42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman reviewed 31 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on ddaspit and Enkidu93).
src/Serval/src/Serval.Client/Client.g.cs line 492 at r1 (raw file):
[System.CodeDom.Compiler.GeneratedCode("NSwag", "14.7.1.0 (NJsonSchema v11.6.1.0 (Newtonsoft.Json v13.0.0.0))")] public partial interface IApiKeysClient
Do we want to expose the API keys API in the Serval Client given regular users of Serval will not be utilizing this API?
Code quote:
public partial interface IApiKeysClientsrc/Serval/src/Serval.ApiKeys/Features/ApiKeys/DeleteApiKey.cs line 9 at r1 (raw file):
public async Task HandleAsync(DeleteApiKey request, CancellationToken cancellationToken = default) { ApiKey? apiKey = await apiKeys.DeleteAsync(request.ApiKeyId, cancellationToken);
Instead of deleting outright, you could add a RevokedAt field, similar to ExpiresAt. This means that we can log use of revoked keys, as opposed to a client just using an incorrect key. Up to you though if you want to go down this path.
Code quote:
ApiKey? apiKey = await apiKeys.DeleteAsync(request.ApiKeyId, cancellationToken);src/Serval/src/Serval.ApiKeys/Serval.ApiKeys.csproj line 16 at r1 (raw file):
<ItemGroup> <PackageReference Include="Asp.Versioning.Abstractions" Version="8.1.0" />
This should be version 10.0.0 (the version we use elsewhere). I think also the versions in the test project are out of date compared to the versions used in other projects.
Code quote:
<PackageReference Include="Asp.Versioning.Abstractions" Version="8.1.0" />src/Serval/src/Serval.ApiKeys/Services/ApiKeyService.cs line 26 at r1 (raw file):
Owner = owner, Name = name, HashedKey = HashKey(key),
Do we want to salt the keys? We don't have to but it is easier to do now than later.
Code quote:
HashedKey = HashKey(key),
Enkidu93
left a comment
There was a problem hiding this comment.
@Enkidu93 reviewed 31 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on ddaspit and pmachapman).
src/Serval/src/Serval.ApiKeys/Services/ApiKeyService.cs line 26 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Do we want to salt the keys? We don't have to but it is easier to do now than later.
It seems like the sort of thing that wouldn't need to be salted since it's already a long, random string - i.e., as opposed to a password a user just came up with.
src/Serval/src/Serval.ApiKeys/Dtos/ApiKeyDto.cs line 7 at r1 (raw file):
public required string Id { get; init; } public required string Url { get; init; } public required string Owner { get; init; }
Just making sure I understand this: The Owner here is the client who will be associated with the key, not the client who creates the key; is that right? So the client who created it is not stored in the database then, right?
src/Serval/src/Serval.ApiKeys/Dtos/ApiKeyDto.cs line 9 at r1 (raw file):
public required string Owner { get; init; } public required string Name { get; init; } public required IList<string> Scopes { get; init; }
Why not IReadOnlyList<string> which we use elsewhere in the dtos? I guess it probably doesn't really matter.
src/Serval/src/Serval.ApiKeys/Dtos/ApiKeyDto.cs line 11 at r1 (raw file):
public required IList<string> Scopes { get; init; } public DateTime? ExpiresAt { get; init; } public required DateTime CreatedAt { get; init; }
I think elsewhere we've named properties like this something like DateCreated but I think this is fine. (I actually like this better just pointing it out for consistency).
- closes #1001
This change is