press icon indicating copy to clipboard operation
press copied to clipboard

fix(login): Add session id verification during login

Open Bowrna opened this issue 2 months ago • 6 comments

Bowrna avatar Oct 09 '25 09:10 Bowrna

Codecov Report

:x: Patch coverage is 0% with 60 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 44.77%. Comparing base (004d817) to head (7e00374). :warning: Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
press/api/site_login.py 0.00% 60 Missing :warning:

:x: Your patch check has failed because the patch coverage (0.00%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3518      +/-   ##
===========================================
- Coverage    44.81%   44.77%   -0.04%     
===========================================
  Files          723      723              
  Lines        49504    49539      +35     
===========================================
- Hits         22185    22182       -3     
- Misses       27319    27357      +38     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Oct 09 '25 09:10 codecov[bot]

I have a question (kind of suggestion).

Why we are not using Frappe Cloud as an pure auth layer only ?

Ideally:

  • We shouldn’t need to sync and store all site user email addresses on Frappe Cloud.
  • Authentication should work independently of whether a user has a Frappe Cloud account or not.

Putting some wire diagram and flows here, which might be simple to implement and maintain

image image image image

Only cons in this design is - You need to contact site for the info, but that's much more secured way of performing this auth imo.

@Bowrna @BreadGenie @ssiyad wdyt ?

tanmoysrt avatar Oct 11 '25 07:10 tanmoysrt

@tanmoysrt looks good. we may need to confirm user before sending the mail though. we don't want to send otp mails to random emails.

ssiyad avatar Oct 11 '25 09:10 ssiyad

@ssiyad yeah we can avoid sending actual mails, based on the info in site.

tanmoysrt avatar Oct 11 '25 10:10 tanmoysrt

@ssiyad i have a doubt. during a signup process, any email id was random one to system and we send OTP right. why do you think we will need to restrict sending otp?

Bowrna avatar Oct 31 '25 07:10 Bowrna

There is a difference is mails sent during signup and login right? When signing up we will be sending a mail saying "This is your OTP to activate the site". But during login, we send an email saying "This is your OTP to login to your site", which may not exist.

ssiyad avatar Oct 31 '25 10:10 ssiyad

@tanmoysrt @ssiyad If FC is put as a pure auth layer, then the current flow, where we show the list of product sites to user wouldn't be available. So i keeping the existing flow intact and worked on this PR. To avoid sending random OTP, i verified if that email id has at least one active site associated with it. During login, still i use the "Site User" doctype to redirect to login as site. I have added a function "verify_user_in_site" that runs sql remotely and give response. However i have not put it to use as we are still collecting data at "Site User". Please review this and let me know if i have to improve on any place.

Bowrna avatar Nov 28 '25 08:11 Bowrna