-
Notifications
You must be signed in to change notification settings - Fork 18
SDK-2771: [MB] Introduce support for auth tokens #504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
I started looking into this PR and I'll need also to test it asap |
Since we're only merging into dev, we were thinking we'd merge it so that the SNAPSHOT can be built & tested. |
5fe9a68 to
a43b758
Compare
a43b758 to
0144ea8
Compare
|
|
Tested locally the |
pn-santos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that familiar with this codebase but fwiw I skimmed through it and nothing jumped out worth pointing out related to the change.
Since @irotech has had more hours looking at this codebase I'll defer to him. As far as I'm concerned feel free to merge.
| } | ||
|
|
||
| SignedRequest createSignedRequest(KeyPair keyPair, String resourcePath, byte[] body) throws AmlException { | ||
| YotiHttpRequest createSignedRequest(String resourcePath, byte[] body) throws AmlException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these methods kinda no longer are exclusively "signed" requests, should they be renamed createRequest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, thank you. I'll take a look and raise a separate change.
irotech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to add few minor comments around reducing the verbosity of member|var|arg names often as same as the type or use sdkId instead of appId everywhere but neither we have a proper coding style nor this suggestions are for new code. As mentioned in previous comment I tested locally the shares and did examples pointing stg env.
|
I don't mind opening a new PR with other changes / improvements in it @irotech |



This one ends up being a large PR touching pretty much every part of the SDK, sorry.
The goal is to introduce support for auth tokens in the Yoti Docs client - we've already release them to prod. A subsequent PR will cover how auth tokens get created, this one is only addressing how they are used in http requests.
The Plan
The plan is that...
for token generation & management on other peoples servers
So we're looking for a simple, maintainable solution here. It is likely we'll retire support for signed requests at some future point and then require all integrators to move to auth tokens but we have no timescale for that.
The Pain
The problem we ran into is there never truly was one way of issuing a signed request. The nonce & timestamp were widely recognised as being part of a signed request but in truth nearly every signed request also needs an
sdkIdsomehow. Some endpoints have that in the request path, some in a Header, some as ansdkIdquery param and some as anappIdquery param. It seems likely that most of these endpoints will not need thesdkIdany more when they come to handle auth token requests.All of the existing clients (& services) in the SDK use the
SignedRequestBuilder. We did look at simply providing an alternativexxxRequestBuilderbut that required a huge amount of duplication - even all the paths have to be redefined or messed about with thesdkIdis so deeply ingrained.So we've invented the
AuthStrategyinterface, and made theSignedRequestBuilderinto aYotiHttpRequestBuilder. The implementations ofAuthStrategy- named for each service they're calling - will apply the correct headers & query params for either a signed request or an auth token request.We see this working fine for the Docs endpoints but the connect bits ought to be looked at @irotech (this will all need a thorough regression test anyway before it goes out).