From 42a97be25f45ca3db50ebf80a984f85462f5ce83 Mon Sep 17 00:00:00 2001 From: Michael Lopez Date: Mon, 2 Nov 2026 20:22:23 +0100 Subject: [PATCH] state: validate the new name and refuse OIDC users in RenameUser (#3642) State.RenameUser went through UpdateUser and skipped the checks db.RenameUser applies, so the API could rename an OIDC user, whose name the identity provider owns, or rename any user to an invalid name. Apply both checks in the update function and map the username validation errors to 400. --- CHANGELOG.md | 1 + hscontrol/api/v1/errors.go | 5 +++++ hscontrol/apiv1_users_test.go | 30 ++++++++++++++++++++++++++++++ hscontrol/state/state.go | 13 ++++++++++++- 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c26901cf..4cca5bb76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -135,6 +135,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`, - API errors are now RFC 7807 `application/problem+json`, including authentication failures, instead of the previous gRPC-status JSON shape [#3324](https://github.com/juanfont/headscale/pull/3324) - Errors that previously returned HTTP 500 — unknown users or nodes, malformed input, duplicate names — now return the correct 404, 400 or 409 [#3324](https://github.com/juanfont/headscale/pull/3324) - The OpenAPI document is OpenAPI 3.1 at `/api/v1/openapi.yaml` (docs at `/api/v1/docs`), replacing Swagger 2.0 at `/swagger` [#3324](https://github.com/juanfont/headscale/pull/3324) +- Renaming a user (`headscale users rename`, `POST /api/v1/user/{id}/rename/{name}`) now returns 400 for an OIDC user, whose name comes from the identity provider, and for an invalid user name; both were previously accepted - V2 hides spent, expired, and revoked auth keys. OAuth key lists need `all=true` for keys created without tokens. Tags do not limit lists. V1 and `headscale preauthkeys list` hide revoked keys. Deleting referenced ephemeral keys revokes them. Referenced rows stay. [#3619](https://github.com/juanfont/headscale/pull/3619) - Tagged OAuth clients with `all` cannot issue tokens outside their tags. OAuth retagging requires permission for all current and requested tags. It replaces the set. Disallowed tags return HTTP 400. [#3619](https://github.com/juanfont/headscale/pull/3619) diff --git a/hscontrol/api/v1/errors.go b/hscontrol/api/v1/errors.go index 91c426b2a..b83b04bdb 100644 --- a/hscontrol/api/v1/errors.go +++ b/hscontrol/api/v1/errors.go @@ -7,6 +7,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/state" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" "gorm.io/gorm" ) @@ -45,6 +46,10 @@ func mapError(msg string, err error) error { errors.Is(err, types.ErrHostinfoTooLarge), errors.Is(err, db.ErrUserStillHasNodes), errors.Is(err, db.ErrCannotChangeOIDCUser), + errors.Is(err, util.ErrUsernameTooShort), + errors.Is(err, util.ErrUsernameMustStartLetter), + errors.Is(err, util.ErrUsernameTooManyAt), + errors.Is(err, util.ErrUsernameInvalidChar), errors.Is(err, db.ErrPreAuthKeyNotTaggedOrOwned), errors.Is(err, db.ErrSingleUseAuthKeyHasBeenUsed), errors.Is(err, state.ErrInvalidPolicy): diff --git a/hscontrol/apiv1_users_test.go b/hscontrol/apiv1_users_test.go index 428ba21c6..dd8d171ea 100644 --- a/hscontrol/apiv1_users_test.go +++ b/hscontrol/apiv1_users_test.go @@ -5,6 +5,8 @@ import ( "net/http" "testing" + "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -68,6 +70,34 @@ func TestAPIV1RenameUser(t *testing.T) { res := h.assertParity(t, http.MethodPost, "/api/v1/user/abc/rename/bob", nil) assertStatus(t, res, http.StatusBadRequest) }) + + t.Run("oidc user refused", func(t *testing.T) { + h := newAPIV1Harness(t) + user := h.app.state.CreateUserForTest("alice") + require.NoError(t, h.app.state.DB().DB.Model(user). + Update("provider", util.RegisterMethodOIDC).Error) + + res := h.callHuma(http.MethodPost, "/api/v1/user/1/rename/bob", nil) + assertStatus(t, res, http.StatusBadRequest) + + got, err := h.app.state.GetUserByID(types.UserID(user.ID)) + require.NoError(t, err) + assert.Equal(t, "alice", got.Name) + }) + + for _, name := range []string{"b", "1bob", "bob!", "bob@@example.com"} { + t.Run("invalid name "+name+" refused", func(t *testing.T) { + h := newAPIV1Harness(t) + seedUsers("alice")(t, h.app) + + res := h.callHuma(http.MethodPost, "/api/v1/user/1/rename/"+name, nil) + assertStatus(t, res, http.StatusBadRequest) + + got, err := h.app.state.GetUserByID(1) + require.NoError(t, err) + assert.Equal(t, "alice", got.Name) + }) + } } func TestAPIV1DeleteUser(t *testing.T) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 08a68270a..57ef214e6 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -733,10 +733,21 @@ func (s *State) DeleteUser(userID types.UserID) (change.Change, error) { return c, nil } -// RenameUser changes a user's name. The new name must be unique. +// RenameUser changes a user's name. The new name must be unique and a valid +// username. OIDC users are refused: their name comes from the identity provider. func (s *State) RenameUser(userID types.UserID, newName string) (*types.User, change.Change, error) { return s.UpdateUser(userID, func(user *types.User) error { + err := util.ValidateUsername(newName) + if err != nil { + return err + } + + if user.Provider == util.RegisterMethodOIDC { + return hsdb.ErrCannotChangeOIDCUser + } + user.Name = newName + return nil }) }