Use JWT to authenticate /api/device-accounts [SDK-2561] #95
Conversation
| @@ -86,7 +86,7 @@ | |||
| @NonNull | |||
| public GuardianAPIRequest<Void> delete(@NonNull Enrollment enrollment) { | |||
| return client | |||
| .device(enrollment.getId(), enrollment.getDeviceToken()) | |||
| .device(enrollment.getId(), enrollment.getUserId(), enrollment.getSigningKey()) | |||
There was a problem hiding this comment.
Minor: why not send the whole enrollment?? since all parameters are string sending the enrollment prevents mixing them up.
There was a problem hiding this comment.
That would be simpler but the way the SDK is structured today is sticking to the principle of least knowledge. In this particular case, that means that the Guardian API Client class doesn't have to know anything about the Enrollment class.
There was a problem hiding this comment.
I don't think that this applies here. We are using a deconstructed enrollment instead of using the actual data object, in terms of what enrollment is about this is a big chunk of what it is.
I guess that the tradeoff here how strong use of the java type system we want to do.
There was a problem hiding this comment.
@joseluisdiaz How I see this SDK, has two main classes for interacting with the API.
- The raw/base API client, called
GuardianAPIClientwhich only takes Strings and Private Keys (JDK classes) and makes the requests. Source here. - The wrapper, called
Guardian, documented as the entry point for consumption of this SDK in the readme. This one takes DTOs, as you suggest above. Source here.
The method signature under discussion here belongs to the GuardianAPIClient class. I'm saying that the class, to be consistent with what's already there, should keep accepting strings instead of being passed the Enrollment instance. Moreover, from that instance, we only require 3 properties of the almost 10 defined. IMHO passing the entire object just to pick these 3 properties is overkill.
In the end, this is not different from how it's implemented in the Swift SDK. If you still think this implementation should be changed, please let me know what changes you want and I'll make them.
There was a problem hiding this comment.
I see your point. I'm concerted about who would create the token actually. This seems to be a good enough approach.
| @@ -86,7 +86,7 @@ | |||
| @NonNull | |||
| public GuardianAPIRequest<Void> delete(@NonNull Enrollment enrollment) { | |||
| return client | |||
| .device(enrollment.getId(), enrollment.getDeviceToken()) | |||
| .device(enrollment.getId(), enrollment.getUserId(), enrollment.getSigningKey()) | |||
There was a problem hiding this comment.
I see your point. I'm concerted about who would create the token actually. This seems to be a good enough approach.
Description
The authentication of the
/api/device-accountsendpoint was using an opaque token, returned by the server upon account enrollment. In some cases, previously enrolled accounts might not be able to un-enroll the device.This PR moves the authentication to use a JWT, since the server also supports it. There are no breaking changes. The old method is still available and functional.
Depends on #94
Resources
See SDK-2561