From 569425772d6d6c2e700fc4df939e5a747cb79405 Mon Sep 17 00:00:00 2001 From: Edward Viaene Date: Sat, 10 Oct 2026 14:11:29 -0500 Subject: [PATCH 1/2] compatibility improvements for existing installs - New SSO users are created regardless of the license count, as before - email_verified=false no longer blocks OIDC logins; it is only checked when the login links to a user that doesn't use OIDC yet - Users can switch between SAML and OIDC logins again - Accept accounts.google.com as Google issuer - OIDC token renewal skips entries without refresh token and resets the retry counter after a successful renewal - ReadDir skips temporary files of an interrupted WriteFile Co-Authored-By: Claude Opus 5.5 --- auth/oidc/store/renewal/renew.go | 5 ++ auth/oidc/store/renewal/worker_test.go | 73 +++++++++++++++++++++++++- auth/oidc/token.go | 48 +++++++++++------ auth/oidc/token_test.go | 12 ++++- auth/oidc/types.go | 2 + rest/auth.go | 25 +-------- rest/prerelease_test.go | 66 ++++++++++++++--------- rest/security_test.go | 2 +- rest/users.go | 23 ++++---- storage/local/path.go | 9 ++-- storage/local/write.go | 6 +++ storage/local/write_test.go | 25 +++++++++ 12 files changed, 210 insertions(+), 86 deletions(-) diff --git a/auth/oidc/store/renewal/renew.go b/auth/oidc/store/renewal/renew.go index 9d6183f..ce0deba 100644 --- a/auth/oidc/store/renewal/renew.go +++ b/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) diff --git a/auth/oidc/store/renewal/worker_test.go b/auth/oidc/store/renewal/worker_test.go index 336a240..daab760 100644 --- a/auth/oidc/store/renewal/worker_test.go +++ b/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) + } +} diff --git a/auth/oidc/token.go b/auth/oidc/token.go index 57881c9..e69dfe2 100644 --- a/auth/oidc/token.go +++ b/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 +} diff --git a/auth/oidc/token_test.go b/auth/oidc/token_test.go index 682e4d8..cf41d66 100644 --- a/auth/oidc/token_test.go +++ b/auth/oidc/token_test.go @@ -43,6 +43,7 @@ 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"} }}, @@ -50,8 +51,12 @@ func TestUpdateOAuth2DataWithTokenValidatesClaims(t *testing.T) { {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) + } }) } } diff --git a/auth/oidc/types.go b/auth/oidc/types.go index 3356d1a..b1f50e3 100644 --- a/auth/oidc/types.go +++ b/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"` } diff --git a/rest/auth.go b/rest/auth.go index 56d2312..98a1f4a 100644 --- a/rest/auth.go +++ b/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 diff --git a/rest/prerelease_test.go b/rest/prerelease_test.go index 1b0a5f4..4e233dc 100644 --- a/rest/prerelease_test.go +++ b/rest/prerelease_test.go @@ -95,9 +95,10 @@ func TestSAMLCallbackCodeSingleUse(t *testing.T) { } } -// TestExternalUserLicenseLimit: SSO logins can't create users beyond the -// license limit. Existing users can still log in. -func TestExternalUserLicenseLimit(t *testing.T) { +// TestExternalUserNotBlockedByLicense: new SSO users are created even when the +// user count is at the license limit (the license count can fall back to a low +// default when license detection fails at boot). +func TestExternalUserNotBlockedByLicense(t *testing.T) { storage := &memorystorage.MockMemoryStorage{} userStore, err := users.NewUserStore(storage, 100) if err != nil { @@ -116,22 +117,17 @@ func TestExternalUserLicenseLimit(t *testing.T) { c.SAML.Client.CreateSession(saml.SessionKey{ProviderID: samlProvider.ID, SessionID: "new"}, saml.AuthenticatedUser{ID: "1", Login: "new@example.com", ExpiresAt: time.Now().Add(time.Hour)}) status, loginResponse := postSAMLCallback(c, samlProvider.ID, "new") - if status != http.StatusOK || !loginResponse.NoLicense || loginResponse.Authenticated || loginResponse.Token != "" { - t.Fatalf("expected noLicense response, got %d %+v", status, loginResponse) + if status != http.StatusOK || !loginResponse.Authenticated || loginResponse.NoLicense { + t.Fatalf("expected new user to log in, got %d %+v", status, loginResponse) } - if c.UserStore.LoginExists("new@example.com") { - t.Fatalf("user was created beyond the license limit") - } - - c.SAML.Client.CreateSession(saml.SessionKey{ProviderID: samlProvider.ID, SessionID: "existing"}, saml.AuthenticatedUser{ID: "2", Login: "existing@example.com", ExpiresAt: time.Now().Add(time.Hour)}) - status, loginResponse = postSAMLCallback(c, samlProvider.ID, "existing") - if status != http.StatusOK || !loginResponse.Authenticated { - t.Fatalf("expected existing user to log in, got %d %+v", status, loginResponse) + if !c.UserStore.LoginExists("new@example.com") { + t.Fatalf("new user was not created") } } -// TestExternalUserLinking: an SSO login is not linked to a user that is bound -// to another type of SSO. +// TestExternalUserLinking: users can switch between saml and oidc logins, but an +// oidc login with an unverified email address can't take over a user that +// doesn't log in with oidc yet. func TestExternalUserLinking(t *testing.T) { c := newTestContext(t) if _, err := c.UserStore.AddUser(users.User{Login: "saml@example.com", SAMLID: "saml-1"}); err != nil { @@ -140,20 +136,38 @@ func TestExternalUserLinking(t *testing.T) { if _, err := c.UserStore.AddUser(users.User{Login: "oidc@example.com", OIDCID: "oidc-1"}); err != nil { t.Fatalf("cannot create user: %s", err) } - if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "saml@example.com", "oidc", "oidc-2"); err == nil { - t.Fatalf("expected oidc login to be refused for saml user") + if _, err := c.UserStore.AddUser(users.User{Login: "local@example.com", Password: "localpass"}); err != nil { + t.Fatalf("cannot create user: %s", err) } - if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "oidc@example.com", "saml", "saml-2"); err == nil { - t.Fatalf("expected saml login to be refused for oidc user") + unverified := externalUserOptions{EmailNotVerified: true} + + // unverified email: refused for users that don't log in with oidc yet + for _, login := range []string{"saml@example.com", "local@example.com"} { + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, login, "oidc", "oidc-x", unverified); err == nil { + t.Fatalf("expected oidc login with unverified email to be refused for %s", login) + } + user, _ := c.UserStore.GetUserByLogin(login) + if user.OIDCID != "" { + t.Fatalf("oidc id was linked for %s", login) + } } - samlUser, _ := c.UserStore.GetUserByLogin("saml@example.com") - oidcUser, _ := c.UserStore.GetUserByLogin("oidc@example.com") - if samlUser.OIDCID != "" || oidcUser.SAMLID != "" { - t.Fatalf("external ids were overwritten: %+v %+v", samlUser, oidcUser) + // unverified email: allowed for an existing oidc user and for a new user + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "oidc@example.com", "oidc", "oidc-2", unverified); err != nil { + t.Fatalf("expected oidc login for existing oidc user: %s", err) + } + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "new@example.com", "oidc", "oidc-3", unverified); err != nil { + t.Fatalf("expected new oidc user to be created: %s", err) + } + // switching between saml and oidc (verified email) works + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "saml@example.com", "oidc", "oidc-4", externalUserOptions{}); err != nil { + t.Fatalf("expected oidc login for saml user: %s", err) } - // same type of login still works - if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "oidc@example.com", "oidc", "oidc-3"); err != nil { - t.Fatalf("expected oidc login for oidc user: %s", err) + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "oidc@example.com", "saml", "saml-2", externalUserOptions{}); err != nil { + t.Fatalf("expected saml login for oidc user: %s", err) + } + samlUser, _ := c.UserStore.GetUserByLogin("saml@example.com") + if samlUser.OIDCID != "oidc-4" { + t.Fatalf("expected saml user to be linked to oidc, got %+v", samlUser) } } diff --git a/rest/security_test.go b/rest/security_test.go index ed9c4be..4a3fdaa 100644 --- a/rest/security_test.go +++ b/rest/security_test.go @@ -406,7 +406,7 @@ func TestExternalLoginReactivatesConnections(t *testing.T) { if err != nil { t.Fatalf("cannot create user: %s", err) } - if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, c.LicenseUserCount, "john@example.com", "oidc", "oidc-1"); err != nil { + if _, err := addOrModifyExternalUser(c.Storage.Client, c.UserStore, "john@example.com", "oidc", "oidc-1", externalUserOptions{}); err != nil { t.Fatalf("addOrModifyExternalUser error: %s", err) } updated, err := c.UserStore.GetUserByID(user.ID) diff --git a/rest/users.go b/rest/users.go index 532d7db..4a20262 100644 --- a/rest/users.go +++ b/rest/users.go @@ -2,7 +2,6 @@ package rest import ( "encoding/json" - "errors" "fmt" "net/http" "time" @@ -237,22 +236,21 @@ func (c *Context) userHandler(w http.ResponseWriter, r *http.Request) { } } -// errNoLicense is returned when a new user can't be added because the user -// license limit has been reached -var errNoLicense = errors.New("no more licenses available") +// externalUserOptions are optional checks for addOrModifyExternalUser +type externalUserOptions struct { + // EmailNotVerified: the IdP says the email address (login) is not verified + EmailNotVerified bool +} -func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, licenseUserCount int, login, authType, externalAuthID string) (users.User, error) { +func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, login, authType, externalAuthID string, options externalUserOptions) (users.User, error) { if userStore.LoginExists(login) { existingUser, err := userStore.GetUserByLogin(login) if err != nil { return existingUser, fmt.Errorf("couldn't find existing user in database: %s", login) } - // don't link a login to a user that is bound to another type of external auth - if authType == "oidc" && existingUser.SAMLID != "" { - return users.User{}, fmt.Errorf("user %s is linked to a saml login, can't login with oidc", login) - } - if authType == "saml" && existingUser.OIDCID != "" { - return users.User{}, fmt.Errorf("user %s is linked to an oidc login, can't login with saml", login) + // an unverified email address can only be used to log in to a user that already logs in with oidc + if authType == "oidc" && options.EmailNotVerified && existingUser.OIDCID == "" { + return users.User{}, fmt.Errorf("email address %s is not verified by the identity provider", login) } if authType == "oidc" { @@ -279,9 +277,6 @@ func addOrModifyExternalUser(storage storage.Iface, userStore *users.UserStore, } return existingUser, nil } else { - if userStore.UserCount() >= licenseUserCount { - return users.User{}, errNoLicense - } newUser := users.User{ Login: login, Role: "user", diff --git a/storage/local/path.go b/storage/local/path.go index 9e6571c..17c9d99 100644 --- a/storage/local/path.go +++ b/storage/local/path.go @@ -86,9 +86,12 @@ func (l *LocalStorage) ReadDir(pathname string) ([]string, error) { if err != nil { return []string{}, err } - resNames := make([]string, len(res)) - for k, v := range res { - resNames[k] = v.Name() + resNames := make([]string, 0, len(res)) + for _, v := range res { + if isTempFile(v.Name()) { // left behind by an interrupted WriteFile + continue + } + resNames = append(resNames, v.Name()) } return resNames, nil } diff --git a/storage/local/write.go b/storage/local/write.go index 79d024a..617e925 100644 --- a/storage/local/write.go +++ b/storage/local/write.go @@ -5,6 +5,7 @@ import ( "io" "os" "path/filepath" + "strings" ) // WriteFile writes the data atomically: the data is written to a temporary file in the same directory, @@ -119,3 +120,8 @@ func (l *LocalStorage) OpenFileForAppending(name string) (io.WriteCloser, error) } return file, nil } + +// isTempFile returns true for the temporary files WriteFile creates (..tmp-) +func isTempFile(name string) bool { + return strings.HasPrefix(name, ".") && strings.Contains(name, ".tmp-") +} diff --git a/storage/local/write_test.go b/storage/local/write_test.go index e169cc1..4984c68 100644 --- a/storage/local/write_test.go +++ b/storage/local/write_test.go @@ -121,3 +121,28 @@ func TestWriteFileReadOnlyDirFallback(t *testing.T) { t.Fatalf("unexpected contents: %s", data) } } + +// TestReadDirSkipsTempFiles: a temporary file left behind by an interrupted +// WriteFile must not show up in directory listings (callers parse every file). +func TestReadDirSkipsTempFiles(t *testing.T) { + l, err := NewWithPath(t.TempDir()) + if err != nil { + t.Fatalf("new storage: %s", err) + } + if err := l.EnsurePath("clients"); err != nil { + t.Fatalf("ensure path: %s", err) + } + if err := l.WriteFile("clients/a.json", []byte(`{}`)); err != nil { + t.Fatalf("write: %s", err) + } + if err := os.WriteFile(filepath.Join(l.path, "clients", ".a.json.tmp-123"), []byte(`{"trunc`), 0600); err != nil { + t.Fatalf("write temp file: %s", err) + } + names, err := l.ReadDir("clients") + if err != nil { + t.Fatalf("read dir: %s", err) + } + if len(names) != 1 || names[0] != "a.json" { + t.Fatalf("expected only a.json, got %v", names) + } +} From 0865ed6305435366dfd50f6c95534822fb36aeed Mon Sep 17 00:00:00 2001 From: Edward Viaene Date: Sat, 10 Oct 2026 14:16:29 -0500 Subject: [PATCH 2/2] keep the saved SCIM token after a restart The SCIM client created at startup didn't get the token from the saved config, so SCIM requests returned 401 after a restart or upgrade until the token was regenerated. Co-Authored-By: Claude Opus 5.5 --- rest/context.go | 4 ++++ rest/prerelease_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/rest/context.go b/rest/context.go index fca66a5..eb76174 100644 --- a/rest/context.go +++ b/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{} diff --git a/rest/prerelease_test.go b/rest/prerelease_test.go index 4e233dc..b8b119d 100644 --- a/rest/prerelease_test.go +++ b/rest/prerelease_test.go @@ -11,6 +11,7 @@ import ( "os" "strings" "testing" + "testing/fstest" "time" "github.com/golang-jwt/jwt/v5" @@ -374,3 +375,38 @@ func TestOIDCProviderClientSecretNotReturned(t *testing.T) { t.Fatalf("listing removed the stored client secret") } } + +// TestSCIMTokenKeptAfterRestart: the saved SCIM token keeps working when the +// server starts with a new scim instance (restart / upgrade). +func TestSCIMTokenKeptAfterRestart(t *testing.T) { + storage := &memorystorage.MockMemoryStorage{} + userStore, err := users.NewUserStore(storage, 100) + if err != nil { + t.Fatalf("userstore error: %s", err) + } + c, err := newContextWithParams(storage, SERVER_TYPE_VPN, userStore, scim.New(storage, userStore, ""), 100, "", map[string]AppClient{}) + if err != nil { + t.Fatalf("cannot create context: %s", err) + } + c.SetupCompleted = true + c.SCIM.EnableSCIM = true + c.SCIM.Token = "saved-scim-token" + c.SCIM.Client.UpdateToken(c.SCIM.Token) + if err := SaveConfig(c); err != nil { + t.Fatalf("save config error: %s", err) + } + + // restart: new context from the saved config, with a new scim instance without token + restarted, err := newContextWithParams(storage, SERVER_TYPE_VPN, userStore, scim.New(storage, userStore, ""), 100, "", map[string]AppClient{}) + if err != nil { + t.Fatalf("cannot create context: %s", err) + } + mux := restarted.getRouter(fstest.MapFS{}, []byte("")) + req := httptest.NewRequest(http.MethodGet, "http://example.com/api/scim/v2/Users", nil) + req.Header.Set("Authorization", "Bearer saved-scim-token") + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + if w.Result().StatusCode != http.StatusOK { + t.Fatalf("expected 200 with the saved scim token after restart, got %d: %s", w.Result().StatusCode, w.Body.String()) + } +}