Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permissions: Editors becomes admin when creating dashboards, folders and teams #15977

Merged
merged 63 commits into from
Mar 19, 2019

Conversation

xlson
Copy link
Contributor

@xlson xlson commented Mar 13, 2019

What this PR does / why we need it:
When it is enabled editors become admin over any Dashboard, Folder or Team they create. The teams list view becomes visible to all users, but, users only see the teams they are members of.

Enable the feature flag to test: editors_can_admin = true

Which issue(s) this PR fixes:
Closes #15598
Closes #15599
Closes #15600
Closes #15602
Closes #11584

Special notes for your reviewer:
We'd appreciate code review as well as manual testing.

Open question: should the feature toggle be enabled or disabled by default?

@xlson xlson added pr/needs-manual-testing Before merge some help with manual testing & verification is requested area/dashboard/folders area/permissions add to changelog labels Mar 13, 2019
Copy link
Contributor

@bergquist bergquist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments. No blockers for merging.

pkg/api/team_members.go Outdated Show resolved Hide resolved
pkg/middleware/auth.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Show resolved Hide resolved
pkg/services/dashboards/acl_service.go Outdated Show resolved Hide resolved
pkg/api/api.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Outdated Show resolved Hide resolved
@xlson xlson requested a review from hugohaggmark March 14, 2019 09:15
@torkelo
Copy link
Member

torkelo commented Mar 14, 2019

Great job! First quick pass through of the code, could not find any major things to improve. The two new global static functions MakeUserAdmin and teamguardian.CanAdmin are two things where I am not sure what the best way forward is.

Grafana has a ton of these global static functions but we have for the last 2 years been trying to move away from them and move things to instances & stop accessing global instances like bus, cfg. But full self-registering services (that inject Cfg & Bus) adds some boilerplate, and adding it as a injection dependency on HttpServer would also then be needed. So for these small functions maybe there is a middle ground that we can use that is not a full blown service but something more light weight that still has no access to global instances?

Also did some manual testing. And as a viewer (org role) & just a normal member of a team, I see the delete button on the team list. I do get an error trying to delete so the backend guardian is doing it's job :)

image

@hugohaggmark
Copy link
Contributor

hugohaggmark commented Mar 14, 2019

Also did some manual testing. And as a viewer (org role) & just a normal member of a team, I see the delete button on the team list. I do get an error trying to delete so the backend guardian is doing it's job :)

Great catch, I'll fix the FrontEnd part after interview!

@xlson
Copy link
Contributor Author

xlson commented Mar 14, 2019

Potential UI problems:

  • A Viewer who has become editor within a Folder does not get a Create Dashboard icon in the sidebar, they have to browse to the Folder (and enter it through the settings icon, which is pretty awkward)
  • We have made it impossible for non-admins to remove the last admin of a team. There is nothing like that for Dashboards and Folders (we originally didn't intend to implement features like this due to how it complicates the logic)

@xlson
Copy link
Contributor Author

xlson commented Mar 14, 2019

@torkelo: @bergquist suggested sending in the bus as a parameter which I think is a good start. Ideally, I'd like to see us remove the bus completely, but as you mention, that is definitely at deeper rabbit hole.

xlson added a commit that referenced this pull request Mar 14, 2019
Made so permissions for the signedInUser flows from backend to frontend (#15977)
Copy link
Contributor

@daniellee daniellee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing but here are the things I have found so far.

Another issue is that viewers can click on the New Team button on the teams page even though the button is disabled.

OrgId int64
TeamId int64
UserId int64
External bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not obvious what this field is for - Leonard explained it to me but should have a comment or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing but here are the things I have found so far.

Another issue is that viewers can click on the New Team button on the teams page even though the button is disabled.

The New Team button should be fixed now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment explaining external

Copy link
Contributor

@daniellee daniellee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a viewer has admin permission to a team, then the settings page crashes when the save button is pressed after making a change.

@hugohaggmark
Copy link
Contributor

When a viewer has admin permission to a team, then the settings page crashes when the save button is pressed after making a change.

Should be fixed now @daniellee, would be interesting to know why we do this. Must be there for a case I couldn't find.

@xlson xlson force-pushed the admin-on-create-poc branch from f27d74b to 94bba58 Compare March 19, 2019 12:56
@xlson xlson force-pushed the admin-on-create-poc branch from 94bba58 to 53322a5 Compare March 19, 2019 13:01
@xlson xlson merged commit f2b06a8 into master Mar 19, 2019
@xlson xlson deleted the admin-on-create-poc branch March 19, 2019 14:02
@daniellee daniellee changed the title Editors becomes admin when creating dashboards, folders & teams Permissions: Editors becomes admin when creating dashboards, folders and teams Mar 26, 2019
@daniellee daniellee added this to the 6.1 milestone Mar 26, 2019
@WhitingPetroleum
Copy link

I like these features but would also like to assign limited editor permissions to users/teams. That way someone can edit certain dashboards (not only ones they create) without needing admin privileges.

@marefr
Copy link
Contributor

marefr commented Apr 29, 2019

@WhitingPetroleum not sure I understand you, but to me you'll assign editor permissions to a folder for a team/user. If I misunderstand please open a feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/dashboard/folders area/permissions pr/needs-manual-testing Before merge some help with manual testing & verification is requested
Projects
None yet
7 participants