Fixed model listing for admin user. #6865
Conversation
Admin user needs to see all models when listing, list models is not the nicest method and does a direct query to mongo which returns a partial set of models owned by the calling user.
state/modeluser.go
Outdated
| @@ -193,6 +193,19 @@ func (e *UserModel) LastConnection() (time.Time, error) { | |||
| return lastConnDoc.LastConnection, nil | |||
| } | |||
|
|
|||
| // AllUserModels returns a list of all models with the user they belong to. | |||
| func (st *State) AllUserModels() ([]*UserModel, error) { | |||
There was a problem hiding this comment.
Should we just be using this function instead of having both "AllModels" and "AllUserModels"?
It looks like the intent might be to suppress the "controller" model, is that true? or is it just annotated with the owner as well?
| } | ||
| var result []*UserModel | ||
| for _, model := range models { | ||
| result = append(result, &UserModel{Model: model, User: model.Owner()}) |
There was a problem hiding this comment.
If you can just call Model.Owner(), why do we need a separate UserModel type? Isn't it just exactly what you have here, which is model.Owner()?
There was a problem hiding this comment.
The idea is to abstract the caller from building these UserModel structs, it was quite noisy to me that we can only ask these for one particular user.
But if I dont manage to convince you that is a good sign that I might be wrong.
| @@ -460,7 +460,12 @@ func (m *ModelManagerAPI) ListModels(user params.Entity) (params.UserModelList, | |||
| return result, errors.Trace(err) | |||
| } | |||
|
|
|||
| models, err := m.state.ModelsForUser(userTag) | |||
| var models []*state.UserModel | |||
| if m.isAdmin { | |||
There was a problem hiding this comment.
I tend to agree with jam that this doesn't seem quite right. It seems that this PR is changing the semantics of ListModels, at least partially, to "show me the models that this user has access to" instead of "show me the models owned by this user". Both are viable semantics, but we should choose one and stick to it. This semantics of ListModels in this PR are half way between - if I'm admin, it shows me all models (all the models I have access to) but if I'm not, it only shows me all models owned by me, not all the ones that I have access to.
I'd suggest changing the semantics such that if a user is specified, the existing semantics are kept - it lists all models owned by the given user. But if the user is left empty, it tries to list all models that the user has access to. So for admin, that would list all models in the system, but for a user, it would list all models that they have permission to access.
What do you think?
|
!!try!! |
perrito666
force-pushed
the
admin_cant_grant_foreign
branch
from
3139fe5
to
9de3aa8
Compare
|
!!retry!! |
|
@jameinel ptal again |
| if err == nil && access.Access == permission.SuperuserAccess { | ||
| return st.allUserModels() | ||
| } | ||
| if err != nil && !errors.IsNotFound(err) { |
There was a problem hiding this comment.
Please add an explicit test that deals with what happens when you get a NotFound error.
| @@ -204,7 +228,7 @@ func (st *State) ModelsForUser(user names.UserTag) ([]*UserModel, error) { | |||
| defer userCloser() | |||
|
|
|||
| var userSlice []userAccessDoc | |||
| err := modelUsers.Find(bson.D{{"user", user.Id()}}).Select(bson.D{{"object-uuid", 1}, {"_id", 1}}).All(&userSlice) | |||
| err = modelUsers.Find(bson.D{{"user", user.Id()}}).Select(bson.D{{"object-uuid", 1}, {"_id", 1}}).All(&userSlice) | |||
There was a problem hiding this comment.
Please add an explicit test that deals with what happens when a non-admin user calls this.
|
|
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Admin user needs to see all models when listing,
list models is not the nicest method and does a direct
query to mongo which returns a partial set of models
owned by the calling user.
QA steps
Bug reference
https://bugs.launchpad.net/juju/+bug/1650643