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

@@ -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)

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):

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) {

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
 	})
 }