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

feat(auth): remove support for credentials.signIn in createUser (signIn is automatic) #513

Closed
AdrienLemaire opened this issue Jul 27, 2018 · 15 comments

Comments

@AdrienLemaire
Copy link

AdrienLemaire commented Jul 27, 2018

Do you want to request a feature or report a bug?

(If this is a usage question, please do not post it here—post it on gitter. If this is not a “feature” or a “bug”, or the phrase “How do I...?” applies, then it's probably a usage question.)
bug

What is the current behavior?
when setting {signIn: false}, a @@reactReduxFirebase/LOGIN action is still triggered.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via codesandbox or similar.

  doSubmit = async ({email, password, firstName, lastName}) => {
    const {dispatch, firebase} = this.props;
    await firebase.createUser({
      email,
      password,
      signIn: false,
    });
    dispatch(postUserAction({email, firstName, lastName}));
  };

What is the expected behavior?
By setting signIn to false, no action @@reactReduxFirebase/LOGIN should be raised.
On the other side, no action seems to be raised for the createUser action... that's weird

Which versions of dependencies, and which browser and OS are affected by this issue? Did this work in previous versions or setups?

$ yarn list|grep react-redux
├─ [email protected]
├─ [email protected]
@prescottprue
Copy link
Owner

prescottprue commented Jul 29, 2018

@Fandekasp The LOGIN action is dispatched when auth state changes, so it does seem like login is being triggered, I will investigate further.

It also seems like the signIn option isn't too well documented, so I will also look into cleaning that up, thanks for reporting!

Just out of curiosity, what is the postUserAction action creator you use to create/dispatch an action for? Is it something outside of user auth and profile?

@karltaylor
Copy link
Contributor

Also getting this problem on v2.1.8

@AdrienLemaire
Copy link
Author

@prescottprue sorry for the late reply. the postUserAction is just a saga to query my backend api. we store user information in our db and don't use firebase profiles.

@BoatPartyJesus
Copy link

Also getting this problem on v2.1.8

@karltaylor
Copy link
Contributor

I'm wondering if it's actually possible to avoid this, under the hood,

createUser is calling: createUserWithEmailAndPassword

export const createUser = (
dispatch,
firebase,
{ email, password, signIn },
profile
) => {
dispatchLoginError(dispatch, null)
if (!email || !password) {
const error = new Error('Email and Password are required to create user')
dispatchLoginError(dispatch, error)
return Promise.reject(error)
}
return firebase
.auth()
.createUserWithEmailAndPassword(email, password)
.then(
userData =>
// Login to newly created account if signIn flag is not set to false
firebase.auth().currentUser || (!!signIn && signIn === false)
? createUserProfile(
dispatch,
firebase,
userData,
profile || { email }
)
: login(dispatch, firebase, { email, password })
.then(() =>
createUserProfile(
dispatch,
firebase,
userData,
profile || { email }
)
)
.catch(err => {
if (err) {
switch (err.code) {
case 'auth/user-not-found':
dispatchLoginError(
dispatch,
new Error('The specified user account does not exist.')
)
break
default:
dispatchLoginError(dispatch, err)
}
}
return Promise.reject(err)
})
)
.catch(err => {
dispatchLoginError(dispatch, err)
return Promise.reject(err)
})
}

However, in the firebase documentation it says:

If the new account was created, the user is signed in automatically. Have a look at the Next steps section below to get the signed in user details.

@prescottprue
Copy link
Owner

@karltaylor It appears you are correct, thanks for pointing that out. That would explain why the auth state change was being fired even though login wasn't being called directly.

With that in mind, this feature may not actually be possible unless it logged out after creating the new user. Not sure what the use case for that would be, but it seems like it would almost be more clear to say that one should call logout after createUser.

@kubalobo
Copy link

I used this workaround to achieve that functionality:
https://stackoverflow.com/a/38013551/7844300
And it's working fine ;)

@prescottprue
Copy link
Owner

@kubalobo Good to know, it seems like that is the signing out right after as I was mentioning. I am open to supporting that, but it doesn't really make sense with signIn: false - it seems like the option should be something like logOutAfterCreation: true.

@kubalobo
Copy link

Not exactly. I needed to allow administrator to create account for new users, so after that nothing should happend - neither singIn or logOut.

@prescottprue
Copy link
Owner

prescottprue commented Nov 30, 2018

@kubalobo Is there a reason that couldn't be done through a cloud function using the firebase-admin sdk? It seems like that might be the more secure way to do that unless you write all of the correct role specific database rules and don't mind the admin user having the user's login info.

The cloud function with firebase-admin is actually what I usually/currently do - that pattern also works nice for having the ability to pre-create accounts with specific info then when the person first logs in the accounts can be merged based in info like email.

I am open to the feature of logging back out right after logging in, but is different than the original intention of signIn: false, which was made to just create the user and not log in when that was an option.

@kubalobo
Copy link

kubalobo commented Dec 1, 2018

Yea it sounds like much better approach. Thanks!

@prescottprue
Copy link
Owner

Glad to know it will work for you 😄

I'm going to switch this issue to capture getting rid of the signIn feature all together since it is pretty unclear. Also, I'm going to try to find a place in the docs for the solution mentioned above (maybe below signIn?).

@prescottprue prescottprue changed the title createUser credentials.signIn not working feat(auth): remove support for credentials.signIn in createUser (signIn is automatic) Dec 1, 2018
@prescottprue prescottprue added this to the v3.0.* milestone Feb 8, 2019
@prescottprue
Copy link
Owner

Planning to have this be one of the "potentially breaking changes" in v3.0.0, should go to pre-release soon. Open to PRs to next if anyone gets a chance, otherwise I will try to get to it soon. Thanks to everyone for the input and patience.

prescottprue pushed a commit that referenced this issue Sep 3, 2019
* feat(auth): remove signIn option from createUser (new user is automatically signed in through Firebase SDK) - #513
* docs(perf): document the correct way reference to state.firestore when creating selectors - #614
* chore(deps): update lodash to 4.17.15
@prescottprue prescottprue mentioned this issue Sep 5, 2019
3 tasks
@prescottprue
Copy link
Owner

Confirmed that createUser does still call LOGIN action and logs the user in v3.0.0-beta. Thanks to everyone for the input

prescottprue added a commit that referenced this issue Sep 6, 2019
* feat(auth): remove `signIn` option from createUser (new user is automatically signed in through Firebase SDK) - #513
* feat(core): new pattern for getting extended firebase instance in thunks (added back `getFirebase` to api) - #635
* fix(HOCs): switch to `UNSAFE_componentWillReceiveProps` in class based HOCs to prevent warnings with 16.9.0 - #755
* fix(HOCs): switch `withFirebase` and `withFirestore` back to pre-hooks compatible logic
* fix(core): replace lodash methods such as `isArray`, `isBoolean`, `isString`, `size`, `compact` and `isFunction` with native methods in a number of places
* chore(deps): update lodash to 4.17.15
* chore(docs): add docs for how to reference data from state for reselect selectors - #614
* chore(docs): update client side role assign example in roles recipes - #699
* chore(docs): add example for assigning role in cloud function - #699
@prescottprue
Copy link
Owner

Released in v3.0.0-beta. Reach out if it doesn't work as expected or doesn't solve your issue.

Thanks for reporting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants