From cfaf41134d6a5c0c4ab49ad4f3e4564dd40a3e4e Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 19 Mar 2024 14:52:18 +0000 Subject: [PATCH 1/3] Update controlsocket to user userservice The controlsocket used the legacy state package to add and remove users. This swaps over the origin of the user service from mongo to dqlite. Permissions aren't migrated, as that still requires the mongo database. Once we transitioned away from legacy state for permissions this should be updated. Also I swapped out the fake/stub mocks in the test for real mocks. --- apiserver/authentication/user.go | 5 +- apiserver/registration_test.go | 4 +- .../stateauthenticator/authenticator_test.go | 3 +- apiserver/stateauthenticator/context.go | 3 +- .../stateauthenticator/services_mock_test.go | 3 +- .../agent/machine/manifolds.go | 12 +- domain/user/service/service.go | 12 +- domain/user/service/service_test.go | 2 +- internal/worker/controlsocket/manifold.go | 80 +++- internal/worker/controlsocket/package_test.go | 2 + .../controlsocket/services_mock_test.go | 141 +++++++ internal/worker/controlsocket/shim.go | 57 --- internal/worker/controlsocket/worker.go | 161 +++++--- internal/worker/controlsocket/worker_test.go | 343 ++++++------------ .../worker/httpserverargs/authenticator.go | 3 +- .../httpserverargs/services_mock_test.go | 3 +- internal/worker/httpserverargs/worker.go | 3 +- 17 files changed, 463 insertions(+), 374 deletions(-) create mode 100644 internal/worker/controlsocket/services_mock_test.go delete mode 100644 internal/worker/controlsocket/shim.go diff --git a/apiserver/authentication/user.go b/apiserver/authentication/user.go index 8b6f9584690..056c024308e 100644 --- a/apiserver/authentication/user.go +++ b/apiserver/authentication/user.go @@ -23,6 +23,7 @@ import ( coremacaroon "github.com/juju/juju/core/macaroon" coreuser "github.com/juju/juju/core/user" usererrors "github.com/juju/juju/domain/user/errors" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/state" ) @@ -38,7 +39,7 @@ var logger = loggo.GetLogger("juju.apiserver.authentication") // authenticate a user. type UserService interface { // GetUserByAuth returns the user with the given name and password. - GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + GetUserByAuth(ctx context.Context, name string, password auth.Password) (coreuser.User, error) // GetUserByName returns the user with the given name. GetUserByName(ctx context.Context, name string) (coreuser.User, error) } @@ -131,7 +132,7 @@ func (u *LocalUserAuthenticator) Authenticate( // We believe we've got a password, so we'll try to authenticate with it. // This will check the user service for the user, ensuring that the user // isn't disabled or deleted. - user, err := u.UserService.GetUserByAuth(ctx, userTag.Name(), authParams.Credentials) + user, err := u.UserService.GetUserByAuth(ctx, userTag.Name(), auth.NewPassword(authParams.Credentials)) if errors.Is(err, usererrors.NotFound) || errors.Is(err, usererrors.Unauthorized) { logger.Debugf("user %s not found", userTag.String()) return nil, errors.Trace(apiservererrors.ErrUnauthorized) diff --git a/apiserver/registration_test.go b/apiserver/registration_test.go index 254b6159092..98fd80b126e 100644 --- a/apiserver/registration_test.go +++ b/apiserver/registration_test.go @@ -99,7 +99,7 @@ func (s *registrationSuite) assertRegisterNoProxy(c *gc.C, hasProxy bool) { password := "hunter2" // It should be not possible to log in as bob with the password "hunter2" // now. - _, err := s.userService.GetUserByAuth(context.Background(), "bob", password) + _, err := s.userService.GetUserByAuth(context.Background(), "bob", auth.NewPassword(password)) c.Assert(err, jc.ErrorIs, usererrors.Unauthorized) validNonce := []byte(strings.Repeat("X", 24)) @@ -123,7 +123,7 @@ func (s *registrationSuite) assertRegisterNoProxy(c *gc.C, hasProxy bool) { // It should be possible to log in as bob with the // password "hunter2" now, and there should be no // secret key any longer. - user, err := s.userService.GetUserByAuth(context.Background(), "bob", password) + user, err := s.userService.GetUserByAuth(context.Background(), "bob", auth.NewPassword(password)) c.Assert(err, jc.ErrorIsNil) c.Check(user.UUID, gc.Equals, s.userUUID) diff --git a/apiserver/stateauthenticator/authenticator_test.go b/apiserver/stateauthenticator/authenticator_test.go index db6ac390157..73a82254616 100644 --- a/apiserver/stateauthenticator/authenticator_test.go +++ b/apiserver/stateauthenticator/authenticator_test.go @@ -15,6 +15,7 @@ import ( "github.com/juju/juju/apiserver/authentication" coreuser "github.com/juju/juju/core/user" + "github.com/juju/juju/internal/auth" statetesting "github.com/juju/juju/state/testing" ) @@ -55,7 +56,7 @@ func (s *agentAuthenticatorSuite) TestAuthenticatorForTag(c *gc.C) { c.Assert(err, jc.ErrorIsNil) c.Check(authenticator, gc.NotNil) - s.userService.EXPECT().GetUserByAuth(context.Background(), "user", "password").Return(user, nil).AnyTimes() + s.userService.EXPECT().GetUserByAuth(context.Background(), "user", auth.NewPassword("password")).Return(user, nil).AnyTimes() entity, err := authenticator.Authenticate(context.Background(), authentication.AuthParams{ AuthTag: tag, diff --git a/apiserver/stateauthenticator/context.go b/apiserver/stateauthenticator/context.go index 429c933b9fe..8e23c3cf010 100644 --- a/apiserver/stateauthenticator/context.go +++ b/apiserver/stateauthenticator/context.go @@ -24,6 +24,7 @@ import ( apiservererrors "github.com/juju/juju/apiserver/errors" coremacaroon "github.com/juju/juju/core/macaroon" coreuser "github.com/juju/juju/core/user" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/state" ) @@ -42,7 +43,7 @@ const ( // authenticate a user. type UserService interface { // GetUserByAuth returns the user with the given name and password. - GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + GetUserByAuth(ctx context.Context, name string, password auth.Password) (coreuser.User, error) // GetUserByName returns the user with the given name. GetUserByName(ctx context.Context, name string) (coreuser.User, error) // UpdateLastLogin updates the last login time for the user with the diff --git a/apiserver/stateauthenticator/services_mock_test.go b/apiserver/stateauthenticator/services_mock_test.go index abaea005fdf..5af5ca0723e 100644 --- a/apiserver/stateauthenticator/services_mock_test.go +++ b/apiserver/stateauthenticator/services_mock_test.go @@ -16,6 +16,7 @@ import ( authentication "github.com/juju/juju/apiserver/authentication" controller "github.com/juju/juju/controller" user "github.com/juju/juju/core/user" + auth "github.com/juju/juju/internal/auth" state "github.com/juju/juju/state" gomock "go.uber.org/mock/gomock" ) @@ -82,7 +83,7 @@ func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { } // GetUserByAuth mocks base method. -func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1, arg2 string) (user.User, error) { +func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1 string, arg2 auth.Password) (user.User, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetUserByAuth", arg0, arg1, arg2) ret0, _ := ret[0].(user.User) diff --git a/cmd/jujud-controller/agent/machine/manifolds.go b/cmd/jujud-controller/agent/machine/manifolds.go index 1f82f0c5c71..d2bad8da678 100644 --- a/cmd/jujud-controller/agent/machine/manifolds.go +++ b/cmd/jujud-controller/agent/machine/manifolds.go @@ -832,11 +832,13 @@ func commonManifolds(config ManifoldsConfig) dependency.Manifolds { // The controlsocket worker runs on the controller machine. controlSocketName: ifController(controlsocket.Manifold(controlsocket.ManifoldConfig{ - StateName: stateName, - Logger: loggo.GetLogger("juju.worker.controlsocket"), - NewWorker: controlsocket.NewWorker, - NewSocketListener: controlsocket.NewSocketListener, - SocketName: path.Join(agentConfig.DataDir(), "control.socket"), + ServiceFactoryName: serviceFactoryName, + Logger: loggo.GetLogger("juju.worker.controlsocket"), + NewWorker: controlsocket.NewWorker, + NewSocketListener: controlsocket.NewSocketListener, + SocketName: path.Join(agentConfig.DataDir(), "control.socket"), + // TODO (stickupkid): Remove state once we add permissions. + StateName: stateName, })), objectStoreName: ifController(objectstore.Manifold(objectstore.ManifoldConfig{ diff --git a/domain/user/service/service.go b/domain/user/service/service.go index c4c80ae8ed6..73a388605c5 100644 --- a/domain/user/service/service.go +++ b/domain/user/service/service.go @@ -183,24 +183,24 @@ func (s *Service) GetUserByName( // usererrors.NotFound will be returned. If supplied with an invalid user name // then an error that satisfies usererrors.UserNameNotValid will be returned. // It will not return users that have been previously removed. -// TODO (manadart 2024-02-13) Should this not accept a typed password? func (s *Service) GetUserByAuth( ctx context.Context, name string, - password string, + password auth.Password, ) (user.User, error) { if err := domainuser.ValidateUserName(name); err != nil { return user.User{}, errors.Annotatef(err, "validating username %q", name) } + if err := password.Validate(); err != nil { + return user.User{}, errors.Trace(err) + } - pass := auth.NewPassword(password) - - usr, err := s.st.GetUserByAuth(ctx, name, pass) + usr, err := s.st.GetUserByAuth(ctx, name, password) if err != nil { // We only need to ensure destruction on an error. // The happy path hashes the password in state, // and in so doing destroys it. - pass.Destroy() + password.Destroy() return user.User{}, errors.Annotatef(err, "getting user %q", name) } diff --git a/domain/user/service/service_test.go b/domain/user/service/service_test.go index 24ae9879d3a..2f7b53514f8 100644 --- a/domain/user/service/service_test.go +++ b/domain/user/service/service_test.go @@ -316,7 +316,7 @@ func (s *serviceSuite) TestGetUserByAuth(c *gc.C) { Name: "user", }, nil) - u, err := s.service().GetUserByAuth(context.Background(), "name", "pass") + u, err := s.service().GetUserByAuth(context.Background(), "name", auth.NewPassword("pass")) c.Assert(err, jc.ErrorIsNil) c.Check(u.UUID, gc.Equals, uuid) } diff --git a/internal/worker/controlsocket/manifold.go b/internal/worker/controlsocket/manifold.go index 15831cc7d99..e7d05c4184d 100644 --- a/internal/worker/controlsocket/manifold.go +++ b/internal/worker/controlsocket/manifold.go @@ -7,31 +7,28 @@ import ( "context" "github.com/juju/errors" + "github.com/juju/names/v5" "github.com/juju/worker/v4" "github.com/juju/worker/v4/dependency" + "github.com/juju/juju/core/permission" + "github.com/juju/juju/domain/servicefactory" "github.com/juju/juju/internal/socketlistener" "github.com/juju/juju/internal/worker/common" workerstate "github.com/juju/juju/internal/worker/state" "github.com/juju/juju/state" ) -// SocketListener describes a worker that listens on a unix socket. -type SocketListener interface { - worker.Worker -} - -func NewSocketListener(config socketlistener.Config) (SocketListener, error) { - return socketlistener.NewSocketListener(config) -} - // ManifoldConfig describes the dependencies required by the controlsocket worker. type ManifoldConfig struct { - StateName string - Logger Logger - NewWorker func(Config) (worker.Worker, error) - NewSocketListener func(socketlistener.Config) (SocketListener, error) - SocketName string + ServiceFactoryName string + Logger Logger + NewWorker func(Config) (worker.Worker, error) + NewSocketListener func(socketlistener.Config) (SocketListener, error) + SocketName string + + // TODO (stickupkid): Delete me once permissions are in place. + StateName string } // Manifold returns a Manifold that encapsulates the controlsocket worker. @@ -39,6 +36,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold { return dependency.Manifold{ Inputs: []string{ config.StateName, + config.ServiceFactoryName, }, Start: config.start, } @@ -46,8 +44,8 @@ func Manifold(config ManifoldConfig) dependency.Manifold { // Validate is called by start to check for bad configuration. func (cfg ManifoldConfig) Validate() error { - if cfg.StateName == "" { - return errors.NotValidf("empty StateName") + if cfg.ServiceFactoryName == "" { + return errors.NotValidf("empty ServiceFactoryName") } if cfg.Logger == nil { return errors.NotValidf("nil Logger") @@ -61,6 +59,9 @@ func (cfg ManifoldConfig) Validate() error { if cfg.SocketName == "" { return errors.NotValidf("empty SocketName") } + if cfg.StateName == "" { + return errors.NotValidf("empty StateName") + } return nil } @@ -87,9 +88,17 @@ func (cfg ManifoldConfig) start(ctx context.Context, getter dependency.Getter) ( } }() + var serviceFactory servicefactory.ControllerFactory + if err = getter.Get(cfg.ServiceFactoryName, &serviceFactory); err != nil { + return nil, errors.Trace(err) + } + var w worker.Worker w, err = cfg.NewWorker(Config{ - State: stateShim{st}, + UserService: serviceFactory.User(), + PermissionService: permissionService{ + state: st, + }, Logger: cfg.Logger, SocketName: cfg.SocketName, NewSocketListener: cfg.NewSocketListener, @@ -99,3 +108,40 @@ func (cfg ManifoldConfig) start(ctx context.Context, getter dependency.Getter) ( } return common.NewCleanupWorker(w, func() { _ = stTracker.Done() }), nil } + +// SocketListener describes a worker that listens on a unix socket. +type SocketListener interface { + worker.Worker +} + +// NewSocketListener is a function that creates a new socket listener. +func NewSocketListener(config socketlistener.Config) (SocketListener, error) { + return socketlistener.NewSocketListener(config) +} + +// TODO (stickupkid): Delete me once permissions are in place, this is just +// thin wrapper around the state to add user permissions. +type permissionService struct { + state *state.State +} + +func (p permissionService) AddUserPermission(ctx context.Context, username string, access permission.Access) error { + model, err := p.state.Model() + if err != nil { + return errors.Annotate(err, "getting model") + } + + if !names.IsValidUserName(username) { + return errors.NotValidf("invalid username %q", username) + } + + _, err = model.AddUser(state.UserAccessSpec{ + User: names.NewUserTag(username), + CreatedBy: names.NewUserTag(userCreator), + Access: access, + }) + if err != nil { + return errors.Annotate(err, "adding user") + } + return nil +} diff --git a/internal/worker/controlsocket/package_test.go b/internal/worker/controlsocket/package_test.go index 9d6bd277301..e04a9e525ed 100644 --- a/internal/worker/controlsocket/package_test.go +++ b/internal/worker/controlsocket/package_test.go @@ -9,6 +9,8 @@ import ( gc "gopkg.in/check.v1" ) +//go:generate go run go.uber.org/mock/mockgen -package controlsocket -destination services_mock_test.go github.com/juju/juju/internal/worker/controlsocket UserService,PermissionService + func Test(t *testing.T) { gc.TestingT(t) } diff --git a/internal/worker/controlsocket/services_mock_test.go b/internal/worker/controlsocket/services_mock_test.go new file mode 100644 index 00000000000..deafb233cd6 --- /dev/null +++ b/internal/worker/controlsocket/services_mock_test.go @@ -0,0 +1,141 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/internal/worker/controlsocket (interfaces: UserService,PermissionService) +// +// Generated by this command: +// +// mockgen -package controlsocket -destination services_mock_test.go github.com/juju/juju/internal/worker/controlsocket UserService,PermissionService +// + +// Package controlsocket is a generated GoMock package. +package controlsocket + +import ( + context "context" + reflect "reflect" + + permission "github.com/juju/juju/core/permission" + user "github.com/juju/juju/core/user" + service "github.com/juju/juju/domain/user/service" + auth "github.com/juju/juju/internal/auth" + gomock "go.uber.org/mock/gomock" +) + +// MockUserService is a mock of UserService interface. +type MockUserService struct { + ctrl *gomock.Controller + recorder *MockUserServiceMockRecorder +} + +// MockUserServiceMockRecorder is the mock recorder for MockUserService. +type MockUserServiceMockRecorder struct { + mock *MockUserService +} + +// NewMockUserService creates a new mock instance. +func NewMockUserService(ctrl *gomock.Controller) *MockUserService { + mock := &MockUserService{ctrl: ctrl} + mock.recorder = &MockUserServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { + return m.recorder +} + +// AddUser mocks base method. +func (m *MockUserService) AddUser(arg0 context.Context, arg1 service.AddUserArg) (user.UUID, []byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddUser", arg0, arg1) + ret0, _ := ret[0].(user.UUID) + ret1, _ := ret[1].([]byte) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// AddUser indicates an expected call of AddUser. +func (mr *MockUserServiceMockRecorder) AddUser(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddUser", reflect.TypeOf((*MockUserService)(nil).AddUser), arg0, arg1) +} + +// GetUserByAuth mocks base method. +func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1 string, arg2 auth.Password) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByAuth", arg0, arg1, arg2) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByAuth indicates an expected call of GetUserByAuth. +func (mr *MockUserServiceMockRecorder) GetUserByAuth(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByAuth", reflect.TypeOf((*MockUserService)(nil).GetUserByAuth), arg0, arg1, arg2) +} + +// GetUserByName mocks base method. +func (m *MockUserService) GetUserByName(arg0 context.Context, arg1 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByName", arg0, arg1) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByName indicates an expected call of GetUserByName. +func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) +} + +// RemoveUser mocks base method. +func (m *MockUserService) RemoveUser(arg0 context.Context, arg1 string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveUser", arg0, arg1) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveUser indicates an expected call of RemoveUser. +func (mr *MockUserServiceMockRecorder) RemoveUser(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveUser", reflect.TypeOf((*MockUserService)(nil).RemoveUser), arg0, arg1) +} + +// MockPermissionService is a mock of PermissionService interface. +type MockPermissionService struct { + ctrl *gomock.Controller + recorder *MockPermissionServiceMockRecorder +} + +// MockPermissionServiceMockRecorder is the mock recorder for MockPermissionService. +type MockPermissionServiceMockRecorder struct { + mock *MockPermissionService +} + +// NewMockPermissionService creates a new mock instance. +func NewMockPermissionService(ctrl *gomock.Controller) *MockPermissionService { + mock := &MockPermissionService{ctrl: ctrl} + mock.recorder = &MockPermissionServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPermissionService) EXPECT() *MockPermissionServiceMockRecorder { + return m.recorder +} + +// AddUserPermission mocks base method. +func (m *MockPermissionService) AddUserPermission(arg0 context.Context, arg1 string, arg2 permission.Access) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddUserPermission", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// AddUserPermission indicates an expected call of AddUserPermission. +func (mr *MockPermissionServiceMockRecorder) AddUserPermission(arg0, arg1, arg2 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddUserPermission", reflect.TypeOf((*MockPermissionService)(nil).AddUserPermission), arg0, arg1, arg2) +} diff --git a/internal/worker/controlsocket/shim.go b/internal/worker/controlsocket/shim.go deleted file mode 100644 index 8f5a4eebcf5..00000000000 --- a/internal/worker/controlsocket/shim.go +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright 2023 Canonical Ltd. -// Licensed under the AGPLv3, see LICENCE file for details. - -package controlsocket - -import ( - "github.com/juju/errors" - "github.com/juju/names/v5" - - "github.com/juju/juju/core/permission" - "github.com/juju/juju/state" -) - -// State defines the state methods that the controlsocket worker needs. -type State interface { - User(tag names.UserTag) (user, error) - AddUser(name string, displayName string, password string, creator string) (user, error) - RemoveUser(tag names.UserTag) error - Model() (model, error) -} - -// stateShim allows the real state to implement State. -type stateShim struct { - st *state.State -} - -func (s stateShim) User(tag names.UserTag) (user, error) { - u, err := s.st.User(tag) - return u, errors.Trace(err) -} - -func (s stateShim) AddUser(name, displayName, password, creator string) (user, error) { - u, err := s.st.AddUser(name, displayName, password, creator) - return u, errors.Trace(err) -} - -func (s stateShim) Model() (model, error) { - m, err := s.st.Model() - return m, errors.Trace(err) -} - -func (s stateShim) RemoveUser(tag names.UserTag) error { - return errors.Trace(s.st.RemoveUser(tag)) -} - -// model defines the model methods that the controlsocket worker needs. -type model interface { - AddUser(state.UserAccessSpec) (permission.UserAccess, error) -} - -// user defines the user methods that the controlsocket worker needs. -type user interface { - Name() string - CreatedBy() string - UserTag() names.UserTag - PasswordValid(string) bool -} diff --git a/internal/worker/controlsocket/worker.go b/internal/worker/controlsocket/worker.go index f4a97bde4fa..0c23f1dd19f 100644 --- a/internal/worker/controlsocket/worker.go +++ b/internal/worker/controlsocket/worker.go @@ -6,6 +6,7 @@ package controlsocket import ( + "context" "encoding/json" "fmt" "io" @@ -15,15 +16,16 @@ import ( "github.com/gorilla/mux" "github.com/juju/errors" - "github.com/juju/names/v5" "github.com/juju/worker/v4" "github.com/juju/worker/v4/catacomb" "github.com/juju/juju/core/permission" + "github.com/juju/juju/core/user" + usererrors "github.com/juju/juju/domain/user/errors" + "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs/bootstrap" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/socketlistener" - "github.com/juju/juju/state" - stateerrors "github.com/juju/juju/state/errors" ) const ( @@ -37,6 +39,50 @@ const ( userCreator = "controller@juju" ) +// UserService is the interface for the user service. +type UserService interface { + // AddUser will add a new user to the database and return the UUID of the + // user if successful. If no password is set in the incoming argument, + // the user will be added with an activation key. + // The following error types are possible from this function: + // - usererrors.UserNameNotValid: When the username supplied is not valid. + // - usererrors.AlreadyExists: If a user with the supplied name already exists. + // - usererrors.CreatorUUIDNotFound: If a creator has been supplied for the user + // and the creator does not exist. + // - auth.ErrPasswordNotValid: If the password supplied is not valid. + AddUser(ctx context.Context, arg service.AddUserArg) (user.UUID, []byte, error) + + // GetUserByName will find and return the user associated with name. If there is no + // user for the user name then an error that satisfies usererrors.NotFound will + // be returned. If supplied with an invalid user name then an error that satisfies + // usererrors.UserNameNotValid will be returned. + // + // GetUserByName will not return users that have been previously removed. + GetUserByName(ctx context.Context, name string) (user.User, error) + + // GetUserByAuth will find and return the user with UUID. If there is no + // user for the name and password, then an error that satisfies + // usererrors.NotFound will be returned. If supplied with an invalid user name + // then an error that satisfies usererrors.UserNameNotValid will be returned. + // It will not return users that have been previously removed. + GetUserByAuth(ctx context.Context, name string, password auth.Password) (user.User, error) + + // RemoveUser marks the user as removed and removes any credentials or + // activation codes for the current users. Once a user is removed they are no + // longer usable in Juju and should never be un removed. + // The following error types are possible from this function: + // - usererrors.UserNameNotValid: When the username supplied is not valid. + // - usererrors.NotFound: If no user by the given UUID exists. + RemoveUser(ctx context.Context, name string) error +} + +// PermissionService is the interface for the permission service. +type PermissionService interface { + // AddUserPermission adds a user to the model with the given access. + // If the user already has the given access, this is a no-op. + AddUserPermission(ctx context.Context, username string, access permission.Access) error +} + // Logger represents the methods used by the worker to log information. type Logger interface { Errorf(string, ...any) @@ -48,21 +94,25 @@ type Logger interface { // Config represents configuration for the controlsocket worker. type Config struct { - State State - Logger Logger + // UserService is the user service for the model. + UserService UserService + // PermissionService is the permission service for the model. + PermissionService PermissionService // SocketName is the socket file descriptor. SocketName string // NewSocketListener is the function that creates a new socket listener. NewSocketListener func(socketlistener.Config) (SocketListener, error) + // Logger is the logger used by the worker. + Logger Logger } // Validate returns an error if config cannot drive the Worker. func (config Config) Validate() error { - if config.State == nil { - return errors.NotValidf("nil State") + if config.UserService == nil { + return errors.NotValidf("nil UserService") } - if config.Logger == nil { - return errors.NotValidf("nil Logger") + if config.PermissionService == nil { + return errors.NotValidf("nil PermissionService") } if config.SocketName == "" { return errors.NotValidf("empty SocketName") @@ -70,13 +120,19 @@ func (config Config) Validate() error { if config.NewSocketListener == nil { return errors.NotValidf("nil NewSocketListener func") } + if config.Logger == nil { + return errors.NotValidf("nil Logger") + } return nil } // Worker is a controlsocket worker. type Worker struct { - config Config catacomb catacomb.Catacomb + + userService UserService + permissionService PermissionService + logger Logger } // NewWorker returns a controlsocket worker with the given config. @@ -86,7 +142,9 @@ func NewWorker(config Config) (worker.Worker, error) { } w := &Worker{ - config: config, + userService: config.UserService, + permissionService: config.PermissionService, + logger: config.Logger, } sl, err := config.NewSocketListener(socketlistener.Config{ Logger: config.Logger, @@ -100,7 +158,7 @@ func NewWorker(config Config) (worker.Worker, error) { err = catacomb.Invoke(catacomb.Plan{ Site: &w.catacomb, - Work: w.run, + Work: w.loop, Init: []worker.Worker{sl}, }) if err != nil { @@ -117,7 +175,7 @@ func (w *Worker) Wait() error { return w.catacomb.Wait() } -func (w *Worker) run() error { +func (w *Worker) loop() error { select { case <-w.catacomb.Dying(): return w.catacomb.ErrDying() @@ -148,7 +206,7 @@ func (w *Worker) handleAddMetricsUser(resp http.ResponseWriter, req *http.Reques return } - code, err := w.addMetricsUser(parsedBody.Username, parsedBody.Password) + code, err := w.addMetricsUser(req.Context(), parsedBody.Username, auth.NewPassword(parsedBody.Password)) if err != nil { w.writeResponse(resp, code, errorf("%v", err)) return @@ -157,23 +215,30 @@ func (w *Worker) handleAddMetricsUser(resp http.ResponseWriter, req *http.Reques w.writeResponse(resp, code, infof("created user %q", parsedBody.Username)) } -func (w *Worker) addMetricsUser(username, password string) (int, error) { +func (w *Worker) addMetricsUser(ctx context.Context, username string, password auth.Password) (int, error) { err := validateMetricsUsername(username) if err != nil { return http.StatusBadRequest, err } - if password == "" { - return http.StatusBadRequest, errors.NotValidf("empty password") + creatorUser, err := w.userService.GetUserByName(ctx, userCreator) + if err != nil { + return http.StatusInternalServerError, errors.Annotatef(err, "retrieving creator user %q: %v", userCreator, err) } - user, err := w.config.State.AddUser(username, username, password, userCreator) + _, _, err = w.userService.AddUser(ctx, service.AddUserArg{ + Name: username, + DisplayName: username, + Password: &password, + CreatorUUID: creatorUser.UUID, + }) + cleanup := true // Error handling here is a bit subtle. switch { - case errors.Is(err, errors.AlreadyExists): + case errors.Is(err, usererrors.AlreadyExists): // Retrieve existing user - user, err = w.config.State.User(names.NewUserTag(username)) + user, err := w.userService.GetUserByAuth(ctx, username, password) if err != nil { return http.StatusInternalServerError, fmt.Errorf("retrieving existing user %q: %v", username, err) @@ -183,11 +248,11 @@ func (w *Worker) addMetricsUser(username, password string) (int, error) { // worker shouldn't mess with users that have not been created by it. // So ensure the user is identical to what we would have created, and // otherwise error. - if user.CreatedBy() != userCreator { - return http.StatusConflict, errors.AlreadyExistsf("user %q (created by %q)", user.Name(), user.CreatedBy()) + if user.Disabled { + return http.StatusForbidden, errors.Forbiddenf("user %q is disabled", user.Name) } - if !user.PasswordValid(password) { - return http.StatusConflict, errors.AlreadyExistsf("user %q", user.Name()) + if user.CreatorName != userCreator { + return http.StatusConflict, errors.AlreadyExistsf("user %q (created by %q)", user.Name, user.CreatorName) } case err == nil: @@ -196,15 +261,15 @@ func (w *Worker) addMetricsUser(username, password string) (int, error) { // If there is an error granting permissions, we should attempt to "rollback" // and remove the user again. defer func() { - if cleanup == false { + if !cleanup { // Operation successful - nothing to clean up return } - err := w.config.State.RemoveUser(user.UserTag()) + err := w.userService.RemoveUser(ctx, username) if err != nil { // Best we can do here is log an error. - w.config.Logger.Warningf("add metrics user failed, but could not clean up user %q: %v", + w.logger.Warningf("add metrics user failed, but could not clean up user %q: %v", username, err) } }() @@ -213,18 +278,7 @@ func (w *Worker) addMetricsUser(username, password string) (int, error) { return http.StatusInternalServerError, errors.Annotatef(err, "creating user %q: %v", username, err) } - // Give the new user permission to access the metrics endpoint. - var model model - model, err = w.config.State.Model() - if err != nil { - return http.StatusInternalServerError, errors.Annotatef(err, "retrieving current model: %v", err) - } - - _, err = model.AddUser(state.UserAccessSpec{ - User: user.UserTag(), - CreatedBy: names.NewUserTag(userCreator), - Access: permission.ReadAccess, - }) + err = w.permissionService.AddUserPermission(ctx, username, permission.ReadAccess) if err != nil && !errors.Is(err, errors.AlreadyExists) { return http.StatusInternalServerError, errors.Annotatef(err, "adding user %q to model %q: %v", username, bootstrap.ControllerModelName, err) } @@ -235,7 +289,7 @@ func (w *Worker) addMetricsUser(username, password string) (int, error) { func (w *Worker) handleRemoveMetricsUser(resp http.ResponseWriter, req *http.Request) { username := mux.Vars(req)["username"] - code, err := w.removeMetricsUser(username) + code, err := w.removeMetricsUser(req.Context(), username) if err != nil { w.writeResponse(resp, code, errorf("%v", err)) return @@ -244,26 +298,25 @@ func (w *Worker) handleRemoveMetricsUser(resp http.ResponseWriter, req *http.Req w.writeResponse(resp, code, infof("deleted user %q", username)) } -func (w *Worker) removeMetricsUser(username string) (int, error) { +func (w *Worker) removeMetricsUser(ctx context.Context, username string) (int, error) { err := validateMetricsUsername(username) if err != nil { return http.StatusBadRequest, err } - userTag := names.NewUserTag(username) // We shouldn't mess with users that weren't created by us. - user, err := w.config.State.User(userTag) - if errors.Is(err, errors.NotFound) || errors.Is(err, errors.UserNotFound) || stateerrors.IsDeletedUserError(err) { + user, err := w.userService.GetUserByName(ctx, username) + if errors.Is(err, usererrors.NotFound) { // succeed as no-op return http.StatusOK, nil } else if err != nil { return http.StatusInternalServerError, err } - if user.CreatedBy() != userCreator { - return http.StatusForbidden, errors.Forbiddenf("cannot remove user %q created by %q", user.Name(), user.CreatedBy()) + if user.CreatorName != userCreator { + return http.StatusForbidden, errors.Forbiddenf("cannot remove user %q created by %q", user.Name, user.CreatorName) } - err = w.config.State.RemoveUser(userTag) + err = w.userService.RemoveUser(ctx, username) // Any "not found" errors should have been caught above, so fail here. if err != nil { return http.StatusInternalServerError, err @@ -277,10 +330,6 @@ func validateMetricsUsername(username string) error { return errors.BadRequestf("missing username") } - if !names.IsValidUserName(username) { - return errors.NotValidf("username %q", username) - } - if !strings.HasPrefix(username, jujuMetricsUserPrefix) { return errors.BadRequestf("metrics username %q should have prefix %q", username, jujuMetricsUserPrefix) } @@ -289,13 +338,13 @@ func validateMetricsUsername(username string) error { } func (w *Worker) writeResponse(resp http.ResponseWriter, statusCode int, body any) { - w.config.Logger.Debugf("operation finished with HTTP status %v", statusCode) + w.logger.Debugf("operation finished with HTTP status %v", statusCode) resp.Header().Set("Content-Type", "application/json") message, err := json.Marshal(body) if err != nil { - w.config.Logger.Errorf("error marshalling response body to JSON: %v", err) - w.config.Logger.Errorf("response body was %#v", body) + w.logger.Errorf("error marshalling response body to JSON: %v", err) + w.logger.Errorf("response body was %#v", body) // Mark this as an "internal server error" statusCode = http.StatusInternalServerError @@ -304,10 +353,10 @@ func (w *Worker) writeResponse(resp http.ResponseWriter, statusCode int, body an } resp.WriteHeader(statusCode) - w.config.Logger.Tracef("returning response %q", message) + w.logger.Tracef("returning response %q", message) _, err = resp.Write(message) if err != nil { - w.config.Logger.Warningf("error writing HTTP response: %v", err) + w.logger.Warningf("error writing HTTP response: %v", err) } } diff --git a/internal/worker/controlsocket/worker_test.go b/internal/worker/controlsocket/worker_test.go index 52fec59c09a..11d9d0edecf 100644 --- a/internal/worker/controlsocket/worker_test.go +++ b/internal/worker/controlsocket/worker_test.go @@ -6,27 +6,31 @@ package controlsocket import ( "context" "encoding/json" - "fmt" "io" "net" "net/http" "path" "strings" - "github.com/juju/errors" - "github.com/juju/loggo/v2" - "github.com/juju/names/v5" + jujutesting "github.com/juju/testing" jc "github.com/juju/testing/checkers" + "go.uber.org/mock/gomock" gc "gopkg.in/check.v1" "github.com/juju/juju/core/permission" - "github.com/juju/juju/state" - stateerrors "github.com/juju/juju/state/errors" + coreuser "github.com/juju/juju/core/user" + usererrors "github.com/juju/juju/domain/user/errors" + "github.com/juju/juju/domain/user/service" + auth "github.com/juju/juju/internal/auth" + "github.com/juju/juju/testing" ) type workerSuite struct { - state *fakeState - logger Logger + jujutesting.IsolationSuite + + logger Logger + userService *MockUserService + permissionService *MockPermissionService } var _ = gc.Suite(&workerSuite{}) @@ -42,17 +46,13 @@ type handlerTest struct { ignoreBody bool // if true, test will not read the request body } -func (s *workerSuite) SetUpTest(c *gc.C) { - s.state = &fakeState{} - s.logger = loggo.GetLogger(c.TestName()) -} - func (s *workerSuite) runHandlerTest(c *gc.C, test handlerTest) { tmpDir := c.MkDir() socket := path.Join(tmpDir, "test.socket") _, err := NewWorker(Config{ - State: s.state, + UserService: s.userService, + PermissionService: s.permissionService, Logger: s.logger, SocketName: socket, NewSocketListener: NewSocketListener, @@ -88,18 +88,9 @@ func (s *workerSuite) runHandlerTest(c *gc.C, test handlerTest) { } } -func (s *workerSuite) assertState(c *gc.C, users []fakeUser) { - c.Assert(len(s.state.users), gc.Equals, len(users)) - - for _, expected := range users { - actual, ok := s.state.users[expected.name] - c.Assert(ok, gc.Equals, true) - c.Check(actual.creator, gc.Equals, expected.creator) - c.Check(actual.password, gc.Equals, expected.password) - } -} - func (s *workerSuite) TestMetricsUsersAddInvalidMethod(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodGet, endpoint: "/metrics-users", @@ -109,6 +100,8 @@ func (s *workerSuite) TestMetricsUsersAddInvalidMethod(c *gc.C) { } func (s *workerSuite) TestMetricsUsersAddMissingBody(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -118,6 +111,8 @@ func (s *workerSuite) TestMetricsUsersAddMissingBody(c *gc.C) { } func (s *workerSuite) TestMetricsUsersAddInvalidBody(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -128,6 +123,8 @@ func (s *workerSuite) TestMetricsUsersAddInvalidBody(c *gc.C) { } func (s *workerSuite) TestMetricsUsersAddMissingUsername(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -137,17 +134,9 @@ func (s *workerSuite) TestMetricsUsersAddMissingUsername(c *gc.C) { }) } -func (s *workerSuite) TestMetricsUsersAddMissingPassword(c *gc.C) { - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0"}`, - statusCode: http.StatusBadRequest, - response: ".*empty password.*", - }) -} - func (s *workerSuite) TestMetricsUsersAddUsernameMissingPrefix(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -158,7 +147,19 @@ func (s *workerSuite) TestMetricsUsersAddUsernameMissingPrefix(c *gc.C) { } func (s *workerSuite) TestMetricsUsersAddSuccess(c *gc.C) { - s.state = newFakeState(nil) + defer s.setupMocks(c).Finish() + + s.userService.EXPECT().GetUserByName(gomock.Any(), userCreator).Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + }, nil) + s.userService.EXPECT().AddUser(gomock.Any(), service.AddUserArg{ + Name: "juju-metrics-r0", + DisplayName: "juju-metrics-r0", + Password: ptr(auth.NewPassword("bar")), + CreatorUUID: coreuser.UUID("deadbeef"), + }).Return(coreuser.UUID("foobar"), nil, nil) + s.permissionService.EXPECT().AddUserPermission(gomock.Any(), "juju-metrics-r0", permission.ReadAccess).Return(nil) + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -166,15 +167,24 @@ func (s *workerSuite) TestMetricsUsersAddSuccess(c *gc.C) { statusCode: http.StatusOK, response: `.*created user \\\"juju-metrics-r0\\\".*`, }) - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, - }) } func (s *workerSuite) TestMetricsUsersAddAlreadyExists(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, - }) + defer s.setupMocks(c).Finish() + + s.userService.EXPECT().GetUserByName(gomock.Any(), userCreator).Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + }, nil) + s.userService.EXPECT().AddUser(gomock.Any(), service.AddUserArg{ + Name: "juju-metrics-r0", + DisplayName: "juju-metrics-r0", + Password: ptr(auth.NewPassword("bar")), + CreatorUUID: coreuser.UUID("deadbeef"), + }).Return(coreuser.UUID("foobar"), nil, usererrors.AlreadyExists) + s.userService.EXPECT().GetUserByAuth(gomock.Any(), "juju-metrics-r0", auth.NewPassword("bar")).Return(coreuser.User{ + CreatorName: "not-you", + }, nil) + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -182,48 +192,51 @@ func (s *workerSuite) TestMetricsUsersAddAlreadyExists(c *gc.C) { statusCode: http.StatusConflict, response: ".*user .* already exists.*", }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "not-you"}, - }) } -func (s *workerSuite) TestMetricsUsersAddDifferentPassword(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: userCreator}, - }) - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusConflict, - response: `.*user \\\"juju-metrics-r0\\\" already exists.*`, - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: userCreator}, - }) -} +func (s *workerSuite) TestMetricsUsersAddAlreadyExistsButDisabled(c *gc.C) { + defer s.setupMocks(c).Finish() -func (s *workerSuite) TestMetricsUsersAddAddErr(c *gc.C) { - s.state = newFakeState(nil) - s.state.addErr = fmt.Errorf("spanner in the works") + s.userService.EXPECT().GetUserByName(gomock.Any(), userCreator).Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + }, nil) + s.userService.EXPECT().AddUser(gomock.Any(), service.AddUserArg{ + Name: "juju-metrics-r0", + DisplayName: "juju-metrics-r0", + Password: ptr(auth.NewPassword("bar")), + CreatorUUID: coreuser.UUID("deadbeef"), + }).Return(coreuser.UUID("foobar"), nil, usererrors.AlreadyExists) + s.userService.EXPECT().GetUserByAuth(gomock.Any(), "juju-metrics-r0", auth.NewPassword("bar")).Return(coreuser.User{ + CreatorName: "not-you", + Disabled: true, + }, nil) s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", + statusCode: http.StatusForbidden, + response: ".*user .* is disabled.*", }) - // Nothing should have changed. - s.assertState(c, nil) } func (s *workerSuite) TestMetricsUsersAddIdempotent(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) + defer s.setupMocks(c).Finish() + + s.userService.EXPECT().GetUserByName(gomock.Any(), userCreator).Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + }, nil) + s.userService.EXPECT().AddUser(gomock.Any(), service.AddUserArg{ + Name: "juju-metrics-r0", + DisplayName: "juju-metrics-r0", + Password: ptr(auth.NewPassword("bar")), + CreatorUUID: coreuser.UUID("deadbeef"), + }).Return(coreuser.UUID("foobar"), nil, usererrors.AlreadyExists) + s.userService.EXPECT().GetUserByAuth(gomock.Any(), "juju-metrics-r0", auth.NewPassword("bar")).Return(coreuser.User{ + CreatorName: userCreator, + }, nil) + s.permissionService.EXPECT().AddUserPermission(gomock.Any(), "juju-metrics-r0", permission.ReadAccess).Return(nil) + s.runHandlerTest(c, handlerTest{ method: http.MethodPost, endpoint: "/metrics-users", @@ -231,27 +244,11 @@ func (s *workerSuite) TestMetricsUsersAddIdempotent(c *gc.C) { statusCode: http.StatusOK, // succeed as a no-op response: `.*created user \\\"juju-metrics-r0\\\".*`, }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) -} - -func (s *workerSuite) TestMetricsUsersAddFailed(c *gc.C) { - s.state = newFakeState(nil) - s.state.model.err = fmt.Errorf("spanner in the works") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodPost, - endpoint: "/metrics-users", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", - }) - s.assertState(c, nil) } func (s *workerSuite) TestMetricsUsersRemoveInvalidMethod(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodGet, endpoint: "/metrics-users/foo", @@ -261,6 +258,8 @@ func (s *workerSuite) TestMetricsUsersRemoveInvalidMethod(c *gc.C) { } func (s *workerSuite) TestMetricsUsersRemoveUsernameMissingPrefix(c *gc.C) { + defer s.setupMocks(c).Finish() + s.runHandlerTest(c, handlerTest{ method: http.MethodDelete, endpoint: "/metrics-users/foo", @@ -270,48 +269,47 @@ func (s *workerSuite) TestMetricsUsersRemoveUsernameMissingPrefix(c *gc.C) { } func (s *workerSuite) TestMetricsUsersRemoveSuccess(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: "controller@juju"}, - }) + defer s.setupMocks(c).Finish() + + s.userService.EXPECT().GetUserByName(gomock.Any(), "juju-metrics-r0").Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + CreatorName: userCreator, + }, nil) + s.userService.EXPECT().RemoveUser(gomock.Any(), "juju-metrics-r0").Return(nil) + s.runHandlerTest(c, handlerTest{ method: http.MethodDelete, endpoint: "/metrics-users/juju-metrics-r0", statusCode: http.StatusOK, response: `.*deleted user \\\"juju-metrics-r0\\\".*`, }) - s.assertState(c, nil) } func (s *workerSuite) TestMetricsUsersRemoveForbidden(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, - }) + defer s.setupMocks(c).Finish() + + s.userService.EXPECT().GetUserByName(gomock.Any(), "juju-metrics-r0").Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + Name: "juju-metrics-r0", + CreatorName: "not-you", + }, nil) + s.runHandlerTest(c, handlerTest{ method: http.MethodDelete, endpoint: "/metrics-users/juju-metrics-r0", statusCode: http.StatusForbidden, response: `.*cannot remove user \\\"juju-metrics-r0\\\" created by \\\"not-you\\\".*`, }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "foo", creator: "not-you"}, - }) } func (s *workerSuite) TestMetricsUsersRemoveNotFound(c *gc.C) { - s.state = newFakeState(nil) - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - statusCode: http.StatusOK, // succeed as a no-op - response: `.*deleted user \\\"juju-metrics-r0\\\".*`, - }) - s.assertState(c, nil) -} + defer s.setupMocks(c).Finish() -func (s *workerSuite) TestMetricsUsersRemoveIdempotent(c *gc.C) { - s.state = newFakeState(nil) - s.state.userErr = stateerrors.NewDeletedUserError("juju-metrics-r0") + s.userService.EXPECT().GetUserByName(gomock.Any(), "juju-metrics-r0").Return(coreuser.User{ + UUID: coreuser.UUID("deadbeef"), + Name: "juju-metrics-r0", + CreatorName: "not-you", + }, usererrors.NotFound) s.runHandlerTest(c, handlerTest{ method: http.MethodDelete, @@ -319,120 +317,17 @@ func (s *workerSuite) TestMetricsUsersRemoveIdempotent(c *gc.C) { statusCode: http.StatusOK, // succeed as a no-op response: `.*deleted user \\\"juju-metrics-r0\\\".*`, }) - // Nothing should have changed. - s.assertState(c, nil) -} - -func (s *workerSuite) TestMetricsUsersRemoveFailed(c *gc.C) { - s.state = newFakeState([]fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) - s.state.removeErr = fmt.Errorf("spanner in the works") - - s.runHandlerTest(c, handlerTest{ - method: http.MethodDelete, - endpoint: "/metrics-users/juju-metrics-r0", - body: `{"username":"juju-metrics-r0","password":"bar"}`, - statusCode: http.StatusInternalServerError, - response: ".*spanner in the works.*", - }) - // Nothing should have changed. - s.assertState(c, []fakeUser{ - {name: "juju-metrics-r0", password: "bar", creator: userCreator}, - }) -} - -type fakeState struct { - users map[string]fakeUser - model *fakeModel - - userErr, addErr, removeErr error -} - -func newFakeState(users []fakeUser) *fakeState { - s := &fakeState{ - users: make(map[string]fakeUser, len(users)), - } - for _, user := range users { - s.users[user.name] = user - } - s.model = &fakeModel{nil} - return s -} - -func (s *fakeState) User(tag names.UserTag) (user, error) { - if s.userErr != nil { - return nil, s.userErr - } - - username := tag.Name() - u, ok := s.users[username] - if !ok { - return nil, errors.UserNotFoundf("user %q", username) - } - return u, nil -} - -func (s *fakeState) AddUser(name, displayName, password, creator string) (user, error) { - if s.addErr != nil { - return nil, s.addErr - } - - if _, ok := s.users[name]; ok { - // The real state code doesn't return the user if it already exists, it - // returns a typed nil value. - return (*fakeUser)(nil), errors.AlreadyExistsf("user %q", name) - } - - u := fakeUser{name, displayName, password, creator} - s.users[name] = u - return u, nil } -func (s *fakeState) RemoveUser(tag names.UserTag) error { - if s.removeErr != nil { - return s.removeErr - } +func (s *workerSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) - username := tag.Name() - if _, ok := s.users[username]; !ok { - return errors.UserNotFoundf("user %q", username) - } + s.userService = NewMockUserService(ctrl) + s.permissionService = NewMockPermissionService(ctrl) - delete(s.users, username) - return nil -} + s.logger = testing.NewCheckLogger(c) -func (s *fakeState) Model() (model, error) { - return s.model, nil -} - -type fakeUser struct { - name, displayName, password, creator string -} - -func (u fakeUser) Name() string { - return u.name -} - -func (u fakeUser) CreatedBy() string { - return u.creator -} - -func (u fakeUser) UserTag() names.UserTag { - return names.NewUserTag(u.name) -} - -func (u fakeUser) PasswordValid(s string) bool { - return s == u.password -} - -type fakeModel struct { - err error -} - -func (m *fakeModel) AddUser(_ state.UserAccessSpec) (permission.UserAccess, error) { - return permission.UserAccess{}, m.err + return ctrl } // Return an *http.Client with custom transport that allows it to connect to @@ -446,3 +341,7 @@ func client(socketPath string) *http.Client { }, } } + +func ptr[T any](v T) *T { + return &v +} diff --git a/internal/worker/httpserverargs/authenticator.go b/internal/worker/httpserverargs/authenticator.go index 989a7ad5d65..89c46d5f896 100644 --- a/internal/worker/httpserverargs/authenticator.go +++ b/internal/worker/httpserverargs/authenticator.go @@ -15,6 +15,7 @@ import ( "github.com/juju/juju/apiserver/stateauthenticator" "github.com/juju/juju/controller" coreuser "github.com/juju/juju/core/user" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/state" ) @@ -28,7 +29,7 @@ type ControllerConfigService interface { // authenticate a user. type UserService interface { // GetUserByAuth returns the user with the given name and password. - GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) + GetUserByAuth(ctx context.Context, name string, password auth.Password) (coreuser.User, error) // GetUserByName returns the user with the given name. GetUserByName(ctx context.Context, name string) (coreuser.User, error) // UpdateLastLogin updates the last login time for the user with the diff --git a/internal/worker/httpserverargs/services_mock_test.go b/internal/worker/httpserverargs/services_mock_test.go index a5b6e0e7708..5891a21174e 100644 --- a/internal/worker/httpserverargs/services_mock_test.go +++ b/internal/worker/httpserverargs/services_mock_test.go @@ -15,6 +15,7 @@ import ( controller "github.com/juju/juju/controller" user "github.com/juju/juju/core/user" + auth "github.com/juju/juju/internal/auth" gomock "go.uber.org/mock/gomock" ) @@ -80,7 +81,7 @@ func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { } // GetUserByAuth mocks base method. -func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1, arg2 string) (user.User, error) { +func (m *MockUserService) GetUserByAuth(arg0 context.Context, arg1 string, arg2 auth.Password) (user.User, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetUserByAuth", arg0, arg1, arg2) ret0, _ := ret[0].(user.User) diff --git a/internal/worker/httpserverargs/worker.go b/internal/worker/httpserverargs/worker.go index 932756ad5e3..2bc211ef154 100644 --- a/internal/worker/httpserverargs/worker.go +++ b/internal/worker/httpserverargs/worker.go @@ -16,6 +16,7 @@ import ( "github.com/juju/juju/apiserver/authentication/macaroon" "github.com/juju/juju/controller" coreuser "github.com/juju/juju/core/user" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/state" ) @@ -142,7 +143,7 @@ func (b *managedServices) ControllerConfig(ctx context.Context) (controller.Conf } // GetUserByName is part of the UserService interface. -func (b *managedServices) GetUserByAuth(ctx context.Context, name, password string) (coreuser.User, error) { +func (b *managedServices) GetUserByAuth(ctx context.Context, name string, password auth.Password) (coreuser.User, error) { return b.userService.GetUserByAuth(b.tomb.Context(ctx), name, password) } From bb7397ca951abd4357a2e9fe3710719565438510 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Tue, 19 Mar 2024 15:42:38 +0000 Subject: [PATCH 2/3] Ensure we add it to the metrics user The metrics user now owns all the control socket users. --- agent/agentbootstrap/bootstrap.go | 8 ++++++++ internal/worker/controlsocket/manifold.go | 20 ++++++++++++++++---- internal/worker/controlsocket/worker.go | 2 +- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/agent/agentbootstrap/bootstrap.go b/agent/agentbootstrap/bootstrap.go index 0c748649e07..0d57dde0467 100644 --- a/agent/agentbootstrap/bootstrap.go +++ b/agent/agentbootstrap/bootstrap.go @@ -231,6 +231,13 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller // and a function to insert it into the database. adminUserUUID, addAdminUser := userbootstrap.AddUserWithPassword(b.adminUser.Name(), auth.NewPassword(info.Password)) + // Add initial metrics user to the database. + metricsPassword, err := password.RandomPassword() + if err != nil { + return nil, err + } + _, addMetricUser := userbootstrap.AddUserWithPassword("juju-metrics", auth.NewPassword(metricsPassword)) + controllerModelUUID := model.UUID( stateParams.ControllerModelConfig.UUID(), ) @@ -256,6 +263,7 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller // The admin user needs to be added before everything else that // requires being owned by a Juju user. addAdminUser, + addMetricUser, ccbootstrap.InsertInitialControllerConfig(stateParams.ControllerConfig), cloudbootstrap.InsertCloud(stateParams.ControllerCloud), credbootstrap.InsertCredential(credential.IdFromTag(cloudCredTag), cloudCred), diff --git a/internal/worker/controlsocket/manifold.go b/internal/worker/controlsocket/manifold.go index e7d05c4184d..77af7c119c9 100644 --- a/internal/worker/controlsocket/manifold.go +++ b/internal/worker/controlsocket/manifold.go @@ -12,7 +12,8 @@ import ( "github.com/juju/worker/v4/dependency" "github.com/juju/juju/core/permission" - "github.com/juju/juju/domain/servicefactory" + "github.com/juju/juju/internal/password" + "github.com/juju/juju/internal/servicefactory" "github.com/juju/juju/internal/socketlistener" "github.com/juju/juju/internal/worker/common" workerstate "github.com/juju/juju/internal/worker/state" @@ -88,7 +89,7 @@ func (cfg ManifoldConfig) start(ctx context.Context, getter dependency.Getter) ( } }() - var serviceFactory servicefactory.ControllerFactory + var serviceFactory servicefactory.ControllerServiceFactory if err = getter.Get(cfg.ServiceFactoryName, &serviceFactory); err != nil { return nil, errors.Trace(err) } @@ -135,13 +136,24 @@ func (p permissionService) AddUserPermission(ctx context.Context, username strin return errors.NotValidf("invalid username %q", username) } + // This password doesn't matter, as we don't read from the state user. + metricsPassword, err := password.RandomPassword() + if err != nil { + return errors.Annotatef(err, "generating random password") + } + + _, err = p.state.AddUser(username, username, metricsPassword, userCreator) + if err != nil { + return errors.Annotate(err, "adding user") + } + _, err = model.AddUser(state.UserAccessSpec{ User: names.NewUserTag(username), - CreatedBy: names.NewUserTag(userCreator), + CreatedBy: names.NewUserTag("controller@juju"), Access: access, }) if err != nil { - return errors.Annotate(err, "adding user") + return errors.Annotate(err, "adding user permission") } return nil } diff --git a/internal/worker/controlsocket/worker.go b/internal/worker/controlsocket/worker.go index 0c23f1dd19f..e7ed0a5f83a 100644 --- a/internal/worker/controlsocket/worker.go +++ b/internal/worker/controlsocket/worker.go @@ -36,7 +36,7 @@ const ( // userCreator is the listed "creator" of metrics users in state. // This user CANNOT be a local user (it must have a domain), otherwise the // model addUser code will complain about the user not existing. - userCreator = "controller@juju" + userCreator = "juju-metrics" ) // UserService is the interface for the user service. From dfa69a5da9b29331b3d300f5a30de40ce0fc2b68 Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 20 Mar 2024 12:48:12 +0000 Subject: [PATCH 3/3] Move metrics user to bootstrap worker The metrics user isn't required to be installed when we seed the database, therefor can be moved to the bootstrap worker. --- agent/agentbootstrap/bootstrap.go | 8 -- core/user/user.go | 9 ++ .../worker/bootstrap/bootstrap_mock_test.go | 60 +++++++++- internal/worker/bootstrap/manifold.go | 15 +++ internal/worker/bootstrap/package_test.go | 4 +- internal/worker/bootstrap/worker.go | 104 ++++++++++++------ internal/worker/bootstrap/worker_test.go | 20 +++- 7 files changed, 175 insertions(+), 45 deletions(-) diff --git a/agent/agentbootstrap/bootstrap.go b/agent/agentbootstrap/bootstrap.go index 0d57dde0467..0c748649e07 100644 --- a/agent/agentbootstrap/bootstrap.go +++ b/agent/agentbootstrap/bootstrap.go @@ -231,13 +231,6 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller // and a function to insert it into the database. adminUserUUID, addAdminUser := userbootstrap.AddUserWithPassword(b.adminUser.Name(), auth.NewPassword(info.Password)) - // Add initial metrics user to the database. - metricsPassword, err := password.RandomPassword() - if err != nil { - return nil, err - } - _, addMetricUser := userbootstrap.AddUserWithPassword("juju-metrics", auth.NewPassword(metricsPassword)) - controllerModelUUID := model.UUID( stateParams.ControllerModelConfig.UUID(), ) @@ -263,7 +256,6 @@ func (b *AgentBootstrap) Initialize(ctx stdcontext.Context) (_ *state.Controller // The admin user needs to be added before everything else that // requires being owned by a Juju user. addAdminUser, - addMetricUser, ccbootstrap.InsertInitialControllerConfig(stateParams.ControllerConfig), cloudbootstrap.InsertCloud(stateParams.ControllerCloud), credbootstrap.InsertCredential(credential.IdFromTag(cloudCredTag), cloudCred), diff --git a/core/user/user.go b/core/user/user.go index 1fd0e31b038..16bc09bf282 100644 --- a/core/user/user.go +++ b/core/user/user.go @@ -51,6 +51,15 @@ func NewUUID() (UUID, error) { return UUID(uuid.String()), nil } +// MustNewUUID returns a new UUID or panics. +func MustNewUUID() UUID { + uuid, err := NewUUID() + if err != nil { + panic(err) + } + return uuid +} + // Validate returns an error if the UUID is invalid. func (u UUID) Validate() error { if u == "" { diff --git a/internal/worker/bootstrap/bootstrap_mock_test.go b/internal/worker/bootstrap/bootstrap_mock_test.go index 6f13502ef91..39b6817213b 100644 --- a/internal/worker/bootstrap/bootstrap_mock_test.go +++ b/internal/worker/bootstrap/bootstrap_mock_test.go @@ -1,9 +1,9 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/juju/juju/internal/worker/bootstrap (interfaces: ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService) +// Source: github.com/juju/juju/internal/worker/bootstrap (interfaces: ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService,UserService) // // Generated by this command: // -// mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/internal/worker/bootstrap ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService +// mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/internal/worker/bootstrap ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService,UserService // // Package bootstrap is a generated GoMock package. @@ -20,8 +20,10 @@ import ( credential "github.com/juju/juju/core/credential" network "github.com/juju/juju/core/network" objectstore "github.com/juju/juju/core/objectstore" + user "github.com/juju/juju/core/user" service "github.com/juju/juju/domain/application/service" service0 "github.com/juju/juju/domain/storage/service" + service1 "github.com/juju/juju/domain/user/service" bootstrap "github.com/juju/juju/internal/bootstrap" services "github.com/juju/juju/internal/charm/services" storage "github.com/juju/juju/internal/storage" @@ -662,3 +664,57 @@ func (mr *MockSpaceServiceMockRecorder) SpaceByName(arg0, arg1 any) *gomock.Call mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SpaceByName", reflect.TypeOf((*MockSpaceService)(nil).SpaceByName), arg0, arg1) } + +// MockUserService is a mock of UserService interface. +type MockUserService struct { + ctrl *gomock.Controller + recorder *MockUserServiceMockRecorder +} + +// MockUserServiceMockRecorder is the mock recorder for MockUserService. +type MockUserServiceMockRecorder struct { + mock *MockUserService +} + +// NewMockUserService creates a new mock instance. +func NewMockUserService(ctrl *gomock.Controller) *MockUserService { + mock := &MockUserService{ctrl: ctrl} + mock.recorder = &MockUserServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockUserService) EXPECT() *MockUserServiceMockRecorder { + return m.recorder +} + +// AddUser mocks base method. +func (m *MockUserService) AddUser(arg0 context.Context, arg1 service1.AddUserArg) (user.UUID, []byte, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AddUser", arg0, arg1) + ret0, _ := ret[0].(user.UUID) + ret1, _ := ret[1].([]byte) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// AddUser indicates an expected call of AddUser. +func (mr *MockUserServiceMockRecorder) AddUser(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddUser", reflect.TypeOf((*MockUserService)(nil).AddUser), arg0, arg1) +} + +// GetUserByName mocks base method. +func (m *MockUserService) GetUserByName(arg0 context.Context, arg1 string) (user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetUserByName", arg0, arg1) + ret0, _ := ret[0].(user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetUserByName indicates an expected call of GetUserByName. +func (mr *MockUserServiceMockRecorder) GetUserByName(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserByName", reflect.TypeOf((*MockUserService)(nil).GetUserByName), arg0, arg1) +} diff --git a/internal/worker/bootstrap/manifold.go b/internal/worker/bootstrap/manifold.go index 80430130af5..6e923448bde 100644 --- a/internal/worker/bootstrap/manifold.go +++ b/internal/worker/bootstrap/manifold.go @@ -20,8 +20,10 @@ import ( "github.com/juju/juju/core/instance" "github.com/juju/juju/core/network" "github.com/juju/juju/core/objectstore" + "github.com/juju/juju/core/user" applicationservice "github.com/juju/juju/domain/application/service" storageservice "github.com/juju/juju/domain/storage/service" + userservice "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs" "github.com/juju/juju/environs/envcontext" "github.com/juju/juju/internal/bootstrap" @@ -99,6 +101,18 @@ type ObjectStoreGetter interface { GetObjectStore(context.Context, string) (objectstore.ObjectStore, error) } +// UserService is the interface that is used to add a new user to the +// database. +type UserService interface { + // AddUser will add a new user to the database and return the UUID of the + // user if successful. If no password is set in the incoming argument, + // the user will be added with an activation key. + AddUser(ctx context.Context, arg userservice.AddUserArg) (user.UUID, []byte, error) + + // GetUserByName will return the user with the given name. + GetUserByName(ctx context.Context, name string) (user.User, error) +} + // ControllerCharmDeployerFunc is the function that is used to upload the // controller charm. type ControllerCharmDeployerFunc func(ControllerCharmDeployerConfig) (bootstrap.ControllerCharmDeployer, error) @@ -342,6 +356,7 @@ func Manifold(config ManifoldConfig) dependency.Manifold { ControllerConfigService: controllerServiceFactory.ControllerConfig(), CredentialService: controllerServiceFactory.Credential(), CloudService: controllerServiceFactory.Cloud(), + UserService: controllerServiceFactory.User(), StorageService: modelServiceFactory.Storage(registry), ProviderRegistry: registry, ApplicationService: modelServiceFactory.Application(registry), diff --git a/internal/worker/bootstrap/package_test.go b/internal/worker/bootstrap/package_test.go index 5d54261a355..880849c8683 100644 --- a/internal/worker/bootstrap/package_test.go +++ b/internal/worker/bootstrap/package_test.go @@ -20,7 +20,7 @@ import ( //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination state_mock_test.go github.com/juju/juju/internal/worker/state StateTracker //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination objectstore_mock_test.go github.com/juju/juju/core/objectstore ObjectStore //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination lock_mock_test.go github.com/juju/juju/internal/worker/gate Unlocker -//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/internal/worker/bootstrap ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService +//go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination bootstrap_mock_test.go github.com/juju/juju/internal/worker/bootstrap ControllerConfigService,FlagService,ObjectStoreGetter,SystemState,HTTPClient,CredentialService,CloudService,StorageService,ApplicationService,SpaceService,UserService //go:generate go run go.uber.org/mock/mockgen -package bootstrap -destination deployer_mock_test.go github.com/juju/juju/internal/bootstrap Model func TestPackage(t *testing.T) { @@ -46,6 +46,7 @@ type baseSuite struct { credentialService *MockCredentialService storageService *MockStorageService applicationService *MockApplicationService + userService *MockUserService spaceService *MockSpaceService flagService *MockFlagService httpClient *MockHTTPClient @@ -72,6 +73,7 @@ func (s *baseSuite) setupMocks(c *gc.C) *gomock.Controller { s.credentialService = NewMockCredentialService(ctrl) s.storageService = NewMockStorageService(ctrl) s.applicationService = NewMockApplicationService(ctrl) + s.userService = NewMockUserService(ctrl) s.spaceService = NewMockSpaceService(ctrl) s.flagService = NewMockFlagService(ctrl) s.httpClient = NewMockHTTPClient(ctrl) diff --git a/internal/worker/bootstrap/worker.go b/internal/worker/bootstrap/worker.go index cb609bb1ece..ec643ae113b 100644 --- a/internal/worker/bootstrap/worker.go +++ b/internal/worker/bootstrap/worker.go @@ -19,8 +19,12 @@ import ( domainstorage "github.com/juju/juju/domain/storage" storageerrors "github.com/juju/juju/domain/storage/errors" storageservice "github.com/juju/juju/domain/storage/service" + usererrors "github.com/juju/juju/domain/user/errors" + userservice "github.com/juju/juju/domain/user/service" + "github.com/juju/juju/internal/auth" "github.com/juju/juju/internal/bootstrap" "github.com/juju/juju/internal/cloudconfig/instancecfg" + "github.com/juju/juju/internal/password" "github.com/juju/juju/internal/storage" "github.com/juju/juju/internal/worker/gate" "github.com/juju/juju/state/binarystorage" @@ -54,6 +58,7 @@ type WorkerConfig struct { ControllerConfigService ControllerConfigService CredentialService CredentialService CloudService CloudService + UserService UserService StorageService StorageService ProviderRegistry storage.ProviderRegistry ApplicationService ApplicationService @@ -91,6 +96,12 @@ func (c *WorkerConfig) Validate() error { if c.CloudService == nil { return errors.NotValidf("nil CloudService") } + if c.UserService == nil { + return errors.NotValidf("nil UserService") + } + if c.StorageService == nil { + return errors.NotValidf("nil StorageService") + } if c.ApplicationService == nil { return errors.NotValidf("nil ApplicationService") } @@ -178,6 +189,11 @@ func (w *bootstrapWorker) loop() error { ctx, cancel := w.scopedContext() defer cancel() + // Insert all the initial users into the state. + if err := w.seedInitialUsers(ctx); err != nil { + return errors.Annotatef(err, "inserting initial users") + } + agentConfig := w.cfg.Agent.CurrentConfig() dataDir := agentConfig.DataDir() @@ -188,13 +204,13 @@ func (w *bootstrapWorker) loop() error { } // Seed the controller charm to the object store. - args, err := w.bootstrapParams(ctx, dataDir) + bootstrapParams, err := w.bootstrapParams(ctx, dataDir) if err != nil { return errors.Annotatef(err, "getting bootstrap params") } // Create the user specified storage pools. - if err := w.seedStoragePools(ctx, args.StoragePools); err != nil { + if err := w.seedStoragePools(ctx, bootstrapParams.StoragePools); err != nil { return errors.Annotate(err, "seeding storage pools") } @@ -202,14 +218,14 @@ func (w *bootstrapWorker) loop() error { if err != nil { return errors.Trace(err) } - bootstrapArgs, err := w.seedControllerCharm(ctx, dataDir, args) - if err != nil { + + if err := w.seedControllerCharm(ctx, dataDir, bootstrapParams); err != nil { return errors.Trace(err) } // Retrieve controller addresses needed to set the API host ports. bootstrapAddresses, err := w.cfg.BootstrapAddressFinder(ctx, BootstrapAddressesConfig{ - BootstrapInstanceID: bootstrapArgs.BootstrapMachineInstanceId, + BootstrapInstanceID: bootstrapParams.BootstrapMachineInstanceId, SystemState: w.cfg.SystemState, CloudService: w.cfg.CloudService, CredentialService: w.cfg.CredentialService, @@ -243,30 +259,30 @@ func (w *bootstrapWorker) loop() error { return nil } -// initialStoragePools extract any storage pools included with the bootstrap params. -func initialStoragePools(registry storage.ProviderRegistry, poolParams map[string]storage.Attrs) ([]*storage.Config, error) { - var result []*storage.Config - defaultStoragePools, err := domainstorage.DefaultStoragePools(registry) +func (w *bootstrapWorker) seedInitialUsers(ctx context.Context) error { + // Any failure should be retryable, so we can re-attempt to bootstrap. + + adminUser, err := w.cfg.UserService.GetUserByName(ctx, "admin") if err != nil { - return nil, errors.Trace(err) + return errors.Annotatef(err, "getting admin user") } - for _, p := range defaultStoragePools { - result = append(result, p) - } - for name, attrs := range poolParams { - pType, _ := attrs[domainstorage.StorageProviderType].(string) - if pType == "" { - return nil, errors.Errorf("missing provider type for storage pool %q", name) - } - delete(attrs, domainstorage.StoragePoolName) - delete(attrs, domainstorage.StorageProviderType) - pool, err := storage.NewConfig(name, storage.ProviderType(pType), attrs) - if err != nil { - return nil, errors.Trace(err) - } - result = append(result, pool) + + pass, err := password.RandomPassword() + if err != nil { + return errors.Annotatef(err, "generating metrics password") + } + password := auth.NewPassword(pass) + _, _, err = w.cfg.UserService.AddUser(ctx, userservice.AddUserArg{ + Name: "juju-metrics", + DisplayName: "Juju Metrics", + Password: &password, + CreatorUUID: adminUser.UUID, + }) + // User already exists, we don't need to do anything in this scenario. + if errors.Is(err, usererrors.AlreadyExists) { + return nil } - return result, nil + return errors.Annotatef(err, "inserting initial users") } func (w *bootstrapWorker) seedStoragePools(ctx context.Context, poolParams map[string]storage.Attrs) error { @@ -371,15 +387,15 @@ func (w *bootstrapWorker) seedAgentBinary(ctx context.Context, dataDir string) ( return cleanup, nil } -func (w *bootstrapWorker) seedControllerCharm(ctx context.Context, dataDir string, bootstrapArgs instancecfg.StateInitializationParams) (instancecfg.StateInitializationParams, error) { +func (w *bootstrapWorker) seedControllerCharm(ctx context.Context, dataDir string, bootstrapArgs instancecfg.StateInitializationParams) error { controllerConfig, err := w.cfg.ControllerConfigService.ControllerConfig(ctx) if err != nil { - return instancecfg.StateInitializationParams{}, errors.Trace(err) + return errors.Trace(err) } objectStore, err := w.cfg.ObjectStoreGetter.GetObjectStore(ctx, w.cfg.SystemState.ControllerModelUUID()) if err != nil { - return instancecfg.StateInitializationParams{}, fmt.Errorf("failed to get object store: %w", err) + return fmt.Errorf("failed to get object store: %w", err) } // Controller charm seeder will populate the charm for the controller. @@ -397,10 +413,10 @@ func (w *bootstrapWorker) seedControllerCharm(ctx context.Context, dataDir strin LoggerFactory: w.cfg.LoggerFactory, }) if err != nil { - return instancecfg.StateInitializationParams{}, errors.Trace(err) + return errors.Trace(err) } - return bootstrapArgs, w.cfg.PopulateControllerCharm(ctx, deployer) + return w.cfg.PopulateControllerCharm(ctx, deployer) } func (w *bootstrapWorker) bootstrapParams(ctx context.Context, dataDir string) (instancecfg.StateInitializationParams, error) { @@ -415,6 +431,32 @@ func (w *bootstrapWorker) bootstrapParams(ctx context.Context, dataDir string) ( return args, nil } +// initialStoragePools extract any storage pools included with the bootstrap params. +func initialStoragePools(registry storage.ProviderRegistry, poolParams map[string]storage.Attrs) ([]*storage.Config, error) { + var result []*storage.Config + defaultStoragePools, err := domainstorage.DefaultStoragePools(registry) + if err != nil { + return nil, errors.Trace(err) + } + for _, p := range defaultStoragePools { + result = append(result, p) + } + for name, attrs := range poolParams { + pType, _ := attrs[domainstorage.StorageProviderType].(string) + if pType == "" { + return nil, errors.Errorf("missing provider type for storage pool %q", name) + } + delete(attrs, domainstorage.StoragePoolName) + delete(attrs, domainstorage.StorageProviderType) + pool, err := storage.NewConfig(name, storage.ProviderType(pType), attrs) + if err != nil { + return nil, errors.Trace(err) + } + result = append(result, pool) + } + return result, nil +} + type agentStorageShim struct { State SystemState } diff --git a/internal/worker/bootstrap/worker_test.go b/internal/worker/bootstrap/worker_test.go index 29e5e40cd34..4a0c85144f9 100644 --- a/internal/worker/bootstrap/worker_test.go +++ b/internal/worker/bootstrap/worker_test.go @@ -22,6 +22,8 @@ import ( "github.com/juju/juju/core/instance" "github.com/juju/juju/core/network" "github.com/juju/juju/core/objectstore" + "github.com/juju/juju/core/user" + userservice "github.com/juju/juju/domain/user/service" "github.com/juju/juju/environs" "github.com/juju/juju/environs/config" "github.com/juju/juju/internal/bootstrap" @@ -47,12 +49,13 @@ func (s *workerSuite) TestKilled(c *gc.C) { s.ensureBootstrapParams(c) s.expectGateUnlock() + s.expectUser(c) s.expectControllerConfig() s.expectAgentConfig() s.expectObjectStoreGetter(2) s.expectBootstrapFlagSet() s.expectSetAPIHostPorts() - s.expectStaateServingInfo() + s.expectStateServingInfo() w := s.newWorker(c) defer workertest.DirtyKill(c, w) @@ -97,7 +100,7 @@ func (s *workerSuite) TestSeedAgentBinary(c *gc.C) { c.Assert(cleanup, gc.NotNil) } -func (s *workerSuite) TestFilterHostPortsEmptyMgmtSpace(c *gc.C) { +func (s *workerSuite) TestFilterHostPortsEmptyManagementSpace(c *gc.C) { defer s.setupMocks(c).Finish() w := &bootstrapWorker{ internalStates: s.states, @@ -246,6 +249,7 @@ func (s *workerSuite) newWorker(c *gc.C) worker.Worker { BootstrapUnlocker: s.bootstrapUnlocker, CharmhubHTTPClient: s.httpClient, SystemState: s.state, + UserService: s.userService, ApplicationService: s.applicationService, ControllerConfigService: s.controllerConfigService, CredentialService: s.credentialService, @@ -309,7 +313,17 @@ func (s *workerSuite) expectControllerConfig() { }, nil).Times(2) } -func (s *workerSuite) expectStaateServingInfo() { +func (s *workerSuite) expectUser(c *gc.C) { + s.userService.EXPECT().GetUserByName(gomock.Any(), "admin").Return(user.User{ + UUID: user.MustNewUUID(), + }, nil) + s.userService.EXPECT().AddUser(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, u userservice.AddUserArg) (user.UUID, []byte, error) { + c.Check(u.Name, gc.Equals, "juju-metrics") + return user.MustNewUUID(), nil, nil + }) +} + +func (s *workerSuite) expectStateServingInfo() { s.agentConfig.EXPECT().StateServingInfo().Return(controller.StateServingInfo{ APIPort: 42, }, true)