Skip to content

Commit

Permalink
Merge pull request juju#17058 from SimonRichardson/control-socket-use…
Browse files Browse the repository at this point in the history
…r-service

juju#17058

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 transition away from legacy state for permissions this should be updated.

Also, I swapped out the fake/stub mocks in the test for real mocks.

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing

## QA steps

### Manual

Check the worker is starting and the socket is being created:
```shell
$ make microk8s-operator-update
$ juju bootstrap [cloud] c
$ juju switch controller
$ juju ssh controller/0
$ stat /var/lib/juju/control.socket
```

Manually curl the socket:
```bash
$ curl -X POST http://a/metrics-users \n -d '{"username":"juju-metrics-test","password":"bar"}' \n --unix-socket /var/lib/juju/control.socket
# {"message":"created user \"juju-metrics-test\""}
```
```bash
$ curl -X DELETE http://a/metrics-users/juju-metrics-test \n --unix-socket /var/lib/juju/control.socket
# {"message":"deleted user \"juju-metrics-test\""}
```

## Links

**Jira card:** JUJU-5691
  • Loading branch information
jujubot authored Mar 20, 2024
2 parents 7b17a76 + dfa69a5 commit 0c2d31b
Show file tree
Hide file tree
Showing 23 changed files with 651 additions and 412 deletions.
5 changes: 3 additions & 2 deletions apiserver/authentication/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions apiserver/registration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)

Expand Down
3 changes: 2 additions & 1 deletion apiserver/stateauthenticator/authenticator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion apiserver/stateauthenticator/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion apiserver/stateauthenticator/services_mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions cmd/jujud-controller/agent/machine/manifolds.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
9 changes: 9 additions & 0 deletions core/user/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down
12 changes: 6 additions & 6 deletions domain/user/service/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion domain/user/service/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
60 changes: 58 additions & 2 deletions internal/worker/bootstrap/bootstrap_mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions internal/worker/bootstrap/manifold.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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),
Expand Down
4 changes: 3 additions & 1 deletion internal/worker/bootstrap/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -46,6 +46,7 @@ type baseSuite struct {
credentialService *MockCredentialService
storageService *MockStorageService
applicationService *MockApplicationService
userService *MockUserService
spaceService *MockSpaceService
flagService *MockFlagService
httpClient *MockHTTPClient
Expand All @@ -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)
Expand Down
Loading

0 comments on commit 0c2d31b

Please sign in to comment.