-
Notifications
You must be signed in to change notification settings - Fork 130
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
Allow Auth credentials to be fetched via callback, backwards compatible #62
Conversation
FOr OIDC or Cognito authentication, auth tokens often expire and need to be re-generated. Using a callback follows the model also used by aws-mobile-appsync-sdk-js
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.
Great job! Just a few minor comments.
sendRequestWithAuth(mutableRequest: mutableRequest, sendRequest: sendRequest) | ||
} | ||
|
||
|
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.
remove extra blank lines
if let prov = self.oidcAuthProvider as? AWSOIDCAuthProviderAsync { | ||
|
||
prov.getLatestAuthToken { token in | ||
if !token.isEmpty { |
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.
What will happen if the token is empty? will the header be not set?
sendRequest( mutableRequest as URLRequest) | ||
} | ||
} else if let prov = self.oidcAuthProvider { | ||
mutableRequest.setValue(prov.getLatestAuthToken(), forHTTPHeaderField: "authorization") |
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.
seems like in async we only set header when token is not empty and we are setting header here irrespective of the token being empty or not.
} else if let prov = self.oidcAuthProvider { | ||
mutableRequest.setValue(prov.getLatestAuthToken(), forHTTPHeaderField: "authorization") | ||
} else { | ||
fatalError("Authentication provide not set") |
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.
provide -> provider
} else { | ||
fatalError("Authentication provide not set") | ||
} | ||
case .amazonCognitoUserPools: |
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.
same 3 comments for OIDC applies here.
@@ -0,0 +1,195 @@ | |||
// | |||
// Copyright 2010-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
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.
2018 or 2018-2018
callback("CognitoUserPoolsAuthProviderAsync") | ||
self.expectation?.fulfill() | ||
} | ||
|
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.
remove blank line
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.
fixed
Thank you @JohnRbk for submitting a new PR. I have given a few comments. |
@kvasukib great feedback. Thank you. After thinking about it, the use of a blank default string in the Async classes causes a silent error. Its not great, but a fatalError is probably a better solution. I ran a few more tests and have pushed my latest commits for review. |
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.
Great job! Thank you for addressing the comments! I have approved the changes. I'll wait on @rohandubal to give a final approval.
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.
This is great! I only have one comment which I feel should be an important consideration(probably also a flaw with the sync pattern): We should have the signature of async function as (String?, Error?) -> Void
rather than just (String) -> Void
This would help the handler also pass in error object when the call fails and we can propagate it to the caller. Thoughts?
@JohnRbk will you have the bandwidth to make that change? If not, I might have to add some changes on top of these changes before we can draft a release. Thanks again for the PR!
@rohandubal Please take a look at my updates and unit test. And yes, you're right that the sync callbacks should likely also have a similar way to propagate an error, but that could be a breaking change. |
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.
This is great! One thing which I am going to do though is add a separate user pools async auth provider to maintain separate auth providers for oidc vs userpools (even though it is duplication)
This is an updated to an earlier PR that provides a backward compatible way for fetching auth tokens in a synchronous or asynchronous way.
Description of changes:
When using a CognitoUserPoolsAuthProvider or AWSOIDCAuthProvider, a client can create an auth provides that alternatively uses a callback:
Using AWSCognitoUserPoolsAuthProvider instead:
See the AuthProviderTests.swift file for some examples. Also see this demo project which shows the OIDCToken generated via a network call.
In order to preserve backwards compatibility, the original AuthProvider classes were preserved and a new protocol was introduced
AWSOIDCAuthProviderAsync
which describes the callback required for classes to implement.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.