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

Refactor user schema to store provider data better #208

Merged
merged 25 commits into from
Sep 28, 2021
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
aeecb42
chore: update test github action
kangmingtay Sep 6, 2021
624f370
Merge branch 'master' into refactor-v2
kangmingtay Sep 7, 2021
7fb2158
fix: fetch provider id on external provider flow
kangmingtay Sep 7, 2021
d16b889
fix(migration): create identities table
kangmingtay Sep 13, 2021
3ff8884
test(connection): update truncate all to cascade deletes
kangmingtay Sep 14, 2021
80c6e0e
refactor: standardise oauth providers to use oidc claims
kangmingtay Sep 15, 2021
86def11
test(external_provider): update tests to check provider_id
kangmingtay Sep 15, 2021
72a0dc8
Merge branch 'master' into refactor-v2
kangmingtay Sep 20, 2021
66a949f
refactor: remove unused constants
kangmingtay Sep 21, 2021
4baa809
fix: add valuer & scanner interfaces to claims
kangmingtay Sep 21, 2021
93bcbcc
fix: update migration to use provider & id as primary key
kangmingtay Sep 21, 2021
3c119a1
add custom error type for missing identity
kangmingtay Sep 21, 2021
68feb34
fix: add identity model
kangmingtay Sep 21, 2021
00b0c0a
fix: add logic to sign-in based on identity
kangmingtay Sep 21, 2021
65ca1d4
test: add identity tests
kangmingtay Sep 22, 2021
30d2846
chore: resolve pr comments
kangmingtay Sep 24, 2021
d84e553
fix: add identities to access token jwt payload
kangmingtay Sep 27, 2021
c365e04
test: update tests
kangmingtay Sep 27, 2021
b743ae2
docs: update example env with saml env vars
kangmingtay Sep 27, 2021
a8cf222
remove unused methods
kangmingtay Sep 27, 2021
a381e56
refactor: handle AuthURL in apple provider
kangmingtay Sep 27, 2021
06de241
fix: raw_app_meta_data returns all providers associated to user
kangmingtay Sep 27, 2021
78ec21b
test: fix tests
kangmingtay Sep 27, 2021
c1bed43
refactor: admin create user raw_app_meta_data
kangmingtay Sep 27, 2021
13c53ec
Merge branch 'master' into refactor-v2
kangmingtay Sep 27, 2021
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 api/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ func (a *API) adminUserCreate(w http.ResponseWriter, r *http.Request) error {
if user.AppMetaData == nil {
user.AppMetaData = make(map[string]interface{})
}
user.AppMetaData["provider"] = "email"
user.AppMetaData["provider"] = []string{"email"}

err = a.db.Transaction(func(tx *storage.Connection) error {
if terr := models.NewAuditLogEntry(tx, instanceID, adminUser, models.UserSignedUpAction, map[string]interface{}{
Expand Down
13 changes: 10 additions & 3 deletions api/admin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ func (ts *AdminTestSuite) makeSuperAdmin(email string) string {

u.Role = "supabase_admin"

token, err := generateAccessToken(u, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
identities, err := models.FindIdentitiesByUser(ts.API.db, u)
require.NoError(ts.T(), err, "Error retrieving identities")

token, err := generateAccessToken(u, identities, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
require.NoError(ts.T(), err, "Error generating access token")

p := jwt.Parser{ValidMethods: []string{jwt.SigningMethodHS256.Name}}
Expand All @@ -69,7 +72,11 @@ func (ts *AdminTestSuite) makeSuperAdmin(email string) string {
func (ts *AdminTestSuite) makeSystemUser() string {
u := models.NewSystemUser(uuid.Nil, ts.Config.JWT.Aud)
u.Role = "service_role"
token, err := generateAccessToken(u, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)

identities, err := models.FindIdentitiesByUser(ts.API.db, u)
require.NoError(ts.T(), err, "Error retrieving identities")

token, err := generateAccessToken(u, identities, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
require.NoError(ts.T(), err, "Error generating access token")

p := jwt.Parser{ValidMethods: []string{jwt.SigningMethodHS256.Name}}
Expand Down Expand Up @@ -278,7 +285,7 @@ func (ts *AdminTestSuite) TestAdminUserCreate() {
require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
assert.Equal(ts.T(), "test1@example.com", data.GetEmail())
assert.Equal(ts.T(), "123456789", data.GetPhone())
assert.Equal(ts.T(), "email", data.AppMetaData["provider"])
assert.Equal(ts.T(), []interface{}{"email"}, data.AppMetaData["provider"])
}

// TestAdminUserGet tests API /admin/user route (GET)
Expand Down
4 changes: 3 additions & 1 deletion api/audit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ func (ts *AuditTestSuite) makeSuperAdmin(email string) string {

u.Role = "supabase_admin"

token, err := generateAccessToken(u, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
identities, err := models.FindIdentitiesByUser(ts.API.db, u)
require.NoError(ts.T(), err, "Error retrieving identities")
token, err := generateAccessToken(u, identities, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
require.NoError(ts.T(), err, "Error generating access token")

p := jwt.Parser{ValidMethods: []string{jwt.SigningMethodHS256.Name}}
Expand Down
159 changes: 101 additions & 58 deletions api/external.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/netlify/gotrue/models"
"github.com/netlify/gotrue/storage"
"github.com/sirupsen/logrus"
"golang.org/x/oauth2"
)

// ExternalProviderClaims are the JWT claims sent as the state in the external oauth provider signup flow
Expand Down Expand Up @@ -87,16 +86,6 @@ func (a *API) ExternalProviderRedirect(w http.ResponseWriter, r *http.Request) e
if err != nil {
return internalServerError("Error storing request token in session").WithInternalError(err)
}
case *provider.AppleProvider:
opts := make([]oauth2.AuthCodeOption, 0, 1)
opts = append(opts, oauth2.SetAuthURLParam("response_mode", "form_post"))
authURL = externalProvider.Config.AuthCodeURL(tokenString, opts...)
if authURL != "" {
if u, err := url.Parse(authURL); err == nil {
u.RawQuery = strings.ReplaceAll(u.RawQuery, "+", "%20")
authURL = u.String()
}
}
default:
authURL = p.AuthCodeURL(tokenString)
}
Expand Down Expand Up @@ -153,53 +142,86 @@ func (a *API) internalExternalProviderCallback(w http.ResponseWriter, r *http.Re
}
} else {
aud := a.requestAud(ctx, r)

// search user using all available emails
var emailData provider.Email
for _, e := range userData.Emails {
if e.Verified || config.Mailer.Autoconfirm {
user, terr = models.FindUserByEmailAndAudience(tx, instanceID, e.Email, aud)
if terr != nil && !models.IsNotFoundError(terr) {
return internalServerError("Error checking for duplicate users").WithInternalError(terr)
}

if user != nil {
emailData = e
break
}
var identityData map[string]interface{}
if userData.Metadata != nil {
identityData, terr = userData.Metadata.ToMap()
if terr != nil {
return terr
}
}

if user == nil {
if config.DisableSignup {
return forbiddenError("Signups not allowed for this instance")
}

// prefer primary email for new signups
emailData = userData.Emails[0]
for _, e := range userData.Emails {
if e.Primary {
emailData = e
break
var identity *models.Identity
// check if identity exists
if identity, terr = models.FindIdentityByIdAndProvider(tx, userData.Metadata.Subject, providerType); terr != nil {
if models.IsNotFoundError(terr) {
// search user using all available emails
for _, e := range userData.Emails {
if e.Verified || config.Mailer.Autoconfirm {
user, terr = models.FindUserByEmailAndAudience(tx, instanceID, e.Email, aud)
if terr != nil && !models.IsNotFoundError(terr) {
return internalServerError("Error checking for duplicate users").WithInternalError(terr)
}
if user != nil {
emailData = e
break
}
}
}
}

params := &SignupParams{
Provider: providerType,
Email: emailData.Email,
Aud: aud,
Data: make(map[string]interface{}),
}
for k, v := range userData.Metadata {
if v != "" {
params.Data[k] = v
if user != nil {
if identity, terr = a.createNewIdentity(tx, user, providerType, identityData); terr != nil {
return terr
}
if terr = user.UpdateAppMetaDataProvider(tx); terr != nil {
return terr
}
} else {
if config.DisableSignup {
return forbiddenError("Signups not allowed for this instance")
}

// prefer primary email for new signups
emailData = userData.Emails[0]
for _, e := range userData.Emails {
if e.Primary {
emailData = e
break
}
}

params := &SignupParams{
Provider: providerType,
Email: emailData.Email,
Aud: aud,
Data: identityData,
}

user, terr = a.signupNewUser(ctx, tx, params)
if terr != nil {
return terr
}

if identity, terr = a.createNewIdentity(tx, user, providerType, identityData); terr != nil {
return terr
}
}
} else {
return terr
}
}

user, terr = a.signupNewUser(ctx, tx, params)
if identity != nil && user == nil {
// get user associated with identity
user, terr = models.FindUserByID(tx, identity.UserID)
if terr != nil {
return terr
}
if terr = tx.UpdateOnly(identity, "identity_data", "last_sign_in_at"); terr != nil {
return terr
}
if terr = user.UpdateAppMetaDataProvider(tx); terr != nil {
return terr
}
}

if !user.IsConfirmed() {
Expand Down Expand Up @@ -282,19 +304,20 @@ func (a *API) processInvite(ctx context.Context, tx *storage.Connection, userDat
return nil, badRequestError("Invited email does not match emails from external provider").WithInternalMessage("invited=%s external=%s", user.Email, strings.Join(emails, ", "))
}

if err := user.UpdateAppMetaData(tx, map[string]interface{}{
"provider": providerType,
}); err != nil {
return nil, internalServerError("Database error updating user").WithInternalError(err)
}

updates := make(map[string]interface{})
for k, v := range userData.Metadata {
if v != "" {
updates[k] = v
var identityData map[string]interface{}
if userData.Metadata != nil {
identityData, err = userData.Metadata.ToMap()
if err != nil {
return nil, internalServerError("Error serialising user metadata").WithInternalError(err)
}
}
if err := user.UpdateUserMetaData(tx, updates); err != nil {
if _, err := a.createNewIdentity(tx, user, providerType, identityData); err != nil {
return nil, err
}
if err = user.UpdateAppMetaDataProvider(tx); err != nil {
return nil, err
}
if err := user.UpdateUserMetaData(tx, identityData); err != nil {
return nil, internalServerError("Database error updating user").WithInternalError(err)
}

Expand Down Expand Up @@ -418,3 +441,23 @@ func (a *API) getExternalRedirectURL(r *http.Request) string {
}
return config.SiteURL
}

func (a *API) createNewIdentity(conn *storage.Connection, user *models.User, providerType string, identityData map[string]interface{}) (*models.Identity, error) {
identity, err := models.NewIdentity(user, providerType, identityData)
if err != nil {
return nil, err
}

err = conn.Transaction(func(tx *storage.Connection) error {
if terr := tx.Create(identity); terr != nil {
return internalServerError("Error creating identity").WithInternalError(terr)
}
return nil
})

if err != nil {
return nil, err
}

return identity, nil
}
28 changes: 14 additions & 14 deletions api/external_azure_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ import (
jwt "github.com/golang-jwt/jwt"
)

const (
azureUser string = `{"name":"Azure Test","email":"azure@example.com","sub":"azuretestid"}`
azureUserNoEmail string = `{"name":"Azure Test","sub":"azuretestid"}`
)

func (ts *ExternalTestSuite) TestSignupExternalAzure() {
req := httptest.NewRequest(http.MethodGet, "http://localhost/authorize?provider=azure", nil)
w := httptest.NewRecorder()
Expand Down Expand Up @@ -61,23 +66,21 @@ func AzureTestSignupSetup(ts *ExternalTestSuite, tokenCount *int, userCount *int

func (ts *ExternalTestSuite) TestSignupExternalAzure_AuthorizationCode() {
ts.Config.DisableSignup = false
ts.createUser("azure@example.com", "Azure Test", "", "")
ts.createUser("azuretestid", "azure@example.com", "Azure Test", "", "")
tokenCount, userCount := 0, 0
code := "authcode"
azureUser := `{"name":"Azure Test","email":"azure@example.com"}`
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUser)
defer server.Close()

u := performAuthorization(ts, "azure", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "azuretestid", "")
}

func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupErrorWhenNoUser() {
ts.Config.DisableSignup = true
tokenCount, userCount := 0, 0
code := "authcode"
azureUser := `{"name":"Azure Test","email":"azure@example.com"}`
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUser)
defer server.Close()

Expand All @@ -90,8 +93,7 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupErrorWhenNoEmai
ts.Config.DisableSignup = true
tokenCount, userCount := 0, 0
code := "authcode"
azureUser := `{"name":"Azure Test"}`
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUser)
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUserNoEmail)
defer server.Close()

u := performAuthorization(ts, "azure", code, "")
Expand All @@ -103,32 +105,30 @@ func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupErrorWhenNoEmai
func (ts *ExternalTestSuite) TestSignupExternalAzureDisableSignupSuccessWithPrimaryEmail() {
ts.Config.DisableSignup = true

ts.createUser("azure@example.com", "Azure Test", "", "")
ts.createUser("azuretestid", "azure@example.com", "Azure Test", "", "")

tokenCount, userCount := 0, 0
code := "authcode"
azureUser := `{"name":"Azure Test","email":"azure@example.com"}`
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUser)
defer server.Close()

u := performAuthorization(ts, "azure", code, "")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "azuretestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureSuccessWhenMatchingToken() {
// name and avatar should be populated from Azure API
ts.createUser("azure@example.com", "", "", "invite_token")
ts.createUser("azuretestid", "azure@example.com", "", "", "invite_token")

tokenCount, userCount := 0, 0
code := "authcode"
azureUser := `{"name":"Azure Test","email":"azure@example.com"}}`
server := AzureTestSignupSetup(ts, &tokenCount, &userCount, code, azureUser)
defer server.Close()

u := performAuthorization(ts, "azure", code, "invite_token")

assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "")
assertAuthorizationSuccess(ts, u, tokenCount, userCount, "azure@example.com", "Azure Test", "azuretestid", "")
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenNoMatchingToken() {
Expand All @@ -143,7 +143,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenNoMatchingToke
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenWrongToken() {
ts.createUser("azure@example.com", "", "", "invite_token")
ts.createUser("azuretestid", "azure@example.com", "", "", "invite_token")

tokenCount, userCount := 0, 0
code := "authcode"
Expand All @@ -156,7 +156,7 @@ func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenWrongToken() {
}

func (ts *ExternalTestSuite) TestInviteTokenExternalAzureErrorWhenEmailDoesntMatch() {
ts.createUser("azure@example.com", "", "", "invite_token")
ts.createUser("azuretestid", "azure@example.com", "", "", "invite_token")

tokenCount, userCount := 0, 0
code := "authcode"
Expand Down
Loading