[WIP] Firebase Auth Component #48

abhinavkrin · 2022-01-23T18:13:25Z

This PR adds the firebase auth component to RC4Community.
Used next-firebase-auth

Set up

set up environment variables for firebase config in .env.local

# Environment variables for firebase-admin
FIREBASE_PROJECT_ID=required
FIREBASE_PRIVATE_KEY=required
FIREBASE_CLIENT_EMAIL=required
FIREBASE_DATABASE_URL=if needed, see initAuth in /app/components/auth/firebase/lib/functions.js

# Environment variables for firebase client config.
NEXT_PUBLIC_FIREBASE_PROJECT_ID=required
NEXT_PUBLIC_FIREBASE_API_KEY=required
NEXT_PUBLIC_FIREBASE_AUTH_DOMAIN=required
NEXT_PUBLIC_FIREBASE_STORAGE_BUCKET=
NEXT_PUBLIC_FIREBASE_MESSAGING_SENDER_ID=
NEXT_PUBLIC_FIREBASE_APP_ID=required
NEXT_PUBLIC_FIREBASE_MEASUREMENT_ID=

# for cookies signing
COOKIE_SECRET_CURRENT=
COOKIE_SECRET_PREVIOUS=

Export the page component with withAuthUserSSR(Page) and use useAuthUser() to get user info.
For SSR,

export const getServerSideProps = withAuthUserSSR(options)(({AuthUser}) => {  })

For more details, see https://github.com/gladly-team/next-firebase-auth
4. The AuthUI component handles UI for login and signup.
5. To use in development env. Set, secure: false in cookies config in initAuth function in file /app/components/auth/firebase/lib/functions.js

cookies: {
      ...
      ....
      sameSite: 'strict',
      secure: false, // set this to false in local (non-HTTPS) development
      signed: true,
    },

CLAassistant · 2022-01-23T18:13:29Z

All committers have signed the CLA.

lgtm-com · 2022-01-23T18:33:58Z

This pull request introduces 9 alerts when merging 9181aff into 43d489e - view on LGTM.com

new alerts:

9 for Unused variable, import, function or class

lgtm-com · 2022-01-23T18:39:26Z

This pull request introduces 7 alerts when merging 1d902eb into 43d489e - view on LGTM.com

new alerts:

7 for Unused variable, import, function or class

cms/package.json

app/components/auth/AuthMenuButton.js

app/components/auth/NoUserAvatar.js

app/components/auth/firebase/ui/AuthUI.js

RonLek · 2022-01-23T19:02:49Z

app/components/auth/firebase/ui/LoginForm.js

