Compatibility improvements for existing installs

auth/oidc/store/renewal/renew.go

@@ -66,6 +66,7 @@ func (r *Renewal) renew(discovery oidc.Discovery, key string, oauth2Data oidc.OA
 	}
 	logging.DebugLog(fmt.Errorf("new token issued at %v: %+v", newToken, newTokenTimestamp))
 	oauth2Data.LastTokenRenewal = newTokenTimestamp
+	oauth2Data.RenewalRetries = 0 // retries only count consecutive failures
 	oauth2Data.Token.AccessToken = newToken.AccessToken
 	oauth2Data.Token.ExpiresIn = newToken.ExpiresIn
 	oauth2Data.Token.RefreshToken = newToken.RefreshToken
@@ -121,6 +122,10 @@ func canRenew(renewalTime time.Duration, oauth2Data oidc.OAuthData, store *oidcs
 		logging.DebugLog(fmt.Errorf("access token empty of oidc id %s", oauth2Data.ID))
 		return false, oidc.OIDCProvider{}, oidc.Discovery{}, nil
 	}
+	if oauth2Data.Token.RefreshToken == "" { // no refresh token (e.g. offline_access scope not requested): can't renew
+		logging.DebugLog(fmt.Errorf("refresh token empty of oidc id %s", oauth2Data.ID))
+		return false, oidc.OIDCProvider{}, oidc.Discovery{}, nil
+	}
 	expirationDate, err := getExpirationDate(oauth2Data.Token.AccessToken)
 	if err != nil {
 		return false, oidc.OIDCProvider{}, oidc.Discovery{}, fmt.Errorf("can't get expiration date of refresh_token (id:%s). error: %s", oauth2Data.ID, err)

auth/oidc/store/renewal/worker_test.go

@@ -26,6 +26,11 @@ func fakeAccessToken(t *testing.T, exp time.Time) string {
 }
 
 func newTestRenewal(t *testing.T, tokenEndpointStatus int) (*Renewal, *httptest.Server) {
+	t.Helper()
+	return newTestRenewalWithTokenResponse(t, tokenEndpointStatus, `{"error": "invalid_grant"}`)
+}
+
+func newTestRenewalWithTokenResponse(t *testing.T, tokenEndpointStatus int, tokenResponse string) (*Renewal, *httptest.Server) {
 	t.Helper()
 	var ts *httptest.Server
 	ts = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -35,7 +40,7 @@ func newTestRenewal(t *testing.T, tokenEndpointStatus int) (*Renewal, *httptest.
 			w.Write(out)
 		case "/token":
 			w.WriteHeader(tokenEndpointStatus)
-			w.Write([]byte(`{"error": "invalid_grant"}`))
+			w.Write([]byte(tokenResponse))
 		default:
 			w.WriteHeader(http.StatusNotFound)
 		}
@@ -125,3 +130,69 @@ func TestWorkerOnlyBacksOffAfterRenewal(t *testing.T) {
 		t.Fatalf("expected no backoff for entries without renewal, got %d", sleeps)
 	}
 }
+
+// TestWorkerSkipsRenewalWithoutRefreshToken: without a refresh token (e.g. the
+// offline_access scope isn't requested) the token can't be renewed, and the
+// user's connections must not be disabled for that.
+func TestWorkerSkipsRenewalWithoutRefreshToken(t *testing.T) {
+	backoffSleep = func() {}
+	r, _ := newTestRenewal(t, http.StatusBadRequest)
+	disabled := 0
+	r.userStore.UserHooks.DisableFunc = func(_ storage.Iface, user users.User) error {
+		disabled++
+		return nil
+	}
+	if _, err := r.userStore.AddUser(users.User{Login: "john@example.com", OIDCID: "oidc-1"}); err != nil {
+		t.Fatalf("add user error: %s", err)
+	}
+	err := r.oidcStore.SaveOAuth2Data(oidc.OAuthData{
+		ID:               "oidc-1",
+		OIDCProviderID:   "prov-1",
+		CreatedAt:        time.Now().Add(-2 * time.Hour),
+		LastTokenRenewal: time.Now().Add(-2 * time.Hour),
+		Token:            oidc.Token{AccessToken: fakeAccessToken(t, time.Now().Add(-time.Minute))},
+	}, "state-1")
+	if err != nil {
+		t.Fatalf("save error: %s", err)
+	}
+	for i := 0; i < RENEWAL_RETRIES+1; i++ {
+		r.renewAll()
+	}
+	if disabled != 0 {
+		t.Fatalf("connections were disabled for a user without refresh token")
+	}
+	oauth2Data, _ := r.oidcStore.GetOAuth2DataByKey("state-1")
+	if oauth2Data.RenewalRetries != 0 || oauth2Data.RenewalFailed {
+		t.Fatalf("renewal should not have been attempted: %+v", oauth2Data)
+	}
+}
+
+// TestRenewalResetsRetriesOnSuccess: earlier failures don't count anymore after
+// a successful renewal, so occasional failures over time don't disable a user.
+func TestRenewalResetsRetriesOnSuccess(t *testing.T) {
+	backoffSleep = func() {}
+	newAccessToken := fakeAccessToken(t, time.Now().Add(time.Hour))
+	r, _ := newTestRenewalWithTokenResponse(t, http.StatusOK, `{"access_token": "`+newAccessToken+`", "refresh_token": "refresh-2", "expires_in": 3600}`)
+	if _, err := r.userStore.AddUser(users.User{Login: "john@example.com", OIDCID: "oidc-1"}); err != nil {
+		t.Fatalf("add user error: %s", err)
+	}
+	err := r.oidcStore.SaveOAuth2Data(oidc.OAuthData{
+		ID:               "oidc-1",
+		OIDCProviderID:   "prov-1",
+		CreatedAt:        time.Now().Add(-2 * time.Hour),
+		LastTokenRenewal: time.Now().Add(-2 * time.Hour),
+		RenewalRetries:   RENEWAL_RETRIES - 1,
+		Token:            oidc.Token{AccessToken: fakeAccessToken(t, time.Now().Add(-time.Minute)), RefreshToken: "refresh"},
+	}, "state-1")
+	if err != nil {
+		t.Fatalf("save error: %s", err)
+	}
+	r.renewAll()
+	oauth2Data, _ := r.oidcStore.GetOAuth2DataByKey("state-1")
+	if oauth2Data.Token.AccessToken != newAccessToken {
+		t.Fatalf("token was not renewed: %+v", oauth2Data)
+	}
+	if oauth2Data.RenewalRetries != 0 {
+		t.Fatalf("expected retries to be reset after a successful renewal, got %d", oauth2Data.RenewalRetries)
+	}
+}

auth/oidc/token.go

@@ -113,6 +113,7 @@ func UpdateOAuth2DataWithToken(jwks Jwks, discovery Discovery, clientID, clientS
 	newOAuthData.Subject = subject.(string)
 	newOAuthData.Issuer = issuer.(string)
 	newOAuthData.UserInfo.Email = validEmail
+	newOAuthData.UserInfo.EmailNotVerified = emailNotVerified(claims)
 	return newOAuthData, nil
 }
 
@@ -140,8 +141,7 @@ func GetPublicKeyForToken(allJwks []Jwks, discoveryProviders []Discovery, token
 	return nil, fmt.Errorf("no matching kid found for token")
 }
 
-// validateIDTokenClaims checks the audience, issuer and email_verified claims of
-// an id token
+// validateIDTokenClaims checks the audience and issuer claims of an id token
 func validateIDTokenClaims(claims jwt.MapClaims, discovery Discovery, clientID string) error {
 	audience, err := claims.GetAudience()
 	if err != nil {
@@ -162,22 +162,38 @@ func validateIDTokenClaims(claims jwt.MapClaims, discovery Discovery, clientID s
 				expectedIssuer = strings.ReplaceAll(expectedIssuer, "{tenantid}", tenantID)
 			}
 		}
-		if issuer != expectedIssuer {
+		if !issuerMatches(issuer, expectedIssuer) {
 			return fmt.Errorf("id token issuer doesn't match the issuer of the oidc provider")
 		}
 	}
-	// only reject when the IdP explicitly says the email address is not verified
-	if emailVerified, ok := claims["email_verified"]; ok {
-		verified := true
-		switch v := emailVerified.(type) {
-		case bool:
-			verified = v
-		case string:
-			verified = !strings.EqualFold(v, "false")
-		}
-		if !verified {
-			return fmt.Errorf("email address in id token is not verified")
-		}
-	}
 	return nil
 }
+
+// issuerMatches compares the id token issuer with the issuer of the discovery
+// document. Google documents both https://accounts.google.com and
+// accounts.google.com as valid issuers.
+func issuerMatches(issuer, expectedIssuer string) bool {
+	if issuer == expectedIssuer {
+		return true
+	}
+	if expectedIssuer == "https://accounts.google.com" && issuer == "accounts.google.com" {
+		return true
+	}
+	return false
+}
+
+// emailNotVerified returns true when the IdP explicitly says the email address
+// is not verified (email_verified=false). A missing claim is not treated as unverified.
+func emailNotVerified(claims jwt.MapClaims) bool {
+	emailVerified, ok := claims["email_verified"]
+	if !ok {
+		return false
+	}
+	switch v := emailVerified.(type) {
+	case bool:
+		return !v
+	case string:
+		return strings.EqualFold(v, "false")
+	}
+	return false
+}

auth/oidc/token_test.go

@@ -43,15 +43,20 @@ func TestUpdateOAuth2DataWithTokenValidatesClaims(t *testing.T) {
 		discoveryIssuer string
 		modify          func(jwt.MapClaims)
 		wantErr         bool
+		wantNotVerified bool
 	}{
 		{name: "valid", discoveryIssuer: "https://idp.inv", modify: func(jwt.MapClaims) {}},
 		{name: "audience array", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["aud"] = []string{"other", "client-1"} }},
 		{name: "wrong audience", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["aud"] = "other-client" }, wantErr: true},
 		{name: "missing audience", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { delete(c, "aud") }, wantErr: true},
 		{name: "wrong issuer", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["iss"] = "https://evil.inv" }, wantErr: true},
 		{name: "email verified", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = true }},
