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(login): resetBeforeLogin option to enable/disable resetting of auth and profile on login #254

Closed
zhang-z opened this Issue Aug 24, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@zhang-z

zhang-z commented Aug 24, 2017

I call firebase.login({token: token}) every time my app starts, which means login() could be called while the user is already logged in (yup this point can be improved, but the potential issue still exist). In this case, store.firebase.auth got reset to {isLoaded: true, isEmpty: true}. Firebase actions were dispatched in this sequence:

  1. AUTHENTICATION_INIT_STARTED
  2. AUTHENTICATION_INIT_FINISHED
  3. LOGIN -- already logged in, action contains auth: {...}
  4. LOGIN_ERROR -- dispatched by login(), see here

I feel the logic of 4th action is not quite right.

To replicate this issue, you can try to run login twice -- 2nd call fired after 1st call finished.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 24, 2017

Owner

LOGIN_ERROR is run with null so that it empties out any login errors that could have occurred from previous login attempts.

For now, it would probably be best to check for auth before you re-run firebase.login.

It seems like we could make a different action that only clears the error, but does not empty the rest of the auth state.

Owner

prescottprue commented Aug 24, 2017

LOGIN_ERROR is run with null so that it empties out any login errors that could have occurred from previous login attempts.

For now, it would probably be best to check for auth before you re-run firebase.login.

It seems like we could make a different action that only clears the error, but does not empty the rest of the auth state.

@zhang-z

This comment has been minimized.

Show comment
Hide comment
@zhang-z

zhang-z Aug 25, 2017

Yup, I will add the check in my project.

make a different action that only clears the error, but does not empty the rest of the auth state

This sounds good. Thanks.

zhang-z commented Aug 25, 2017

Yup, I will add the check in my project.

make a different action that only clears the error, but does not empty the rest of the auth state

This sounds good. Thanks.

@marekolszewski

This comment has been minimized.

Show comment
Hide comment
@marekolszewski

marekolszewski Aug 28, 2017

There are times when you might want to log the user in a second time. I believe I sometimes do that when Firebase requires a fresh login to change a user's email or password.

marekolszewski commented Aug 28, 2017

There are times when you might want to log the user in a second time. I believe I sometimes do that when Firebase requires a fresh login to change a user's email or password.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 28, 2017

Owner

@marekolszewski In that case, when logging in the second time, wouldn't you want the auth/profile to be removed since it is new data?

Owner

prescottprue commented Aug 28, 2017

@marekolszewski In that case, when logging in the second time, wouldn't you want the auth/profile to be removed since it is new data?

@marekolszewski

This comment has been minimized.

Show comment
Hide comment
@marekolszewski

marekolszewski Aug 28, 2017

@prescottprue Unfortunately, in this case, no. Removing auth redirects the user away from the settings page because they are briefly no longer logged in. Also, the user data doesn't change, though it's true that the auth token will be refreshed and likely different.

I think I solved the issue on my end by using a login popup when someone is accessing a page behind a login wall, but others might still have this issue. I think firebase has a special re-login flow, but I didn't look into it too much. Maybe we can expose that through react-redux-firebase.

marekolszewski commented Aug 28, 2017

@prescottprue Unfortunately, in this case, no. Removing auth redirects the user away from the settings page because they are briefly no longer logged in. Also, the user data doesn't change, though it's true that the auth token will be refreshed and likely different.

I think I solved the issue on my end by using a login popup when someone is accessing a page behind a login wall, but others might still have this issue. I think firebase has a special re-login flow, but I didn't look into it too much. Maybe we can expose that through react-redux-firebase.

@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 28, 2017

Owner

@marekolszewski and @zhang-z What are your thoughts on a config option (something like emptyBeforeLogin: false) to control this?

In v1 it would have to remain true by default to remain backwards compatible.

I think in v2 it should be false by default, but open to input on that front.

Owner

prescottprue commented Aug 28, 2017

@marekolszewski and @zhang-z What are your thoughts on a config option (something like emptyBeforeLogin: false) to control this?

In v1 it would have to remain true by default to remain backwards compatible.

I think in v2 it should be false by default, but open to input on that front.

prescottprue added a commit that referenced this issue Aug 29, 2017

Expose messaging. resetBeforeLogin option.
* Firebase Messaging exposed (`firebase.messaging()`)
* `resetBeforeLogin` option added (defaults to true to keep current
behavior) - #254
@zhang-z

This comment has been minimized.

Show comment
Hide comment
@zhang-z

zhang-z Aug 29, 2017

resetBeforeLogin sounds a better name over emptyBeforeLogin. Thanks!

zhang-z commented Aug 29, 2017

resetBeforeLogin sounds a better name over emptyBeforeLogin. Thanks!

prescottprue added a commit that referenced this issue Aug 29, 2017

Populating once queries
* Populate once queries - #256
* emptyOnLogin config option added - #254
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Aug 29, 2017

Owner

@zhang-z Glad you saw the new name, was going to mention that. Just to clarify for others, the new config parameter will be resetBeforeLogin.

The update should be released soon, but in the meantime you can try it out on the v2.0.0-beta.8 branch.

Owner

prescottprue commented Aug 29, 2017

@zhang-z Glad you saw the new name, was going to mention that. Just to clarify for others, the new config parameter will be resetBeforeLogin.

The update should be released soon, but in the meantime you can try it out on the v2.0.0-beta.8 branch.

@prescottprue prescottprue self-assigned this Aug 29, 2017

@prescottprue prescottprue changed the title from Calling "login" twice logs out the user to feat(login): resetBeforeLogin option to enable/disable resetting of auth and profile on login Aug 29, 2017

@prescottprue prescottprue referenced this issue Aug 30, 2017

Merged

v1.4.7 #258

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Aug 30, 2017

v1.4.7 (#258)
* Firebase Messaging service exposed (`firebase.messaging()`)
* `resetBeforeLogin` config option added (defaults to `true` to keep current behavior) - #254

@prescottprue prescottprue referenced this issue Sep 1, 2017

Merged

v2.0.0-beta.8 #263

3 of 3 tasks complete

prescottprue added a commit that referenced this issue Sep 2, 2017

v2.0.0-beta.8 (#263)
* fix(reducer): `MERGE` action added and `SET` action to reverted to v1 behavior - #255
* feat(populate): population of ordered data - #239
* feat(populate) populate once queries - #256
* feat(auth): emptyOnLogin config option added (defaults to `true`) - #254
* feat(query): emit `NO_VALUE` for `once` queries that are empty - #265
* fix(populate) Fixed populate function to return null for null paths
* app instance handling now uses `firebase_` instead of `extendApp` (more functionality) - #250
* Removed no longer in use code from v1 (validateConfig)
@prescottprue

This comment has been minimized.

Show comment
Hide comment
@prescottprue

prescottprue Sep 2, 2017

Owner

Released resetBeforeLogin option in v2.0.0-beta.8. Let me know if it does not work as expected :).

Owner

prescottprue commented Sep 2, 2017

Released resetBeforeLogin option in v2.0.0-beta.8. Let me know if it does not work as expected :).

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