+            }
+
+        } catch(error){
+            switch(error.code){


Check if error includes a message field. Directly setError with error.message instead of using switch-case.

Firebase functions throw errors with a certain code. I check the error.message property. Its same as the code. In order to use custom error messages. I used switch cases.

RonLek · 2022-01-23T19:07:24Z

app/components/auth/firebase/ui/LoginForm.js

+                await linkWithCredential(userCred.user,OAuthProvider.credentialFromError(diffCredError.error));
+            }
+        } catch (e){
+            switch(e.code){


Use field similar to e.message instead of switch.

RonLek · 2022-01-23T19:08:15Z

app/components/auth/firebase/ui/SignupForm.js

+            await sendEmailVerification(userCred.user);
+            onSignupComplete && onSignupComplete();
+        } catch(error){
+            switch(error.code){


No switch-case needed.

RonLek · 2022-01-23T19:10:00Z

@abhinavkrin please highly improve code-quality, follow DRY and end new files with new-lines.

app/pages/_app.js

Sing-Li · 2022-01-25T23:48:04Z

@abhinavkrin pls resolve the trivial conflicts when you get a chance.

app/components/auth/firebase/lib/functions.js

RonLek · 2022-01-26T08:02:07Z

app/components/auth/firebase/lib/functions.js

+        projectId: process.env.FIREBASE_PROJECT_ID,
+        clientEmail: process.env.FIREBASE_CLIENT_EMAIL,
+        // The private key must not be accessible on the client side.
+        privateKey: process.env.FIREBASE_PRIVATE_KEY,


Instead of having the user to extract these fields from the service-account file can we instead give the path to the sa file itself? Ref: https://stackoverflow.com/a/66984271/8316412

Yes, we can. But while deploying users may refrain from uploading their service-key.json file. We can set the contents of the file in an environment variable. For now, I have edited the readme.md for firebase auth and mentioned where to find the values. Waiting for your comment on this.

Users wouldn't be required to upload their sa file to Git. Instead of scouring through the file for the variables (3 vars), it would be far easier to directly give path to the sa file (1 var), while having the user keep their file even outside the RC4Community project (the absolute path would be specified in this case).

Moreover, there is a case of having multiple sa files with different privileges. Instead of replacing the 3 env variables every time, it'll be easier for the user to just change the path to the file.

RonLek · 2022-01-26T08:14:51Z

@abhinavkrin . Please:

Create a .env.local.sample file with dummy values to let users know which environment variables do they need.
End files with a new line. Read this.
Remove the cms/package-lock.json file from the PR.
Resolve merge conflicts.
~~Squash your commits into one.~~

Sing-Li · 2022-01-26T19:05:26Z

@abhinavkrin . Please:

Create a .env.local.sample file with dummy values to let users know which environment variables do they need.

End files with a new line. Read this.

Remove the cms/package-lock.json file from the PR.

Resolve merge conflicts.

Squash your commits into one.

@RonLek is there a difference between him squashing before commit versus us squashing when we merge? If not, perhaps we should just squash every time.

…munity into firebase-auth

Sing-Li · 2022-01-27T02:27:43Z

@abhinavkrin right now, if a user just build according to instructions and does not configure firebase (set environment variables etc), then the startup will crash with a cryptic message.

Can we please do a check on the required ENV VAR, and if they don't exist --- render the component into a "do nothing" sample component ?

Having it build successfully and not crash, even though auth doesn't work without ENV VARs/configs, will be a significantly better developer experience.

You can also log a loud errror message to the console - providing a URL to the doc on how to configure firebase.

In fact, the component itself can render : "You must configure Firebase to use this component. See https://github.com/rocketchat/.... for more information"

lgtm-com · 2022-01-28T20:30:58Z

This pull request introduces 2 alerts when merging 5f46a00 into 8c2999d - view on LGTM.com

new alerts:

2 for Assignment to constant

abhinavkrin · 2022-01-28T20:41:11Z

@abhinavkrin right now, if a user just build according to instructions and does not configure firebase (set environment variables etc), then the startup will crash with a cryptic message.

Can we please do a check on the required ENV VAR, and if they don't exist --- render the component into a "do nothing" sample component ?

Having it build successfully and not crash, even though auth doesn't work without ENV VARs/configs, will be a significantly better developer experience.

You can also log a loud errror message to the console - providing a URL to the doc on how to configure firebase.

In fact, the component itself can render: "You must configure Firebase to use this component. See https://github.com/rocketchat/.... for more information"

@Sing-Li Wrote some wrapper components: withFirebaseAuthUser, withFirebaseAuthUserSSR, withFirebaseAuthUserTokenSSR and useFirebaseAuthUser . These will check if the firebase app has initialized without any errors or not. Hence preventing build fails. Though using components directly from next-firebase-auth is recommended.

This reverts commit 65d5d88.

* Revert "Add Firebase Auth Component (#48)" This reverts commit 65d5d88. * revert done * remove other firebase components

* Revert "Add Firebase Auth Component (RocketChat#48)" This reverts commit 65d5d88. * revert done * remove other firebase components

abhinavkrin added 2 commits January 23, 2022 23:16

firebase auth initial

623b089

improved app/components/auth/AuthMenuButton.js

f3cccee

abhinavkrin added 2 commits January 23, 2022 23:49

[FIXED] user photo not visible in auth button in menubar

f623926

Merge branch 'master' into firebase-auth

9181aff

menubar.js

1d902eb

abhinavkrin added 2 commits January 24, 2022 00:10

lint

598b884

AuthMenuButton.js

5307f53

abhinavkrin marked this pull request as draft January 23, 2022 18:55

abhinavkrin changed the title ~~Firebase Auth Component~~ [WIP] Firebase Auth Component Jan 23, 2022

RonLek requested changes Jan 23, 2022

View reviewed changes

Sing-Li reviewed Jan 23, 2022

View reviewed changes

app/pages/_app.js Outdated Show resolved Hide resolved

added next-firebase-auth & code improvements

d2354c8

abhinavkrin marked this pull request as ready for review January 24, 2022 00:21

abhinavkrin marked this pull request as draft January 24, 2022 00:22

abhinavkrin added 2 commits January 25, 2022 01:25

renamed firebase auth components

5871c69

import typo fix

cd4d217

RonLek requested changes Jan 26, 2022

View reviewed changes

abhinavkrin added 2 commits January 26, 2022 21:33

.env.local.sample

7f97434

files end with newline

a7f848f

abhinavkrin added 4 commits January 27, 2022 00:48

firebase auth README.md

58b39de

Merge branch 'master' into firebase-auth

d7a0199

firebase auth README.md

d767330

Update README.md

66fdfff

abhinavkrin added 2 commits January 27, 2022 02:01

removed changes in cms/package-lock.json

995832a

Merge branch 'firebase-auth' of https://github.com/abhinavkrin/RC4Com…

063a2c8

…munity into firebase-auth

Prevent BUILD FAIL if firebase not configured

5f46a00

fix assignment to const

2c1a142

abhinavkrin marked this pull request as ready for review January 28, 2022 20:41

Sing-Li approved these changes Jan 28, 2022

View reviewed changes

Sing-Li merged commit 65d5d88 into RocketChat:master Jan 28, 2022

Dnouv added a commit to Dnouv/RC4Community that referenced this pull request Oct 31, 2022

Revert "Add Firebase Auth Component (RocketChat#48)"

1e8e1bf

This reverts commit 65d5d88.

Dnouv mentioned this pull request Oct 31, 2022

[CLP] Remove firebase Auth #191

Merged

4 tasks

Sing-Li pushed a commit that referenced this pull request Oct 31, 2022

[CLP] Remove firebase Auth (#191)

7241ded

* Revert "Add Firebase Auth Component (#48)" This reverts commit 65d5d88. * revert done * remove other firebase components

Dnouv added a commit to Dnouv/RC4Community that referenced this pull request Nov 3, 2022

[CLP] Remove firebase Auth (RocketChat#191)

eaabff5

* Revert "Add Firebase Auth Component (RocketChat#48)" This reverts commit 65d5d88. * revert done * remove other firebase components

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Firebase Auth Component #48

[WIP] Firebase Auth Component #48

abhinavkrin commented Jan 23, 2022 •

edited

Loading

CLAassistant commented Jan 23, 2022 •

edited

Loading

lgtm-com bot commented Jan 23, 2022

lgtm-com bot commented Jan 23, 2022

RonLek Jan 23, 2022 •

edited

Loading

abhinavkrin Jan 24, 2022

RonLek Jan 23, 2022

RonLek Jan 23, 2022

RonLek commented Jan 23, 2022

Sing-Li commented Jan 25, 2022

RonLek Jan 26, 2022

abhinavkrin Jan 26, 2022

RonLek Jan 27, 2022

RonLek commented Jan 26, 2022 •

edited

Loading

Sing-Li commented Jan 26, 2022

Sing-Li commented Jan 27, 2022 •

edited

Loading

lgtm-com bot commented Jan 28, 2022

abhinavkrin commented Jan 28, 2022

[WIP] Firebase Auth Component #48

[WIP] Firebase Auth Component #48

Conversation

abhinavkrin commented Jan 23, 2022 • edited Loading

Set up

CLAassistant commented Jan 23, 2022 • edited Loading

lgtm-com bot commented Jan 23, 2022

lgtm-com bot commented Jan 23, 2022

RonLek Jan 23, 2022 • edited Loading

Choose a reason for hiding this comment

abhinavkrin Jan 24, 2022

Choose a reason for hiding this comment

RonLek Jan 23, 2022

Choose a reason for hiding this comment

RonLek Jan 23, 2022

Choose a reason for hiding this comment

RonLek commented Jan 23, 2022

Sing-Li commented Jan 25, 2022

RonLek Jan 26, 2022

Choose a reason for hiding this comment

abhinavkrin Jan 26, 2022

Choose a reason for hiding this comment

RonLek Jan 27, 2022

Choose a reason for hiding this comment

RonLek commented Jan 26, 2022 • edited Loading

Sing-Li commented Jan 26, 2022

Sing-Li commented Jan 27, 2022 • edited Loading

lgtm-com bot commented Jan 28, 2022

abhinavkrin commented Jan 28, 2022

abhinavkrin commented Jan 23, 2022 •

edited

Loading

CLAassistant commented Jan 23, 2022 •

edited

Loading

RonLek Jan 23, 2022 •

edited

Loading

RonLek commented Jan 26, 2022 •

edited

Loading

Sing-Li commented Jan 27, 2022 •

edited

Loading