SDK-less Google SignIn – Part 5 – Connect the pieces

[mokagio]

There's still work to do (#741) but, at last, here's the PR that puts all the pieces together.

You can test the behavior by running the demo app and going through the various login options.

---

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[mokagio]

I need to double check this... The WordPress backend should be handling this for us already. Not sure whether it makes sense to handle creating an account in the context of the demo app.

[jkmassel]

FWIW the demo app shouldn't have the capability to create a new user anyway – that's restricted based on the OAuth token granted to the WP/JP apps.

If this flow allows it then sure, I guess, but I'd say for the purposes of the demo app it's ok to say "you need to have an account" instead of implementing the signup flow?

I know this project also contains signup code, and I think for that case we could modify the back-end a little to make a public set of OAuth credentials and special-case the server to respond with something like "if this were a real app, this would've worked, but it's not so here's a real-looking response you can use to test the flow but please don't try to do anything useful with it"

[mokagio]

@jkmassel

FWIW the demo app shouldn't have the capability to create a new user anyway – that's restricted based on the OAuth token granted to the WP/JP apps.

With the above, do you mean that our backend should only allow creating new users with a token that has been generated using the Google apps (i.e. the client id, secret, and audience) for WordPress and Jetpack?

I hadn't considered that, but I think it makes sense. Also, I think we can safely test the new user creation in the context of Jetpack/WordPress and add a notice in the demo app that says "you should have a valid user already", as you suggested.

We can always revisit later on in case we decided to do some work around signup.

[mokagio]

I tweaked the presentation and method name in the demo app to make it clearer this code path only tests getting the token from Google.

See 485b906

[jkmassel]

our backend should only allow creating new users with a token that has been generated using the Google apps (i.e. the client id, secret, and audience) for WordPress and Jetpack?