-		{name: "email not verified", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = false }, wantErr: true},
-		{name: "email not verified (string)", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = "false" }, wantErr: true},
+		// an unverified email doesn't block the login, it's recorded and checked when linking to an existing user
+		{name: "email not verified", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = false }, wantNotVerified: true},
+		{name: "email not verified (string)", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["email_verified"] = "false" }, wantNotVerified: true},
+		{name: "google issuer without scheme", discoveryIssuer: "https://accounts.google.com", modify: func(c jwt.MapClaims) { c["iss"] = "accounts.google.com" }},
+		{name: "google issuer with scheme", discoveryIssuer: "https://accounts.google.com", modify: func(c jwt.MapClaims) { c["iss"] = "https://accounts.google.com" }},
+		{name: "issuer without scheme for other idp", discoveryIssuer: "https://idp.inv", modify: func(c jwt.MapClaims) { c["iss"] = "idp.inv" }, wantErr: true},
 		{name: "tenant issuer", discoveryIssuer: "https://login.inv/{tenantid}/v2.0", modify: func(c jwt.MapClaims) {
 			c["iss"] = "https://login.inv/tenant-1/v2.0"
 			c["tid"] = "tenant-1"
@@ -90,6 +95,9 @@ func TestUpdateOAuth2DataWithTokenValidatesClaims(t *testing.T) {
 			if oauthData.UserInfo.Email != "john@example.inv" || oauthData.Subject != "john" {
 				t.Fatalf("unexpected oauth data: %+v", oauthData)
 			}
+			if oauthData.UserInfo.EmailNotVerified != tc.wantNotVerified {
+				t.Fatalf("EmailNotVerified = %v, want %v", oauthData.UserInfo.EmailNotVerified, tc.wantNotVerified)
+			}
 		})
 	}
 }

