Talk:Gerrit/Privilege policy

Jump to navigation Jump to search

About this board

Scope of "Requesting Gerrit privileges" secion

2
Tgr (WMF) (talkcontribs)

It seems common sense that the section would only apply to code in Wikimedia production, just like the +2 policy, but the text doesn't actually say so. Could that be clarified? We don't want to force people to go through Phabricator privilege requests for repos under labs/tools (Toolforge), for example.

MarcoAurelio (talkcontribs)

On Gerrit/Privilege_policy#Requesting_Gerrit_privileges it says that "To request membership in another group, create a new task under the Gerrit-Privilege-Requests project in Phabricator", "If there is a consensus of trusted developers on the Phabricator task, any of the Gerrit administrators can resolve the request" and "Gerrit administrators should not grant repository ownership to ordinary developers". Labs/Cloud is not part of Gerrit/Privilege_policy#Expedited_process_for_trusted_organisations and in light of Topic:Uswroi0fiud9on8o as well, it looks to me that (common sense or lack thereof) all access requests shall go through a Phab task.

Reply to "Scope of "Requesting Gerrit privileges" secion"

mediawiki/extensions.git submodules additions/removals

2
MarcoAurelio (talkcontribs)

Can we continue to self-merge this kind of changes? Given that mediawiki/extensions.git changes mostly involve adding or removing submodules to track newly added or archived extensions, can I consider this, in light of this new policy, that this qualifies as trivial to self-merge? (exception for the python and shell files which do indeed require review and I wouldn't self-merge). It looks (and thus why I've been doing that) that most patches are self-merged there when it's just adding or removing a submodule as it's purely routine. Thanks.

Edit: other cases I need to self-merge is to when I am creating a new Gerrit repo or adjusting its permissions.

Tim Starling (WMF) (talkcontribs)

If you're doing it already and nobody cares, it's probably fine. The section on self merging is merely descriptive of the current situation. It has plenty of wiggle room to allow things that are happening already.

I also want to emphasize that we're not going to suddenly revoke +2 access of a valued contributor for violating some narrowly interpreted clause in the policy. I can't speak for all situations or all committee members, but if someone complains that you shouldn't be doing a particular variety of self merges, the obvious outcome of that is to ask you to stop doing it.

The policy defines an escalation path for complaints which allows them to be handled with common sense and without needless public humiliation.

How to get added to "Trusted Contributors" on Gerrit?

2
Summary by MarcoAurelio

See phab:T238651#6189506. The Trusted Contributors gerrit group is self-managed and is fine to remain viral. Any member can add new members.

AKlapper (WMF) (talkcontribs)
MarcoAurelio (talkcontribs)

Email address to reach all gerrit admins

1
MarcoAurelio (talkcontribs)

Hello,

I have since retired from development so this does not affect me much, if any, nowadays. But in the past, each time I was in need to request something to the gerrit administrators (mostly settings, or when clerking on MediaWiki +2 granting/removal Tasks) there was no easy way to reach the Gerrit administrators as a group; so I had to send some emails here and there until someone replied; which made me feel rather bad as I felt I was always putting the burden on the same couple of people that were responsive.

Given that now -and since some time- Gerrit adminship is managed through an LDAP group, I was wondering if we could create an email alias (EXIM? Wikimedia Google group?) such as "gerritadmin[at]wikimedia[dot]org" and subscribe all Gerrit Administrators there (except the bot I guess) so in case anyone needs an admin there's an easy way to reach 'em all through a single email to a single place.

I think that a gerritadmin[at]wikimedia[dot]org address exist or existed per phab:T54150 and got deleted per phab:T220843. If I remember rightly, I discussed the idea briefly with Hashar or Tyler a while ago in the RelEng channel. I can't remember. The later task also mentions a gerrit-admin[at]... address too.

Thanks.

Reply to "Email address to reach all gerrit admins"

! [remote rejected] HEAD -> refs/changes/79/550379/2 (cannot add patch set to 550379.)

8
Summary by MarcoAurelio

Amending patches from other users requires you to be added to the Trusted-Contributors gerrit group, or be in privileged gerrit groups.

Saper (talkcontribs)
Kizule (talkcontribs)

Hi @Saper,

changes (in this case patches) which are not your, can't be amended by you or me. Only by original uploader or user which have +2 in related repository.


Best regards,

Zoran.

MarcoAurelio (talkcontribs)

@Saper Please try again. I made something.

Saper (talkcontribs)
MarcoAurelio (talkcontribs)

@Saper What Zoran mentioned above is true. Due to abusive use in the past of the ability to add patch sets to the work made by others, this ability is now restricted to patch owners, repository owners and members of some gerrit groups. This includes a group called Trusted-Contributors. I added you yesterday to test if my lecture of the gerrit permissions was right. I have removed you for now from said group because with this new Gerrit Privilege Policy in place it looks each and every Gerrit permission must be discussed and approved in advance. I have filed phab:T238649 requesting that you be formally added. Sorry for the bureaucracy, but it looks those are the rules now. I hope it won't take long to have you formally approved. Best regards.

Saper (talkcontribs)

Thanks... the policy says nothing about "trusted users" so I was a bit baffled if this is a glitch or intentional thing. Thank you again - will follow on Phab!

MarcoAurelio (talkcontribs)

The policy is not quite clear for me in what a "privilege" is or how that should be interpreted. I opted to follow the safests of the paths. If the discussion results in that there's no need for Tasks to add people to that group, I'll be more than happy to follow that consensus. Regards.

Saper (talkcontribs)

@MarcoAurelio I appreciate your approach and the efforts. It worries me that this has become so burdensome. So much for "attracting new developers to the project" etc. etc. Although I am a pretty old one :)

Clarify reference to "Gerrit Administrators"

14
DKinzler (WMF) (talkcontribs)

The policy currently refers to "Gerrit Administrators" as being responsible or empowered to perform certain actions by this policy. It would however be more useful to associate these powers and responsibilities with gerrit permissions rather than a specific group, allowing the granularity of groups and permissions on gerrit to be improved as discussed on phab:T219012#5057232.

I propose to replace all references to "Gerrit Administrators" with "people with the necessary permissions on gerrit", (or an equivalent but less clunky phrase).

Hashar (talkcontribs)

On one side there is the administration of the Gerrit software itself. That is the Administrators built-in group in Gerrit which is https://gerrit.wikimedia.org/r/#/admin/groups/1,members This is the equivalent of having read/write access everywhere (disk or database).

On the other hand, the Gerrit Managers group https://gerrit.wikimedia.org/r/#/admin/groups/119,members is for administration of policy / access lists / repositories etc. It is less privileged than the administrators of the software itself, but is still a very large set of permissions. The group was originally named Project and Group Managers but got renamed to Gerrit Managers in Feb 2018 by https://gerrit.wikimedia.org/r/plugins/gitiles/All-Projects/+/10107cf5056eb6ef3903f49d59cd27387831c5b5%5E%21/#F0 It is semantically broader and might be confusing :D

Jdforrester (WMF) (talkcontribs)

I think changing it to "Gerrit Managers" would be the most reasonable change.

DKinzler (WMF) (talkcontribs)

> I think changing it to "Gerrit Managers" would be the most reasonable change.

It sounds reasonable, but in T219012 there is talk of creating a mediawiki-administrators. And further groups may be created in the future, for other repos, or with different rights.

This is why I want to avoid any group names in the policy. The policy should apply to whoever has the respective rights.


Jdforrester (WMF) (talkcontribs)

Then call it "the 'Gerrit Managers' group of individuals, or whatever it's called this week". Let them bike-shed in peace.

Nikerabbit (talkcontribs)
Jdforrester (WMF) (talkcontribs)
Tim Starling (talkcontribs)

Gerrit Managers can change group membership only for groups they own, which is only 201 groups out of 1630. I volunteered to update the policy to say that Gerrit Managers can change groups, but I'm not going to do that when it's not true. 201 is suspiciously close to a round number, is it possible that someone tried to edit all the groups to be owned by Gerrit Managers but it failed due to a batch size limit? If I do the following query on the Gerrit database:

select group_id,name,owner_group_uuid='93b1e277b72d0e0a883afbc0a87948dd6dd0d7b7' as managed from account_groups where name like 'extension-%';

I get 1022 such extension-specific groups, of which 118 are owned by Gerrit Manager. The groups owned by Gerrit Manager are not contiguous when sorted by ID, UUID or name.

DKinzler (WMF) (talkcontribs)

Uh, that does sound odd...

I think the policy should just say "whoever has the permission to do it".

Tim Starling (talkcontribs)

How about we make all groups be owned by Gerrit Manager? Then we will actually be able to explain who has the ability to do this.

DKinzler (WMF) (talkcontribs)

I thought the idea was that no single group should have all the power, for security reasons. Wasn't that why the Administrators group was changed, after we had vandalism on Gerrit?

Jdforrester (WMF) (talkcontribs)

No. The built-in Administrators group has powers that shouldn't generally be wielded, and which we can't remove or restrict (but would almost never need to use). The idea was to reduce the number of accounts with world-ending powers; it wasn't related to the kinds of actions this policy covers.

DKinzler (WMF) (talkcontribs)

So Gerrit Manager should indeed own all groups?

MarcoAurelio (talkcontribs)

However Gerrit Managers don't have the ability to add people to the MediaWiki group. If you go with Gerrit Managers, we should also empower them to add/remove people from there or leave things as they are and let the LDAP group gerritadmin people to add people to the mediawiki group as it happens now AIUI.

Reply to "Clarify reference to "Gerrit Administrators""

Pair Programming Exception

4
Summary by DKinzler (WMF)

The fact that there were "four eyes" on the code should be documented on gerrit. That's the essential part.

TCipriani (WMF) (talkcontribs)

Under the bulleted exceptions to Merging without review, there is a point that "A commit in Gerrit may have two authors" mentioning "an owner and a reviewer who uploads and amendment"; however, there is no exception for pair programming explicitly. I am not sure how common this is for MediaWiki core and Extensions (cf: scope?), but it is common for several repos I work in.

That is, a patch could be written during a hangout/pairing session between two people, uploaded by one, and merged by the same. As a recommendation, it could contain a Co-Authored-By: snippet in the commit or two Signed-off-by: tags.

Jdforrester (WMF) (talkcontribs)

Isn't there a risk that pair-programmed code suffers from the same individual "group" think that we worry about for sole-author patches? Do we want to encourage the practice of pair-programmed code being merged by one of said pair?

TCipriani (WMF) (talkcontribs)

I think code review and pair programming both ensure that two people understand and endorse a particular patch. I view pair programming as a real-time review of code being written, whereas code review is asynchronous.

I think code review and pair programming are both equally subject to groupthink and/or power inequalities. I am unsure if one is more so than the other.

DKinzler (WMF) (talkcontribs)

I agree that pair programming is kind of a live review process. There is no good way to represent shared authorship on gerrit, so one person should upload the patch, and the other can approve it right away. MAybe it would be nice to mention the fact that the patch was done in a pair programming session, but I don't think it's necessary.

In any case, the fact that there were "four eyes" on the code should be documented on gerrit. That's the essential part.

Summary by DKinzler (WMF)

The scope of the policy extends over all repositories on WMF's gerrit instance. However, there are several bits that only apply to MediaWiki, or only to code deployed on the Wikimedia cluster.

Smalyshev (WMF) (talkcontribs)

The document talks mostly about Mediawiki deployment and Mediawiki extensions, but there are more projects in Gerrit than this. I think it should be clarified whether these policies are to apply to them too and then they need to be written in a bit more generic way or just to "big Mediawiki" (i.e. Mediawiki plus extensions plus puppet, etc.) but not other code hosted on WMF Gerrit.

Tim Starling (WMF) (talkcontribs)

It's intended to apply to all projects that are in WMF Gerrit. There are some bits that are fairly specific to MediaWiki, do you think they should be removed?

TCipriani (WMF) (talkcontribs)

FWIW, I initially had the same reaction, many of the arguments surrounding the importance of +2 make direct reference to the particular configuration of MediaWiki and extensions Gerrit and Jenkins setups.

Putting that aside, the section on merging without code review feels particularly prescriptive; however, it makes the point towards the end:

For extensions (and other projects) not deployed to the Wikimedia cluster, the code review policy is up to the maintainer or author of the extension.

I think I would prefer if that were easier to glean from an inspectional reading of the policy rather than a close reading.

Also, it's important to note that self-merge is common in some repositories (integration/config for instance), and there may be differences between projects in what exceptions there are to merging without code review; however, in these projects, +2 is still "a strong expression of trust", only the expectations are different.

Trusted organizations and the mediawiki group

5
Summary by DKinzler (WMF)

In effect, this means that WMDE staff can added to the mediawiki group per default at onboarding, but can also be removed from this group again without removing them from the WMDE group.

DKinzler (WMF) (talkcontribs)

The event that triggered the drafting of this policy was the request from Wikimedia Deutschland to have new hires be added to the mediawiki group immediately. According to this policy, that would not be the case: WMDE hires would still have to go through the request process to get +2 on core. Is this intentional? I seem to recall that there was agreement that it would be ok to have staff of trusted orgs get +2 per default. The alternative would be to include the wmde group in the mediawiki group, but I would advise against this - WMDE may want to grant access to their own stuff to a volunteer, without granting access to *everything*. Also, WMF may withhold access to core from specific people who work at WMDE, while still allowing them to be a member of the WMDE group, and have +2 on repos managed by WMDE.

Mobrovac-WMF (talkcontribs)

That's not how I read it. New WM Deutschland employees would be automatically added to the wmde group, which by my interpretation of this document, is assumed to automatically have +2 on the whole mediawiki/ tree.

Nikerabbit (talkcontribs)
Leszek Manicki (WMDE) (talkcontribs)

@Nikerabbit is right wmde LDAP group is not yet included under mediawiki project.

My understanding was, that based on the new policy, as a next wmde group will be added there. This is a technicality, which I am not sure has not be made more explicit in the policy.

Regarding one point from @DKinzler (WMF): wmde LDAP group should only be for WMDE staff IMO. Same as wmf LDAP group is in my understanding for WMF staff only (with "staff" I also mean active contractors etc). for possible volunteer access, some other groups would be used etc. But this again seems to me be a detail, which is not really in the scope of the policy.

DKinzler (WMF) (talkcontribs)

@Leszek Manicki (WMDE) originally, the idea was to make the WMDE group an "included" group of the MediaWiki group. However, this would make it impossible to have people in the WMDE group but not in the MediaWiki group. There have been cases in the past of WMDE staff being denied +2 on the mediawiki group, and that possibility should still exist.

Instead, the idea is to have a list of "associated" groups for trusted organizations, for which these organizations can request members to be added without the need for prior public discussion.

In effect, this means that WMDE staff can added to the mediawiki group per default at onboarding, but can also be removed from this group again without removing them from the WMDE group.

Automatic revocation for staff

4
Summary by DKinzler (WMF)

Automatic revocation at the end of employment protects the individual's privacy. No fault is implied.

Tgr (WMF) (talkcontribs)

It is WMF policy to revoke all privileges when staff members depart, even if those privileges were granted prior to the beginning of employment by virtue of volunteer work. Consistent application of this policy helps to protect the privacy of departing staff members: no fault is implied.

Automatically revoking privileges that were given as part of someone's job makes sense. Revoking privileges received as a volunteer seems pretty hostile and not a great way to encourage further volunteer contributions. I don't get the privacy reference at all. Is that about not disclosing the fact that someone might have been fired in a way that makes us not trust them with +2 any longer (and there might be legal restrictions on disclosing even that fact)?

Anomie (talkcontribs)
DKinzler (WMF) (talkcontribs)

> I don't get the privacy reference at all. Is that about not disclosing the fact that someone might have been fired in a way that makes us not trust them with +2 any longer (and there might be legal restrictions on disclosing even that fact)?

Yes, this is indeed the reason. I don't see a good way around this, especially since there is a lot of grey between "fired for bad faith behavior" and "quit because they found a nicer place".

Tgr (WMF) (talkcontribs)

Ask people to indicate whether they need the permissions in the future, let them keep it if they do, and suggest people leaving under a cloud that they should let them lapse and spare themselves the embarrassment of being specifically denied. Unless you are considering the situation where the person leaving is not trusted but unaware of it, but in that case you will probably have to deal with it anyway when they re-request them.