IINM, do the OAuth dance and get a token from Google, which we can pass to WP.com to authenticate as the given user. When we perform that request, we use the WP.com OAuth clientID/secret to authenticate as the app (but not as any user because we aren't logged in at that point). That WP.com clientID/secret is what is allowed to create new users.

Does that explain it better? 😅

[mokagio]

Gotcha 👍

[mokagio]

I created a new object to keep things tidy and made it public to use it in the demo app.

However, this code could, and probably should, be folded into GoogleAuthenticator.

I like the idea of exposing the building blocks for authentication in the library, so that consumers can decide whether to use the UI the library offers, or roll their own. But one could argue that's a bigger design change in the library and it ought to be considered and implemented outside the scope of the SDK-less Google SignIn work.

[jkmassel]

But one could argue that's a bigger design change in the library and it ought to be considered and implemented outside the scope of the SDK-less Google SignIn work.

I don't mind if this is a special case tbh – there are very very few consumers of this library, so I think to say "third party auth providers are different" is reasonable – especially since SIWA is like one button and a callback 😂

[mokagio]

Keen on ideas on this. But it could also be that it's okay to start the session on the main thread from here.

[jkmassel]

I think the only way to get around this would be to write a callback-based version of this method that uses an assertion to ensure it's running on the main thread. Then from there, wrap that in withCheckedThrowingContinuation and mark the parent method as @MainThread – I'm not 100% sure that'll work because I don't know if withCheckedThrowingContinuation guarantees that it runs its code on the parent Task context though.

Alternatively, you could look at a delegate-based approach which would let you make guarantees about which thread that call runs on, but it'd be more complex.

[mokagio]

I just tried this without the DispatchQueue.main.async in WordPress and got the same kind of error I got with the demo app

=================================================================
Main Thread Checker: UI API called on a background thread: -[UIViewController view]
PID: 25962, TID: 52550464, Thread name: (none), Queue name: com.apple.root.user-initiated-qos.cooperative, QoS: 25
Backtrace:
4   WordPress                           0x0000000105261db0 $s22WordPressAuthenticator24GoogleAuthViewControllerC18presentationAnchor3forSo8UIWindowCSo26ASWebAuthenticationSessionC_tF + 132
5   WordPress                           0x0000000105261f24 $s22WordPressAuthenticator24GoogleAuthViewControllerC18presentationAnchor3forSo8UIWindowCSo26ASWebAuthenticationSessionC_tFTo + 52
6   AuthenticationServices              0x000000011033a558 -[ASWebAuthenticationSession _startDryRun:] + 92
7   WordPress                           0x00000001052d8fc0 $s22WordPressAuthenticator09NewGoogleC0C6getURL8clientId6scheme4pkce10Foundation0G0VAA0e6ClientI0V_SSAA23ProofKeyForCodeExchangeVtYaKFyScCyAJs5Error_pGXEfU_ + 512

I'd say for the time being it should be safer to keep the dispatch call in here and document why we did so.

[mokagio]

Dropped a comment about this with ecd5888

[mokagio]

@jkmassel @crazytonyli this is still a draft because it's based on a not yet merged branch, but I'd love your feedback anyway.

[jkmassel]

Left some thoughts – I hope they're helpful?

[jkmassel]

FWIW the demo app shouldn't have the capability to create a new user anyway – that's restricted based on the OAuth token granted to the WP/JP apps.

If this flow allows it then sure, I guess, but I'd say for the purposes of the demo app it's ok to say "you need to have an account" instead of implementing the signup flow?

I know this project also contains signup code, and I think for that case we could modify the back-end a little to make a public set of OAuth credentials and special-case the server to respond with something like "if this were a real app, this would've worked, but it's not so here's a real-looking response you can use to test the flow but please don't try to do anything useful with it"

[jkmassel]

Does presentAlert give us any guarantees that it runs on the main thread?

[mokagio]

I couldn't find anything in the docs about whether the UIKit method runs on the main thread.

This is a custom method so we can definitely force it to run on the main thread.

However, I'm not sure why we would go to "such effort" in the context of the demo app? What am I missing?

FWIW, TSan doesn't complain in the successful branch of the do catch.

[jkmassel]

But one could argue that's a bigger design change in the library and it ought to be considered and implemented outside the scope of the SDK-less Google SignIn work.

I don't mind if this is a special case tbh – there are very very few consumers of this library, so I think to say "third party auth providers are different" is reasonable – especially since SIWA is like one button and a callback 😂

[jkmassel]

I think the only way to get around this would be to write a callback-based version of this method that uses an assertion to ensure it's running on the main thread. Then from there, wrap that in withCheckedThrowingContinuation and mark the parent method as @MainThread – I'm not 100% sure that'll work because I don't know if withCheckedThrowingContinuation guarantees that it runs its code on the parent Task context though.

Alternatively, you could look at a delegate-based approach which would let you make guarantees about which thread that call runs on, but it'd be more complex.

[mokagio]

Thank you @jkmassel and @crazytonyli for the great feedback along the way. This should now be ready for one final look. 🙇‍♂️

[crazytonyli]

This onSynced is not called anywhere.

[mokagio]

Good eyes!

Addressed in d1b6c91

Thanks 🙇‍♂️

[crazytonyli]

I'm wondering if it'd be better to move the viewController initializer argument to here as a function argument. It appears we don't need to hold on to the view controller instance across the whole life of the NewGoogleAuthenticator instance? And something like googleAuthenticator.getOAuthToken(from: viewController) seems like better indicating that a view controller will be used to present the authentication flow.

[mokagio]

That's along what I was thinking here:

WordPressAuthenticator-iOS/WordPressAuthenticator/Unified Auth/GoogleAuthenticator.swift

Lines 310 to 322 in d1b6c91

	// We might want to change this in subsequent iterations, perhaps by moving the
	// `contextProvider` to the `getOAuthToken()` method so to allow it to be stored
	// as a `lazy` property?
	let sdkLessGoogleAuthenticator = NewGoogleAuthenticator(
	clientId: authConfig.googleClientId,
	scheme: authConfig.googleLoginScheme,
	audience: authConfig.googleLoginServerClientId,
	contextProvider: WebAuthenticationPresentationContext(viewController: viewController),
	urlSession: .shared
	)
	await SVProgressHUD.show()
	return try await sdkLessGoogleAuthenticator.getOAuthToken()

If that's okay with you, I'd like to tackle it in a dedicated PR

[mokagio]

Addressed in #755

[crazytonyli]

We probably should make the OAuthError public, so that when the app captures the error thrown by the NewGoogleAuthenticator.getOAuthToken, they cast the error to the correct type and handle it accordingly.

[mokagio]

Good point. I addressed it in 3c49abd.

so that when the app captures the error thrown by the NewGoogleAuthenticator.getOAuthToken

Worth pointing out that, for the time being, no app will call this directly. Still, it's handy to be ready for when / if they'll want to.

[crazytonyli]

Similar to https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/743/files#r1110446829, it's probably safer to run this Task in the main actor (i.e. adding a @MainActor annotation: Task { @MainActor in ), so that the didSignIn function below are guaranteed to be called from the main thread.

[mokagio]

Good point.

Addressed in 76fd3c4. Thanks!