Add email to GoogleUser #65
Conversation
| @@ -73,7 +73,8 @@ internal class GoogleAuthUiProviderImpl( | |||
| GoogleUser( | |||
| idToken = googleIdTokenCredential.idToken, | |||
| accessToken = null, | |||
| displayName = googleIdTokenCredential.displayName ?: "", | |||
| email = googleIdTokenCredential.id, | |||
| displayName = googleIdTokenCredential.displayName, | |||
There was a problem hiding this comment.
better to keep as it was before:
googleIdTokenCredential.displayName?:""
| @@ -52,7 +52,8 @@ internal class GoogleLegacyAuthentication( | |||
| GoogleUser( | |||
| idToken = idToken, | |||
| accessToken = null, | |||
| displayName = account.displayName ?: "", | |||
| email = account.email, | |||
| displayName = account.displayName, | |||
There was a problem hiding this comment.
better to keep as it was before:
account.displayName?:""
| val displayName: String = "", | ||
| val accessToken:String? = null, | ||
| val email: String? = null, | ||
| val displayName: String? = null, |
There was a problem hiding this comment.
don't want to break things with new update. So instead of nullable displayName, it should be non nullable as before,and should be set as empty string if it is null
There was a problem hiding this comment.
ok I see, I ll revert the update.
I think that you should consider this update when making major version increase because hiding nullability is not a good practice, with the current state it's not possible to distinguish the case when id token doesn't contain name from the case when user has actually an empty name. It's not a big difference but in general it's easier to let clients to handle nullability :)
There was a problem hiding this comment.
yes, good point. For major update I'll keep this in my mind. Thank you for pointing out that @doolle89
| @@ -27,7 +27,8 @@ internal class GoogleAuthUiProviderImpl : GoogleAuthUiProvider { | |||
| val googleUser = GoogleUser( | |||
| idToken = idToken, | |||
| accessToken = accessToken, | |||
| displayName = profile?.name ?: "", | |||
| email = user?.userID, | |||
| displayName = profile?.name, | |||
There was a problem hiding this comment.
better to keep as it was before:
profile.name?:""
| @@ -54,6 +54,7 @@ internal class GoogleAuthUiProviderImpl(private val credentials: GoogleAuthCrede | |||
| return null | |||
| } | |||
| val jwt = JWT().decodeJwt(idToken) | |||
| val email = jwt.getClaim("email")?.asString() // User's name | |||
There was a problem hiding this comment.
comment part -> //user's email
| @@ -65,7 +66,8 @@ internal class GoogleAuthUiProviderImpl(private val credentials: GoogleAuthCrede | |||
| return GoogleUser( | |||
| idToken = idToken, | |||
| accessToken = null, | |||
| displayName = name ?: "", | |||
| email = email, | |||
| displayName = name, | |||
There was a problem hiding this comment.
should be kept as before name?:""
There was a problem hiding this comment.
Thank you very much @doolle89 for this PR. Looks good to me. Just requested minor changes related to displayName field as I don't want to break things for new api
doolle89
force-pushed
the
google_user_email
branch
from
4a9b27a to
6f9ba7c
Compare
|
I updated points from the review... Just please test it if you can before merging, I can't test sample app, most probably because my keystore fingerprint is not matching the one in GPC |
|
Thank you @doolle89 for changes. After testing I'll merge it |
|
Thanks again @doolle89 for this, just tested, and published new release. This will be available in |
Is it ok to retrieve email directly from idToken on client device?
As pointed out in this issue email is sometimes omitted from idToken but it seems it is mandatory field in GoogleIdTokenCredential so we can get it from there