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

[FEAT] Added Rocket.Chat Auth component supporting Google OAuth #183

Merged

Conversation

Palanikannan1437
Copy link
Contributor

@Palanikannan1437 Palanikannan1437 commented Sep 14, 2022

Closes the Rocket.Chat Auth component supporting Google OAuth part of #182

Issue in brief

Evolution of the RC4Community platform has been happening on RC4Conferences project. In order to start the above process, Auth re-work has to be done, starting with supporting Google OAuth in Rocket.Chat Auth!

Suggested Fixes/Changes

  • Back-ported from RC4Conferences
  • Added the essential functions for login and logout via Google OAuth
  • Fixed adding hashmail to cookies
  • Added necessary env variable in .env.local.sample for hashing
  • Changed relevant directory and file names to match Rocket.Chat Auth Component for future support!

Demo of the above Suggested Changes

  • Now the user is able to login and logout into the Rocket.Chat instance with Google as the OAuth Provider!

rocket-chat-ff

Fixes/Changes after suggestions

  • Added function to resend email code 2FA
  • Added Resume token logic to login to Rocket.Chat via RC4Community using the Click to Chat button!
  • Removed Click to Chat button from UI if user is not logged in!
  • Fixed context of user object from auth for accessibility across necessary components

Demo of the new Changes after suggestions

rocket-chat-ff

@Palanikannan1437
Copy link
Contributor Author

Thanks a lot for all the guidance and support @Sing-Li and @Dnouv!!

It'd be awesome if you guys could review this and suggest changes to be made, if any😄

@lgtm-com
Copy link

lgtm-com bot commented Sep 14, 2022

This pull request introduces 1 alert when merging 6c5d966 into 04f9a06 - view on LGTM.com

new alerts:

  • 1 for Clear text transmission of sensitive cookie

@Sing-Li
Copy link
Member

Sing-Li commented Sep 14, 2022

This is awesome work @Palanikannan1437 ! Please wire up the "Click to Chat" button top right for proper behavior.

It will demonstrate to all users how to use the auxiliary data obtained from Auth. (which is a major feature of the auth component)

Copy link
Member

@Sing-Li Sing-Li left a comment

Choose a reason for hiding this comment

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

Not a good idea to commit package-lock.json -- most of the time it will cause problem for others.

@lgtm-com
Copy link

lgtm-com bot commented Sep 18, 2022

This pull request introduces 1 alert when merging 4efe59b into 04f9a06 - view on LGTM.com

new alerts:

  • 1 for Clear text transmission of sensitive cookie

@Palanikannan1437 Palanikannan1437 force-pushed the rocketchat-auth-component branch from 4efe59b to d051992 Compare September 18, 2022 16:14
@Palanikannan1437
Copy link
Contributor Author

Not a good idea to commit package-lock.json -- most of the time it will cause problem for others.

Ohh! I had no idea, thank you so much for letting me know about this! I have made made the requested changes @Sing-Li

Also, w.r.t to the given tasks...I'm still trying to understand the entire workflow of the RC4Conferences app...will come up with a PR for the same soon💪

@lgtm-com
Copy link

lgtm-com bot commented Sep 18, 2022

This pull request introduces 1 alert when merging d051992 into 04f9a06 - view on LGTM.com

new alerts:

  • 1 for Clear text transmission of sensitive cookie

@Sing-Li
Copy link
Member

Sing-Li commented Sep 18, 2022

Not a good idea to commit package-lock.json -- most of the time it will cause problem for others.

Ohh! I had no idea, thank you so much for letting me know about this! I have made made the requested changes @Sing-Li

Also, w.r.t to the given tasks...I'm still trying to understand the entire workflow of the RC4Conferences app...will come up with a PR for the same soon💪

Cool. You probably don't need to know the entire workflow to wire the click-to-chat button. That's the whole idea of being componentized - you don't need to know what other components (such as the RC4Conferences components) are doing.

app/pages/api/encrypt/signRole.js Outdated Show resolved Hide resolved
app/pages/api/encrypt/unsignCook.js Outdated Show resolved Hide resolved
app/pages/api/encrypt/signCook.js Outdated Show resolved Hide resolved
app/lib/conferences/eventCall.js Outdated Show resolved Hide resolved
app/package.json Outdated Show resolved Hide resolved
app/.env.local.sample Outdated Show resolved Hide resolved
app/components/menubar.js Show resolved Hide resolved
@Palanikannan1437
Copy link
Contributor Author

Not a good idea to commit package-lock.json -- most of the time it will cause problem for others.

