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.

Sign up for GitHub

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

feat: deprecate xxx_token columns from users table (phase iii) #1614

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

feat: deprecate xxx_token columns from users table (phase iii) #1614

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6e5d8fb
fix: check if ott exists in isOtpValid
kangmingtay May 28, 2024
738eb12
chore: CreateOneTimeToken should return ott
kangmingtay May 29, 2024
0bd61bd
Merge branch 'master' into km/phase-iii-ott
kangmingtay Jun 6, 2024
6bddb19
fix: generate link should use ott instead of tokens in user model
kangmingtay Jun 6, 2024
b42f1b3
feat: deprecate token columns in user model
kangmingtay Jun 7, 2024
0782fc7
chore: add comment to indicate deprecated columns
kangmingtay Jun 7, 2024
61182b8
chore: fix verify tests
kangmingtay Jun 7, 2024
521c55d
chore: fix tests to check against otts
kangmingtay Jun 7, 2024
fdfbfd5
chore: format test logs to use json
kangmingtay Jun 7, 2024
b7d4a76
chore: ignore lint for deprecated fields
kangmingtay Jun 7, 2024
ac75297
chore: resolve pr comments
kangmingtay Jun 10, 2024
2e3f9d4
chore: remove unnecessary comments & check for pkce prefix in verify
kangmingtay Jun 10, 2024
e3c55b5
Merge branch 'master' into km/phase-iii-ott
kangmingtay Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hack/test.env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ GOTRUE_API_HOST=localhost
PORT=9999
API_EXTERNAL_URL="http://localhost:9999"
GOTRUE_LOG_SQL=none
GOTRUE_LOG_LEVEL=warn
GOTRUE_LOG_LEVEL=info
GOTRUE_SITE_URL=https://example.netlify.com
GOTRUE_URI_ALLOW_LIST="http://localhost:3000"
GOTRUE_OPERATOR_TOKEN=foobar
Expand Down
24 changes: 14 additions & 10 deletions internal/api/admin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,20 +585,20 @@
// create user
u, err := models.NewUser("123456789", "[email protected]", "secret", ts.Config.JWT.Aud, map[string]interface{}{"name": "test"})
require.NoError(ts.T(), err)
u.ConfirmationToken = "some_token"
u.RecoveryToken = "some_token"
u.EmailChangeTokenCurrent = "some_token"
u.EmailChangeTokenNew = "some_token"
u.PhoneChangeToken = "some_token"
u.AppMetaData = map[string]interface{}{
"provider": "email",
}
require.NoError(ts.T(), ts.API.db.Create(u))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.ConfirmationToken, models.ConfirmationToken))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.RecoveryToken, models.RecoveryToken))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenCurrent, models.EmailChangeTokenCurrent))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), u.EmailChangeTokenNew, models.EmailChangeTokenNew))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), u.PhoneChangeToken, models.PhoneChangeToken))
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.ConfirmationToken)
require.NoError(ts.T(), err)
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.RecoveryToken)
require.NoError(ts.T(), err)
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.EmailChangeTokenCurrent)
require.NoError(ts.T(), err)
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetEmail(), "some_token", models.EmailChangeTokenNew)
require.NoError(ts.T(), err)
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, u.GetPhone(), "some_token", models.PhoneChangeToken)
require.NoError(ts.T(), err)