auth/oidc/types.go

@@ -75,4 +75,6 @@ type OAuthData struct {
 
 type UserInfo struct {
 	Email string `json:"email"`
+	// EmailNotVerified is set when the IdP explicitly says the email address is not verified
+	EmailNotVerified bool `json:"emailNotVerified,omitempty"`
 }

rest/auth.go

@@ -2,7 +2,6 @@ package rest
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"net/http"
 	"strings"
@@ -218,17 +217,7 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) {
 			}
 
 			// add user to the user database (or modify existing one)
-			user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, samlSession.Login, "saml", samlSession.ID)
-			if errors.Is(err, errNoLicense) {
-				loginResponse.NoLicense = true
-				out, err := json.Marshal(loginResponse)
-				if err != nil {
-					c.returnError(w, fmt.Errorf("loginResponse Marshal error: %s", err), http.StatusBadRequest)
-					return
-				}
-				c.write(w, out)
-				return
-			}
+			user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, samlSession.Login, "saml", samlSession.ID, externalUserOptions{})
 			if err != nil {
 				c.returnError(w, fmt.Errorf("couldn't add/modify user in database: %s", err), http.StatusBadRequest)
 				return
@@ -309,17 +298,7 @@ func (c *Context) authMethodsByID(w http.ResponseWriter, r *http.Request) {
 						return
 					}
 					// add user to the user database (or modify existing one)
-					user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, updatedOauth2data.UserInfo.Email, "oidc", updatedOauth2data.ID)
-					if errors.Is(err, errNoLicense) {
-						loginResponse.NoLicense = true
-						out, err := json.Marshal(loginResponse)
-						if err != nil {
-							c.returnError(w, fmt.Errorf("loginResponse Marshal error: %s", err), http.StatusBadRequest)
-							return
-						}
-						c.write(w, out)
-						return
-					}
+					user, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, updatedOauth2data.UserInfo.Email, "oidc", updatedOauth2data.ID, externalUserOptions{EmailNotVerified: updatedOauth2data.UserInfo.EmailNotVerified})
 					if err != nil {
 						c.returnError(w, fmt.Errorf("couldn't add/modify user in database: %s", err), http.StatusBadRequest)
 						return

rest/context.go

@@ -86,6 +86,10 @@ func newContextWithParams(storage storage.Iface, serverType string, userStore *u
 		}
 	} else {
 		c.SCIM.Client = scimInstance
+		// the saved token isn't known to a new scim instance (e.g. after a restart)
+		if scimInstance != nil && c.SCIM.Token != "" {
+			scimInstance.UpdateToken(c.SCIM.Token)
+		}
 	}
 	if c.SAML == nil {
 		providers := []saml.Provider{}