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

Support making nonce nullable #169

Merged
merged 9 commits into from
Jan 31, 2019

Conversation

FelipeBuiles
Copy link
Contributor

@FelipeBuiles FelipeBuiles commented Sep 3, 2018

AppAuth-iOS worked correctly with AWS Cognito up until its 0.92 release, later they added support that added support for a nonce parameter. The issue lies in the inabilty of Cognito to handle the nonce parameter (mentioned in their forums here).
There is a comment on the OIDAuthorizationRequest.h file from AppAuth-iOS that mentions something about it being nullable:

    @param nonce String value used to associate a Client session with an ID Token. Can be set to nil
        if not using OpenID Connect, although pure OAuth servers should ignore params they don't
        understand anyway.

The idea here is to add a new optional param useNonce that when set to false, tells the iOS implementation to not generate a nonce attribute automatically. This only happens on the iOS implementation, Android does not have this issue.

@FelipeBuiles FelipeBuiles changed the title Suport making nonce nullable Support making nonce nullable Sep 3, 2018
@@ -48,6 +49,10 @@ export const authorize = ({
];
if (Platform.OS === 'android') {
nativeMethodArguments.push(dangerouslyAllowInsecureHttpRequests);
} else {
// add a new useNonce param on iOS to support making it optional
const nonceParamIndex = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a bit fiddly, I'd prefer to just add it to the end as the android-only method above

nativeMethodArguments.push(useNonce);

@kadikraman
Copy link
Contributor

Hi @FelipeBuiles thanks a lot for the PR!

Since this is AWS Cognito specific, would you be able to add a config example (something like this).

Could you also please add the new parameter to the config.

@dreyks
Copy link

dreyks commented Oct 20, 2018

actually i have this same issue with Dropbox API so I have high hopes for seeing that merged

@dreyks
Copy link

dreyks commented Oct 20, 2018

ugh... tried @FelipeBuiles fork, now getting unknown field 'code_challenge'. FFS dropbox, why not follow the RFC?!

So this PR alone won't help me. Also need to turn off PKCE as described in openid/AppAuth-iOS#305 (comment)

@dreyks
Copy link

dreyks commented Oct 20, 2018

nvm. Dropbox refuses to redirect to urls not starting from https in responseType code. And token is not supported by AppAuth so I have to find another OAuth library

@nol13
Copy link

nol13 commented Dec 27, 2018

same issue moving from 2.4.1, so far so good with this fix.

Not cognito and possibly could update a config somewhere in back-end, but this is working for us.

might depend on our setup but the experience was the redirect not working on first attempt vs. a complete failure to authenticate.

@kadikraman kadikraman merged commit a83ce89 into FormidableLabs:master Jan 31, 2019
@kadikraman kadikraman mentioned this pull request Jan 31, 2019
4 tasks
@kadikraman kadikraman added the enhancement A new feature label Feb 1, 2019
@kadikraman kadikraman added this to the Next Release 🚀 milestone Feb 1, 2019
@kadikraman kadikraman modified the milestones: Next Major Release 🚀, Next Minor Release 🎉 Feb 18, 2019
@kadikraman
Copy link
Contributor

Thanks for your help! This has been published in v4.1.0.

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

Successfully merging this pull request may close these issues.

4 participants