From bf113d36217e70cd1af8b73b49e46d309481c6ac Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Wed, 4 Sep 2024 11:49:42 +0100 Subject: [PATCH] feat: remove usages of model.Users The model user collection is being removed in the move to 4.0. Usages of it are being replaced with GetModelUsers on the model service and the collection in mongo will soon be deleted. As part of this, the model migration user doc is also removed from state. This is used when checking if a user trying to access a migrated model gets told it has been migrated or not. If the user had no permissions they were not told. This check can no longer be performed. TODOs have been put in the code and a card created on Jira to address this. --- apiserver/admin.go | 5 +- apiserver/admin_test.go | 7 +-- apiserver/common/modelmanagerinterface.go | 4 +- apiserver/facades/client/client/backend.go | 1 - .../client/client/package_mock_test.go | 39 --------------- .../facades/client/controller/controller.go | 10 ++-- .../controller/migrationmaster/mocks/state.go | 39 --------------- state/model.go | 43 ----------------- state/modelmigration.go | 48 ------------------- state/modelmigration_test.go | 25 ---------- 10 files changed, 17 insertions(+), 204 deletions(-) diff --git a/apiserver/admin.go b/apiserver/admin.go index c122b286706..fb5068c3023 100644 --- a/apiserver/admin.go +++ b/apiserver/admin.go @@ -350,7 +350,7 @@ func (a *admin) authenticate(ctx context.Context, req params.LoginRequest) (*aut } func (a *admin) maybeEmitRedirectError(modelUUID model.UUID, authTag names.Tag) error { - userTag, ok := authTag.(names.UserTag) + _, ok := authTag.(names.UserTag) if !ok { return nil } @@ -377,7 +377,8 @@ func (a *admin) maybeEmitRedirectError(modelUUID model.UUID, authTag names.Tag) // granted access, do not return a redirect error. // We need to return redirects if possible for anonymous logins in order // to ensure post-migration operation of CMRs. - if mig == nil || (userTag.Id() != api.AnonymousUsername && mig.ModelUserAccess(userTag) == permission.NoAccess) { + // TODO(aflynn): reinstate check for unauthorised user (JUJU-6669). + if mig == nil { return nil } diff --git a/apiserver/admin_test.go b/apiserver/admin_test.go index e517cd90a01..6cafc6d5638 100644 --- a/apiserver/admin_test.go +++ b/apiserver/admin_test.go @@ -616,9 +616,10 @@ func (s *loginSuite) TestMigratedModelLogin(c *gc.C) { // Attempt to open an API connection to the migrated model as a user // that had NO access to the model before it got migrated. The server // should return a not-authorized error when attempting to log in. - info.Tag = names.NewUserTag("some-other-user") - _, err = api.Open(context.Background(), info, fastDialOpts) - c.Assert(params.ErrCode(errors.Cause(err)), gc.Equals, params.CodeUnauthorized) + // TODO(aflynn): reinstate check for unauthorised user (JUJU-6669). + //info.Tag = names.NewUserTag("some-other-user") + //_, err = api.Open(context.Background(), info, fastDialOpts) + //c.Assert(params.ErrCode(errors.Cause(err)), gc.Equals, params.CodeUnauthorized) // Attempt to open an API connection to the migrated model as the // anonymous user; this should also be allowed on account of CMRs. diff --git a/apiserver/common/modelmanagerinterface.go b/apiserver/common/modelmanagerinterface.go index 8ed1402b507..a93750d6b16 100644 --- a/apiserver/common/modelmanagerinterface.go +++ b/apiserver/common/modelmanagerinterface.go @@ -199,7 +199,9 @@ func (st modelManagerStateShim) GetBackend(modelUUID string) (ModelManagerBacken return nil, nil, errors.Trace(mErr) } - if mig == nil || mig.ModelUserAccess(st.user) == permission.NoAccess { + // TODO(aflynn): Also return this error if, in the migration info, the + // user had access to the migrated model (JUJU-6669). + if mig == nil { return nil, nil, errors.Trace(err) // return original NotFound error } diff --git a/apiserver/facades/client/client/backend.go b/apiserver/facades/client/client/backend.go index c352c66744c..2d6d6b5d455 100644 --- a/apiserver/facades/client/client/backend.go +++ b/apiserver/facades/client/client/backend.go @@ -65,7 +65,6 @@ type Model interface { Config() (*config.Config, error) Owner() names.UserTag AddUser(state.UserAccessSpec) (permission.UserAccess, error) - Users() ([]permission.UserAccess, error) StatusHistory(status.StatusHistoryFilter) ([]status.StatusInfo, error) LatestToolsVersion() version.Number Status() (status.StatusInfo, error) diff --git a/apiserver/facades/client/client/package_mock_test.go b/apiserver/facades/client/client/package_mock_test.go index 8020aa9ec72..e3a7c911904 100644 --- a/apiserver/facades/client/client/package_mock_test.go +++ b/apiserver/facades/client/client/package_mock_test.go @@ -1389,42 +1389,3 @@ func (c *MockModelUUIDCall) DoAndReturn(f func() string) *MockModelUUIDCall { c.Call = c.Call.DoAndReturn(f) return c } - -// Users mocks base method. -func (m *MockModel) Users() ([]permission.UserAccess, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Users") - ret0, _ := ret[0].([]permission.UserAccess) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// Users indicates an expected call of Users. -func (mr *MockModelMockRecorder) Users() *MockModelUsersCall { - mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Users", reflect.TypeOf((*MockModel)(nil).Users)) - return &MockModelUsersCall{Call: call} -} - -// MockModelUsersCall wrap *gomock.Call -type MockModelUsersCall struct { - *gomock.Call -} - -// Return rewrite *gomock.Call.Return -func (c *MockModelUsersCall) Return(arg0 []permission.UserAccess, arg1 error) *MockModelUsersCall { - c.Call = c.Call.Return(arg0, arg1) - return c -} - -// Do rewrite *gomock.Call.Do -func (c *MockModelUsersCall) Do(f func() ([]permission.UserAccess, error)) *MockModelUsersCall { - c.Call = c.Call.Do(f) - return c -} - -// DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockModelUsersCall) DoAndReturn(f func() ([]permission.UserAccess, error)) *MockModelUsersCall { - c.Call = c.Call.DoAndReturn(f) - return c -} diff --git a/apiserver/facades/client/controller/controller.go b/apiserver/facades/client/controller/controller.go index 0fa9d3cedd8..b1211b6cc62 100644 --- a/apiserver/facades/client/controller/controller.go +++ b/apiserver/facades/client/controller/controller.go @@ -82,6 +82,9 @@ type ModelService interface { Model(ctx context.Context, uuid coremodel.UUID) (coremodel.Model, error) // ControllerModel returns the model used for housing the Juju controller. ControllerModel(ctx context.Context) (coremodel.Model, error) + // GetModelUsers will retrieve basic information about users with permissions on + // the given model UUID. + GetModelUsers(ctx context.Context, modelUUID coremodel.UUID) ([]coremodel.ModelUserInfo, error) } // ModelConfigService provides access to the model configuration. @@ -1038,17 +1041,18 @@ func makeModelInfo(ctx context.Context, st *state.State, return empty, ul, errors.Trace(err) } - users, err := model.Users() + modelID := coremodel.UUID(model.UUID()) + + users, err := modelService.GetModelUsers(ctx, modelID) if err != nil { return empty, ul, errors.Trace(err) } ul.users = set.NewStrings() for _, u := range users { - ul.users.Add(u.UserName.Name()) + ul.users.Add(u.Name.Name()) } // Retrieve agent version for the model. - modelID := coremodel.UUID(model.UUID()) modelInfo, err := modelService.Model(ctx, modelID) if err != nil { return empty, userList{}, fmt.Errorf("getting model %q: %w", modelID, err) diff --git a/apiserver/facades/controller/migrationmaster/mocks/state.go b/apiserver/facades/controller/migrationmaster/mocks/state.go index bb832e30622..411c8ee3fb7 100644 --- a/apiserver/facades/controller/migrationmaster/mocks/state.go +++ b/apiserver/facades/controller/migrationmaster/mocks/state.go @@ -14,7 +14,6 @@ import ( time "time" migration "github.com/juju/juju/core/migration" - permission "github.com/juju/juju/core/permission" state "github.com/juju/juju/state" names "github.com/juju/names/v5" gomock "go.uber.org/mock/gomock" @@ -272,44 +271,6 @@ func (c *MockModelMigrationModelUUIDCall) DoAndReturn(f func() string) *MockMode return c } -// ModelUserAccess mocks base method. -func (m *MockModelMigration) ModelUserAccess(arg0 names.Tag) permission.Access { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ModelUserAccess", arg0) - ret0, _ := ret[0].(permission.Access) - return ret0 -} - -// ModelUserAccess indicates an expected call of ModelUserAccess. -func (mr *MockModelMigrationMockRecorder) ModelUserAccess(arg0 any) *MockModelMigrationModelUserAccessCall { - mr.mock.ctrl.T.Helper() - call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ModelUserAccess", reflect.TypeOf((*MockModelMigration)(nil).ModelUserAccess), arg0) - return &MockModelMigrationModelUserAccessCall{Call: call} -} - -// MockModelMigrationModelUserAccessCall wrap *gomock.Call -type MockModelMigrationModelUserAccessCall struct { - *gomock.Call -} - -// Return rewrite *gomock.Call.Return -func (c *MockModelMigrationModelUserAccessCall) Return(arg0 permission.Access) *MockModelMigrationModelUserAccessCall { - c.Call = c.Call.Return(arg0) - return c -} - -// Do rewrite *gomock.Call.Do -func (c *MockModelMigrationModelUserAccessCall) Do(f func(names.Tag) permission.Access) *MockModelMigrationModelUserAccessCall { - c.Call = c.Call.Do(f) - return c -} - -// DoAndReturn rewrite *gomock.Call.DoAndReturn -func (c *MockModelMigrationModelUserAccessCall) DoAndReturn(f func(names.Tag) permission.Access) *MockModelMigrationModelUserAccessCall { - c.Call = c.Call.DoAndReturn(f) - return c -} - // Phase mocks base method. func (m *MockModelMigration) Phase() (migration.Phase, error) { m.ctrl.T.Helper() diff --git a/state/model.go b/state/model.go index 60063a56161..fafcf552388 100644 --- a/state/model.go +++ b/state/model.go @@ -19,7 +19,6 @@ import ( "github.com/juju/juju/controller" "github.com/juju/juju/core/constraints" - "github.com/juju/juju/core/permission" "github.com/juju/juju/core/status" "github.com/juju/juju/environs/config" internalpassword "github.com/juju/juju/internal/password" @@ -370,11 +369,6 @@ func (ctlr *Controller) NewModel(configSchemaGetter config.ConfigSchemaSourceGet _, _ = probablyUpdateStatusHistory(newSt.db(), newModel.Kind(), modelGlobalKey, modelGlobalKey, modelStatusDoc) } - _, err = newSt.SetUserAccess(newModel.Owner(), newModel.ModelTag(), permission.AdminAccess) - if err != nil { - return nil, nil, errors.Annotate(err, "granting admin permission to the owner") - } - // TODO(storage) - we need to add the default storage pools using the new dqlite model service //storageService, registry, err := newSt.storageServices() //if err != nil { @@ -820,43 +814,6 @@ func (m *Model) AllEndpointBindings() (map[string]*Bindings, error) { return appEndpointBindings, nil } -// Users returns a slice of all users for this model. -func (m *Model) Users() ([]permission.UserAccess, error) { - coll, closer := m.st.db().GetCollection(modelUsersC) - defer closer() - - var userDocs []userAccessDoc - err := coll.Find(nil).All(&userDocs) - if err != nil { - return nil, errors.Trace(err) - } - - var modelUsers []permission.UserAccess - for _, doc := range userDocs { - // check if the User belonging to this model user has - // been deleted, in this case we should not return it. - userTag := names.NewUserTag(doc.UserName) - if userTag.IsLocal() { - _, err := m.st.User(userTag) - if err != nil { - if !IsDeletedUserError(err) { - // We ignore deleted users for now. So if it is not a - // DeletedUserError we return the error. - return nil, errors.Trace(err) - } - continue - } - } - mu, err := NewModelUserAccess(m.st, doc) - if err != nil { - return nil, errors.Trace(err) - } - modelUsers = append(modelUsers, mu) - } - - return modelUsers, nil -} - // IsControllerModel returns a boolean indicating whether // this model is responsible for running a controller. func (m *Model) IsControllerModel() bool { diff --git a/state/modelmigration.go b/state/modelmigration.go index 9a11c680cdf..c3b224ee176 100644 --- a/state/modelmigration.go +++ b/state/modelmigration.go @@ -17,7 +17,6 @@ import ( "gopkg.in/macaroon.v2" "github.com/juju/juju/core/migration" - "github.com/juju/juju/core/permission" "github.com/juju/juju/core/status" "github.com/juju/juju/internal/mongo" ) @@ -94,10 +93,6 @@ type ModelMigration interface { // Refresh updates the contents of the ModelMigration from the // underlying state. Refresh() error - - // ModelUserAccess returns the type of access that the given tag had to - // the model prior to it being migrated. - ModelUserAccess(names.Tag) permission.Access } // MinionReports indicates the sets of agents whose migration minion @@ -159,14 +154,6 @@ type modelMigDoc struct { // TargetMacaroons holds the macaroons to use with TargetAuthTag // when authenticating. TargetMacaroons string `bson:"target-macaroons,omitempty"` - - // The list of users and their access-level to the model being migrated. - ModelUsers []modelMigUserDoc `bson:"model-users,omitempty"` -} - -type modelMigUserDoc struct { - UserID string `bson:"user_id"` - Access permission.Access `bson:"access"` } // modelMigStatusDoc tracks the progress of a migration attempt for a @@ -644,18 +631,6 @@ func (mig *modelMigration) Refresh() error { return nil } -// ModelUserAccess implements ModelMigration. -func (mig *modelMigration) ModelUserAccess(tag names.Tag) permission.Access { - id := tag.Id() - for _, user := range mig.doc.ModelUsers { - if user.UserID == id { - return user.Access - } - } - - return permission.NoAccess -} - // MigrationSpec holds the information required to create a // ModelMigration instance. type MigrationSpec struct { @@ -722,11 +697,6 @@ func (st *State) CreateMigration(spec MigrationSpec) (ModelMigration, error) { return nil, errors.Trace(err) } - userDocs, err := modelUserDocs(model) - if err != nil { - return nil, errors.Trace(err) - } - id := fmt.Sprintf("%s:%d", modelUUID, attempt) doc = modelMigDoc{ Id: id, @@ -740,7 +710,6 @@ func (st *State) CreateMigration(spec MigrationSpec) (ModelMigration, error) { TargetAuthTag: spec.TargetInfo.AuthTag.String(), TargetPassword: spec.TargetInfo.Password, TargetMacaroons: macsJSON, - ModelUsers: userDocs, } statusDoc = modelMigStatusDoc{ @@ -788,23 +757,6 @@ func (st *State) CreateMigration(spec MigrationSpec) (ModelMigration, error) { }, nil } -func modelUserDocs(m *Model) ([]modelMigUserDoc, error) { - users, err := m.Users() - if err != nil { - return nil, err - } - - var docs []modelMigUserDoc - for _, user := range users { - docs = append(docs, modelMigUserDoc{ - UserID: user.UserTag.Id(), - Access: user.Access, - }) - } - - return docs, nil -} - func macaroonsToJSON(m []macaroon.Slice) (string, error) { if len(m) == 0 { return "", nil diff --git a/state/modelmigration_test.go b/state/modelmigration_test.go index 97a405d2c1b..dbcafc48b50 100644 --- a/state/modelmigration_test.go +++ b/state/modelmigration_test.go @@ -15,7 +15,6 @@ import ( "gopkg.in/macaroon.v2" "github.com/juju/juju/core/migration" - "github.com/juju/juju/core/permission" "github.com/juju/juju/internal/testing" "github.com/juju/juju/internal/testing/factory" "github.com/juju/juju/internal/uuid" @@ -850,30 +849,6 @@ func (s *MigrationSuite) TestWatchMinionReportsMultiModel(c *gc.C) { wc3.AssertOneChange() } -func (s *MigrationSuite) TestModelUserAccess(c *gc.C) { - model, err := s.State2.Model() - c.Assert(err, jc.ErrorIsNil) - c.Assert(model.MigrationMode(), gc.Equals, state.MigrationModeNone) - - // Get users that had access to the model before the migration - modelUsers, err := model.Users() - c.Assert(err, jc.ErrorIsNil) - c.Assert(len(modelUsers), gc.Not(gc.Equals), 0) - - mig, err := s.State2.CreateMigration(s.stdSpec) - c.Assert(err, jc.ErrorIsNil) - - for _, modelUser := range modelUsers { - c.Logf("check that migration doc lists user %q having permission %q", modelUser.UserTag, modelUser.Access) - perm := mig.ModelUserAccess(modelUser.UserTag) - c.Assert(perm, gc.Equals, modelUser.Access) - } - - // Querying for any other user should yield permission.NoAccess - perm := mig.ModelUserAccess(names.NewUserTag("bogus")) - c.Assert(perm, gc.Equals, permission.NoAccess) -} - func (s *MigrationSuite) createStatusWatcher(c *gc.C, st *state.State) ( state.NotifyWatcher, statetesting.NotifyWatcherC, ) {