diff --git a/docs/v1-ogen/CHANGES.md b/docs/v1-ogen/CHANGES.md index a3a6927b..90da17e8 100644 --- a/docs/v1-ogen/CHANGES.md +++ b/docs/v1-ogen/CHANGES.md @@ -43,6 +43,18 @@ status code itself is unchanged for equivalent conditions (e.g. unknown user → ## Behaviour +### Unknown resources return 404 consistently + +**What:** operations that look up a resource by id return `404` when it does not +exist. Several gRPC handlers (e.g. `RenameUser`, `DeleteUser`) returned a plain +Go error, which grpc-gateway rendered as `500`; only a few (e.g. `GetNode`) used +an explicit not-found status. + +**Why:** a missing resource is a client error, not a server error; 404 is the +correct, consistent status. + +**Client impact:** clients that treated these as 500 should treat them as 404. + ### Health on database failure **What:** `GET /api/v1/health` returns `200 {"databaseConnectivity": true}` when diff --git a/hscontrol/api/v1/convert.go b/hscontrol/api/v1/convert.go new file mode 100644 index 00000000..f0788527 --- /dev/null +++ b/hscontrol/api/v1/convert.go @@ -0,0 +1,58 @@ +package apiv1 + +import ( + oas "github.com/juanfont/headscale/gen/api/v1" + v1 "github.com/juanfont/headscale/gen/go/headscale/v1" + "google.golang.org/protobuf/types/known/timestamppb" +) + +// This file bridges the existing proto response builders (the Proto() methods +// on the state types) to the ogen API types. Reusing Proto() guarantees the +// new HTTP API surfaces exactly the same data the gRPC/gateway stack did +// (username fallback, masked key prefixes, online computation, the +// TaggedDevices substitution, …) without reimplementing it. +// +// Unlike grpc-gateway (which marshalled with EmitUnpopulated), these converters +// omit zero-value and absent fields — empty strings, false booleans, zero +// numbers, empty arrays, nil timestamps/objects, and the unspecified register +// method. See docs/v1-ogen/CHANGES.md. When the proto stack is removed, these +// converters are rewritten to read the state types directly. +// +// Converters are added here as each resource group is migrated. + +func optString(s string) oas.OptString { + if s == "" { + return oas.OptString{} + } + + return oas.NewOptString(s) +} + +func optUint64(v uint64) oas.OptUint64 { + if v == 0 { + return oas.OptUint64{} + } + + return oas.NewOptUint64(v) +} + +func optTime(ts *timestamppb.Timestamp) oas.OptDateTime { + if ts == nil { + return oas.OptDateTime{} + } + + return oas.NewOptDateTime(ts.AsTime()) +} + +func oasUser(u *v1.User) oas.User { + return oas.User{ + ID: optUint64(u.GetId()), + Name: optString(u.GetName()), + CreatedAt: optTime(u.GetCreatedAt()), + DisplayName: optString(u.GetDisplayName()), + Email: optString(u.GetEmail()), + ProviderId: optString(u.GetProviderId()), + Provider: optString(u.GetProvider()), + ProfilePicUrl: optString(u.GetProfilePicUrl()), + } +} diff --git a/hscontrol/api/v1/errors.go b/hscontrol/api/v1/errors.go index 9be73a94..e174fbeb 100644 --- a/hscontrol/api/v1/errors.go +++ b/hscontrol/api/v1/errors.go @@ -7,6 +7,7 @@ import ( "net/http" oas "github.com/juanfont/headscale/gen/api/v1" + "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/state" "github.com/juanfont/headscale/hscontrol/types" "github.com/rs/zerolog/log" @@ -54,7 +55,8 @@ func internalError(detail string) *oas.ErrorStatusCode { func mapStateError(err error) *oas.ErrorStatusCode { switch { case errors.Is(err, gorm.ErrRecordNotFound), - errors.Is(err, state.ErrNodeNotFound): + errors.Is(err, state.ErrNodeNotFound), + errors.Is(err, db.ErrUserNotFound): return notFound(err.Error()) case errors.Is(err, types.ErrPolicyUpdateIsDisabled): return badRequest(err.Error()) diff --git a/hscontrol/api/v1/users.go b/hscontrol/api/v1/users.go new file mode 100644 index 00000000..fd9021da --- /dev/null +++ b/hscontrol/api/v1/users.go @@ -0,0 +1,112 @@ +package apiv1 + +import ( + "cmp" + "context" + "slices" + + oas "github.com/juanfont/headscale/gen/api/v1" + "github.com/juanfont/headscale/hscontrol/types" + "gorm.io/gorm" +) + +// CreateUser creates a user and distributes the resulting policy change. +func (s *Server) CreateUser( + _ context.Context, + req *oas.CreateUserReq, +) (*oas.CreateUserOK, error) { + newUser := types.User{ + Name: req.Name.Or(""), + DisplayName: req.DisplayName.Or(""), + Email: req.Email.Or(""), + ProfilePicURL: req.PictureUrl.Or(""), + } + + user, policyChanged, err := s.state.CreateUser(newUser) + if err != nil { + return nil, internalError("creating user: " + err.Error()) + } + + s.change(policyChanged) + + return &oas.CreateUserOK{User: oas.NewOptUser(oasUser(user.Proto()))}, nil +} + +// ListUsers lists users, optionally filtered by id, name, or email, sorted by id. +func (s *Server) ListUsers( + _ context.Context, + params oas.ListUsersParams, +) (*oas.ListUsersOK, error) { + var ( + users []types.User + err error + ) + + switch { + case params.Name.Or("") != "": + users, err = s.state.ListUsersWithFilter(&types.User{Name: params.Name.Or("")}) + case params.Email.Or("") != "": + users, err = s.state.ListUsersWithFilter(&types.User{Email: params.Email.Or("")}) + case params.ID.Or(0) != 0: + users, err = s.state.ListUsersWithFilter( + &types.User{Model: gorm.Model{ID: uint(params.ID.Or(0))}}, + ) + default: + users, err = s.state.ListAllUsers() + } + + if err != nil { + return nil, mapStateError(err) + } + + slices.SortFunc(users, func(a, b types.User) int { return cmp.Compare(a.ID, b.ID) }) + + out := make([]oas.User, len(users)) + for i := range users { + out[i] = oasUser(users[i].Proto()) + } + + return &oas.ListUsersOK{Users: out}, nil +} + +// RenameUser renames a user and distributes the resulting policy change. +func (s *Server) RenameUser( + _ context.Context, + params oas.RenameUserParams, +) (*oas.RenameUserOK, error) { + oldUser, err := s.state.GetUserByID(types.UserID(params.OldID)) + if err != nil { + return nil, mapStateError(err) + } + + _, c, err := s.state.RenameUser(types.UserID(oldUser.ID), params.NewName) + if err != nil { + return nil, mapStateError(err) + } + + s.change(c) + + newUser, err := s.state.GetUserByName(params.NewName) + if err != nil { + return nil, mapStateError(err) + } + + return &oas.RenameUserOK{User: oas.NewOptUser(oasUser(newUser.Proto()))}, nil +} + +// DeleteUser deletes a user and distributes the resulting policy change. +func (s *Server) DeleteUser(_ context.Context, params oas.DeleteUserParams) error { + user, err := s.state.GetUserByID(types.UserID(params.ID)) + if err != nil { + return mapStateError(err) + } + + policyChanged, err := s.state.DeleteUser(types.UserID(user.ID)) + if err != nil { + return mapStateError(err) + } + + s.change(policyChanged) + + return nil +} diff --git a/hscontrol/servertest/apiv1_helpers_test.go b/hscontrol/servertest/apiv1_helpers_test.go new file mode 100644 index 00000000..0114b987 --- /dev/null +++ b/hscontrol/servertest/apiv1_helpers_test.go @@ -0,0 +1,30 @@ +package servertest_test + +import ( + "testing" + + apiv1 "github.com/juanfont/headscale/gen/api/v1" + "github.com/juanfont/headscale/hscontrol/servertest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// apiClient spins up a server and returns an authenticated v1 API client plus +// the server, for HTTP-parity tests. +func apiClient(t *testing.T) (*servertest.TestServer, *apiv1.Client) { + t.Helper() + + srv := servertest.NewServer(t) + + return srv, srv.APIClient(t, srv.CreateAPIKey(t)) +} + +// requireProblem asserts that err is an RFC 7807 problem with the given HTTP +// status code. +func requireProblem(t *testing.T, err error, status int) { + t.Helper() + + var problem *apiv1.ErrorStatusCode + require.ErrorAsf(t, err, &problem, "expected *apiv1.ErrorStatusCode, got %T: %v", err, err) + assert.Equal(t, status, problem.StatusCode) +} diff --git a/hscontrol/servertest/apiv1_users_test.go b/hscontrol/servertest/apiv1_users_test.go new file mode 100644 index 00000000..bf4ee016 --- /dev/null +++ b/hscontrol/servertest/apiv1_users_test.go @@ -0,0 +1,94 @@ +package servertest_test + +import ( + "context" + "net/http" + "testing" + + apiv1 "github.com/juanfont/headscale/gen/api/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAPIv1_CreateUser(t *testing.T) { + srv, client := apiClient(t) + ctx := context.Background() + + resp, err := client.CreateUser(ctx, &apiv1.CreateUserReq{ + Name: apiv1.NewOptString("alice"), + DisplayName: apiv1.NewOptString("Alice"), + Email: apiv1.NewOptString("alice@example.com"), + }) + require.NoError(t, err) + + user := resp.User.Value + assert.NotZero(t, user.ID.Value) + assert.Equal(t, "alice", user.Name.Value) + assert.Equal(t, "Alice", user.DisplayName.Value) + assert.Equal(t, "alice@example.com", user.Email.Value) + + // Side effect: the user is persisted. + got, err := srv.State().GetUserByName("alice") + require.NoError(t, err) + assert.Equal(t, uint64(got.ID), user.ID.Value) +} + +func TestAPIv1_ListUsers(t *testing.T) { + srv, client := apiClient(t) + ctx := context.Background() + + srv.CreateUser(t, "alice") + srv.CreateUser(t, "bob") + + all, err := client.ListUsers(ctx, apiv1.ListUsersParams{}) + require.NoError(t, err) + require.Len(t, all.Users, 2) + // Sorted by id ascending. + assert.Less(t, all.Users[0].ID.Value, all.Users[1].ID.Value) + + byName, err := client.ListUsers(ctx, apiv1.ListUsersParams{Name: apiv1.NewOptString("bob")}) + require.NoError(t, err) + require.Len(t, byName.Users, 1) + assert.Equal(t, "bob", byName.Users[0].Name.Value) + + none, err := client.ListUsers(ctx, apiv1.ListUsersParams{Name: apiv1.NewOptString("nobody")}) + require.NoError(t, err) + assert.Empty(t, none.Users) +} + +func TestAPIv1_RenameUser(t *testing.T) { + srv, client := apiClient(t) + ctx := context.Background() + + u := srv.CreateUser(t, "alice") + + resp, err := client.RenameUser(ctx, apiv1.RenameUserParams{ + OldID: uint64(u.ID), + NewName: "alice2", + }) + require.NoError(t, err) + assert.Equal(t, "alice2", resp.User.Value.Name.Value) + + _, err = srv.State().GetUserByName("alice2") + require.NoError(t, err) + + // Unknown user is a 404 (the gRPC stack returned 500 here). + _, err = client.RenameUser(ctx, apiv1.RenameUserParams{OldID: 99999, NewName: "ghost"}) + requireProblem(t, err, http.StatusNotFound) +} + +func TestAPIv1_DeleteUser(t *testing.T) { + srv, client := apiClient(t) + ctx := context.Background() + + u := srv.CreateUser(t, "alice") + + require.NoError(t, client.DeleteUser(ctx, apiv1.DeleteUserParams{ID: uint64(u.ID)})) + + _, err := srv.State().GetUserByName("alice") + require.Error(t, err) + + // Deleting an unknown user is a 404. + err = client.DeleteUser(ctx, apiv1.DeleteUserParams{ID: 99999}) + requireProblem(t, err, http.StatusNotFound) +}