-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Improve Google Sign In error logging #25446
Improve Google Sign In error logging #25446
Conversation
Errors that didn't have specific handling weren't being logged properly. Update logging and print all status codes so we can also see errors we aren't specifically handling.
@shawnborton @0xmiroslav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are fine, can't test until tomorrow morning though
@lindboe we had a temporary issue with our AdHoc builds. Would you mind pulling main and merging it into your PR? Thanks |
Merged main 👍 |
Run adhoc build again, the new QR codes will be available soon |
🧪🧪 Use the links below to test this build in android and iOS. Happy testing! 🧪🧪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol. Thanks, GSO, very helpful!
At least we have a code number:
[alrt] [Google Sign In] Error Code: 10. DEVELOPER_ERROR - {}
Might be related to sha key mismatch |
Perhaps we just need to add the AdHoc signing key to Google Cloud for the AdHoc app. Though I'm not sure if that even exists :/ |
@Julesssss A signing key of some kind definitely exists, but it may not work the same way as prod. Debug mode builds are given a generated key (https://developer.android.com/studio/publish/app-signing#debug-mode), so if adhoc builds are in debug mode, (which I would be surprised by, but it's not impossible), then the debug cert might be different per machine and I suppose that could cause problems? At the same time, I don't think that's likely, as that would cause issues testing debug development builds locally, which we've been able to do just fine with Google Sign In on Android. But otherwise, a key of some kind does need to be provided for release builds. When I looked last, the SHA-1 of the ad-hoc build I had was Are you able to access the Google cloud console to change the SHA-1 value for the adhoc app's OAuth config? Can't really do any harm to test new values if the current one isn't working. |
Thanks for explaining. Yeah, I'll create an issue for trying that AdHoc key. |
I fixed the AdHoc SSO issue in the Google Console, but I think this improvement could still be merged to simplify debugging of future issues |
I'm going to merge this. The PR checklist has changed since it was added, so I'm going to ignore that check. |
@Julesssss looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.84-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.85-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
Details
@Julesssss reported that Google Sign-In was not working on ad hoc builds: https://expensify.slack.com/archives/C01GTK53T8Q/p1692302447001879
Errors that didn't have specific handling weren't being logged properly. Update logging and print all status codes so we can also see errors we aren't specifically handling.
Fixed Issues
$ #7079
PROPOSAL: #7079 (comment)
Tests
if
branch for the google sign-in error insrc/components/SignInButtons/GoogleSignIn/index.native.js
so that all errors use theLog.alert
statement insteadOffline tests
n/a, not affected by offline status.
QA Steps
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
n/a, no user-facing changes