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.

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

add flow for checking previously used accounts #40

Conversation

bom-bakbang
Copy link
Contributor

Hi! I'm Lukas, a South Korean Android Developer.
I tried using KMPAuth and find it very useful! Thanks for your work.

As far as I know, I could find there exists only one option for GoogleAuthCredentials: serverId,
which is used for building GetGoogleIdOption while other options are fixed in your android implementation like below.

private fun getGoogleIdOption(serverClientId: String): GetGoogleIdOption {
return GetGoogleIdOption.Builder()
.setFilterByAuthorizedAccounts(false)
.setAutoSelectEnabled(true)
.setServerClientId(serverClientId)
.build()

However, there exists an issue for crash in Credentials when you set other options like above: https://stackoverflow.com/questions/78538579/new-google-credential-manager-is-throwing-a-transactiontoolargeexception

I also got similar issue related to TransactionTooLargeException, because I already had SAMSUNG Credentials.
So I could solve the problem by set .setFilterByAuthorizedAccounts(true) first for filtering out SAMSUNG Credentials.

Also, when you see official recommendation at https://developer.android.com/identity/sign-in/credential-manager-siwg#enable-sign-up,
they guide to check whether there exist previously used accounts by set .setFilterByAuthorizedAccounts(true) first.
If there is no available accounts, it will throw NoCredentialException.

So I suggest adding flow for checking previously used accounts.
This is my first time to participate open source, so please let me know if there exist more steps I should know.

Sincerly, Lukas.

@mirzemehdi
Copy link
Owner

@bom-bakbang thank you for providing detailed info and your contribution, I requested some small changes. Please, check them

- move repeated logics to one function
- change parameter name
@bom-bakbang
Copy link
Contributor Author

@bom-bakbang thank you for providing detailed info and your contribution, I requested some small changes. Please, check them

Thanks for detail code review. Sorry for late correction changes, I got accident so I couldn't focus 😅.
I changed all your comments, please check again :)

@mirzemehdi mirzemehdi changed the base branch from main to rel_v2.1.0 October 5, 2024 15:57
Copy link
Owner

@mirzemehdi mirzemehdi left a comment

Choose a reason for hiding this comment

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

Thank you @bom-bakbang. LGTM. Great job!

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