Skip to content

Conversation

@AkshatG6
Copy link
Contributor

This pull request updates the existing implementation of GIDSignIn class and GIDSignInInternalOptions class.

Key changes:

  • Updates the signWith... initializers to provide public api's to request for tokenClaims.
  • Updates GIDSignInInternalOptions to store tokenClaims.
  • Updates the sign-in flow to include tokenClaims within the OIDAuthorizationRequest.
  • Provides unit tests to validate the implementation.

@AkshatG6 AkshatG6 requested a review from mdmathias September 10, 2025 16:24
}
}

- (BOOL)processTokenClaimsForOptions:(GIDSignInInternalOptions *)options
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may be following an existing pattern in this file, but this method "works" by relying on side effect behavior, which is not great. The side effect, setting the token claims JSON on the passed options, is separated from the return, a BOOL. IMHO side effects like this make code harder to read (e.g., in the first snippet pasted below).

So, you're code above would go from:

// If tokenClaims are invalid or JSON serialization fails, return with an error.
if (![self processTokenClaimsForOptions:options error:&claimsError]) {
  if (options.completion) {
    self->_currentOptions = nil;
    dispatch_async(dispatch_get_main_queue(), ^{
      options.completion(nil, claimsError);
    });
  }
  return;
}

To this:

// If tokenClaims are invalid or JSON serialization fails, return with an error.
options.tokenClaimsAsJSON = [self processTokenClaimsForOptions:options error:&claimsError])
if (options.completion && /* Check that token claims are good on options; otherwise, set completion with error */) {
  self->_currentOptions = nil;
  dispatch_async(dispatch_get_main_queue(), ^{
    options.completion(nil, claimsError);
  });
  return;
}

Wouldn't it be simpler to just return the token claims json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. I have updated the function accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we even need processTokenClaimsForOptions.


if (tokenClaimsError) {
XCTestExpectation *tokenClaimsErrorExpectation =
[self expectationWithDescription:@"Callback called"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not reuse "Callback called" since it's extremely vague. Please use something more descriptive like "Token claims error expectation."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this expectation altogether.

}

if (tokenClaimsError) {
XCTestExpectation *tokenClaimsErrorExpectation =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused by this...you make an expectation and then fulfill it right away? Can you help me to understand what is going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated this condition.

@AkshatG6 AkshatG6 requested a review from mdmathias September 11, 2025 00:15
@AkshatG6 AkshatG6 merged commit 0762758 into gandhiakshat/sbc-auth_time Sep 12, 2025
14 of 16 checks passed
@AkshatG6 AkshatG6 deleted the gandhiakshat/sbc-token_claim-public-sign_in-api branch September 12, 2025 21:44
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.

2 participants