Ohh! I had no idea, thank you so much for letting me know about this! I have made made the requested changes @Sing-Li
Also, w.r.t to the given tasks...I'm still trying to understand the entire workflow of the RC4Conferences app...will come up with a PR for the same soon💪

Cool. You probably don't need to know the entire workflow to wire the click-to-chat button. That's the whole idea of being componentized - you don't need to know what other components (such as the RC4Conferences components) are doing.

Yup that's definitely true! I got it working!! I'll send a PR asap after cleaning the code a bit!

I just wanted to know the entire workflow in order to make significant contributions in the future and to get a basic idea of how things are working as a whole😁

@lgtm-com
Copy link

lgtm-com bot commented Sep 25, 2022

This pull request introduces 2 alerts when merging 2ec67ff into 04f9a06 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class
  • 1 for Clear text transmission of sensitive cookie

@lgtm-com
Copy link

lgtm-com bot commented Sep 30, 2022

This pull request introduces 3 alerts when merging 786f260 into 04f9a06 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Clear text transmission of sensitive cookie

@Palanikannan1437
Copy link
Contributor Author

This is awesome work @Palanikannan1437 ! Please wire up the "Click to Chat" button top right for proper behavior.

It will demonstrate to all users how to use the auxiliary data obtained from Auth. (which is a major feature of the auth component)

Hey @Sing-Li I've added the requested changes to the Click to Chat button, now

  1. If user is logged in, the click to chat button opens up a new R.C instance window with the user already logged into rocket chat
  2. If user is not logged in, the button redirects to the home page of the given R.C instance

Current Issues I'm facing,

  1. Once say the user logs in to Rocket chat via google auth, I use the authToken provided by Rocket.Chat app instance to use it as a resume token...but after using the resume token once and say if the user logs out of the Rocket.Chat app instance, the current resume token doesn't work anymore....how do I revalidate the resumetoken/accessToken?....the only way I could think of is calling the api/v1/login route again for a new authToken

@Sing-Li
Copy link
Member

Sing-Li commented Oct 1, 2022

@Palanikannan1437 thank you.

  1. If user is not logged in, the button redirects to the home page of the given R.C instance

Instead of redirecting, can we pop up the login dialog (as if they clicked the login button)?

@Palanikannan1437
Copy link
Contributor Author

Palanikannan1437 commented Oct 1, 2022

@Palanikannan1437 thank you.

  1. If user is not logged in, the button redirects to the home page of the given R.C instance

Instead of redirecting, can we pop up the login dialog (as if they clicked the login button)?

Ohh yeah sure, like the login modal from the login button would have to be triggered or a new similar modal from this button has to be triggered? @Sing-Li

@Sing-Li
Copy link
Member

Sing-Li commented Oct 4, 2022

@Palanikannan1437 hope you've concluded with @Dnouv that the "click to chat" button should not be visible (as with certain other TopNav elements) if the user is logged out.

@Palanikannan1437
Copy link
Contributor Author

@Palanikannan1437 hope you've concluded with @Dnouv that the "click to chat" button should not be visible (as with certain other TopNav elements) if the user is logged out.

Ohhh okay, I'll make these changes asap then!

@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request introduces 3 alerts when merging b2d54e7 into 04f9a06 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class
  • 1 for Clear text transmission of sensitive cookie

@Palanikannan1437 Palanikannan1437 marked this pull request as ready for review October 5, 2022 06:32
@Palanikannan1437
Copy link
Contributor Author

@Sing-Li and @Dnouv could you please review the latest changes? This is how everything works now!

rocket-chat-ff

@Palanikannan1437 Palanikannan1437 changed the title Added Rocket.Chat Auth component supporting Google OAuth [FEAT] Added Rocket.Chat Auth component supporting Google OAuth Oct 5, 2022
@lgtm-com
Copy link

lgtm-com bot commented Oct 5, 2022

This pull request introduces 2 alerts when merging a74e9e6 into 04f9a06 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class
  • 1 for Clear text transmission of sensitive cookie

@Sing-Li
Copy link
Member

Sing-Li commented Oct 6, 2022

Thanks @Palanikannan1437

@Sing-Li Sing-Li merged commit 2635a69 into RocketChat:master Oct 6, 2022
Dnouv pushed a commit to Dnouv/RC4Community that referenced this pull request Nov 3, 2022
…etChat#183)

* added Rocket.Chat Auth component supporting Google OAuth

* removed conf related hashing functions and dependencies

* click to chat button functionality with google oauth added

* 2fa resend method added and click to chat logic improved

* hide click to chat from UI if not logged in

* channel name made configurable via parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants