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

Using Firebase Admin SDK #5628

Closed
wants to merge 13 commits into from
Closed

Using Firebase Admin SDK #5628

wants to merge 13 commits into from

Conversation

nhuethmayr
Copy link

@nhuethmayr nhuethmayr commented Oct 24, 2022

New adapter that uses the Firebase Admin instead of the client-side SDK

☕️ Reasoning

The current implementation of firebase-adapter uses the client SDK. This has the severe drawback that the Firestore Rules need to be opened up. Instead of using the client-side SDK, we should be using the server-side one. That way we can keep the Firestore rules closed and still integrate nicely with FB

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

This PR solves an architectural problem with the firebase-adapter as descibed in discussion 5049

📌 Resources

❓ Questions

  • Do we want to release this code as an independent adapter? firebase-admin-adapter
  • Do we want to replace the current implementation altogether?

@vercel
Copy link

vercel bot commented Oct 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
next-auth ✅ Ready (Inspect) Visit Preview Nov 10, 2022 at 2:05AM (UTC)

@github-actions github-actions bot added the adapters Changes related to the core code concerning database adapters label Oct 24, 2022
@shriharip
Copy link

Thanks for the efforts on making use of the admin SDK, could we plz have this merged soon.

@nhuethmayr nhuethmayr mentioned this pull request Oct 24, 2022
3 tasks
@ndom91
Copy link
Member

ndom91 commented Oct 24, 2022

Thanks for reopening!

@vercel vercel bot temporarily deployed to Preview October 25, 2022 15:32 Inactive
@nhuethmayr
Copy link
Author

@RonAlmog I still need to add in tests (hopefully today, maybe tomorrow) but what about the "Verify (javascript)" task that is pending?

@bcthakre
Copy link

Thanks @NorbertHuethmayr

feat(adapters): add Xata adapter (#4911)
@vercel vercel bot temporarily deployed to Preview October 29, 2022 19:42 Inactive
@MalteBoehm
Copy link

Any news on this one?

MalteBoehm
MalteBoehm approved these changes Oct 30, 2022
@nhuethmayr nhuethmayr requested a review from ubbe-xyz as a code owner October 30, 2022 16:09
@vercel vercel bot temporarily deployed to Preview October 30, 2022 16:11 Inactive
@bcthakre
Copy link

Does it mean we can use FirestoreAdapter now?

@MalteBoehm
Copy link

Nah, merge is blocked

@bcthakre
Copy link

bcthakre commented Nov 2, 2022

Probably go with Clerk for now. Also, NextJS 13 has issues with NextAuth

@MalteBoehm
Copy link

Probably go with Clerk for now. Also, NextJS 13 has issues with NextAuth

I don't care too much about Next13 since it's kind of interesting beta. It just would be nice to use the firebase adapter to save some time :)

@vercel vercel bot temporarily deployed to Preview November 2, 2022 20:09 Inactive
@MalteBoehm
Copy link

@ndom91 hey there, for me it all works and looks as expected. Could you perhaps do a review on this too? :)

@vercel vercel bot temporarily deployed to Preview November 10, 2022 02:06 Inactive
@vedantmgoyal9
Copy link
Contributor

@NorbertHuethmayr please resolve the conflicts!

@Chibionos
Copy link

@ndom91 @lluia @ThangHuuVu
Can we get a PR for this please ? The change is required for NextAuth Firebase to be used in any Enterprise scenario as we cannot use the current Firebase Adapter due to a huge security issue we talked about here.

#5049

FYI @NorbertHuethmayr @shriharip @bcthakre

@shriharip
Copy link

Yes, I agree, would be best to have this in quickly. I have also now upvoted the PR. @NorbertHuethmayr @Chibionos

@mehallhm
Copy link

I opened PR on @NorbertHuethmayr fork... contributer used the windows command prompt only del command. Instead, it should be the rm command considering the build is being run on a Linux build server.

Hopefully should at least fix the build from throwing that error

@nhuethmayr nhuethmayr deleted the branch nextauthjs:main December 13, 2022 19:38
@nhuethmayr nhuethmayr closed this Dec 13, 2022
@nhuethmayr nhuethmayr deleted the main branch December 13, 2022 19:38
@nhuethmayr
Copy link
Author

I opened PR on @NorbertHuethmayr fork... contributer used the windows command prompt only del command. Instead, it should be the rm command considering the build is being run on a Linux build server.

Hopefully should at least fix the build from throwing that error

Done 👍 - now I need to pull in changes from main to be up to date.

@bcthakre
Copy link

bcthakre commented Dec 17, 2022 via email

@BossBele
Copy link

@bcthakre The PR works on having the firebase adapter using admin SDK and not on updating the firebase web SDK module. In that case, the security concern leading to this proposed PR is still not addressed.

+1 for this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters Changes related to the core code concerning database adapters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants