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

fix: set hook for email change when only phone identity present #1865

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

J0
Copy link
Contributor

@J0 J0 commented Dec 9, 2024

What kind of change does this PR introduce?

Currently, there is no token passed in into the email hook when an email change is attempted right after a phone sign up.

@coveralls
Copy link

coveralls commented Dec 9, 2024

Pull Request Test Coverage Report for Build 12375398184

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 9 (0.0%) changed or added relevant lines in 1 file are covered.
  • 165 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.01%) to 57.257%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/mail.go 0 9 0.0%
Files with Coverage Reduction New Missed Lines %
internal/api/token_oidc.go 3 16.11%
internal/api/logout.go 5 81.58%
internal/mailer/validate.go 5 77.78%
internal/api/verify.go 66 74.14%
internal/models/user.go 86 75.53%
Totals Coverage Status
Change from base Build 12192682911: -0.01%
Covered Lines: 9709
Relevant Lines: 16957

💛 - Coveralls

@cstockton
Copy link
Contributor

Are you ready to move this out of draft @J0 - change looks good.

@J0 J0 marked this pull request as ready for review December 17, 2024 22:18
@J0 J0 requested a review from a team as a code owner December 17, 2024 22:18
@J0
Copy link
Contributor Author

J0 commented Dec 17, 2024

Thanks for looking through the PRs! Have manually tested here but wanted to also:

  1. Document the expected behaviour with secure / non-secure email change as it's currently not surfaced in the docs
    a. Email Change (w/o secure) - new_token and new_token_hash should be populated
    b. Secure Email Change - new_token, token, new_token_hash and token_hash should all be populated in the hook payload
  2. Maybe write an automated test (maybe a contract test like mentioned in channel) but not sure where it'd live as not sure we should introduce a framework solely for htis PR

if emailActionType == mail.EmailChangeVerification && !config.Mailer.SecureEmailChangeEnabled && u.GetEmail() != "" {
otp = otpNew
}

emailData := mail.EmailData{
Token: otp,
Copy link
Member

Choose a reason for hiding this comment

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

Currently, there is no token passed in into the email hook when an email change is attempted right after a phone sign up.

what is the initial value of otp here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be "" if there's secure email change is not enabled. If it's enabled, and there's no phone linked to it it doesn't get set so it remains as "".

ref

// When secure email change is disabled, we place the token for the new email on emailData.Token
if !config.Mailer.SecureEmailChangeEnabled {
// Token Hash should already be set above
emailData.Token = otpNew
Copy link
Member

Choose a reason for hiding this comment

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

does this mean that the actual value in TokenHash will be different from the otpNew ?

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

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

what is the value of ignore (typo)

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