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.

Sign up for GitHub

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

Manual nonce generation option #244

Closed
wants to merge 5 commits into from
Closed

Manual nonce generation option #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a593b4f
feat(sign-in): configure sign in request with external nonce value
jsamol Jul 14, 2021
d0cf7cd
fix: compile
toshi0383 Oct 25, 2022
6673687
Merge branch 'main' into main
toshi0383 Oct 27, 2022
b0543cd
Merge remote-tracking branch 'upstream/main'
toshi0383 Jan 25, 2023
76acf9c
Fix conflicts
toshi0383 Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions GoogleSignIn/Sources/GIDConfiguration.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
// The key for the openIDRealm property to be used with NSSecureCoding.
static NSString *const kOpenIDRealmKey = @"openIDRealm";

// The key for the nonce property to be used with NSSecureCoding.
static NSString *const kNonceKey = @"nonce";

NS_ASSUME_NONNULL_BEGIN

@implementation GIDConfiguration
Expand All @@ -34,41 +37,46 @@ - (instancetype)initWithClientID:(NSString *)clientID {
return [self initWithClientID:clientID
serverClientID:nil
hostedDomain:nil
openIDRealm:nil];
openIDRealm:nil
nonce:nil];
}

- (instancetype)initWithClientID:(NSString *)clientID
serverClientID:(nullable NSString *)serverClientID {
return [self initWithClientID:clientID
serverClientID:serverClientID
hostedDomain:nil
openIDRealm:nil];
openIDRealm:nil
nonce:nil];
}

- (instancetype)initWithClientID:(NSString *)clientID
serverClientID:(nullable NSString *)serverClientID
hostedDomain:(nullable NSString *)hostedDomain
openIDRealm:(nullable NSString *)openIDRealm {
openIDRealm:(nullable NSString *)openIDRealm
nonce:(nullable NSString *)nonce {
self = [super init];
if (self) {
_clientID = [clientID copy];
_serverClientID = [serverClientID copy];
_hostedDomain = [hostedDomain copy];
_openIDRealm = [openIDRealm copy];
_nonce = [nonce copy];
}
return self;
}

// Extend NSObject's default description for easier debugging.
- (NSString *)description {
return [NSString stringWithFormat:
@"<%@: %p, clientID: %@, serverClientID: %@, hostedDomain: %@, openIDRealm: %@>",
@"<%@: %p, clientID: %@, serverClientID: %@, hostedDomain: %@, openIDRealm: %@, nonce: $@>",
NSStringFromClass([self class]),
self,
_clientID,
_serverClientID,
_hostedDomain,
_openIDRealm];
_openIDRealm,
_nonce];
}

