Skip to content
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

Fix passing nil for nonce and add example #476

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

mdmathias
Copy link
Collaborator

This PR fixes an issue introduced by #402 related to AppAuth's use of a nonce, fixes an issue in GSI's -[GIDSignIn maybeFetchToken:], and provides an example in the DaysUntilBirthday app.

Issue in #402

#402 provides the ability to pass a manual nonce via GSI to AppAuth (e.g., via this OIDAuthorizationRequest initializer that takes a nonce parameter).

Unfortunately, this addition to GSI breaks AppAuth's automatic use of its own nonce if one is not supplied since #402 will always pass through -[GIDSignInOptions nonce]. So, if the nonce in GIDSignInOptions is nil, then nil will be passed to AppAuth for the nonce, and therefore AppAuth will not produce its own nonce via this initializer.

Fixing -[GIDSignIn maybeFetchToken:]

-maybeFetchToken previously used a method on AppAuth (+[OIDAuthorizationService performTokenRequest:callback:])
that would pass nil for the last authorization response. The consequence of this is that the last authorization response's nonce will not be available for comparison within AppAuth since the last auth response and its nonce would be nil in the comparison.

This PR fixes the issue by always passing along the last authorization response in -[GIDSignIn maybeFetchToken:].

DaysUntilBirthdayExample

This PR also provides an example of the client passing in a manual nonce, and then comparing the received nonce in the authorization response to the manual nonce. It provides a simple example that clients may use to develop their own solution for comparing their manual nonce to the authorization's returned nonce

@mdmathias
Copy link
Collaborator Author

Cc @vonovak (author of #402) for your thoughts and review.

Copy link
Contributor

@vonovak vonovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, apologies for missing this. The changes look good to me!

GoogleSignIn/Sources/GIDSignIn.m Outdated Show resolved Hide resolved
@mdmathias
Copy link
Collaborator Author

I'm going to merge since integration tests are passing locally. Seems like something is awry in the workflow/GitHub runner...

@mdmathias mdmathias merged commit 72b9e63 into main Oct 3, 2024
11 checks passed
@mdmathias mdmathias deleted the mdmathias/fix-manual-nonce branch October 3, 2024 19:49
@riderx
Copy link

riderx commented Oct 4, 2024

@mdmathias do we know in what version it will be available?

@vonovak
Copy link
Contributor

vonovak commented Oct 17, 2024

@mdmathias I would also love to see this released. When can we expect it out?

It's been one year since my nonce PR in AppAuth openid/AppAuth-iOS#788 was merged. Would really appreciate it out.

Thanks so much!

@brnnmrls
Copy link
Member

Hi everyone! Inserting myself in here, and thanks for bringing it to our attention.

We're planning to include this in the next minor release, which we expect to roll out in the next month or two. Additionally we have a minor issue in the workflow/GitHub runner that we're addressing, but this should not a blocker.

Hope this brings some clarity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants