877948 - Treestatus admin permissions should be finer grained

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[hwine]

Discovered tonight that many team members (and RyanVM) who used to have permission to add new trees no longer do.

Fortunately, philor was around, who still does have permission. :)

Comment 1

[hwine]

Ed, with catlee on pto, you likely have the most insight into this.

Comment 2

[Ed Morley [:emorley]]

Moving to the treestatus component.

Treestatus has two permission bits for users - "sheriff" and "admin". It appears that bug 823618 made adding a tree require admin permissions - though this likely should have been the original behaviour (even more so since that bug also added delete tree functionality, which I'd like to make as hard to accidentally use as possible).

Ryan not having the admin bit set was an oversight, I have just corrected this.

I guess we could have slightly finer grained permissions, ie:
* sheriff (though kind of redundant, since it makes more sense to just delete a user than not give them sheriff access)
* manage users
* manage trees
...so that we don't have to give "manage users" permissions to more people than necessary. Though this would require schema changes and thus maybe not worth the churn given how infrequently we need to add trees when one of myself, philor, ryanvm or catlee are around.

Comment 3

[Justin Wood (:Callek)]

If nothing else, and/or no-one else, we should have hal, joduinn and coop *able* to create trees themselves (even if it grants them the ability to delete trees). Catch me or hal in IRC for why these people should have said access.

The tree creation ability was also useful for me to have since I have used it before (for e.g. SeaMonkey/community needs) but I don't forsee me needing it again in the near term.

Comment 4

[Ed Morley [:emorley]]

I don't have any issue with releng having delete trees functionality, it's that I have no way to give "manage trees" without "manage users" that is the problem.

Comment 5

[Ed Morley [:emorley]]

Given that TBPL requires patches to add new trees, I feel like we can just add them at the same time as updating TBPL & pushing to production?

Comment 6

[Ed Morley [:emorley]]

Sorry hit submit too soon (again).

Note I'll happily take patches for the finer grained permissions idea in comment 2, it's just not something I'd be able to find time to work on myself straight away.

Comment 7

[Justin Wood (:Callek)]

In the mean time, since hal is in charge of the "standup new repos" process in releng can we give at least him the access that is required, whatever that entails (even if its the manage users role)?

Comment 8

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #7)
> In the mean time, since hal is in charge of the "standup new repos" process
> in releng can we give at least him the access that is required, whatever
> that entails (even if its the manage users role)?

(sorry myself for hitting submit too soon) -- the reason this is necessary is because we *create* the repo before we have all the plumbing in place, but we want to have it on treestatus so we can manage a closure message and status even before we can see it on tbpl or have buildbot taking jobs for it, etc.

Comment 9

[Ed Morley [:emorley]]

Make sense - have set the admin bit for Hal :-)

Morphing bug to be about the ideal fix.

Comment 10

[hwine]

Reading the above, part of the "ideal fix" should include notifying folks of changes. I'm not sure anything needs be changed in the tool -- this bug arose from people being surprised.

Now we know, so I'd be fine with a "wont fix" resolution -- not sure it's worth anyone's time -- we "shouldn't" have last minute branch creations.

Comment 11

[Ed Morley [:emorley]]

(In reply to Hal Wine [:hwine] from comment #10)
> Reading the above, part of the "ideal fix" should include notifying folks of
> changes.

Yeah the problem was that it wasn't noticed in bug 823618 that the patch there was even making a change (comment 2).

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

This is now managed via LDAP groups.