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

signupNewUser implementation #33

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

JacobCube
Copy link

Hey!
I'm currently working on a compose multiplatform project and need this EP to be implemented, so here it is.
I pretty much just copied the solution for the verifyPassword, so there shouldn't be anything new or pricky.

@nbransby
Copy link
Member

Nice work! Can we get a test for it?

@JacobCube
Copy link
Author

I don't see any similar test, what do you have in mind?

@nbransby
Copy link
Member

Is there one in the https://github.com/GitLiveApp/firebase-kotlin-sdk maybe you can lift it from there

simple tests added for createUserWithEmailAndPassword, signInWithEmailAndPassword, and signInAnonymously.
@JacobCube JacobCube requested a review from nbransby September 13, 2024 17:23
@JacobCube
Copy link
Author

JacobCube commented Sep 13, 2024

I've added the email property in order to properly run the sign in test. Please, have a look @nbransby .

@nbransby
Copy link
Member

Thanks for the reminder I will try and have a look this weekend

@JacobCube
Copy link
Author

JacobCube commented Sep 30, 2024

Just a friendly reminder this PR is waiting for approval :-), and issue 17 is directly resolved by this.

@nbransby
Copy link
Member

nbransby commented Oct 5, 2024

Tests are running now but failing, also we should add tests to verify the email property is never an empty string - of course the tests will fail with the current implementation but then you'll need to get them passing because if we add the email property it will need to work 100%

@nbransby
Copy link
Member

nbransby commented Oct 5, 2024

this is the behavior we would need to verify:

getEmail

public abstract @Nullable String getEmail()

Returns the main email address of the user, as stored in the Firebase project's user database. Unlike the email property from instances of UserInfo corresponding to authentication providers (like Github), which is not modifiable, this email address can be updated at any time by calling updateEmail.

This field will be automatically populated on account creation if the AuthCredential used on signInWithCredential contained such information, or if the account was created with createUserWithEmailAndPassword. However, this is not true if the setting "Multiple Accounts per Email" is enabled in the Firebase Console - in that case this will be null unless the account was created with createUserWithEmailAndPassword or updateEmail has been called.

If the user also has a password, this email address can be used to sign in into the account using signInWithEmailAndPassword.

This email address is displayed in the Users section of the Firebase console.

@JacobCube
Copy link
Author

I'll allocate the time and implement it soon

@JacobCube
Copy link
Author

So I implemented a couple more things to make the email property a bit more complete. Feel free to give feedback on that, then. Other than that, ActionCodeSettings and signInWithCredential can be implemented in the future, as there is other unrelated stuff to be done. That doesn't mean I can't contribute to its implementation in the future, though, just don't think this PR needs to implement the whole deal ✌️.

@nbransby
Copy link
Member

Thanks @JacobCube. Looks like there is a conflict to resolve.

I think we need a test for signing in with a custom token and then testing the email matches because looking at the code in this scenario it would appear they would get null even if there was an email set on the user. Would be good to run the same test on android first and verify the behavior as its not clear from the docs, if it does return the correct email on android I believe that must mean they make a second request to the REST api to populate the users email on sign in with custom token.

@JacobCube
Copy link
Author

JacobCube commented Oct 30, 2024

I may look into it soon, I recently found out that implementing pictureURL might be helpful for our KMP project as well, so I might as well implement that here if no one objects:-). I saw an REST API for the custom token, I just didn't want to bother with all the various tokens, but I can implement what will be readily available. Thank you for the feedback @nbransby.

@JacobCube
Copy link
Author

JacobCube commented Nov 7, 2024

Hey, unfortunately, I'm unable to write a test for signInWithCustomToken, as creating a custom token requires Admin SDK on BE. I could make a Firebase Function for that on this project, but it's unavailable due to the project plan (Spark). I'll just add a call to retrieve the user information to make it full.

@nbransby any tips or ideas?

@nbransby
Copy link
Member

nbransby commented Nov 7, 2024

Maybe its possible to get the test running locally via the emulator? https://firebase.google.com/docs/emulator-suite/connect_auth#emulated_custom_token_authentication

Also it would make sense to add the test to the firebase-kotlin-sdk, then it can also confirm the android behavior (if the android sdk doesn't populate the email address on signInWithCustomToken then we don't need to support it in java either)

@JacobCube
Copy link
Author

JacobCube commented Nov 7, 2024

@nbransby I tried running it on the emulator and the closest I got was to use the idToken from the emulator (which should obviously not work) -> "third-party tokens cannot be used with signInWithCustomToken". Maybe I just misunderstood something. I could see running the Firebase function locally on the emulator and generating a custom token like that, then, using that custom token to sign in with the emulator. Is that what you meant? Outside of that, I've added a call that returns user information within signInWithCustomToken which seems to work. Will appreciate any help.

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