From 9e9ef3bc73625531ada8c321a15325feec459876 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 15 Aug 2024 16:36:44 +0100 Subject: [PATCH 1/3] feat: migrate model user permissions import and export model user permissions. In mongo there was the concept of model users, these have been reduced to model user permissions in 4.0 since the controller holds all extra information about the users. Use the model users struct to migrate these model permissions --- domain/access/modelmigration/export.go | 95 ++++++ domain/access/modelmigration/export_test.go | 102 +++++++ domain/access/modelmigration/import.go | 109 +++++++ domain/access/modelmigration/import_test.go | 118 ++++++++ .../modelmigration/migrations_mock_test.go | 282 ++++++++++++++++++ domain/access/modelmigration/package_test.go | 16 + domain/access/service/user.go | 16 + domain/access/service/user_test.go | 35 ++- domain/model/state/state_test.go | 2 +- domain/modelmigration/export.go | 2 + domain/modelmigration/import.go | 2 + state/migration_export.go | 43 --- state/migration_export_test.go | 45 --- state/migration_import.go | 47 --- state/migration_import_test.go | 67 ----- 15 files changed, 770 insertions(+), 211 deletions(-) create mode 100644 domain/access/modelmigration/export.go create mode 100644 domain/access/modelmigration/export_test.go create mode 100644 domain/access/modelmigration/import.go create mode 100644 domain/access/modelmigration/import_test.go create mode 100644 domain/access/modelmigration/migrations_mock_test.go create mode 100644 domain/access/modelmigration/package_test.go diff --git a/domain/access/modelmigration/export.go b/domain/access/modelmigration/export.go new file mode 100644 index 00000000000..e2618150670 --- /dev/null +++ b/domain/access/modelmigration/export.go @@ -0,0 +1,95 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package modelmigration + +import ( + "context" + "time" + + "github.com/juju/description/v8" + "github.com/juju/errors" + + "github.com/juju/juju/core/logger" + coremodel "github.com/juju/juju/core/model" + "github.com/juju/juju/core/modelmigration" + corepermission "github.com/juju/juju/core/permission" + "github.com/juju/juju/core/user" + accesserrors "github.com/juju/juju/domain/access/errors" + "github.com/juju/juju/domain/access/service" + "github.com/juju/juju/domain/access/state" +) + +// RegisterExport registers the export operations with the given coordinator. +func RegisterExport(coordinator Coordinator, logger logger.Logger) { + coordinator.Add(&exportOperation{ + logger: logger, + }) +} + +// ExportService provides a subset of the access domain +// service methods needed for model permissions export. +type ExportService interface { + // ReadAllUserAccessForTarget return a slice of user access for all users + // with access to the given target. A NotValid error is returned if the + // target is not valid. Any errors from the state layer are passed through. + ReadAllUserAccessForTarget(ctx context.Context, target corepermission.ID) ([]corepermission.UserAccess, error) + // LastModelLogin will return the last login time of the specified user. + // The following error types are possible from this function: + // - accesserrors.UserNameNotValid: When the username is not valid. + // - accesserrors.UserNotFound: When the user cannot be found. + // - modelerrors.NotFound: If no model by the given modelUUID exists. + // - accesserrors.UserNeverAccessedModel: If there is no record of the user + // accessing the model. + LastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) (time.Time, error) +} + +// exportOperation describes a way to execute a migration for +// exporting model user permissions. +type exportOperation struct { + modelmigration.BaseOperation + + logger logger.Logger + service ExportService +} + +// Name returns the name of this operation. +func (e *exportOperation) Name() string { + return "export model user permissions" +} + +// Setup implements Operation. +func (e *exportOperation) Setup(scope modelmigration.Scope) error { + e.service = service.NewService( + state.NewState(scope.ControllerDB(), e.logger), + ) + return nil +} + +// Execute the export, adding the model user permissions to the model. +func (e *exportOperation) Execute(ctx context.Context, model description.Model) error { + modelUUID := model.Tag().Id() + userAccesses, err := e.service.ReadAllUserAccessForTarget(ctx, corepermission.ID{ + ObjectType: corepermission.Model, + Key: modelUUID, + }) + if err != nil { + return errors.Annotatef(err, "getting user access on model") + } + for _, userAccess := range userAccesses { + lastModelLogin, err := e.service.LastModelLogin(ctx, userAccess.UserName, coremodel.UUID(modelUUID)) + if err != nil && !errors.Is(err, accesserrors.UserNeverAccessedModel) { + return errors.Annotatef(err, "getting user last login on model") + } + arg := description.UserArgs{ + Name: userAccess.UserTag, + DisplayName: userAccess.DisplayName, + CreatedBy: userAccess.CreatedBy, + DateCreated: userAccess.DateCreated, + LastConnection: lastModelLogin, + Access: string(userAccess.Access), + } + model.AddUser(arg) + } + return nil +} diff --git a/domain/access/modelmigration/export_test.go b/domain/access/modelmigration/export_test.go new file mode 100644 index 00000000000..8823ac714dc --- /dev/null +++ b/domain/access/modelmigration/export_test.go @@ -0,0 +1,102 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package modelmigration + +import ( + "context" + "time" + + "github.com/juju/description/v8" + "github.com/juju/names/v5" + jc "github.com/juju/testing/checkers" + "go.uber.org/mock/gomock" + gc "gopkg.in/check.v1" + + coremodel "github.com/juju/juju/core/model" + "github.com/juju/juju/core/permission" + "github.com/juju/juju/core/user" +) + +type exportSuite struct { + coordinator *MockCoordinator + service *MockExportService +} + +var _ = gc.Suite(&exportSuite{}) + +func (s *exportSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + + s.coordinator = NewMockCoordinator(ctrl) + s.service = NewMockExportService(ctrl) + + return ctrl +} + +func (s *exportSuite) newExportOperation() *exportOperation { + return &exportOperation{ + service: s.service, + } +} + +func (s *exportSuite) TestExport(c *gc.C) { + defer s.setupMocks(c).Finish() + + dst := description.NewModel(description.ModelArgs{}) + + bobTag := names.NewUserTag("bob") + bobName := user.NameFromTag(bobTag) + bazzaTag := names.NewUserTag("bazza") + bazzaName := user.NameFromTag(bazzaTag) + steveTag := names.NewUserTag("steve") + + userAccesses := []permission.UserAccess{{ + UserTag: bazzaTag, + Access: permission.ReadAccess, + CreatedBy: bobTag, + DateCreated: time.Now(), + DisplayName: bazzaName.Name(), + UserName: bazzaName, + }, { + UserTag: bobTag, + Access: permission.AdminAccess, + CreatedBy: steveTag, + DateCreated: time.Now(), + DisplayName: bobName.Name(), + UserName: bobName, + }} + + s.service.EXPECT().ReadAllUserAccessForTarget(gomock.Any(), permission.ID{ + ObjectType: permission.Model, + Key: dst.Tag().Id(), + }).Return(userAccesses, nil) + + bobTime := time.Now().Round(time.Minute).UTC() + bazzaTime := bobTime.Add(time.Minute) + s.service.EXPECT().LastModelLogin( + gomock.Any(), bobName, coremodel.UUID(dst.Tag().Id()), + ).Return(bobTime, nil) + s.service.EXPECT().LastModelLogin( + gomock.Any(), bazzaName, coremodel.UUID(dst.Tag().Id()), + ).Return(bazzaTime, nil) + + op := s.newExportOperation() + err := op.Execute(context.Background(), dst) + c.Assert(err, jc.ErrorIsNil) + + users := dst.Users() + c.Assert(users, gc.HasLen, 2) + c.Check(users[0].Name(), gc.Equals, userAccesses[0].UserTag) + c.Check(users[0].Access(), gc.Equals, string(userAccesses[0].Access)) + c.Check(users[0].CreatedBy(), gc.Equals, userAccesses[0].CreatedBy) + c.Check(users[0].DateCreated(), gc.Equals, userAccesses[0].DateCreated) + c.Check(users[0].DisplayName(), gc.Equals, userAccesses[0].DisplayName) + c.Check(users[0].LastConnection(), gc.Equals, bazzaTime) + c.Check(users[1].Name(), gc.Equals, userAccesses[1].UserTag) + c.Check(users[1].Access(), gc.Equals, string(userAccesses[1].Access)) + c.Check(users[1].CreatedBy(), gc.Equals, userAccesses[1].CreatedBy) + c.Check(users[1].DateCreated(), gc.Equals, userAccesses[1].DateCreated) + c.Check(users[1].DisplayName(), gc.Equals, userAccesses[1].DisplayName) + c.Check(users[1].LastConnection(), gc.Equals, bobTime) +} diff --git a/domain/access/modelmigration/import.go b/domain/access/modelmigration/import.go new file mode 100644 index 00000000000..f43be9fe9d1 --- /dev/null +++ b/domain/access/modelmigration/import.go @@ -0,0 +1,109 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package modelmigration + +import ( + "context" + "time" + + "github.com/juju/description/v8" + "github.com/juju/errors" + + "github.com/juju/juju/core/logger" + coremodel "github.com/juju/juju/core/model" + "github.com/juju/juju/core/modelmigration" + corepermission "github.com/juju/juju/core/permission" + "github.com/juju/juju/core/user" + accesserrors "github.com/juju/juju/domain/access/errors" + "github.com/juju/juju/domain/access/service" + "github.com/juju/juju/domain/access/state" +) + +// Coordinator is the interface that is used to add operations to a migration. +type Coordinator interface { + // Add adds the given operation to the migration. + Add(modelmigration.Operation) +} + +// RegisterImport registers the import operations with the given coordinator. +func RegisterImport(coordinator Coordinator, logger logger.Logger) { + coordinator.Add(&importOperation{ + logger: logger, + }) +} + +// ImportService provides a subset of the access domain +// service methods needed for model permissions import. +type ImportService interface { + // CreatePermission gives the user access per the provided spec. + // If the user provided does not exist or is marked removed, + // accesserrors.PermissionNotFound is returned. + // If the user provided exists but is marked disabled, + // accesserrors.UserAuthenticationDisabled is returned. + // If a permission for the user and target key already exists, + // accesserrors.PermissionAlreadyExists is returned. + CreatePermission(ctx context.Context, spec corepermission.UserAccessSpec) (corepermission.UserAccess, error) + // SetLastModelLogin will set the last login time for the user to the given + // value. The following error types are possible from this function: - + // accesserrors.UserNameNotValid: When the username supplied is not valid. - + // accesserrors.UserNotFound: When the user cannot be found. - + // modelerrors.NotFound: If no model by the given modelUUID exists. + SetLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, time time.Time) error +} + +type importOperation struct { + modelmigration.BaseOperation + + logger logger.Logger + service ImportService +} + +// Name returns the name of this operation. +func (i *importOperation) Name() string { + return "import model user permissions" +} + +// Setup implements Operation. +func (i *importOperation) Setup(scope modelmigration.Scope) error { + i.service = service.NewService( + state.NewState(scope.ControllerDB(), i.logger)) + return nil +} + +// Execute the import on the model user permissions contained in the model. +func (i *importOperation) Execute(ctx context.Context, model description.Model) error { + modelUUID := model.Tag().Id() + for _, u := range model.Users() { + name := user.NameFromTag(u.Name()) + access := corepermission.Access(u.Access()) + if err := access.Validate(); err != nil { + return errors.Annotatef(err, "importing access for user %q", name) + } + _, err := i.service.CreatePermission(ctx, corepermission.UserAccessSpec{ + AccessSpec: corepermission.AccessSpec{ + Target: corepermission.ID{ + ObjectType: corepermission.Model, + Key: modelUUID, + }, + Access: access, + }, + User: name, + }) + if err != nil && !errors.Is(err, accesserrors.PermissionAlreadyExists) { + // If the permission already exists then it must be the model owner + // who is granted admin access when the model is created. + return errors.Annotatef(err, "creating permission for user %q", name) + } + + lastLogin := u.LastConnection() + if !lastLogin.IsZero() { + err := i.service.SetLastModelLogin(ctx, name, coremodel.UUID(modelUUID), lastLogin) + if err != nil { + return errors.Annotatef(err, "setting model last login for user %q", name) + } + } + + } + return nil +} diff --git a/domain/access/modelmigration/import_test.go b/domain/access/modelmigration/import_test.go new file mode 100644 index 00000000000..7f837ac2d53 --- /dev/null +++ b/domain/access/modelmigration/import_test.go @@ -0,0 +1,118 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package modelmigration + +import ( + "context" + "time" + + "github.com/juju/description/v8" + "github.com/juju/names/v5" + jc "github.com/juju/testing/checkers" + "go.uber.org/mock/gomock" + gc "gopkg.in/check.v1" + + coremodel "github.com/juju/juju/core/model" + "github.com/juju/juju/core/permission" + "github.com/juju/juju/core/user" + loggertesting "github.com/juju/juju/internal/logger/testing" +) + +type importSuite struct { + coordinator *MockCoordinator + service *MockImportService +} + +var _ = gc.Suite(&importSuite{}) + +func (s *importSuite) setupMocks(c *gc.C) *gomock.Controller { + ctrl := gomock.NewController(c) + + s.coordinator = NewMockCoordinator(ctrl) + s.service = NewMockImportService(ctrl) + + return ctrl +} + +func (s *importSuite) newImportOperation() *importOperation { + return &importOperation{ + service: s.service, + } +} + +func (s *importSuite) TestRegisterImport(c *gc.C) { + defer s.setupMocks(c).Finish() + + s.coordinator.EXPECT().Add(gomock.Any()) + + RegisterImport(s.coordinator, loggertesting.WrapCheckLog(c)) +} + +func (s *importSuite) TestNoModelUserPermissions(c *gc.C) { + defer s.setupMocks(c).Finish() + + // Empty model. + model := description.NewModel(description.ModelArgs{}) + + op := s.newImportOperation() + err := op.Execute(context.Background(), model) + c.Assert(err, jc.ErrorIsNil) +} + +func (s *importSuite) TestImport(c *gc.C) { + defer s.setupMocks(c).Finish() + + model := description.NewModel(description.ModelArgs{}) + modelUUID := model.Tag().Id() + modelID := permission.ID{ + ObjectType: permission.Model, + Key: modelUUID, + } + steveTag := names.NewUserTag("steve") + bobTag := names.NewUserTag("bob") + bobName := user.NameFromTag(bobTag) + bobTime := time.Now().Round(time.Minute).UTC() + bob := description.UserArgs{ + Name: bobTag, + Access: string(permission.AdminAccess), + CreatedBy: steveTag, + DateCreated: time.Now(), + DisplayName: bobTag.Name(), + LastConnection: bobTime, + } + bazzaTag := names.NewUserTag("bazza") + bazzaName := user.NameFromTag(bazzaTag) + bazzaTime := bobTime.Add(time.Minute) + bazza := description.UserArgs{ + Name: bazzaTag, + Access: string(permission.ReadAccess), + CreatedBy: bobTag, + DateCreated: time.Now(), + DisplayName: bazzaTag.Name(), + LastConnection: bazzaTime, + } + + model.AddUser(bob) + model.AddUser(bazza) + s.service.EXPECT().CreatePermission(gomock.Any(), permission.UserAccessSpec{ + AccessSpec: permission.AccessSpec{ + Target: modelID, + Access: permission.Access(bazza.Access), + }, + User: bazzaName, + }) + s.service.EXPECT().CreatePermission(gomock.Any(), permission.UserAccessSpec{ + AccessSpec: permission.AccessSpec{ + Target: modelID, + Access: permission.Access(bob.Access), + }, + User: bobName, + }) + s.service.EXPECT().SetLastModelLogin(gomock.Any(), bazzaName, coremodel.UUID(modelUUID), bazzaTime) + s.service.EXPECT().SetLastModelLogin(gomock.Any(), bobName, coremodel.UUID(modelUUID), bobTime) + + op := s.newImportOperation() + err := op.Execute(context.Background(), model) + c.Assert(err, jc.ErrorIsNil) +} diff --git a/domain/access/modelmigration/migrations_mock_test.go b/domain/access/modelmigration/migrations_mock_test.go new file mode 100644 index 00000000000..eb7eb03abee --- /dev/null +++ b/domain/access/modelmigration/migrations_mock_test.go @@ -0,0 +1,282 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/juju/juju/domain/access/modelmigration (interfaces: Coordinator,ImportService,ExportService) +// +// Generated by this command: +// +// mockgen -typed -package modelmigration -destination migrations_mock_test.go github.com/juju/juju/domain/access/modelmigration Coordinator,ImportService,ExportService +// + +// Package modelmigration is a generated GoMock package. +package modelmigration + +import ( + context "context" + reflect "reflect" + time "time" + + model "github.com/juju/juju/core/model" + modelmigration "github.com/juju/juju/core/modelmigration" + permission "github.com/juju/juju/core/permission" + user "github.com/juju/juju/core/user" + gomock "go.uber.org/mock/gomock" +) + +// MockCoordinator is a mock of Coordinator interface. +type MockCoordinator struct { + ctrl *gomock.Controller + recorder *MockCoordinatorMockRecorder +} + +// MockCoordinatorMockRecorder is the mock recorder for MockCoordinator. +type MockCoordinatorMockRecorder struct { + mock *MockCoordinator +} + +// NewMockCoordinator creates a new mock instance. +func NewMockCoordinator(ctrl *gomock.Controller) *MockCoordinator { + mock := &MockCoordinator{ctrl: ctrl} + mock.recorder = &MockCoordinatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockCoordinator) EXPECT() *MockCoordinatorMockRecorder { + return m.recorder +} + +// Add mocks base method. +func (m *MockCoordinator) Add(arg0 modelmigration.Operation) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Add", arg0) +} + +// Add indicates an expected call of Add. +func (mr *MockCoordinatorMockRecorder) Add(arg0 any) *MockCoordinatorAddCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Add", reflect.TypeOf((*MockCoordinator)(nil).Add), arg0) + return &MockCoordinatorAddCall{Call: call} +} + +// MockCoordinatorAddCall wrap *gomock.Call +type MockCoordinatorAddCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockCoordinatorAddCall) Return() *MockCoordinatorAddCall { + c.Call = c.Call.Return() + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockCoordinatorAddCall) Do(f func(modelmigration.Operation)) *MockCoordinatorAddCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockCoordinatorAddCall) DoAndReturn(f func(modelmigration.Operation)) *MockCoordinatorAddCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// MockImportService is a mock of ImportService interface. +type MockImportService struct { + ctrl *gomock.Controller + recorder *MockImportServiceMockRecorder +} + +// MockImportServiceMockRecorder is the mock recorder for MockImportService. +type MockImportServiceMockRecorder struct { + mock *MockImportService +} + +// NewMockImportService creates a new mock instance. +func NewMockImportService(ctrl *gomock.Controller) *MockImportService { + mock := &MockImportService{ctrl: ctrl} + mock.recorder = &MockImportServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockImportService) EXPECT() *MockImportServiceMockRecorder { + return m.recorder +} + +// CreatePermission mocks base method. +func (m *MockImportService) CreatePermission(arg0 context.Context, arg1 permission.UserAccessSpec) (permission.UserAccess, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreatePermission", arg0, arg1) + ret0, _ := ret[0].(permission.UserAccess) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreatePermission indicates an expected call of CreatePermission. +func (mr *MockImportServiceMockRecorder) CreatePermission(arg0, arg1 any) *MockImportServiceCreatePermissionCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreatePermission", reflect.TypeOf((*MockImportService)(nil).CreatePermission), arg0, arg1) + return &MockImportServiceCreatePermissionCall{Call: call} +} + +// MockImportServiceCreatePermissionCall wrap *gomock.Call +type MockImportServiceCreatePermissionCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockImportServiceCreatePermissionCall) Return(arg0 permission.UserAccess, arg1 error) *MockImportServiceCreatePermissionCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockImportServiceCreatePermissionCall) Do(f func(context.Context, permission.UserAccessSpec) (permission.UserAccess, error)) *MockImportServiceCreatePermissionCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockImportServiceCreatePermissionCall) DoAndReturn(f func(context.Context, permission.UserAccessSpec) (permission.UserAccess, error)) *MockImportServiceCreatePermissionCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// SetLastModelLogin mocks base method. +func (m *MockImportService) SetLastModelLogin(arg0 context.Context, arg1 user.Name, arg2 model.UUID, arg3 time.Time) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetLastModelLogin", arg0, arg1, arg2, arg3) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetLastModelLogin indicates an expected call of SetLastModelLogin. +func (mr *MockImportServiceMockRecorder) SetLastModelLogin(arg0, arg1, arg2, arg3 any) *MockImportServiceSetLastModelLoginCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLastModelLogin", reflect.TypeOf((*MockImportService)(nil).SetLastModelLogin), arg0, arg1, arg2, arg3) + return &MockImportServiceSetLastModelLoginCall{Call: call} +} + +// MockImportServiceSetLastModelLoginCall wrap *gomock.Call +type MockImportServiceSetLastModelLoginCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockImportServiceSetLastModelLoginCall) Return(arg0 error) *MockImportServiceSetLastModelLoginCall { + c.Call = c.Call.Return(arg0) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockImportServiceSetLastModelLoginCall) Do(f func(context.Context, user.Name, model.UUID, time.Time) error) *MockImportServiceSetLastModelLoginCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockImportServiceSetLastModelLoginCall) DoAndReturn(f func(context.Context, user.Name, model.UUID, time.Time) error) *MockImportServiceSetLastModelLoginCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// MockExportService is a mock of ExportService interface. +type MockExportService struct { + ctrl *gomock.Controller + recorder *MockExportServiceMockRecorder +} + +// MockExportServiceMockRecorder is the mock recorder for MockExportService. +type MockExportServiceMockRecorder struct { + mock *MockExportService +} + +// NewMockExportService creates a new mock instance. +func NewMockExportService(ctrl *gomock.Controller) *MockExportService { + mock := &MockExportService{ctrl: ctrl} + mock.recorder = &MockExportServiceMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockExportService) EXPECT() *MockExportServiceMockRecorder { + return m.recorder +} + +// LastModelLogin mocks base method. +func (m *MockExportService) LastModelLogin(arg0 context.Context, arg1 user.Name, arg2 model.UUID) (time.Time, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LastModelLogin", arg0, arg1, arg2) + ret0, _ := ret[0].(time.Time) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LastModelLogin indicates an expected call of LastModelLogin. +func (mr *MockExportServiceMockRecorder) LastModelLogin(arg0, arg1, arg2 any) *MockExportServiceLastModelLoginCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LastModelLogin", reflect.TypeOf((*MockExportService)(nil).LastModelLogin), arg0, arg1, arg2) + return &MockExportServiceLastModelLoginCall{Call: call} +} + +// MockExportServiceLastModelLoginCall wrap *gomock.Call +type MockExportServiceLastModelLoginCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockExportServiceLastModelLoginCall) Return(arg0 time.Time, arg1 error) *MockExportServiceLastModelLoginCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockExportServiceLastModelLoginCall) Do(f func(context.Context, user.Name, model.UUID) (time.Time, error)) *MockExportServiceLastModelLoginCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockExportServiceLastModelLoginCall) DoAndReturn(f func(context.Context, user.Name, model.UUID) (time.Time, error)) *MockExportServiceLastModelLoginCall { + c.Call = c.Call.DoAndReturn(f) + return c +} + +// ReadAllUserAccessForTarget mocks base method. +func (m *MockExportService) ReadAllUserAccessForTarget(arg0 context.Context, arg1 permission.ID) ([]permission.UserAccess, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ReadAllUserAccessForTarget", arg0, arg1) + ret0, _ := ret[0].([]permission.UserAccess) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ReadAllUserAccessForTarget indicates an expected call of ReadAllUserAccessForTarget. +func (mr *MockExportServiceMockRecorder) ReadAllUserAccessForTarget(arg0, arg1 any) *MockExportServiceReadAllUserAccessForTargetCall { + mr.mock.ctrl.T.Helper() + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReadAllUserAccessForTarget", reflect.TypeOf((*MockExportService)(nil).ReadAllUserAccessForTarget), arg0, arg1) + return &MockExportServiceReadAllUserAccessForTargetCall{Call: call} +} + +// MockExportServiceReadAllUserAccessForTargetCall wrap *gomock.Call +type MockExportServiceReadAllUserAccessForTargetCall struct { + *gomock.Call +} + +// Return rewrite *gomock.Call.Return +func (c *MockExportServiceReadAllUserAccessForTargetCall) Return(arg0 []permission.UserAccess, arg1 error) *MockExportServiceReadAllUserAccessForTargetCall { + c.Call = c.Call.Return(arg0, arg1) + return c +} + +// Do rewrite *gomock.Call.Do +func (c *MockExportServiceReadAllUserAccessForTargetCall) Do(f func(context.Context, permission.ID) ([]permission.UserAccess, error)) *MockExportServiceReadAllUserAccessForTargetCall { + c.Call = c.Call.Do(f) + return c +} + +// DoAndReturn rewrite *gomock.Call.DoAndReturn +func (c *MockExportServiceReadAllUserAccessForTargetCall) DoAndReturn(f func(context.Context, permission.ID) ([]permission.UserAccess, error)) *MockExportServiceReadAllUserAccessForTargetCall { + c.Call = c.Call.DoAndReturn(f) + return c +} diff --git a/domain/access/modelmigration/package_test.go b/domain/access/modelmigration/package_test.go new file mode 100644 index 00000000000..dc7fd73299e --- /dev/null +++ b/domain/access/modelmigration/package_test.go @@ -0,0 +1,16 @@ +// Copyright 2024 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package modelmigration + +import ( + "testing" + + gc "gopkg.in/check.v1" +) + +//go:generate go run go.uber.org/mock/mockgen -typed -package modelmigration -destination migrations_mock_test.go github.com/juju/juju/domain/access/modelmigration Coordinator,ImportService,ExportService + +func TestPackage(t *testing.T) { + gc.TestingT(t) +} diff --git a/domain/access/service/user.go b/domain/access/service/user.go index b4699c4dd0c..b5369f905ff 100644 --- a/domain/access/service/user.go +++ b/domain/access/service/user.go @@ -294,6 +294,22 @@ func (s *UserService) UpdateLastModelLogin(ctx context.Context, name user.Name, return nil } +// SetLastModelLogin will set the last login time for the user to the given +// value. The following error types are possible from this function: - +// accesserrors.UserNameNotValid: When the username supplied is not valid. - +// accesserrors.UserNotFound: When the user cannot be found. - +// modelerrors.NotFound: If no model by the given modelUUID exists. +func (s *UserService) SetLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, lastLogin time.Time) error { + if name.IsZero() { + return errors.Annotatef(accesserrors.UserNameNotValid, "empty username") + } + + if err := s.st.UpdateLastModelLogin(ctx, name, modelUUID, lastLogin); err != nil { + return errors.Annotatef(err, "setting last login for user %q", name) + } + return nil +} + // LastModelLogin will return the last login time of the specified user. // The following error types are possible from this function: // - accesserrors.UserNameNotValid: When the username is not valid. diff --git a/domain/access/service/user_test.go b/domain/access/service/user_test.go index aca132a195d..c970ac08ba0 100644 --- a/domain/access/service/user_test.go +++ b/domain/access/service/user_test.go @@ -535,6 +535,33 @@ func (s *userServiceSuite) TestUpdateLastModelLogin(c *gc.C) { c.Assert(err, jc.ErrorIsNil) } +// TestUpdateLastModelLogin tests a bad username for UpdateLastModelLogin. +func (s *userServiceSuite) TestUpdateLastModelLoginBadUsername(c *gc.C) { + defer s.setupMocks(c).Finish() + modelUUID := modeltesting.GenModelUUID(c) + err := s.service().UpdateLastModelLogin(context.Background(), user.Name{}, modelUUID) + c.Assert(err, jc.ErrorIs, usererrors.UserNameNotValid) +} + +// TestSetLastModelLogin tests the happy path for SetLastModelLogin. +func (s *userServiceSuite) TestSetLastModelLogin(c *gc.C) { + defer s.setupMocks(c).Finish() + modelUUID := modeltesting.GenModelUUID(c) + lastLogin := time.Now() + s.state.EXPECT().UpdateLastModelLogin(gomock.Any(), coreusertesting.GenNewName(c, "name"), modelUUID, lastLogin) + + err := s.service().SetLastModelLogin(context.Background(), coreusertesting.GenNewName(c, "name"), modelUUID, lastLogin) + c.Assert(err, jc.ErrorIsNil) +} + +// TestSetLastModelLogin tests a bad username for SetLastModelLogin. +func (s *userServiceSuite) TestSetLastModelLoginBadUsername(c *gc.C) { + defer s.setupMocks(c).Finish() + modelUUID := modeltesting.GenModelUUID(c) + err := s.service().SetLastModelLogin(context.Background(), user.Name{}, modelUUID, time.Time{}) + c.Assert(err, jc.ErrorIs, usererrors.UserNameNotValid) +} + // TestLastModelLogin tests the happy path for LastModelLogin. func (s *userServiceSuite) TestLastModelLogin(c *gc.C) { defer s.setupMocks(c).Finish() @@ -547,14 +574,6 @@ func (s *userServiceSuite) TestLastModelLogin(c *gc.C) { c.Assert(lastConnection, gc.Equals, t) } -// TestUpdateLastModelLogin tests a bad username for UpdateLastModelLogin. -func (s *userServiceSuite) TestUpdateLastModelLoginBadUsername(c *gc.C) { - defer s.setupMocks(c).Finish() - modelUUID := modeltesting.GenModelUUID(c) - err := s.service().UpdateLastModelLogin(context.Background(), user.Name{}, modelUUID) - c.Assert(err, jc.ErrorIs, usererrors.UserNameNotValid) -} - // TestLastModelLoginBadUUID tests a bad UUID given to LastModelLogin. func (s *userServiceSuite) TestLastModelLoginBadUUID(c *gc.C) { defer s.setupMocks(c).Finish() diff --git a/domain/model/state/state_test.go b/domain/model/state/state_test.go index 6694c377eca..ca176d275b9 100644 --- a/domain/model/state/state_test.go +++ b/domain/model/state/state_test.go @@ -165,7 +165,7 @@ func (m *stateSuite) SetUpTest(c *gc.C) { ) c.Assert(err, jc.ErrorIsNil) - err = userState.UpdateLastModelLogin(context.Background(), m.userName, m.uuid) + err = userState.UpdateLastModelLogin(context.Background(), m.userName, m.uuid, time.Time{}) c.Assert(err, jc.ErrorIsNil) err = modelSt.Activate(context.Background(), m.uuid) diff --git a/domain/modelmigration/export.go b/domain/modelmigration/export.go index 21b2f68a13b..2e1cbbe8037 100644 --- a/domain/modelmigration/export.go +++ b/domain/modelmigration/export.go @@ -5,6 +5,7 @@ package modelmigration import ( "github.com/juju/juju/core/logger" + access "github.com/juju/juju/domain/access/modelmigration" application "github.com/juju/juju/domain/application/modelmigration" blockdevice "github.com/juju/juju/domain/blockdevice/modelmigration" credential "github.com/juju/juju/domain/credential/modelmigration" @@ -26,6 +27,7 @@ func ExportOperations( logger logger.Logger, ) { modelconfig.RegisterExport(coordinator) + access.RegisterExport(coordinator, logger.Child("access")) credential.RegisterExport(coordinator, logger.Child("credential")) network.RegisterExport(coordinator, logger.Child("network")) externalcontroller.RegisterExport(coordinator) diff --git a/domain/modelmigration/import.go b/domain/modelmigration/import.go index 9b9f3cb860e..7c1d9ab4858 100644 --- a/domain/modelmigration/import.go +++ b/domain/modelmigration/import.go @@ -6,6 +6,7 @@ package modelmigration import ( "github.com/juju/juju/core/logger" "github.com/juju/juju/core/modelmigration" + access "github.com/juju/juju/domain/access/modelmigration" application "github.com/juju/juju/domain/application/modelmigration" blockdevice "github.com/juju/juju/domain/blockdevice/modelmigration" credential "github.com/juju/juju/domain/credential/modelmigration" @@ -43,6 +44,7 @@ func ImportOperations( credential.RegisterImport(coordinator, logger.Child("credential")) model.RegisterImport(coordinator, logger.Child("model")) modelconfig.RegisterImport(coordinator, modelDefaultsProvider) + access.RegisterImport(coordinator, logger.Child("access")) network.RegisterImport(coordinator, logger.Child("network")) machine.RegisterImport(coordinator, logger.Child("machine")) application.RegisterImport(coordinator, registry, logger.Child("application")) diff --git a/state/migration_export.go b/state/migration_export.go index 5d960e178f5..4e0c39e1a75 100644 --- a/state/migration_export.go +++ b/state/migration_export.go @@ -162,9 +162,6 @@ func (st *State) exportImpl(cfg ExportConfig, leaders map[string]string, store o if err := export.modelStatus(); err != nil { return nil, errors.Trace(err) } - if err := export.modelUsers(); err != nil { - return nil, errors.Trace(err) - } if err := export.machines(); err != nil { return nil, errors.Trace(err) } @@ -313,30 +310,6 @@ func (e *exporter) modelStatus() error { return nil } -func (e *exporter) modelUsers() error { - users, err := e.dbModel.Users() - if err != nil { - return errors.Trace(err) - } - lastConnections, err := e.readLastConnectionTimes() - if err != nil { - return errors.Trace(err) - } - for _, user := range users { - lastConn := lastConnections[strings.ToLower(user.UserName.Name())] - arg := description.UserArgs{ - Name: user.UserTag, - DisplayName: user.DisplayName, - CreatedBy: user.CreatedBy, - DateCreated: user.DateCreated, - LastConnection: lastConn, - Access: string(user.Access), - } - e.model.AddUser(arg) - } - return nil -} - func (e *exporter) machines() error { machines, err := e.st.AllMachines() if err != nil { @@ -1704,22 +1677,6 @@ func (e *exporter) cloudContainer(doc *cloudContainerDoc) *description.CloudCont return result } -func (e *exporter) readLastConnectionTimes() (map[string]time.Time, error) { - lastConnections, closer := e.st.db().GetCollection(modelUserLastConnectionC) - defer closer() - - var docs []modelUserLastConnectionDoc - if err := lastConnections.Find(nil).All(&docs); err != nil { - return nil, errors.Trace(err) - } - - result := make(map[string]time.Time) - for _, doc := range docs { - result[doc.UserName] = doc.LastConnection.UTC() - } - return result, nil -} - func (e *exporter) readAllConstraints() error { constraintsCollection, closer := e.st.db().GetCollection(constraintsC) defer closer() diff --git a/state/migration_export_test.go b/state/migration_export_test.go index a1bfa7fd06d..1ee5fc6af6b 100644 --- a/state/migration_export_test.go +++ b/state/migration_export_test.go @@ -24,7 +24,6 @@ import ( "github.com/juju/juju/core/instance" "github.com/juju/juju/core/network" "github.com/juju/juju/core/payloads" - "github.com/juju/juju/core/permission" "github.com/juju/juju/core/resources" resourcetesting "github.com/juju/juju/core/resources/testing" "github.com/juju/juju/core/status" @@ -191,50 +190,6 @@ func (s *MigrationExportSuite) TestModelInfo(c *gc.C) { }) } -func (s *MigrationExportSuite) TestModelUsers(c *gc.C) { - // Make sure we have some last connection times for the admin user, - // and create a few other users. - lastConnection := state.NowToTheSecond(s.State) - owner, err := s.State.UserAccess(s.Owner, s.Model.ModelTag()) - c.Assert(err, jc.ErrorIsNil) - err = state.UpdateModelUserLastConnection(s.State, owner, lastConnection) - c.Assert(err, jc.ErrorIsNil) - - bobTag := names.NewUserTag("bob@external") - bob, err := s.Model.AddUser(state.UserAccessSpec{ - User: bobTag, - CreatedBy: s.Owner, - Access: permission.ReadAccess, - }) - c.Assert(err, jc.ErrorIsNil) - err = state.UpdateModelUserLastConnection(s.State, bob, lastConnection) - c.Assert(err, jc.ErrorIsNil) - - model, err := s.State.Export(map[string]string{}, state.NewObjectStore(c, s.State.ModelUUID())) - c.Assert(err, jc.ErrorIsNil) - - users := model.Users() - c.Assert(users, gc.HasLen, 2) - - exportedBob := users[0] - // admin is "test-admin", and results are sorted - exportedAdmin := users[1] - - c.Assert(exportedAdmin.Name(), gc.Equals, s.Owner) - c.Assert(exportedAdmin.DisplayName(), gc.Equals, owner.DisplayName) - c.Assert(exportedAdmin.CreatedBy(), gc.Equals, s.Owner) - c.Assert(exportedAdmin.DateCreated(), gc.Equals, owner.DateCreated) - c.Assert(exportedAdmin.LastConnection(), gc.Equals, lastConnection) - c.Assert(exportedAdmin.Access(), gc.Equals, "admin") - - c.Assert(exportedBob.Name(), gc.Equals, bobTag) - c.Assert(exportedBob.DisplayName(), gc.Equals, "") - c.Assert(exportedBob.CreatedBy(), gc.Equals, s.Owner) - c.Assert(exportedBob.DateCreated(), gc.Equals, bob.DateCreated) - c.Assert(exportedBob.LastConnection(), gc.Equals, lastConnection) - c.Assert(exportedBob.Access(), gc.Equals, "read") -} - func (s *MigrationExportSuite) TestMachines(c *gc.C) { s.assertMachinesMigrated(c, constraints.MustParse("arch=amd64 mem=8G tags=foo,bar spaces=dmz")) } diff --git a/state/migration_import.go b/state/migration_import.go index a7857a75e7b..56913999a02 100644 --- a/state/migration_import.go +++ b/state/migration_import.go @@ -26,7 +26,6 @@ import ( corelogger "github.com/juju/juju/core/logger" "github.com/juju/juju/core/network" "github.com/juju/juju/core/payloads" - "github.com/juju/juju/core/permission" "github.com/juju/juju/core/status" "github.com/juju/juju/environs/config" "github.com/juju/juju/internal/charm" @@ -150,10 +149,6 @@ func (ctrl *Controller) Import( if err := restore.operations(); err != nil { return nil, nil, errors.Annotate(err, "operations") } - - if err := restore.modelUsers(); err != nil { - return nil, nil, errors.Annotate(err, "modelUsers") - } if err := restore.machines(); err != nil { return nil, nil, errors.Annotate(err, "machines") } @@ -309,48 +304,6 @@ func (i *importer) sequences() error { return nil } -func (i *importer) modelUsers() error { - i.logger.Debugf("importing users") - - // The user that was auto-added when we created the model will have - // the wrong DateCreated, so we remove it, and add in all the users we - // know about. It is also possible that the owner of the model no - // longer has access to the model due to changes over time. - if err := i.st.RemoveUserAccess(i.dbModel.Owner(), i.dbModel.ModelTag()); err != nil { - return errors.Trace(err) - } - - users := i.model.Users() - modelUUID := i.dbModel.UUID() - var ops []txn.Op - for _, user := range users { - ops = append(ops, createModelUserOps( - modelUUID, - user.Name(), - user.CreatedBy(), - user.DisplayName(), - user.DateCreated(), - permission.Access(user.Access()))..., - ) - } - if err := i.st.db().RunTransaction(ops); err != nil { - return errors.Trace(err) - } - // Now set their last connection times. - for _, user := range users { - i.logger.Debugf("user %s", user.Name()) - lastConnection := user.LastConnection() - if lastConnection.IsZero() { - continue - } - err := i.dbModel.updateLastModelConnection(user.Name(), lastConnection) - if err != nil { - return errors.Trace(err) - } - } - return nil -} - func (i *importer) machines() error { i.logger.Debugf("importing machines") for _, m := range i.model.Machines() { diff --git a/state/migration_import_test.go b/state/migration_import_test.go index 2d1ce6e4a28..9c780955b18 100644 --- a/state/migration_import_test.go +++ b/state/migration_import_test.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "sort" - "time" // only uses time.Time values "github.com/juju/description/v8" "github.com/juju/errors" @@ -24,7 +23,6 @@ import ( "github.com/juju/juju/core/instance" "github.com/juju/juju/core/network" "github.com/juju/juju/core/payloads" - "github.com/juju/juju/core/permission" "github.com/juju/juju/core/status" jujuversion "github.com/juju/juju/core/version" "github.com/juju/juju/internal/charm" @@ -203,71 +201,6 @@ func (s *MigrationImportSuite) TestNewModel(c *gc.C) { c.Assert(blocks[0].Message(), gc.Equals, "locked down") } -func (s *MigrationImportSuite) newModelUser(c *gc.C, name string, readOnly bool, lastConnection time.Time) permission.UserAccess { - access := permission.AdminAccess - if readOnly { - access = permission.ReadAccess - } - user, err := s.Model.AddUser(state.UserAccessSpec{ - User: names.NewUserTag(name), - CreatedBy: s.Owner, - Access: access, - }) - c.Assert(err, jc.ErrorIsNil) - if !lastConnection.IsZero() { - err = state.UpdateModelUserLastConnection(s.State, user, lastConnection) - c.Assert(err, jc.ErrorIsNil) - } - return user -} - -func (s *MigrationImportSuite) AssertUserEqual(c *gc.C, newUser, oldUser permission.UserAccess) { - c.Assert(newUser.UserName, gc.Equals, oldUser.UserName) - c.Assert(newUser.DisplayName, gc.Equals, oldUser.DisplayName) - c.Assert(newUser.CreatedBy, gc.Equals, oldUser.CreatedBy) - c.Assert(newUser.DateCreated, gc.Equals, oldUser.DateCreated) - c.Assert(newUser.Access, gc.Equals, newUser.Access) - - connTime, err := s.Model.LastModelConnection(oldUser.UserTag) - if state.IsNeverConnectedError(err) { - _, err := s.Model.LastModelConnection(newUser.UserTag) - // The new user should also return an error for last connection. - c.Assert(err, jc.Satisfies, state.IsNeverConnectedError) - } else { - c.Assert(err, jc.ErrorIsNil) - newTime, err := s.Model.LastModelConnection(newUser.UserTag) - c.Assert(err, jc.ErrorIsNil) - c.Assert(newTime, gc.Equals, connTime) - } -} - -func (s *MigrationImportSuite) TestModelUsers(c *gc.C) { - // To be sure with this test, we create three env users, and remove - // the owner. - err := s.State.RemoveUserAccess(s.Owner, s.modelTag) - c.Assert(err, jc.ErrorIsNil) - - lastConnection := state.NowToTheSecond(s.State) - - bravo := s.newModelUser(c, "bravo@external", false, lastConnection) - charlie := s.newModelUser(c, "charlie@external", true, lastConnection) - delta := s.newModelUser(c, "delta@external", true, coretesting.ZeroTime()) - - newModel, newSt := s.importModel(c, s.State) - - // Check the import values of the users. - for _, user := range []permission.UserAccess{bravo, charlie, delta} { - newUser, err := newSt.UserAccess(user.UserTag, newModel.Tag()) - c.Assert(err, jc.ErrorIsNil) - s.AssertUserEqual(c, newUser, user) - } - - // Also make sure that there aren't any more. - allUsers, err := newModel.Users() - c.Assert(err, jc.ErrorIsNil) - c.Assert(allUsers, gc.HasLen, 3) -} - func (s *MigrationImportSuite) AssertMachineEqual(c *gc.C, newMachine, oldMachine *state.Machine) { c.Check(newMachine.Id(), gc.Equals, oldMachine.Id()) c.Check(newMachine.Principals(), jc.DeepEquals, oldMachine.Principals()) From 92e7d2bcfa776b021125ae0941335e57c043e7f5 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 23 Aug 2024 13:59:44 +0100 Subject: [PATCH 2/3] fix: model last login flaky test The model last login test called time.Now twice and assumed that they would be at least a microsecond apart. This is not the case on a certain someones shiney new laptop. Change the UpdateModelLastLogin function to take an explict time value to enter into the database. The service function calls time.Now. (This is also useful for migration as we can set old model last logins now). Change the test to use time values that are explicty set and checked against rather than relying on the differece between calls to time.Now. --- domain/access/service/service.go | 2 +- domain/access/service/state_mock_test.go | 12 +++++----- domain/access/service/user.go | 2 +- domain/access/service/user_test.go | 2 +- domain/access/state/types.go | 15 ++++-------- domain/access/state/user.go | 29 ++++++++++++------------ domain/access/state/user_test.go | 26 +++++++++++++-------- 7 files changed, 44 insertions(+), 44 deletions(-) diff --git a/domain/access/service/service.go b/domain/access/service/service.go index 1bf868dae26..29dc83616d4 100644 --- a/domain/access/service/service.go +++ b/domain/access/service/service.go @@ -136,7 +136,7 @@ type UserState interface { // - accesserrors.UserNameNotValid: When the username is not valid. // - accesserrors.UserNotFound: When the user cannot be found. // - modelerrors.NotFound: If no model by the given modelUUID exists. - UpdateLastModelLogin(context.Context, user.Name, coremodel.UUID) error + UpdateLastModelLogin(context.Context, user.Name, coremodel.UUID, time.Time) error // LastModelLogin will return the last login time of the specified user. // The following error types are possible from this function: diff --git a/domain/access/service/state_mock_test.go b/domain/access/service/state_mock_test.go index 8916d0910ae..3a5b0afe7c9 100644 --- a/domain/access/service/state_mock_test.go +++ b/domain/access/service/state_mock_test.go @@ -974,17 +974,17 @@ func (c *MockStateSetPasswordHashCall) DoAndReturn(f func(context.Context, user. } // UpdateLastModelLogin mocks base method. -func (m *MockState) UpdateLastModelLogin(arg0 context.Context, arg1 user.Name, arg2 model.UUID) error { +func (m *MockState) UpdateLastModelLogin(arg0 context.Context, arg1 user.Name, arg2 model.UUID, arg3 time.Time) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateLastModelLogin", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "UpdateLastModelLogin", arg0, arg1, arg2, arg3) ret0, _ := ret[0].(error) return ret0 } // UpdateLastModelLogin indicates an expected call of UpdateLastModelLogin. -func (mr *MockStateMockRecorder) UpdateLastModelLogin(arg0, arg1, arg2 any) *MockStateUpdateLastModelLoginCall { +func (mr *MockStateMockRecorder) UpdateLastModelLogin(arg0, arg1, arg2, arg3 any) *MockStateUpdateLastModelLoginCall { mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastModelLogin", reflect.TypeOf((*MockState)(nil).UpdateLastModelLogin), arg0, arg1, arg2) + call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLastModelLogin", reflect.TypeOf((*MockState)(nil).UpdateLastModelLogin), arg0, arg1, arg2, arg3) return &MockStateUpdateLastModelLoginCall{Call: call} } @@ -1000,13 +1000,13 @@ func (c *MockStateUpdateLastModelLoginCall) Return(arg0 error) *MockStateUpdateL } // Do rewrite *gomock.Call.Do -func (c *MockStateUpdateLastModelLoginCall) Do(f func(context.Context, user.Name, model.UUID) error) *MockStateUpdateLastModelLoginCall { +func (c *MockStateUpdateLastModelLoginCall) Do(f func(context.Context, user.Name, model.UUID, time.Time) error) *MockStateUpdateLastModelLoginCall { c.Call = c.Call.Do(f) return c } // DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockStateUpdateLastModelLoginCall) DoAndReturn(f func(context.Context, user.Name, model.UUID) error) *MockStateUpdateLastModelLoginCall { +func (c *MockStateUpdateLastModelLoginCall) DoAndReturn(f func(context.Context, user.Name, model.UUID, time.Time) error) *MockStateUpdateLastModelLoginCall { c.Call = c.Call.DoAndReturn(f) return c } diff --git a/domain/access/service/user.go b/domain/access/service/user.go index b5369f905ff..52769d12a6f 100644 --- a/domain/access/service/user.go +++ b/domain/access/service/user.go @@ -288,7 +288,7 @@ func (s *UserService) UpdateLastModelLogin(ctx context.Context, name user.Name, return errors.Annotatef(accesserrors.UserNameNotValid, "empty username") } - if err := s.st.UpdateLastModelLogin(ctx, name, modelUUID); err != nil { + if err := s.st.UpdateLastModelLogin(ctx, name, modelUUID, time.Now()); err != nil { return errors.Annotatef(err, "updating last login for user %q", name) } return nil diff --git a/domain/access/service/user_test.go b/domain/access/service/user_test.go index c970ac08ba0..6526b922bfd 100644 --- a/domain/access/service/user_test.go +++ b/domain/access/service/user_test.go @@ -529,7 +529,7 @@ func FuzzGetUser(f *testing.F) { func (s *userServiceSuite) TestUpdateLastModelLogin(c *gc.C) { defer s.setupMocks(c).Finish() modelUUID := modeltesting.GenModelUUID(c) - s.state.EXPECT().UpdateLastModelLogin(gomock.Any(), coreusertesting.GenNewName(c, "name"), modelUUID) + s.state.EXPECT().UpdateLastModelLogin(gomock.Any(), coreusertesting.GenNewName(c, "name"), modelUUID, gomock.Any()) err := s.service().UpdateLastModelLogin(context.Background(), coreusertesting.GenNewName(c, "name"), modelUUID) c.Assert(err, jc.ErrorIsNil) diff --git a/domain/access/state/types.go b/domain/access/state/types.go index 84123e72ead..65efe73a755 100644 --- a/domain/access/state/types.go +++ b/domain/access/state/types.go @@ -185,11 +185,12 @@ type permInOut struct { Access string `db:"access_type"` } -// dbModelAccess is a struct used to record a users logging in to a particular +// dbModelLastLogin is a struct used to record a users logging in to a particular // model. -type dbModelAccess struct { - UserUUID string `db:"user_uuid"` - ModelUUID string `db:"model_uuid"` +type dbModelLastLogin struct { + UserUUID string `db:"user_uuid"` + ModelUUID string `db:"model_uuid"` + Time time.Time `db:"time"` } // dbModelUUID is a struct used to record a model UUID from the database. @@ -203,12 +204,6 @@ type dbModelExists struct { Exists bool `db:"exists"` } -// loginTime is used to record the last time a user logged in when reading from -// model_last_login. -type loginTime struct { - Time time.Time `db:"time"` -} - // dbEveryoneExternal represents the permissions of the everyone@external user. type dbEveryoneExternal dbPermission diff --git a/domain/access/state/user.go b/domain/access/state/user.go index 0e5e2590758..d08d9f3940d 100644 --- a/domain/access/state/user.go +++ b/domain/access/state/user.go @@ -738,7 +738,7 @@ func AddUserWithPermission( // - accesserrors.UserNameNotValid: When the username is not valid. // - accesserrors.UserNotFound: When the user cannot be found. // - modelerrors.NotFound: If no model by the given modelUUID exists. -func (st *UserState) UpdateLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) error { +func (st *UserState) UpdateLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, lastLogin time.Time) error { db, err := st.DB() if err != nil { return errors.Annotate(err, "getting DB access") @@ -751,11 +751,9 @@ func (st *UserState) UpdateLastModelLogin(ctx context.Context, name user.Name, m insertModelLoginStmt, err := st.Prepare(` INSERT INTO model_last_login (model_uuid, user_uuid, time) --- The strftime formatter below inserts the time with millisecond precision. --- This is useful for testing. -VALUES ($dbModelAccess.model_uuid, $dbModelAccess.user_uuid, strftime('%Y-%m-%d %H:%M:%f', 'now')) +VALUES ($dbModelLastLogin.*) ON CONFLICT(model_uuid, user_uuid) DO UPDATE SET - time = excluded.time`, dbModelAccess{}) + time = excluded.time`, dbModelLastLogin{}) if err != nil { return errors.Annotate(err, "preparing insert model login query") } @@ -766,11 +764,13 @@ ON CONFLICT(model_uuid, user_uuid) DO UPDATE SET return errors.Trace(err) } - ma := dbModelAccess{ + mll := dbModelLastLogin{ UserUUID: userUUID.String(), ModelUUID: modelUUID.String(), + Time: lastLogin.Round(time.Second), } - if err := tx.Query(ctx, insertModelLoginStmt, ma).Run(); err != nil { + + if err := tx.Query(ctx, insertModelLoginStmt, mll).Run(); err != nil { if internaldatabase.IsErrConstraintForeignKey(err) { // The foreign key constrain may be triggered if the user or the // model does not exist. However, the user must exist or the @@ -806,13 +806,13 @@ func (st *UserState) LastModelLogin(ctx context.Context, name user.Name, modelUU } getLastModelLoginTime := ` -SELECT time AS &loginTime.time +SELECT time AS &dbModelLastLogin.time FROM model_last_login -WHERE model_uuid = $dbModelAccess.model_uuid -AND user_uuid = $dbModelAccess.user_uuid +WHERE model_uuid = $dbModelLastLogin.model_uuid +AND user_uuid = $dbModelLastLogin.user_uuid ORDER BY time DESC LIMIT 1; ` - getLastModelLoginTimeStmt, err := st.Prepare(getLastModelLoginTime, loginTime{}, dbModelAccess{}) + getLastModelLoginTimeStmt, err := st.Prepare(getLastModelLoginTime, dbModelLastLogin{}) if err != nil { return time.Time{}, errors.Annotate(err, "preparing select getLastModelLoginTime query") } @@ -824,12 +824,11 @@ ORDER BY time DESC LIMIT 1; return errors.Trace(err) } - var result loginTime - ma := dbModelAccess{ + mll := dbModelLastLogin{ ModelUUID: modelUUID.String(), UserUUID: userUUID.String(), } - err = tx.Query(ctx, getLastModelLoginTimeStmt, ma).Get(&result) + err = tx.Query(ctx, getLastModelLoginTimeStmt, mll).Get(&mll) if errors.Is(err, sql.ErrNoRows) { if exists, err := st.checkModelExists(ctx, tx, modelUUID); err != nil { return errors.Annotate(err, "checking model exists") @@ -841,7 +840,7 @@ ORDER BY time DESC LIMIT 1; return errors.Annotatef(err, "running query getLastModelLoginTime") } - lastConnection = result.Time + lastConnection = mll.Time if err != nil { return errors.Annotate(err, "parsing time") } diff --git a/domain/access/state/user_test.go b/domain/access/state/user_test.go index 48ea8e3cb2c..3fadbafba7b 100644 --- a/domain/access/state/user_test.go +++ b/domain/access/state/user_test.go @@ -1313,9 +1313,10 @@ func (s *userStateSuite) TestUpdateLastModelLogin(c *gc.C) { modelUUID := modeltesting.CreateTestModel(c, s.TxnRunnerFactory(), "test-update-last-login-model") st := NewUserState(s.TxnRunnerFactory()) name, adminUUID := s.addTestUser(c, st, "admin") + loginTime := time.Now() // Update last login. - err := st.UpdateLastModelLogin(context.Background(), name, modelUUID) + err := st.UpdateLastModelLogin(context.Background(), name, modelUUID, loginTime) c.Assert(err, jc.ErrorIsNil) // Check that the last login was updated correctly. @@ -1334,7 +1335,7 @@ WHERE user_uuid = ? err = row.Scan(&dbUserUUID, &dbModelUUID, &lastLogin) c.Assert(err, jc.ErrorIsNil) - c.Assert(lastLogin, gc.NotNil) + c.Assert(lastLogin.UTC(), gc.Equals, loginTime.Round(time.Second).UTC()) c.Assert(dbUserUUID, gc.Equals, string(adminUUID)) c.Assert(dbModelUUID, gc.Equals, string(modelUUID)) } @@ -1346,7 +1347,7 @@ func (s *userStateSuite) TestUpdateLastModelLoginModelNotFound(c *gc.C) { c.Assert(err, jc.ErrorIsNil) // Update last login. - err = st.UpdateLastModelLogin(context.Background(), name, badModelUUID) + err = st.UpdateLastModelLogin(context.Background(), name, badModelUUID, time.Time{}) c.Assert(err, gc.ErrorMatches, ".*model not found.*") c.Assert(err, jc.ErrorIs, modelerrors.NotFound) } @@ -1356,27 +1357,32 @@ func (s *userStateSuite) TestLastModelLogin(c *gc.C) { st := NewUserState(s.TxnRunnerFactory()) username1, _ := s.addTestUser(c, st, "user1") username2, _ := s.addTestUser(c, st, "user2") + expectedTime1 := time.Now() + expectedTime2 := expectedTime1.Add(time.Minute) // Simulate two logins to the model. - err := st.UpdateLastModelLogin(context.Background(), username1, modelUUID) + err := st.UpdateLastModelLogin(context.Background(), username1, modelUUID, expectedTime1) c.Assert(err, jc.ErrorIsNil) - err = st.UpdateLastModelLogin(context.Background(), username2, modelUUID) + err = st.UpdateLastModelLogin(context.Background(), username2, modelUUID, expectedTime2) c.Assert(err, jc.ErrorIsNil) - // Check user2 was the last to login. + // Check login times. time1, err := st.LastModelLogin(context.Background(), username1, modelUUID) c.Assert(err, jc.ErrorIsNil) + c.Check(time1.UTC(), gc.Equals, expectedTime1.Round(time.Second).UTC()) time2, err := st.LastModelLogin(context.Background(), username2, modelUUID) c.Assert(err, jc.ErrorIsNil) - c.Check(time1.Before(time2), jc.IsTrue, gc.Commentf("time1 is after time2 (%s is after %s)", time1, time2)) + c.Check(time2.UTC(), gc.Equals, expectedTime2.Round(time.Second).UTC()) + // Simulate a new login from user1 - err = st.UpdateLastModelLogin(context.Background(), username1, modelUUID) + expectedTime3 := expectedTime2.Add(time.Minute) + err = st.UpdateLastModelLogin(context.Background(), username1, modelUUID, expectedTime3) c.Assert(err, jc.ErrorIsNil) // Check the time for user1 was updated. - time1, err = st.LastModelLogin(context.Background(), username1, modelUUID) + time3, err := st.LastModelLogin(context.Background(), username1, modelUUID) c.Assert(err, jc.ErrorIsNil) - c.Check(time2.Before(time1), jc.IsTrue) + c.Check(time3, gc.Equals, expectedTime3.Round(time.Second).UTC()) } func (s *userStateSuite) TestLastModelLoginModelNotFound(c *gc.C) { From 623b11065e4ab6dcf24ff487b1e88bc1e6efa7d4 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 30 Aug 2024 16:54:34 +0100 Subject: [PATCH 3/3] fix: documentation on last login functions and tests Fix documentation on model last login functions and add some tests for model user permissions import. Also use Truncate instead of Round for model last login. --- domain/access/modelmigration/export.go | 15 ++-- domain/access/modelmigration/export_test.go | 4 +- domain/access/modelmigration/import.go | 14 ++-- domain/access/modelmigration/import_test.go | 78 +++++++++++++++++++-- domain/access/service/user.go | 22 +++--- domain/access/state/permission.go | 2 + domain/access/state/user.go | 16 ++--- domain/access/state/user_test.go | 8 +-- 8 files changed, 117 insertions(+), 42 deletions(-) diff --git a/domain/access/modelmigration/export.go b/domain/access/modelmigration/export.go index e2618150670..6f0c7df021f 100644 --- a/domain/access/modelmigration/export.go +++ b/domain/access/modelmigration/export.go @@ -31,15 +31,18 @@ func RegisterExport(coordinator Coordinator, logger logger.Logger) { // service methods needed for model permissions export. type ExportService interface { // ReadAllUserAccessForTarget return a slice of user access for all users - // with access to the given target. A NotValid error is returned if the - // target is not valid. Any errors from the state layer are passed through. + // with access to the given target. + // An [errors.NotValid] error is returned if the target is not valid. Any + // errors from the state layer are passed through. + // An [accesserrors.PermissionNotFound] error is returned if no permissions + // can be found on the target. ReadAllUserAccessForTarget(ctx context.Context, target corepermission.ID) ([]corepermission.UserAccess, error) // LastModelLogin will return the last login time of the specified user. // The following error types are possible from this function: - // - accesserrors.UserNameNotValid: When the username is not valid. - // - accesserrors.UserNotFound: When the user cannot be found. - // - modelerrors.NotFound: If no model by the given modelUUID exists. - // - accesserrors.UserNeverAccessedModel: If there is no record of the user + // - [accesserrors.UserNameNotValid] when the username is not valid. + // - [accesserrors.UserNotFound] when the user cannot be found. + // - [modelerrors.NotFound] if no model by the given modelUUID exists. + // - [accesserrors.UserNeverAccessedModel] if there is no record of the user // accessing the model. LastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) (time.Time, error) } diff --git a/domain/access/modelmigration/export_test.go b/domain/access/modelmigration/export_test.go index 8823ac714dc..fc8471ff3b3 100644 --- a/domain/access/modelmigration/export_test.go +++ b/domain/access/modelmigration/export_test.go @@ -72,8 +72,8 @@ func (s *exportSuite) TestExport(c *gc.C) { Key: dst.Tag().Id(), }).Return(userAccesses, nil) - bobTime := time.Now().Round(time.Minute).UTC() - bazzaTime := bobTime.Add(time.Minute) + bobTime := time.Now().Truncate(time.Minute).UTC() + bazzaTime := time.Now().Truncate(time.Minute).UTC().Add(-time.Minute) s.service.EXPECT().LastModelLogin( gomock.Any(), bobName, coremodel.UUID(dst.Tag().Id()), ).Return(bobTime, nil) diff --git a/domain/access/modelmigration/import.go b/domain/access/modelmigration/import.go index f43be9fe9d1..9f748c91b43 100644 --- a/domain/access/modelmigration/import.go +++ b/domain/access/modelmigration/import.go @@ -38,17 +38,17 @@ func RegisterImport(coordinator Coordinator, logger logger.Logger) { type ImportService interface { // CreatePermission gives the user access per the provided spec. // If the user provided does not exist or is marked removed, - // accesserrors.PermissionNotFound is returned. + // [accesserrors.PermissionNotFound] is returned. // If the user provided exists but is marked disabled, - // accesserrors.UserAuthenticationDisabled is returned. + // [accesserrors.UserAuthenticationDisabled] is returned. // If a permission for the user and target key already exists, - // accesserrors.PermissionAlreadyExists is returned. + // [accesserrors.PermissionAlreadyExists] is returned. CreatePermission(ctx context.Context, spec corepermission.UserAccessSpec) (corepermission.UserAccess, error) // SetLastModelLogin will set the last login time for the user to the given - // value. The following error types are possible from this function: - - // accesserrors.UserNameNotValid: When the username supplied is not valid. - - // accesserrors.UserNotFound: When the user cannot be found. - - // modelerrors.NotFound: If no model by the given modelUUID exists. + // value. The following error types are possible from this function: + // [accesserrors.UserNameNotValid] when the username supplied is not valid. + // [accesserrors.UserNotFound] when the user cannot be found. + // [modelerrors.NotFound] if no model by the given modelUUID exists. SetLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, time time.Time) error } diff --git a/domain/access/modelmigration/import_test.go b/domain/access/modelmigration/import_test.go index 7f837ac2d53..8c1bdcf69b3 100644 --- a/domain/access/modelmigration/import_test.go +++ b/domain/access/modelmigration/import_test.go @@ -16,6 +16,7 @@ import ( coremodel "github.com/juju/juju/core/model" "github.com/juju/juju/core/permission" "github.com/juju/juju/core/user" + accesserrors "github.com/juju/juju/domain/access/errors" loggertesting "github.com/juju/juju/internal/logger/testing" ) @@ -69,21 +70,21 @@ func (s *importSuite) TestImport(c *gc.C) { ObjectType: permission.Model, Key: modelUUID, } - steveTag := names.NewUserTag("steve") + creatorTag := names.NewUserTag("creator") bobTag := names.NewUserTag("bob") bobName := user.NameFromTag(bobTag) - bobTime := time.Now().Round(time.Minute).UTC() + bobTime := time.Now().Truncate(time.Minute).UTC() bob := description.UserArgs{ Name: bobTag, Access: string(permission.AdminAccess), - CreatedBy: steveTag, + CreatedBy: creatorTag, DateCreated: time.Now(), DisplayName: bobTag.Name(), LastConnection: bobTime, } bazzaTag := names.NewUserTag("bazza") bazzaName := user.NameFromTag(bazzaTag) - bazzaTime := bobTime.Add(time.Minute) + bazzaTime := time.Now().Truncate(time.Minute).UTC().Add(-time.Minute) bazza := description.UserArgs{ Name: bazzaTag, Access: string(permission.ReadAccess), @@ -116,3 +117,72 @@ func (s *importSuite) TestImport(c *gc.C) { err := op.Execute(context.Background(), model) c.Assert(err, jc.ErrorIsNil) } + +// TestImportPermissionAlreadyExists tests that permissions that already exist +// are ignored. This covers the permission of the model creator which is added +// the model is added. +func (s *importSuite) TestImportPermissionAlreadyExists(c *gc.C) { + defer s.setupMocks(c).Finish() + + model := description.NewModel(description.ModelArgs{}) + modelUUID := model.Tag().Id() + modelID := permission.ID{ + ObjectType: permission.Model, + Key: modelUUID, + } + adminTag := names.NewUserTag("admin") + admin := description.UserArgs{ + Name: adminTag, + Access: string(permission.AdminAccess), + CreatedBy: adminTag, + DateCreated: time.Now(), + DisplayName: adminTag.Id(), + LastConnection: time.Time{}, + } + model.AddUser(admin) + s.service.EXPECT().CreatePermission(gomock.Any(), permission.UserAccessSpec{ + AccessSpec: permission.AccessSpec{ + Target: modelID, + Access: permission.AdminAccess, + }, + User: user.AdminUserName, + }).Return(permission.UserAccess{}, accesserrors.PermissionAlreadyExists) + + op := s.newImportOperation() + err := op.Execute(context.Background(), model) + c.Assert(err, jc.ErrorIsNil) +} + +// TestImportPermissionUserDisabled tests that this error is returned to the +// user. +func (s *importSuite) TestImportPermissionUserDisabled(c *gc.C) { + defer s.setupMocks(c).Finish() + + model := description.NewModel(description.ModelArgs{}) + modelUUID := model.Tag().Id() + modelID := permission.ID{ + ObjectType: permission.Model, + Key: modelUUID, + } + disabledUserTag := names.NewUserTag("disabledUser") + disabledUser := description.UserArgs{ + Name: disabledUserTag, + Access: string(permission.AdminAccess), + CreatedBy: disabledUserTag, + DateCreated: time.Now(), + DisplayName: disabledUserTag.Id(), + LastConnection: time.Time{}, + } + model.AddUser(disabledUser) + s.service.EXPECT().CreatePermission(gomock.Any(), permission.UserAccessSpec{ + AccessSpec: permission.AccessSpec{ + Target: modelID, + Access: permission.AdminAccess, + }, + User: user.NameFromTag(disabledUserTag), + }).Return(permission.UserAccess{}, accesserrors.UserAuthenticationDisabled) + + op := s.newImportOperation() + err := op.Execute(context.Background(), model) + c.Assert(err, jc.ErrorIs, accesserrors.UserAuthenticationDisabled) +} diff --git a/domain/access/service/user.go b/domain/access/service/user.go index 52769d12a6f..3e1b83dbdd3 100644 --- a/domain/access/service/user.go +++ b/domain/access/service/user.go @@ -280,9 +280,9 @@ func (s *UserService) DisableUserAuthentication(ctx context.Context, name user.N // UpdateLastModelLogin will update the last login time for the user. // The following error types are possible from this function: -// - accesserrors.UserNameNotValid: When the username supplied is not valid. -// - accesserrors.UserNotFound: When the user cannot be found. -// - modelerrors.NotFound: If no model by the given modelUUID exists. +// - [accesserrors.UserNameNotValid] when the username supplied is not valid. +// - [accesserrors.UserNotFound] when the user cannot be found. +// - [modelerrors.NotFound] if no model by the given modelUUID exists. func (s *UserService) UpdateLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) error { if name.IsZero() { return errors.Annotatef(accesserrors.UserNameNotValid, "empty username") @@ -295,10 +295,10 @@ func (s *UserService) UpdateLastModelLogin(ctx context.Context, name user.Name, } // SetLastModelLogin will set the last login time for the user to the given -// value. The following error types are possible from this function: - -// accesserrors.UserNameNotValid: When the username supplied is not valid. - -// accesserrors.UserNotFound: When the user cannot be found. - -// modelerrors.NotFound: If no model by the given modelUUID exists. +// value. The following error types are possible from this function: +// [accesserrors.UserNameNotValid] when the username supplied is not valid. +// [accesserrors.UserNotFound] when the user cannot be found. +// [modelerrors.NotFound] if no model by the given modelUUID exists. func (s *UserService) SetLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, lastLogin time.Time) error { if name.IsZero() { return errors.Annotatef(accesserrors.UserNameNotValid, "empty username") @@ -312,10 +312,10 @@ func (s *UserService) SetLastModelLogin(ctx context.Context, name user.Name, mod // LastModelLogin will return the last login time of the specified user. // The following error types are possible from this function: -// - accesserrors.UserNameNotValid: When the username is not valid. -// - accesserrors.UserNotFound: When the user cannot be found. -// - modelerrors.NotFound: If no model by the given modelUUID exists. -// - accesserrors.UserNeverAccessedModel: If there is no record of the user +// - [accesserrors.UserNameNotValid] when the username is not valid. +// - [accesserrors.UserNotFound] when the user cannot be found. +// - [modelerrors.NotFound] if no model by the given modelUUID exists. +// - [accesserrors.UserNeverAccessedModel] if there is no record of the user // accessing the model. func (s *UserService) LastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) (time.Time, error) { if name.IsZero() { diff --git a/domain/access/state/permission.go b/domain/access/state/permission.go index 261c074582c..7c781ae2002 100644 --- a/domain/access/state/permission.go +++ b/domain/access/state/permission.go @@ -457,6 +457,8 @@ WHERE u.removed = false // ReadAllUserAccessForTarget return a slice of user access for all users // with access to the given target. +// An [accesserrors.PermissionNotFound] error is returned if no permissions can +// be found on the target. func (st *PermissionState) ReadAllUserAccessForTarget(ctx context.Context, target corepermission.ID) ([]corepermission.UserAccess, error) { db, err := st.DB() if err != nil { diff --git a/domain/access/state/user.go b/domain/access/state/user.go index d08d9f3940d..6e68fb9583d 100644 --- a/domain/access/state/user.go +++ b/domain/access/state/user.go @@ -735,9 +735,9 @@ func AddUserWithPermission( // UpdateLastModelLogin updates the last login time for the user // with the supplied uuid on the model with the supplied model uuid. // The following error types are possible from this function: -// - accesserrors.UserNameNotValid: When the username is not valid. -// - accesserrors.UserNotFound: When the user cannot be found. -// - modelerrors.NotFound: If no model by the given modelUUID exists. +// - [accesserrors.UserNameNotValid] when the username is not valid. +// - [accesserrors.UserNotFound] when the user cannot be found. +// - [modelerrors.NotFound] if no model by the given modelUUID exists. func (st *UserState) UpdateLastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID, lastLogin time.Time) error { db, err := st.DB() if err != nil { @@ -767,7 +767,7 @@ ON CONFLICT(model_uuid, user_uuid) DO UPDATE SET mll := dbModelLastLogin{ UserUUID: userUUID.String(), ModelUUID: modelUUID.String(), - Time: lastLogin.Round(time.Second), + Time: lastLogin.Truncate(time.Second), } if err := tx.Query(ctx, insertModelLoginStmt, mll).Run(); err != nil { @@ -789,10 +789,10 @@ ON CONFLICT(model_uuid, user_uuid) DO UPDATE SET // LastModelLogin returns when the specified user last connected to the // specified model in UTC. The following errors can be returned: -// - accesserrors.UserNameNotValid: When the username is not valid. -// - accesserrors.UserNotFound: When the user cannot be found. -// - modelerrors.NotFound: If no model by the given modelUUID exists. -// - accesserrors.UserNeverAccessedModel: If there is no record of the user +// - [accesserrors.UserNameNotValid] when the username is not valid. +// - [accesserrors.UserNotFound] when the user cannot be found. +// - [modelerrors.NotFound] if no model by the given modelUUID exists. +// - [accesserrors.UserNeverAccessedModel] if there is no record of the user // accessing the model. func (st *UserState) LastModelLogin(ctx context.Context, name user.Name, modelUUID coremodel.UUID) (time.Time, error) { db, err := st.DB() diff --git a/domain/access/state/user_test.go b/domain/access/state/user_test.go index 3fadbafba7b..ed43f0cfd93 100644 --- a/domain/access/state/user_test.go +++ b/domain/access/state/user_test.go @@ -1335,7 +1335,7 @@ WHERE user_uuid = ? err = row.Scan(&dbUserUUID, &dbModelUUID, &lastLogin) c.Assert(err, jc.ErrorIsNil) - c.Assert(lastLogin.UTC(), gc.Equals, loginTime.Round(time.Second).UTC()) + c.Assert(lastLogin.UTC(), gc.Equals, loginTime.Truncate(time.Second).UTC()) c.Assert(dbUserUUID, gc.Equals, string(adminUUID)) c.Assert(dbModelUUID, gc.Equals, string(modelUUID)) } @@ -1369,10 +1369,10 @@ func (s *userStateSuite) TestLastModelLogin(c *gc.C) { // Check login times. time1, err := st.LastModelLogin(context.Background(), username1, modelUUID) c.Assert(err, jc.ErrorIsNil) - c.Check(time1.UTC(), gc.Equals, expectedTime1.Round(time.Second).UTC()) + c.Check(time1.UTC(), gc.Equals, expectedTime1.Truncate(time.Second).UTC()) time2, err := st.LastModelLogin(context.Background(), username2, modelUUID) c.Assert(err, jc.ErrorIsNil) - c.Check(time2.UTC(), gc.Equals, expectedTime2.Round(time.Second).UTC()) + c.Check(time2.UTC(), gc.Equals, expectedTime2.Truncate(time.Second).UTC()) // Simulate a new login from user1 expectedTime3 := expectedTime2.Add(time.Minute) @@ -1382,7 +1382,7 @@ func (s *userStateSuite) TestLastModelLogin(c *gc.C) { // Check the time for user1 was updated. time3, err := st.LastModelLogin(context.Background(), username1, modelUUID) c.Assert(err, jc.ErrorIsNil) - c.Check(time3, gc.Equals, expectedTime3.Round(time.Second).UTC()) + c.Check(time3, gc.Equals, expectedTime3.Truncate(time.Second).UTC()) } func (s *userStateSuite) TestLastModelLoginModelNotFound(c *gc.C) {