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: update aal requirements to update user #1766

Merged
merged 7 commits into from
Sep 17, 2024

Conversation

J0
Copy link
Contributor

@J0 J0 commented Sep 5, 2024

What kind of change does this PR introduce?

If a user has verified factors (mfa enabled) we should require an AAL2 session in order to proceed with any operation

We restrict phone, email, and password from updates as we consider those as sensitive fields

Context: https://supabase.slack.com/archives/C02AK9166FR/p1725466764804889

@J0 J0 marked this pull request as ready for review September 11, 2024 11:11
@J0 J0 requested a review from a team as a code owner September 11, 2024 11:11
@coveralls
Copy link

coveralls commented Sep 11, 2024

Pull Request Test Coverage Report for Build 10812950524

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

  • 4 of 10 (40.0%) changed or added relevant lines in 2 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.01%) to 57.908%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/user.go 1 4 25.0%
internal/models/user.go 3 6 50.0%
Files with Coverage Reduction New Missed Lines %
internal/models/cleanup.go 4 88.37%
Totals Coverage Status
Change from base Build 10684529517: -0.01%
Covered Lines: 9142
Relevant Lines: 15787

💛 - Coveralls

internal/models/user.go Outdated Show resolved Hide resolved
internal/api/user.go Outdated Show resolved Hide resolved
J0 and others added 2 commits September 11, 2024 16:42
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
@J0 J0 requested a review from hf September 11, 2024 13:50
Copy link
Contributor

@cstockton cstockton left a comment

Choose a reason for hiding this comment

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

Code wise I see no issues long as the logic behind the added checks is sound.

@J0 J0 merged commit 25d9874 into master Sep 17, 2024
3 checks passed
@J0 J0 deleted the j0/require_appropriate_aal_for_pw_update branch September 17, 2024 20:31
hf pushed a commit that referenced this pull request Sep 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.161.0](v2.160.0...v2.161.0)
(2024-09-24)


### Features

* add `x-sb-error-code` header, show error code in logs
([#1765](#1765))
([ed91c59](ed91c59))
* add webauthn configuration variables
([#1773](#1773))
([77d5897](77d5897))
* config reloading
([#1771](#1771))
([6ee0091](6ee0091))


### Bug Fixes

* add additional information around errors for missing content type
header ([#1576](#1576))
([c2b2f96](c2b2f96))
* add token to hook payload for non-secure email change
([#1763](#1763))
([7e472ad](7e472ad))
* update aal requirements to update user
([#1766](#1766))
([25d9874](25d9874))
* update mfa admin methods
([#1774](#1774))
([567ea7e](567ea7e))
* user sanitization should clean up email change info too
([#1759](#1759))
([9d419b4](9d419b4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@tjmcewan
Copy link

tjmcewan commented Oct 8, 2024

This can break password reset workflows. 😓 Be sure to update your flow to elevate to AAL2 before asking the user to set their new password.

LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

If a user has verified factors (mfa enabled) we should require an AAL2
session in order to proceed with any operation

We restrict phone, email, and password from updates as we consider those
as sensitive fields

Context:
https://supabase.slack.com/archives/C02AK9166FR/p1725466764804889

---------

Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
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.

5 participants