Extend Token with HasClaims for custom claims
#204
Conversation
The IETF RFCs for Tokens not only include a couple extra properties that were not previously addressed, but it states that the responses may be extended with custom information. Authorization servers, such as Keycloak, includes metadata in the token response indicating refresh token expiration, or other data. Since Token responses can be extended, it stands to reason that these objects should conform to `HasClaims` just like other models, to allow for them to be adapted to different authentication scenarios. This update further extends the support for Claims to resolve edge-cases in how convertible values were handled, unifying support around mapping claims to scalar values, arrays, or dictionaries, and cleaning up how JSON objects are mapped. Finally, since the result of refresh operations often doesn't include data provided in the initial token exchange request (such as `device_secret`), the process for merging tokens during refresh operations has been moved to a protocol. This is not yet public, but this may be exposed in the future.
| extension Array<String>: ClaimConvertable {} | ||
| extension Dictionary<String, String>: ClaimConvertable {} |
There was a problem hiding this comment.
Does JWTClaim take care of array and dictionary?
There was a problem hiding this comment.
It's unnecessary since the accessors for working with claims (e.g. value(...) or subscript methods) address arrays or dictionaries already.
The goal for ClaimConvertable is to map some JSON primitive supplied from the server into a more developer-friendly value (e.g. from a string to URL, or to an enum, etc.). As a result, arrays and dictionaries themselves aren't all that interesting to be converted.
In fact, the only reason Array and Dictionary were made to be convertable in the first place was because the value functions were being used inconsistently, which prevented their values from being properly converted in the first place.
| @@ -12,25 +12,42 @@ | |||
|
|
|||
| import Foundation | |||
|
|
|||
| @_documentation(visibility: private) | |||
There was a problem hiding this comment.
curious, so this hides the extension from generated docs or from Xcode as well?
There was a problem hiding this comment.
From the generated docs. Many of these just clutter up the docs. For reference, here's the list of underscored attributes.
| clientId: "clientid", | ||
| scopes: "openid"), | ||
| clientSettings: [ "client_id": "foo" ])) | ||
| let token = try! Token(id: "TokenId", |
There was a problem hiding this comment.
| let token = try! Token(id: "TokenId", | |
| guard let token = try? Token(id: "TokenId", ... else { XCTFail() }? |
There was a problem hiding this comment.
That's in a class property, not a function block, so I opted to simply force-unwrapping since it's in a unit test. Though I could move initializing the Token to the setUpWithError function instead.
| clientId: "clientid", | ||
| scopes: scopes), | ||
| clientSettings: [ "client_id": "clientid" ])) | ||
| return try! Token(id: "TokenId", |
There was a problem hiding this comment.
Do we want to test anything about these tokens once created? Or is the constructor enough of a test by itself?
There was a problem hiding this comment.
There's the Tests/AuthFoundationTests/TokenTests.swift class that exercises the various ways the token can be initialized, including migrating from V1 data.
This mock token utility is simply there to return tokens for use in other tests.
Force unwrapping makes me feel icky, but as a utility class, if I required all the other tests using this to try, it would make the tests messier.
The IETF RFCs for Tokens not only include a couple extra properties that were not previously addressed, but it states that the responses may be extended with custom information. Authorization servers, such as Keycloak, includes metadata in the token response indicating refresh token expiration, or other data.
Since Token responses can be extended, it stands to reason that these objects should conform to
HasClaimsjust like other models, to allow for them to be adapted to different authentication scenarios.This update further extends the support for Claims to resolve edge-cases in how convertible values were handled, unifying support around mapping claims to scalar values, arrays, or dictionaries, and cleaning up how JSON objects are mapped.
Finally, since the result of refresh operations often doesn't include data provided in the initial token exchange request (such as
device_secret), the process for merging tokens during refresh operations has been moved to a protocol. This is not yet public, but this may be exposed in the future.This PR: