Add Firebase Sign In #5
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5 +/- ##
===========================================
- Coverage 61.82% 50.14% -11.67%
===========================================
Files 9 51 +42
Lines 55 359 +304
Branches 13 51 +38
===========================================
+ Hits 34 180 +146
- Misses 20 171 +151
- Partials 1 8 +7
Continue to review full report in Codecov by Sentry.
|
arkadiuszbachorski
force-pushed
the
arek/add-firebase
branch
from
b24b181 to
cf8a677
Compare
|
This might still update, because we need tests and probably I'll modularize sign-in a bit more, but opening to review as we could handle it as follow-up PRs. |
There was a problem hiding this comment.
Thank you for working on the feature @arkadiuszbachorski!
I started the project locally, tested it with out dev credentials, and could successfully login using email and password 👍
It might be great to explore if and how we can write a small integration tests that uses the firebase emulator + an automated test to validate the login logic + functionality.
I had some smaller comments throughout the PR and added them to the relevant files. Great job 🚀
As described in other comments, I've restructured everything to make ir I've added many unit and integration tests. I didn't cover complete flow of sign in + redirect + guards. I think this requires full E2E flow, which is quite time consuming to write and set up, so I'd leave it for now. What do you think? Could you please re-review the changes? |
There was a problem hiding this comment.
Thank you for continuing to work in this @arkadiuszbachorski; some great next steps and improvements!
I had some smaller and bigger comments in the PR diff that would be great to be addressed. I can also see them being addressed in smaller follow-up PRs as we start to address the reusability and code separation of some elements.
One main element that I would suggest doing before merging the PR would be testing the Stanford SSO integration using a Firebase deployment (comment further down details why the deployment is failing at this point) to ensure that we can test this properly in a deployed use case before merging the PR. This might mean that we have to disable the sign in check cloud function temporarily (you can do this in the Firebase Authentication setting on firebase by selecting that no function should be run on sign up).
Fell free to merge the PR after you have verified the deployment works + SSO is configured correctly. I have also fixed the codecov setup in #6 & #7 which allows us to get a proper coverage report for this PR as well.
I am fine with a coverage drop for now for this PR and you have admin permissions to merge the PR despite a failed check. I would aim to avoid this in the future and use that as a motivation to continuously build out a reasonable testing coverage as we build out new features or refactor the code base to make it reusable across different projects.
env.ts
Outdated
There was a problem hiding this comment.
We need to ensure that these variables are available during the deployment outside of the GitHub Actions + Docker setup.
The Firebase setup is using an "automated" & "worry free" deployment and we will need to define these secrets as part of the Firebase App Hosting deployment as well: https://firebase.google.com/docs/app-hosting/configure.
I have tested the deployment and as expected it fails with the missing secrets. Seems like we need to use cloud secrets manager for this.
There was a problem hiding this comment.
@PSchmiedmayer I don't have access to Google Secret Manager to modify secret myself with role of "Firebase Admin".
Missing permissions:
secretmanager.secrets.list
There was a problem hiding this comment.
@arkadiuszbachorski Sorry for the late reply! Just added you as an owner to the Google Cloud project. Just our dev/testing project so feel free to modify it to your needs 👍
arkadiuszbachorski
force-pushed
the
arek/add-firebase
branch
2 times, most recently
from
6877cce to
0f24e25
Compare
arkadiuszbachorski
force-pushed
the
arek/add-firebase
branch
from
34605f2 to
5ef0cff
Compare
There was a problem hiding this comment.
Thank for the improvements here; let me know where I can help to get this across the finish line. As noted on Slack the user accounts seems to be successfully created but the web dashboard doesn't render as planned.
I will also push some additions to the README to detail how to setup Stanford SSO for the Stanford Setup to ensure that things are easy to reproduce for someone else.
apphosting.yaml
Outdated
There was a problem hiding this comment.
@arkadiuszbachorski It would be great to add some documentation to the README that links to documentation where to add these secrets & maybe a screenshot of the relevant Google Cloud Console Screen to make it easy for Stanford IT or someone else to setup the system deployment themselves.
We should do the same thing with a link to Google Cloud Firebase Hosting.
| @@ -28,7 +30,20 @@ You can run the project using the following command. You will need to install No | |||
| npm install | |||
| ``` | |||
|
|
|||
| 1. Start the Next.js Application | |||
| 2. Setup Firebase Environment Values | |||
There was a problem hiding this comment.
@arkadiuszbachorski I have added some more information about the .env setup here as I updated the README. Might make sense to integrate this with a local Firebase emulator setup & add some default values for this in this repo as we do it for the iOS and other apps.
|
|
||
| ### Deployment Configuration | ||
|
|
||
| ... |
There was a problem hiding this comment.
Added some deployment SSO configuration information. Would be great if you can add some missing elements here about the secrets and the other general deployment information.
Add Firebase Sign In
♻️ Current situation & Problem
Application has no Firebase integration and Sign In.
⚙️ Release Notes
📚 Documentation
✅ Testing
Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: