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 5ce87115009..4c7eb9fa531 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. @@ -1040,17 +1043,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, ) {