// create user identities
_, err = ts.API.createNewIdentity(ts.API.db, u, "email", map[string]interface{}{
Expand Down Expand Up @@ -628,12 +628,16 @@
deletedUser, err := models.FindUserByID(ts.API.db, u.ID)
require.NoError(ts.T(), err)

// TODO(km): remove once the db columns are removed
require.Empty(ts.T(), deletedUser.ConfirmationToken)

Check failure on line 632 in internal/api/admin_test.go

View workflow job for this annotation

GitHub Actions / test (1.22.x) 

deletedUser.ConfirmationToken is deprecated: use the one_time_tokens model instead  (SA1019)
require.Empty(ts.T(), deletedUser.RecoveryToken)

Check failure on line 633 in internal/api/admin_test.go

View workflow job for this annotation

GitHub Actions / test (1.22.x) 

deletedUser.RecoveryToken is deprecated: use the one_time_tokens model instead  (SA1019)
require.Empty(ts.T(), deletedUser.EmailChangeTokenCurrent)

Check failure on line 634 in internal/api/admin_test.go

View workflow job for this annotation

GitHub Actions / test (1.22.x) 

deletedUser.EmailChangeTokenCurrent is deprecated: use the one_time_tokens model instead  (SA1019)
require.Empty(ts.T(), deletedUser.EmailChangeTokenNew)

Check failure on line 635 in internal/api/admin_test.go

View workflow job for this annotation

GitHub Actions / test (1.22.x) 

deletedUser.EmailChangeTokenNew is deprecated: use the one_time_tokens model instead  (SA1019)
require.Empty(ts.T(), deletedUser.EncryptedPassword)
require.Empty(ts.T(), deletedUser.PhoneChangeToken)

Check failure on line 637 in internal/api/admin_test.go

View workflow job for this annotation

GitHub Actions / test (1.22.x) 

deletedUser.PhoneChangeToken is deprecated: use the one_time_tokens model instead  (SA1019)

// one time tokens table should be empty after a soft deletion
require.Empty(ts.T(), u.OneTimeTokens)
require.Empty(ts.T(), deletedUser.UserMetaData)
require.Empty(ts.T(), deletedUser.AppMetaData)
require.NotEmpty(ts.T(), deletedUser.DeletedAt)
Expand Down
3 changes: 2 additions & 1 deletion internal/api/anonymous_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,9 @@ func (ts *AnonymousTestSuite) TestConvertAnonymousUserToPermanent() {

switch c.verificationType {
case mail.EmailChangeVerification:
emailChangeToken := user.OneTimeTokens[0].TokenHash
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
"token_hash": user.EmailChangeTokenNew,
"token_hash": emailChangeToken,
"type": c.verificationType,
}))
case phoneChangeVerification:
Expand Down
5 changes: 5 additions & 0 deletions internal/api/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/supabase/auth/internal/conf"
"github.com/supabase/auth/internal/crypto"
"github.com/supabase/auth/internal/observability"
"github.com/supabase/auth/internal/storage"
"github.com/supabase/auth/internal/storage/test"
)
Expand All @@ -32,6 +33,10 @@ func setupAPIForTestWithCallback(cb func(*conf.GlobalConfiguration, *storage.Con
return nil, nil, err
}

if err := observability.ConfigureLogging(&config.Logging); err != nil {
return nil, nil, err
}

kangmingtay marked this conversation as resolved.
Show resolved Hide resolved
if cb != nil {
cb(config, nil)
}
Expand Down
7 changes: 2 additions & 5 deletions internal/api/external_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,12 @@ func (ts *ExternalTestSuite) createUser(providerId string, email string, name st
userData["avatar_url"] = avatar
}
u, err := models.NewUser("", email, "test", ts.Config.JWT.Aud, userData)

if confirmationToken != "" {
u.ConfirmationToken = confirmationToken
}
ts.Require().NoError(err, "Error making new user")
ts.Require().NoError(ts.API.db.Create(u), "Error creating user")

if confirmationToken != "" {
ts.Require().NoError(models.CreateOneTimeToken(ts.API.db, u.ID, email, u.ConfirmationToken, models.ConfirmationToken), "Error creating one-time confirmation/invite token")
_, err = models.CreateOneTimeToken(ts.API.db, u.ID, email, confirmationToken, models.ConfirmationToken)
ts.Require().NoError(err, "Error creating one-time confirmation/invite token")
}

i, err := models.NewIdentity(u, "email", map[string]interface{}{
Expand Down
10 changes: 6 additions & 4 deletions internal/api/invite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,10 +208,10 @@ func (ts *InviteTestSuite) TestVerifyInvite() {
user.InvitedAt = &now
user.ConfirmationSentAt = &now
user.EncryptedPassword = ""
user.ConfirmationToken = crypto.GenerateTokenHash(c.email, c.requestBody["token"].(string))
require.NoError(ts.T(), err)
require.NoError(ts.T(), ts.API.db.Create(user))
require.NoError(ts.T(), models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), user.ConfirmationToken, models.ConfirmationToken))
_, err = models.CreateOneTimeToken(ts.API.db, user.ID, user.GetEmail(), crypto.GenerateTokenHash(c.email, c.requestBody["token"].(string)), models.ConfirmationToken)
require.NoError(ts.T(), err)

// Find test user
_, err = models.FindUserByEmailAndAudience(ts.API.db, c.email, ts.Config.JWT.Aud)
Expand Down Expand Up @@ -279,9 +279,10 @@ func (ts *InviteTestSuite) TestInviteExternalGitlab() {
// Find test user
user, err := models.FindUserByEmailAndAudience(ts.API.db, "[email protected]", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)
inviteToken := user.OneTimeTokens[0].TokenHash

// get redirect url w/ state
req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+user.ConfirmationToken, nil)
req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+inviteToken, nil)
w = httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)
ts.Require().Equal(http.StatusFound, w.Code)
Expand Down Expand Up @@ -371,9 +372,10 @@ func (ts *InviteTestSuite) TestInviteExternalGitlab_MismatchedEmails() {
// Find test user
user, err := models.FindUserByEmailAndAudience(ts.API.db, "[email protected]", ts.Config.JWT.Aud)
require.NoError(ts.T(), err)
inviteToken := user.OneTimeTokens[0].TokenHash

// get redirect url w/ state
req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+user.ConfirmationToken, nil)
req = httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=gitlab&invite_token="+inviteToken, nil)
w = httptest.NewRecorder()
ts.API.handler.ServeHTTP(w, req)
ts.Require().Equal(http.StatusFound, w.Code)
Expand Down
Loading
Loading