Skip to content

feat(examples/firebase-auth-firestore): use rest api for server auth#3362

Merged
kentcdodds merged 1 commit intoremix-run:mainfrom
penx:firebase-rest-auth
Jun 7, 2022
Merged

feat(examples/firebase-auth-firestore): use rest api for server auth#3362
kentcdodds merged 1 commit intoremix-run:mainfrom
penx:firebase-rest-auth

Conversation

@penx
Copy link
Contributor

@penx penx commented Jun 1, 2022

Use the Firebase Auth REST API for authenticating server side, instead of the Firebase client SDK.

The Firebase Client Auth SDK is stateful. It seems to have been built for the purpose of using it within an application where only one user is logged in at a time (i.e. client applications). It may not pose a security risk if it is only used for authentication calls, but login state is persisted in the module and this could lead to security issues if other features such as firebase/firestore were used, as these would behave as the last user to log in.

The Firebase Auth REST API gives us the same functionality with no external dependencies and control over state.

This is the API that the client SDK is calling, so it is unnecessary to include the client sdk to call a single endpoint.

As far as I'm aware, there are rate limiting issues with both approaches, which I plan to address in a follow up PR by calling the REST API client side and falling back to server side.

This was previously discussed at #1811 (comment)

Closes: discussion at #1811 (comment)

  • Docs
  • Tests

Testing Strategy:

  • ensuring examples/firebase-auth-firestore works with and without JavaScript against emulator and production instances

Use the Firebase REST API for authenticating server side.

The client SDK is stateful. It _may_ not pose a security risk if it is _only_ used for authentication calls, but could lead to severe security issues if users were to also address other libraries such as Firestore through the client SDK.

The Firebase Auth REST API gives us equivalent functionality.

As far as I'm aware, there are rate limiting issues with both approaches, which I plan to address in a follow up PR.

This was previously discussed at remix-run#1811 (comment)
@kentcdodds
Copy link
Member

which I plan to address in a follow up PR by calling the REST API client side and falling back to server side.

really don't want to do that. Is it possible to avoid this at all? Sounds like the rate-limiting doesn't impact this PR anyway, so I can probably merge as-is. I'm just really disappointed that Firebase seems to require that we do auth on the client. Is there really no way to avoid that?

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Definitely an improvement over what we've got currently. Thank you!

@kentcdodds kentcdodds merged commit 9290988 into remix-run:main Jun 7, 2022
@penx
Copy link
Contributor Author

penx commented Jun 7, 2022

I'm just really disappointed that Firebase seems to require that we do auth on the client. Is there really no way to avoid that?

@kentcdodds pretty sure I’d need someone from Firebase to give a definitive answer. I could try support.

really don't want to do that.

I've already done the work but I have no problem with it being discarded if it's not wanted.

Though it would be good to know your specific objections, as this would:

  • be done with progressive enhancement
  • not require the Firebase SDK client side
  • fall back to server side auth
  • have a very small client side bundle impact.

The only thing I can think of that is slightly ugly is we have to event.preventDefault() before the async function that does auth is called, so if there was a client side error after that then the fallback form post wouldn’t happen.

edit: this is what I was proposing #3417

@penx penx deleted the firebase-rest-auth branch June 9, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants