Repository: supabase/gotrue
Pull request #667 (2022-09-06 10:03:49)

fix: handle null session id
## What kind of change does this PR introduce?
* omit `session_id` from access token jwt if `session_id` is `uuid.Nil` 
* `createRefreshToken` should create a new session if one doesn't exist (`uuid.Nil`)
* `maybeLoadUserOrSession` should skip reading `session_id` from claim if it's a `uuid.Nil`
* Add tests for auth middleware functions


Patch:
diff --git a/api/auth.go b/api/auth.go
index 6c42066c..797bc6fb 100644
--- a/api/auth.go
+++ b/api/auth.go
@@ -15,7 +15,7 @@ import (
 
 // requireAuthentication checks incoming requests for tokens presented using the Authorization header
 func (a *API) requireAuthentication(w http.ResponseWriter, r *http.Request) (context.Context, error) {
-	token, err := a.extractBearerToken(w, r)
+	token, err := a.extractBearerToken(r)
 	config := a.config
 	if err != nil {
 		a.clearCookieTokens(config, w)
@@ -53,7 +53,7 @@ func (a *API) requireAdmin(ctx context.Context, w http.ResponseWriter, r *http.R
 	return nil, unauthorizedError("User not allowed")
 }
 
-func (a *API) extractBearerToken(w http.ResponseWriter, r *http.Request) (string, error) {
+func (a *API) extractBearerToken(r *http.Request) (string, error) {
 	authHeader := r.Header.Get("Authorization")
 	matches := bearerRegexp.FindStringSubmatch(authHeader)
 	if len(matches) != 2 {
@@ -103,13 +103,13 @@ func (a *API) maybeLoadUserOrSession(ctx context.Context) (context.Context, erro
 	}
 
 	var session *models.Session
-	if claims.SessionId != "" {
+	if claims.SessionId != "" && claims.SessionId != uuid.Nil.String() {
 		sessionId, err := uuid.FromString(claims.SessionId)
 		if err != nil {
 			return ctx, err
 		}
 		session, err = models.FindSessionById(a.db, sessionId)
-		if err != nil {
+		if err != nil && !models.IsNotFoundError(err) {
 			return ctx, err
 		}
 		ctx = withSession(ctx, session)
diff --git a/api/context.go b/api/context.go
index 394e701e..f4d1737f 100644
--- a/api/context.go
+++ b/api/context.go
@@ -73,6 +73,9 @@ func withUser(ctx context.Context, u *models.User) context.Context {
 
 // getUser reads the user from the context.
 func getUser(ctx context.Context) *models.User {
+	if ctx == nil {
+		return nil
+	}
 	obj := ctx.Value(userKey)
 	if obj == nil {
 		return nil
@@ -87,6 +90,9 @@ func withSession(ctx context.Context, s *models.Session) context.Context {
 
 // getSession reads the session from the context.
 func getSession(ctx context.Context) *models.Session {
+	if ctx == nil {
+		return nil
+	}
 	obj := ctx.Value(sessionKey)
 	if obj == nil {
 		return nil
diff --git a/api/middleware.go b/api/middleware.go
index 69c76701..ff604d96 100644
--- a/api/middleware.go
+++ b/api/middleware.go
@@ -97,7 +97,7 @@ func (a *API) limitEmailSentHandler() middlewareHandler {
 }
 
 func (a *API) requireAdminCredentials(w http.ResponseWriter, req *http.Request) (context.Context, error) {
-	t, err := a.extractBearerToken(w, req)
+	t, err := a.extractBearerToken(req)
 	if err != nil || t == "" {
 		return nil, err
 	}
diff --git a/api/token.go b/api/token.go
index cd535670..1d267012 100644
--- a/api/token.go
+++ b/api/token.go
@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/coreos/go-oidc/v3/oidc"
+	"github.com/gofrs/uuid"
 	"github.com/golang-jwt/jwt"
 	"github.com/netlify/gotrue/conf"
 	"github.com/netlify/gotrue/metering"
@@ -26,7 +27,7 @@ type GoTrueClaims struct {
 	AppMetaData  map[string]interface{} `json:"app_metadata"`
 	UserMetaData map[string]interface{} `json:"user_metadata"`
 	Role         string                 `json:"role"`
-	SessionId    string                 `json:"session_id"`
+	SessionId    string                 `json:"session_id,omitempty"`
 }
 
 // AccessTokenResponse represents an OAuth2 success response
@@ -325,7 +326,7 @@ func (a *API) RefreshTokenGrant(ctx context.Context, w http.ResponseWriter, r *h
 			}
 		}
 
-		tokenString, terr = generateAccessToken(user, newToken.SessionId.UUID.String(), time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
+		tokenString, terr = generateAccessToken(user, newToken.SessionId, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
 		if terr != nil {
 			return internalServerError("error generating jwt token").WithInternalError(terr)
 		}
@@ -526,7 +527,11 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
 	return sendJSON(w, http.StatusOK, token)
 }
 
-func generateAccessToken(user *models.User, sessionId string, expiresIn time.Duration, secret string) (string, error) {
+func generateAccessToken(user *models.User, sessionId *uuid.UUID, expiresIn time.Duration, secret string) (string, error) {
+	sid := ""
+	if sessionId != nil {
+		sid = sessionId.String()
+	}
 	claims := &GoTrueClaims{
 		StandardClaims: jwt.StandardClaims{
 			Subject:   user.ID.String(),
@@ -538,7 +543,7 @@ func generateAccessToken(user *models.User, sessionId string, expiresIn time.Dur
 		AppMetaData:  user.AppMetaData,
 		UserMetaData: user.UserMetaData,
 		Role:         user.Role,
-		SessionId:    sessionId,
+		SessionId:    sid,
 	}
 
 	token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
@@ -561,7 +566,7 @@ func (a *API) issueRefreshToken(ctx context.Context, conn *storage.Connection, u
 			return internalServerError("Database error granting user").WithInternalError(terr)
 		}
 
-		tokenString, terr = generateAccessToken(user, refreshToken.SessionId.UUID.String(), time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
+		tokenString, terr = generateAccessToken(user, refreshToken.SessionId, time.Second*time.Duration(config.JWT.Exp), config.JWT.Secret)
 		if terr != nil {
 			return internalServerError("error generating jwt token").WithInternalError(terr)
 		}
diff --git a/models/refresh_token.go b/models/refresh_token.go
index eb2ab3d5..847244ff 100644
--- a/models/refresh_token.go
+++ b/models/refresh_token.go
@@ -5,7 +5,6 @@ import (
 	"net/http"
 	"time"
 
-	"github.com/gobuffalo/nulls"
 	"github.com/gobuffalo/pop/v5"
 	"github.com/gofrs/uuid"
 	"github.com/netlify/gotrue/crypto"
@@ -22,7 +21,7 @@ type RefreshToken struct {
 	UserID uuid.UUID `db:"user_id"`
 
 	Parent    storage.NullString `db:"parent"`
-	SessionId nulls.UUID         `db:"session_id"`
+	SessionId *uuid.UUID         `db:"session_id"`
 
 	Revoked   bool      `db:"revoked"`
 	CreatedAt time.Time `db:"created_at"`
@@ -69,7 +68,7 @@ func GrantRefreshTokenSwap(r *http.Request, tx *storage.Connection, user *User,
 func RevokeTokenFamily(tx *storage.Connection, token *RefreshToken) error {
 	var err error
 	tablename := (&pop.Model{Value: RefreshToken{}}).TableName()
-	if token.SessionId.Valid && len(token.SessionId.UUID) > 0 {
+	if token.SessionId != nil {
 		err = tx.RawQuery(`update `+tablename+` set revoked = true where session_id = ?;`, token.SessionId).Exec()
 	} else {
 		err = tx.RawQuery(`
@@ -108,12 +107,15 @@ func createRefreshToken(tx *storage.Connection, user *User, oldToken *RefreshTok
 	if oldToken != nil {
 		token.Parent = storage.NullString(oldToken.Token)
 		token.SessionId = oldToken.SessionId
-	} else {
+	}
+
+	if token.SessionId == nil {
+		// Existing refresh tokens may have a null session_id if they were created before v2.15.3
 		session, err := CreateSession(tx, user)
 		if err != nil {
 			return nil, errors.Wrap(err, "Error generated unique session id")
 		}
-		token.SessionId = nulls.NewUUID(session.ID)
+		token.SessionId = &session.ID
 	}
 
 	if err := tx.Create(token); err != nil {


Test patch:
diff --git a/api/audit_test.go b/api/audit_test.go
index ad868de3..0c65c565 100644
--- a/api/audit_test.go
+++ b/api/audit_test.go
@@ -48,7 +48,7 @@ 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)
+	token, err := generateAccessToken(u, nil, 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}}
diff --git a/api/auth_test.go b/api/auth_test.go
new file mode 100644
index 00000000..24a9995f
--- /dev/null
+++ b/api/auth_test.go
@@ -0,0 +1,168 @@
+package api
+
+import (
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/gofrs/uuid"
+	jwt "github.com/golang-jwt/jwt"
+	"github.com/netlify/gotrue/conf"
+	"github.com/netlify/gotrue/models"
+	"github.com/stretchr/testify/require"
+	"github.com/stretchr/testify/suite"
+)
+
+type AuthTestSuite struct {
+	suite.Suite
+	API    *API
+	Config *conf.GlobalConfiguration
+}
+
+func TestAuth(t *testing.T) {
+	api, config, err := setupAPIForTest()
+	require.NoError(t, err)
+
+	ts := &AuthTestSuite{
+		API:    api,
+		Config: config,
+	}
+	defer api.db.Close()
+	suite.Run(t, ts)
+}
+
+func (ts *AuthTestSuite) SetupTest() {
+	models.TruncateAll(ts.API.db)
+
+	// Create user
+	u, err := models.NewUser("", "test@example.com", "password", ts.Config.JWT.Aud, nil)
+	require.NoError(ts.T(), err, "Error creating test user model")
+	require.NoError(ts.T(), ts.API.db.Create(u), "Error saving new test user")
+}
+
+func (ts *AuthTestSuite) TestExtractBearerToken() {
+	userClaims := &GoTrueClaims{
+		Role: "authenticated",
+	}
+	userJwt, err := jwt.NewWithClaims(jwt.SigningMethodHS256, userClaims).SignedString([]byte(ts.Config.JWT.Secret))
+	require.NoError(ts.T(), err)
+	req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
+	req.Header.Set("Authorization", "Bearer "+userJwt)
+
+	token, err := ts.API.extractBearerToken(req)
+	require.NoError(ts.T(), err)
+	require.Equal(ts.T(), userJwt, token)
+}
+
+func (ts *AuthTestSuite) TestParseJWTClaims() {
+	userClaims := &GoTrueClaims{
+		Role: "authenticated",
+	}
+	userJwt, err := jwt.NewWithClaims(jwt.SigningMethodHS256, userClaims).SignedString([]byte(ts.Config.JWT.Secret))
+	require.NoError(ts.T(), err)
+
+	req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
+	req.Header.Set("Authorization", "Bearer "+userJwt)
+	w := httptest.NewRecorder()
+	ctx, err := ts.API.parseJWTClaims(userJwt, req, w)
+	require.NoError(ts.T(), err)
+
+	// check if token is stored in context
+	token := getToken(ctx)
+	require.Equal(ts.T(), userJwt, token.Raw)
+}
+
+func (ts *AuthTestSuite) TestMaybeLoadUserOrSession() {
+	u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
+	require.NoError(ts.T(), err)
+
+	s, err := models.CreateSession(ts.API.db, u)
+	require.NoError(ts.T(), err)
+
+	cases := []struct {
+		Desc            string
+		UserJwtClaims   *GoTrueClaims
+		ExpectedError   error
+		ExpectedUser    *models.User
+		ExpectedSession *models.Session
+	}{
+		{
+			Desc: "Missing Subject Claim",
+			UserJwtClaims: &GoTrueClaims{
+				StandardClaims: jwt.StandardClaims{
+					Subject: "",
+				},
+				Role: "authenticated",
+			},
+			ExpectedError: errors.New("invalid claim: subject missing"),
+			ExpectedUser:  nil,
+		},
+		{
+			Desc: "Valid Subject Claim",
+			UserJwtClaims: &GoTrueClaims{
+				StandardClaims: jwt.StandardClaims{
+					Subject: u.ID.String(),
+				},
+				Role: "authenticated",
+			},
+			ExpectedError: nil,
+			ExpectedUser:  u,
+		},
+		{
+			Desc: "Empty Session ID Claim",
+			UserJwtClaims: &GoTrueClaims{
+				StandardClaims: jwt.StandardClaims{
+					Subject: u.ID.String(),
+				},
+				Role:      "authenticated",
+				SessionId: "",
+			},
+			ExpectedError: nil,
+			ExpectedUser:  u,
+		},
+		{
+			Desc: "Invalid Session ID Claim",
+			UserJwtClaims: &GoTrueClaims{
+				StandardClaims: jwt.StandardClaims{
+					Subject: u.ID.String(),
+				},
+				Role:      "authenticated",
+				SessionId: uuid.Nil.String(),
+			},
+			ExpectedError: nil,
+			ExpectedUser:  u,
+		},
+		{
+			Desc: "Valid Session ID Claim",
+			UserJwtClaims: &GoTrueClaims{
+				StandardClaims: jwt.StandardClaims{
+					Subject: u.ID.String(),
+				},
+				Role:      "authenticated",
+				SessionId: s.ID.String(),
+			},
+			ExpectedError:   nil,
+			ExpectedUser:    u,
+			ExpectedSession: s,
+		},
+	}
+
+	for _, c := range cases {
+		ts.Run(c.Desc, func() {
+			userJwt, err := jwt.NewWithClaims(jwt.SigningMethodHS256, c.UserJwtClaims).SignedString([]byte(ts.Config.JWT.Secret))
+			require.NoError(ts.T(), err)
+
+			req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
+			req.Header.Set("Authorization", "Bearer "+userJwt)
+
+			w := httptest.NewRecorder()
+			ctx, err := ts.API.parseJWTClaims(userJwt, req, w)
+			require.NoError(ts.T(), err)
+			ctx, err = ts.API.maybeLoadUserOrSession(ctx)
+			require.Equal(ts.T(), c.ExpectedError, err)
+			require.Equal(ts.T(), c.ExpectedUser, getUser(ctx))
+			require.Equal(ts.T(), c.ExpectedSession, getSession(ctx))
+		})
+	}
+}
diff --git a/api/invite_test.go b/api/invite_test.go
index 52a8e092..70cccea3 100644
--- a/api/invite_test.go
+++ b/api/invite_test.go
@@ -58,7 +58,7 @@ func (ts *InviteTestSuite) makeSuperAdmin(email string) string {
 
 	u.Role = "supabase_admin"
 
-	token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+	token, err := generateAccessToken(u, nil, 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}}
diff --git a/api/phone_test.go b/api/phone_test.go
index 5f5f9cc6..31434f97 100644
--- a/api/phone_test.go
+++ b/api/phone_test.go
@@ -126,7 +126,7 @@ func (ts *PhoneTestSuite) TestMissingSmsProviderConfig() {
 	u.PhoneConfirmedAt = &now
 	require.NoError(ts.T(), ts.API.db.Update(u), "Error updating new test user")
 
-	token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+	token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 	require.NoError(ts.T(), err)
 
 	cases := []struct {
diff --git a/api/user_test.go b/api/user_test.go
index 83163bd1..8a0d5fc9 100644
--- a/api/user_test.go
+++ b/api/user_test.go
@@ -47,7 +47,7 @@ func (ts *UserTestSuite) SetupTest() {
 func (ts *UserTestSuite) TestUserGet() {
 	u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
 	require.NoError(ts.T(), err, "Error finding user")
-	token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+	token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 	require.NoError(ts.T(), err, "Error generating access token")
 
 	req := httptest.NewRequest(http.MethodGet, "http://localhost/user", nil)
@@ -111,7 +111,7 @@ func (ts *UserTestSuite) TestUserUpdateEmail() {
 			require.NoError(ts.T(), u.SetPhone(ts.API.db, c.userData["phone"]), "Error setting user phone")
 			require.NoError(ts.T(), ts.API.db.Create(u), "Error saving test user")
 
-			token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+			token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 			require.NoError(ts.T(), err, "Error generating access token")
 
 			var buffer bytes.Buffer
@@ -170,7 +170,7 @@ func (ts *UserTestSuite) TestUserUpdatePhoneAutoconfirmEnabled() {
 
 	for _, c := range cases {
 		ts.Run(c.desc, func() {
-			token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+			token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 			require.NoError(ts.T(), err, "Error generating access token")
 
 			var buffer bytes.Buffer
@@ -244,7 +244,7 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
 			req := httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
 			req.Header.Set("Content-Type", "application/json")
 
-			token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+			token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 			require.NoError(ts.T(), err)
 			req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
 
@@ -272,7 +272,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() {
 	u.EmailConfirmedAt = &now
 	require.NoError(ts.T(), ts.API.db.Update(u), "Error updating new test user")
 
-	token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+	token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 	require.NoError(ts.T(), err)
 
 	// request for reauthentication nonce
diff --git a/api/verify_test.go b/api/verify_test.go
index 3b17a75c..8482a1ef 100644
--- a/api/verify_test.go
+++ b/api/verify_test.go
@@ -103,7 +103,7 @@ func (ts *VerifyTestSuite) TestVerifySecureEmailChange() {
 	req.Header.Set("Content-Type", "application/json")
 
 	// Generate access token for request
-	token, err := generateAccessToken(u, "", time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
+	token, err := generateAccessToken(u, nil, time.Second*time.Duration(ts.Config.JWT.Exp), ts.Config.JWT.Secret)
 	require.NoError(ts.T(), err)
 	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))