Skip to content

Commit

Permalink
fix: respect last_sign_in_at on secure password update (supabase#1164)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Updating a user's password should only require reauthentication if the
current session is not recent (created more than 24hrs ago)
  • Loading branch information
kangmingtay authored and LashaJini committed Nov 13, 2024
1 parent a8bc0de commit 99c15a9
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 17 deletions.
16 changes: 10 additions & 6 deletions internal/api/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"errors"
"net/http"
"time"

"github.com/fatih/structs"
"github.com/gofrs/uuid"
Expand Down Expand Up @@ -68,10 +69,6 @@ func (p *UserUpdateParams) Validate(conn *storage.Connection, user *models.User,
if len(*p.Password) < config.PasswordMinLength {
return invalidPasswordLengthError(config.PasswordMinLength)
}
// if password reauthentication is enabled, user can only update password together with a nonce sent
if config.Security.UpdatePasswordRequireReauthentication && p.Nonce == "" {
return unauthorizedError("Password update requires reauthentication.")
}
}
if p.AppData != nil {
if !isAdmin(user, config) {
Expand Down Expand Up @@ -129,8 +126,15 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
var terr error
if params.Password != nil {
if config.Security.UpdatePasswordRequireReauthentication {
if terr = a.verifyReauthentication(params.Nonce, tx, config, user); terr != nil {
return terr
now := time.Now()
// we require reauthentication if the user hasn't signed in recently in the current session
if session == nil || now.After(session.CreatedAt.Add(24*time.Hour)) {
if len(params.Nonce) == 0 {
return badRequestError("Password update requires reauthentication")
}
if terr = a.verifyReauthentication(params.Nonce, tx, config, user); terr != nil {
return terr
}
}
}

Expand Down
62 changes: 51 additions & 11 deletions internal/api/user_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"
"time"

"github.com/gofrs/uuid"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"github.com/supabase/gotrue/internal/conf"
Expand Down Expand Up @@ -201,6 +202,23 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)

r, err := models.GrantAuthenticatedUser(ts.API.db, u, models.GrantParams{})
require.NoError(ts.T(), err)

r2, err := models.GrantAuthenticatedUser(ts.API.db, u, models.GrantParams{})
require.NoError(ts.T(), err)

// create a session and modify it's created_at time to simulate a session that is not recently logged in
notRecentlyLoggedIn, err := models.FindSessionByID(ts.API.db, *r2.SessionId)
require.NoError(ts.T(), err)

// cannot use Update here because Update doesn't removes the created_at field
require.NoError(ts.T(), ts.API.db.RawQuery(
"update "+notRecentlyLoggedIn.TableName()+" set created_at = ? where id = ?",
time.Now().Add(-24*time.Hour),
notRecentlyLoggedIn.ID).Exec(),
)

type expected struct {
code int
isAuthenticated bool
Expand All @@ -211,36 +229,57 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
newPassword string
nonce string
requireReauthentication bool
sessionId *uuid.UUID
expected expected
}{
{
desc: "Valid password length",
newPassword: "newpassword",
nonce: "",
requireReauthentication: false,
expected: expected{code: http.StatusOK, isAuthenticated: true},
},
{
desc: "Invalid password length",
newPassword: "",
nonce: "",
requireReauthentication: false,
sessionId: nil,
expected: expected{code: http.StatusUnprocessableEntity, isAuthenticated: false},
},
{
desc: "No reauthentication provided",
desc: "No nonce provided",
newPassword: "newpassword123",
nonce: "",
requireReauthentication: true,
expected: expected{code: http.StatusUnauthorized, isAuthenticated: false},
sessionId: nil,
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
},
{
desc: "Invalid nonce",
desc: "Need reauthentication because outside of recently logged in window",
newPassword: "newpassword123",
nonce: "",
requireReauthentication: true,
sessionId: r2.SessionId,
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
},
{
desc: "No need reauthentication because recently logged in",
newPassword: "newpassword123",
nonce: "",
requireReauthentication: true,
sessionId: r.SessionId,
expected: expected{code: http.StatusOK, isAuthenticated: true},
},
{
desc: "Invalid nonce",
newPassword: "newpassword1234",
nonce: "123456",
requireReauthentication: true,
sessionId: nil,
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
},
{
desc: "Valid password length",
newPassword: "newpassword",
nonce: "",
requireReauthentication: false,
sessionId: nil,
expected: expected{code: http.StatusOK, isAuthenticated: true},
},
}

for _, c := range cases {
Expand All @@ -253,7 +292,8 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
req.Header.Set("Content-Type", "application/json")

var token string
token, err = generateAccessToken(ts.API.db, u, nil, &ts.Config.JWT)

token, err = generateAccessToken(ts.API.db, u, c.sessionId, &ts.Config.JWT)
require.NoError(ts.T(), err)
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))

Expand Down

0 comments on commit 99c15a9

Please sign in to comment.