feat: allow providing custom nonce #402
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
vonovak
force-pushed
the
feat/custom-nonce
branch
from
f81e466
to
699a005
Compare
|
@vonovak did you sign the CLA? I can take a look at the PR after you confirm. |
|
hello @mdmathias yes I did! :) 
|
| @@ -64,6 +64,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
| /// The login hint to be used during the flow. | |||
| @property(nonatomic, copy, nullable) NSString *loginHint; | |||
|
|
|||
| /// A cryptographically random value used to associate a Client session with an ID Token, and to mitigate replay attacks. | |||
There was a problem hiding this comment.
Does this doc comment exceed 100 chars? If so, can you wrap to the next line?
| @@ -64,6 +64,9 @@ NS_ASSUME_NONNULL_BEGIN | |||
| /// The login hint to be used during the flow. | |||
| @property(nonatomic, copy, nullable) NSString *loginHint; | |||
|
|
|||
| /// A cryptographically random value used to associate a Client session with an ID Token, and to mitigate replay attacks. | |||
| @property(nonatomic, copy, nullable) NSString *nonce; | |||
There was a problem hiding this comment.
Should this be read only?
| @@ -77,6 +80,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
| loginHint:(nullable NSString *)loginHint | |||
| addScopesFlow:(BOOL)addScopesFlow | |||
| scopes:(nullable NSArray *)scopes | |||
| nonce:(nullable NSString *)nonce | |||
There was a problem hiding this comment.
Let's add a method so that we do not break existing API and force major version release.
| @@ -91,6 +95,7 @@ NS_ASSUME_NONNULL_BEGIN | |||
| loginHint:(nullable NSString *)loginHint | |||
| addScopesFlow:(BOOL)addScopesFlow | |||
| scopes:(nullable NSArray *)scopes | |||
| nonce:(nullable NSString *)nonce | |||
There was a problem hiding this comment.
Same: let's add a method instead of changing existing API.
| additionalScopes:@[] | ||
| manualNonce:manualNonce]; | ||
|
|
||
| XCTAssertEqualObjects(_savedAuthorizationRequest.nonce, manualNonce, @"nonce provided to signInWithPresenting[ViewController/Window] should be the same as the one in the authorization request."); |
There was a problem hiding this comment.
Please follow the style here use line breaks on the arguments.
Uses: openid/AppAuth-iOS#788. Motivation is explained there and also in issue #135
Fixes: #135
Supersedes: #244. It takes a slightly different approach where nonce is not provided via GIDConfiguration but via a parameter to
signInWithPresentingViewController/WindowI'm leaving for a vacation soon, so if anyone who needs this feels that I'm not responsive, please don't mention me, I will not be responding. I will address the review after I get back, on ~ 22 of May.