Skip to content

Conversation

@delhilad
Copy link

Describe the technical changes contained in this PR
This will enable the login using the facebook and Google Oauth token, for the pre-approved email records.

Previous behaviour
Before this change, user could login using their email and password to login.

New behaviour
User will no longer be able to login using the usename and password, now they will need an email id linked to their Google or Facebook login to login to the webapp. The email linked to their social media account will be already registered to enable their account. First they will verify the email id to be used for the login, and then they will provide authentication token using the linked social media(Google or Facebook) account.

Related issues addressed by this PR
List issue numbers using the "Fixes #xxx" syntax

Have the following been addressed?

  • Have test cases been created for all of the changes?
  • Do all of the test cases pass?
  • Has the testing been done using the default docker-compose environment?
  • Are documentation changes required?
  • Does this change break or alter existing behaviour?

@delhilad
Copy link
Author

Changes for enabling Facebook and Google login. There is also a video included with the demo of functionality.

@shanthisa
Copy link
Collaborator

Thank you for implementing social login. It works as expected in the happy path. However, I have a few concerns regarding its impact on the existing functionality and overall security:

  1. Regression of Existing Functionality
    Removing the ability to log in using the existing mealplanner username and password flow breaks access for current admin users, meal designers, and existing client users. Since this project is already in production, it is critical that we preserve all existing authentication paths. Social login should be implemented as an additional onboarding option, not a replacement.

  2. Security Concerns
    The current implementation allows app_anonymous to fetch person details and sets an auth cookie. This introduces a serious security risk, as it could be exploited to gain unauthorized access to user data. We should not permit anonymous access to authenticated endpoints or allow identity to be inferred without proper validation.

  3. Codebase Cleanliness
    Videos or other large binary files should not be checked into the codebase. Instead, they should be attached as a separate file in the PR or shared through an external link for review.

Please address these issues before merging, and let us ensure we do not compromise existing users or production stability while introducing new features.

@delhilad - If you are unable to take it up due to your existing commitments we can transfer this PR to the existing team. Please let me know.

@shanthisa
Copy link
Collaborator

Security_Problem.mp4

Hi Please find the video for the security problem because of allowing an app_anonymous user to set the session token. Auth cookie should be set when you are verifying the jwt token.
Also the pattern of verifying whether the email exists and then allowing social login given an easy way for the hacker to put random emails to know who are existing in the system. That also needs to be verified only after authentication in the backend and should not be explicitly given in the frontend.

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.

2 participants