#pragma mark - NSCopying
Expand All @@ -89,6 +97,7 @@ - (nullable instancetype)initWithCoder:(NSCoder *)coder {
NSString *serverClientID = [coder decodeObjectOfClass:[NSString class] forKey:kServerClientIDKey];
NSString *hostedDomain = [coder decodeObjectOfClass:[NSString class] forKey:kHostedDomainKey];
NSString *openIDRealm = [coder decodeObjectOfClass:[NSString class] forKey:kOpenIDRealmKey];
NSString *nonce = [coder decodeObjectOfClass:[NSString class] forKey:kNonceKey];

// We must have a client ID.
if (!clientID) {
Expand All @@ -98,14 +107,16 @@ - (nullable instancetype)initWithCoder:(NSCoder *)coder {
return [self initWithClientID:clientID
serverClientID:serverClientID
hostedDomain:hostedDomain
openIDRealm:openIDRealm];
openIDRealm:openIDRealm
nonce:nonce];
}

- (void)encodeWithCoder:(NSCoder *)coder {
[coder encodeObject:_clientID forKey:kClientIDKey];
[coder encodeObject:_serverClientID forKey:kServerClientIDKey];
[coder encodeObject:_hostedDomain forKey:kHostedDomainKey];
[coder encodeObject:_openIDRealm forKey:kOpenIDRealmKey];
[coder encodeObject:_nonce forKey:kNonceKey];
}

@end
Expand Down
3 changes: 2 additions & 1 deletion GoogleSignIn/Sources/GIDGoogleUser.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ - (GIDConfiguration *)configuration {
_cachedConfiguration = [[GIDConfiguration alloc] initWithClientID:clientID
serverClientID:serverClientID
hostedDomain:[self hostedDomain]
openIDRealm:openIDRealm];
openIDRealm:openIDRealm
nonce:nil];
};
}
return _cachedConfiguration;
Expand Down
24 changes: 18 additions & 6 deletions GoogleSignIn/Sources/GIDSignIn.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ - (void)signInWithPresentingViewController:(UIViewController *)presentingViewCon
- (void)addScopes:(NSArray<NSString *> *)scopes
presentingViewController:(UIViewController *)presentingViewController
completion:(nullable GIDSignInCompletion)completion {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove space.

GIDConfiguration *configuration = self.currentUser.configuration;
GIDSignInInternalOptions *options =
[GIDSignInInternalOptions defaultOptionsWithConfiguration:configuration
Expand Down Expand Up @@ -570,21 +571,31 @@ - (void)authenticateInteractivelyWithOptions:(GIDSignInInternalOptions *)options

#if TARGET_OS_IOS && !TARGET_OS_MACCATALYST
[additionalParameters addEntriesFromDictionary:
[GIDEMMSupport parametersWithParameters:options.extraParams
emmSupport:emmSupport
isPasscodeInfoRequired:NO]];
[GIDEMMSupport parametersWithParameters:options.extraParams
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fix indentation.

emmSupport:emmSupport
isPasscodeInfoRequired:NO]];
#elif TARGET_OS_OSX || TARGET_OS_MACCATALYST
[additionalParameters addEntriesFromDictionary:options.extraParams];
#endif // TARGET_OS_OSX || TARGET_OS_MACCATALYST
additionalParameters[kSDKVersionLoggingParameter] = GIDVersion();
additionalParameters[kEnvironmentLoggingParameter] = GIDEnvironment();

NSString *codeVerifier = [OIDAuthorizationRequest generateCodeVerifier];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part is a little tricky for me because it looks like GSI is taking on some of the implementation details within AppAuth-iOS. IIUC, we have to create codeVerifier, codeChallenge, and the nonce because we don't have an initializer that we can easily call on OIDAuthorizationRequest that takes only the nonce.

Ideally, we would have an initializer on OIDAuthorizationRequest that only adds the nonce parameter to the simplest initializer. That way, we can keep these implementation details/default values inside AppAuth.

What do you think?

NSString *codeChallenge = [OIDAuthorizationRequest codeChallengeS256ForVerifier:codeVerifier];
NSString *nonce = options.configuration.nonce ? options.configuration.nonce : [OIDAuthorizationRequest generateState];

OIDAuthorizationRequest *request =
[[OIDAuthorizationRequest alloc] initWithConfiguration:_appAuthConfiguration
clientId:options.configuration.clientID
scopes:options.scopes
clientSecret:nil
scope:[OIDScopeUtilities scopesWithArray:options.scopes]
redirectURL:redirectURL
responseType:OIDResponseTypeCode
state:[OIDAuthorizationRequest generateState]
nonce:nonce
codeVerifier:codeVerifier
codeChallenge:codeChallenge
codeChallengeMethod:OIDOAuthorizationRequestCodeChallengeMethodS256
additionalParameters:additionalParameters];

_currentAuthorizationFlow = [OIDAuthorizationService
Expand Down Expand Up @@ -1031,13 +1042,14 @@ + (nullable GIDConfiguration *)configurationFromBundle:(NSBundle *)bundle {
forKey:kConfigServerClientIDKey];
NSString *hostedDomain = [GIDSignIn configValueFromBundle:bundle forKey:kConfigHostedDomainKey];
NSString *openIDRealm = [GIDSignIn configValueFromBundle:bundle forKey:kConfigOpenIDRealmKey];

// If we have at least a client ID, try to construct a configuration.
if (clientID) {
configuration = [[GIDConfiguration alloc] initWithClientID:clientID
serverClientID:serverClientID
hostedDomain:hostedDomain
openIDRealm:openIDRealm];
openIDRealm:openIDRealm
nonce:nil];
}

return configuration;
Expand Down
6 changes: 5 additions & 1 deletion GoogleSignIn/Sources/Public/GoogleSignIn/GIDConfiguration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ NS_ASSUME_NONNULL_BEGIN
/// Identifier in the OpenID Connect ID token.
@property(nonatomic, readonly, nullable) NSString *openIDRealm;

/// A value generated by the app that enables replay protection.
@property(nonatomic, readonly, nullable) NSString *nonce;

/// Unavailable. Please use `initWithClientID:` or one of the other initializers below.
/// :nodoc:
+ (instancetype)new NS_UNAVAILABLE;
Expand Down Expand Up @@ -69,7 +72,8 @@ NS_ASSUME_NONNULL_BEGIN
- (instancetype)initWithClientID:(NSString *)clientID
serverClientID:(nullable NSString *)serverClientID
hostedDomain:(nullable NSString *)hostedDomain
openIDRealm:(nullable NSString *)openIDRealm NS_DESIGNATED_INITIALIZER;
openIDRealm:(nullable NSString *)openIDRealm
nonce:(nullable NSString *)nonce NS_DESIGNATED_INITIALIZER;

@end

Expand Down
3 changes: 2 additions & 1 deletion GoogleSignIn/Tests/Unit/GIDConfiguration+Testing.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ + (instancetype)testInstance {
return [[GIDConfiguration alloc] initWithClientID:OIDAuthorizationRequestTestingClientID
serverClientID:kServerClientID
hostedDomain:kHostedDomain
openIDRealm:kOpenIDRealm];
openIDRealm:kOpenIDRealm
nonce:nil];
}

@end
19 changes: 11 additions & 8 deletions GoogleSignIn/Tests/Unit/GIDSignInTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,10 +639,11 @@ - (void)testAddScopes {
}

- (void)testOpenIDRealm {
_signIn.configuration = [[GIDConfiguration alloc] initWithClientID:kClientId
serverClientID:nil
hostedDomain:nil
openIDRealm:kOpenIDRealm];
_signIn._configuration = [[GIDConfiguration alloc] initWithClientID:kClientId
serverClientID:nil
hostedDomain:nil
openIDRealm:kOpenIDRealm
nonce:nil];

[self OAuthLoginWithAddScopesFlow:NO
authError:nil
Expand Down Expand Up @@ -674,10 +675,12 @@ - (void)testOAuthLogin_LoginHint {
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that GIDSignInTest is cluttered and that GIDSignIn is a difficult class to test (we're working on this), but could you add a test that verifies the passed in nonce is matched in the OIDAuthorizationResponse we back in GIDSignIn?

It'd be great to have a unit test that asserts: XCTAssertEqualObjects(originalNonce, authorizationResponse.request.nonce).

- (void)testOAuthLogin_HostedDomain {
_signIn.configuration = [[GIDConfiguration alloc] initWithClientID:kClientId
serverClientID:nil
hostedDomain:kHostedDomain
openIDRealm:nil];

_signIn._configuration = [[GIDConfiguration alloc] initWithClientID:kClientId
serverClientID:nil
hostedDomain:kHostedDomain
openIDRealm:nil
nonce:nil];

[self OAuthLoginWithAddScopesFlow:NO
authError:nil
Expand Down