Yeah, I do think it is weird to mix the two user rights like this.

This had the effect of making it impossible for global sysops to delete anything on login.wikimedia.org, since they cannot edit those pages but loginwiki is in the GS set.

Not that it particularly matters that much, but it's clear this change was a kneejerk reaction without a lot of thought as to the effects.

Change 153362 had a related patch set uploaded by Parent5446:
Revert "Do not allow a user to delete a page they can't edit"

https://gerrit.wikimedia.org/r/153362

It greatly concerns me that this was merged so hastily without any sort of review or discussion. There is a very obvious problem with making user permissions dependent upon each other and thus violating the atomicity of each permission.

This causes a number of problems, specifically:

1. It makes it more difficult for administrators to fully understand what permissions they are assigning.

2. It makes certain workflows (as mentioned in gerrit) impossible entirely.

3. It opens the door to future dependencies, thus creating a hierarchy of user permissions, which would be completely unnecessary considering MediaWiki uses RBAC, which allows for group hierarchies configurable by the wiki administrator.

This should be reverted immediately until consensus is achieved on this issue, or at the very least until somebody can provide an actual reasoning for why this change was made.

(In reply to Tyler Romeo from comment #4)

It greatly concerns me that this was merged so hastily without any sort of
review or discussion.

Yes, https://gerrit.wikimedia.org/r/153345 was a very bad show. I agree with the proposal to revert it; https://gerrit.wikimedia.org/r/153362 should be merged.

This should be reverted immediately until consensus is achieved on this
issue, or at the very least until somebody can provide an actual reasoning
for why this change was made.

A few developers and system administrators are being forced to implement these changes by Erik over a dispute ostensibly about MediaViewer on the German Wikipedia. This is Wikimedia affecting MediaWiki. The Gerrit comments and associated discussion on the mailing lists and elsewhere make this clear(er).

(In reply to Kunal Mehta (Legoktm) from comment #0)

Background: "superprotect" launched on de.wp which prevented admins from
editing the page, but they were still able to delete it and undelete it,
which then removed the protection status.

While that's how the bug was discovered, when I heard "deleting and undeleting a page bypasses protection" I immediately thought "That's a security bug, and I probably know how to fix it." And also "Ugh, people posted a security bug on a public mailing list?"

I've have written the same patch if the issue came in without attached drama, although I'd have properly attached it to the security bug in that case; here there didn't seem a point since it was already public.

But because it did come with attached drama, people are over-analyzing everything and claiming hypothetical workflows are somehow super-important

Political issues aside, should we require users to be able to edit the page
before they can take an action upon it?
I don't believe that we should, and that userrights and the different
actions should be independent of each other. It's called "edit" protection,
and should only stop against edits.

If we don't use the 'edit' protection for these other actions, then we'd need to add individual protections for every action. That'll be a big and potentially-confusing protect form, and we'll likely need to retroactively update existing protections, but it could work. I imagine JS-using admins will continue to mostly leave the "Unlock further protect options" checkbox checked.

(In reply to Brad Jorsch from comment #6)

(In reply to Kunal Mehta (Legoktm) from comment #0)

Background: "superprotect" launched on de.wp which prevented admins from
editing the page, but they were still able to delete it and undelete it,
which then removed the protection status.

While that's how the bug was discovered, when I heard "deleting and
undeleting a page bypasses protection" I immediately thought "That's a
security bug, and I probably know how to fix it." And also "Ugh, people
posted a security bug on a public mailing list?"

I think your use of the term "security bug" is pretty dubious. The fact that pages lose protection status between deletion and restoration is not new (cf. bug 12343).

If we don't use the 'edit' protection for these other actions, then we'd
need to add individual protections for every action.

Need to? What specific use-case or problem are you addressing?

Thanks for filing; certainly consistency is needed here.

I agree with comment 7, there was no security bug and bug 12343 can be fixed in way better ways.

(In reply to MZMcBride from comment #7)

I think your use of the term "security bug" is pretty dubious.

A method for bypassing access restriction is solidly in the category of "security bug".

Need to? What specific use-case or problem are you addressing?

The fact that it was possible to bypass access restrictions. The currently-merged patch solves this by requiring 'edit' in order to delete. It would work as well to add an additional access restriction for delete directly.

(In reply to Nemo from comment #8)

bug 12343 can be fixed in way better ways.

While fixing bug 12343 might well be a good thing, it wouldn't solve the security hole here: the attacker could delete the page and then undelete all except the revision they're trying to revert.

Change 153362 abandoned by Siebrand:
Revert "Do not allow a user to delete a page they can't edit"

https://gerrit.wikimedia.org/r/153362

An attacker with delete rights? Let's stop kidding please.

Change 153362 restored by Parent5446:
Revert "Do not allow a user to delete a page they can't edit"

Reason:
I'd appreciate it if people did not needlessly abandon my changes just because they do not agree with them. It is very disrespectful, and I can assure you my reasons for reverting this change have absolutely nothing to do with whatever superprotect / german wiki / mediaviewer nonsense is going on, primarily because I don't even know what is going on.

https://gerrit.wikimedia.org/r/153362

Perhaps the problem could be made easier by first identifying points of agreement. For instance, is there a consensus that a user should have the permission to *read* a page in order to make an action on it? If yes, is this already enforced by core and how?

https://gerrit.wikimedia.org/r/153362 says:

In this specific case, there is a use case scenario for a wiki where only one
group of users can create and edit pages, and another group of users can only
delete pages, but not edit or create. This setup is useful in cases where only
certain users are allowed to author content, but other users are given the
ability to perform maintenance tasks.

Do we also have specific existing use cases/workflows to consider? Investigators could start by looking into how the "eliminator" group is used on ckb.wiki, ja.wiki and pt.wiki, as well as ru.wiki's "closer" group and frwiktionary's "move-rootuserpages": none of these groups appears to have editinterface, editprotected or similar. Please add cc's and notify those wikis as appropriate.

(In reply to Nemo from comment #13)

is there a consensus that a user should have the
permission to *read* a page in order to make an action on it? If yes, is
this already enforced by core and how?

In core, you currently can't edit pages unless you can read them. However, this permission might be a bad example, since consensus around the issue of reading generally seems to be that if you can't read the entire wiki, you shouldn't be able to edit at all.

Requiring being able to edit a page to delete it doesn't make a whole lot of sense in core: default MediaWiki comes with no situations in which anyone would be able to delete anything they can't edit, and few MediaWiki installs will have much reason to change the default structure in such a way.

Of course in some cases it is possible that system administrators may add configuration where this may occur, and the use cases there could be something to look into, but really if this comes up, shouldn't they be using extensions in order to properly fine-tune the permissions?

I think what this really concerns is extensions. Extensions are what will change the behaviour (unless we actually get some kind of consensus otherwise), and also what will be affected by such changes. Any extension that adds an interface where a user will not be editing a page directly could run into problems depending on how it is implemented (is there any standard?), because while editing can take a lot of different forms, some less direct than others (from using a special form to using a bot), deleting does not. Consider the example of talk pages - extensions such as LQT provide a new frontend for these, where users do not edit the base talk pages directly. But as I recall, it's the same old interface as ever to actually delete a LQT board, because it's still just a page underneath.

Being able to edit by deleting then creating is a minor elevation of privilege, although I doubt anyone would assign it a cve. Mysql lets you do it.

On the idea that all privileges need to be independent, I don't think that should always be the case-- mysql, and hopefully mediawiki, doesn't allow deletion without the ability to read. That could lead to several problems. But I definitely think it's an ideal, to keep things as simple as possible.

For mediawiki, allowing delete (or create) actions without edit rights seems unexpected to me, but I'm honestly happy to let the community come to a consensus on it. I think it would be worth (somewhere) spelling out the dependencies among the rights.

And I would be ok with reverting change 153345, if the problem it was trying to address (a user can delete a page that they have been protected against editing) is fixed another way.

I would think that, politics aside, the code should require superprotect to delete a superprotected page, and do nothing more...

Putting such a significant change as was made yesterday may push us into certain code paths that people assumed would not be taken, and I would think that a good deal of examination should be done before tearing down this assumption.

(In reply to Tyler Romeo from comment #4)

2. It makes certain workflows (as mentioned in gerrit) impossible entirely.

Sorry, I read the Gerrit comments but didn't see any description of any actual workflow. All I could find was a vague suggestion of "what if someone wants to delete but can't edit", but without any explanation of why such a structure would be useful. If I am missing something, maybe you can point it out?

(In reply to rschen7754.wiki from comment #2)

This had the effect of making it impossible for global sysops to delete
anything on login.wikimedia.org, since they cannot edit those pages but
loginwiki is in the GS set.

I think that is an improvement. Global sysops should not be able to delete pages on login.wikimedia.org if they can't even edit them.

(In reply to Nemo from comment #13)

Do we also have specific existing use cases/workflows to consider?
Investigators could start by looking into how the "eliminator" group is used
on ckb.wiki, ja.wiki and pt.wiki, as well as ru.wiki's "closer" group and
frwiktionary's "move-rootuserpages": none of these groups appears to have
editinterface, editprotected or similar. Please add cc's and notify those
wikis as appropriate.

Thanks Nemo, that is helpful. The "eliminator" and "closer" groups are relatively unprivileged groups, able to delete pages but not able to do potentially dangerous site-wide things like edit the MediaWiki namespace. I think it makes perfect sense to prevent such users from deleting superprotected pages -- or indeed, any MediaWiki namespace page.

It seems to me that there are three options:

1. Keep the patch, require edit permissions in order to move. There are plenty of precedents for linking of rights in this way in core, so it's not as inelegant as Tyler makes out.

2. Make the delete action be a separate restriction type, so that pages can be delete-protected in the same way as they can be move-protected

There are already four actions treated in this way: edit, create, move and upload, so on the face of it, adding a fifth doesn't seem too terrible. The number of boxes on the protection form is currently about 2 or 3, if you include Pending Changes which also adds a box here, and that number would increase by 1. The interface is not too terrible with the "unlock further protection options" box unchecked -- the delete protection level would implicitly be the same as the edit protection level.

3. Do not show a delete protection UI, but implicitly treat edit protection as equivalent to delete protection. Edit protection would be conflated, rather than edit rights. If you have to conflate two things, then I suppose it makes sense to choose narrower things to conflate, and protection is narrower than rights. A careful implementation of this option would allow future migration to option 2, if that is necessary.

Broadly: option 1 has a convenient UI and has beneficial side-effects, such as preventing "eliminators" from deleting MediaWiki namespace pages when they don't have editinterface rights. Its disadvantage is its inflexibility and lack of a migration path towards more fine-grained controls. Option 2 has an ugly UI but an elegant backend. Option 3 has a convenient UI and a good migration path, but the side-effects of option 1 may have to be implemented in some other way.

I don't think merely reverting the patch is a suitable long-term option -- we don't want to incentivize deletion of super-protected pages, since history is lost, and pages with missing JS may be cached into Varnish before the page is recreated.

Change 153619 had a related patch set uploaded by Jackmcbarn:
Test only against protection for deleting

https://gerrit.wikimedia.org/r/153619

(In reply to Tim Starling from comment #18)

Option 3 has a convenient UI and a
good migration path, but the side-effects of option 1 may have to be
implemented in some other way.

In a quick glance through the code, it looks like the only side effects would be for hooks on 'TitleQuickPermissions', 'userCan', 'getUserPermissionsErrors', or 'getUserPermissionsErrorsExpensive' that test for action=edit rather than just applying for all actions.

Title::checkSpecialsAndNSPermissions(), Title::checkCSSandJSPermissions(), and Title::checkCascadingSourcesRestrictions() appear to already apply their checks to all actions, and Title::checkUserBlock() currently applies a more-restrictive check for action=delete than for action=edit.

Change 153362 abandoned by Parent5446:
Revert "Do not allow a user to delete a page they can't edit"

Reason:
In favor of Change-Id: I5a7c4147bd241dc086fda6c16827f9554d78599b

https://gerrit.wikimedia.org/r/153362

(In reply to Tim Starling from comment #18)

(In reply to Tyler Romeo from comment #4)

2. It makes certain workflows (as mentioned in gerrit) impossible entirely.

Sorry, I read the Gerrit comments but didn't see any description of any
actual workflow. All I could find was a vague suggestion of "what if someone
wants to delete but can't edit", but without any explanation of why such a
structure would be useful. If I am missing something, maybe you can point it
out?

Why are you asking for a workflow when you acknowledged one later in your same comment?

(In reply to Tim Starling from comment #18)

(In reply to Nemo from comment #13)

1. Keep the patch, require edit permissions in order to move. There are

plenty of precedents for linking of rights in this way in core, so it's not
as inelegant as Tyler makes out.

Precedent does not make it an elegant solution. Just because bad examples exist does not mean we should be adding more of them.

One of the purposes of RBAC is the maximal customization. Since the permissions are atomic, the administrator can define any user group he/she wants. Imposing our own permission dependencies where not technically necessary imposes a restriction that would not otherwise exist.

I do not see the advantage of restricting administrators' customization abilities and making the permissions system more complicated for absolutely no reason at all, even if a practical workflow has not been demonstrated.

2. Make the delete action be a separate restriction type, so that pages can

be delete-protected in the same way as they can be move-protected

There are already four actions treated in this way: edit, create, move and
upload, so on the face of it, adding a fifth doesn't seem too terrible. The
number of boxes on the protection form is currently about 2 or 3, if you
include Pending Changes which also adds a box here, and that number would
increase by 1. The interface is not too terrible with the "unlock further
protection options" box unchecked -- the delete protection level would
implicitly be the same as the edit protection level.

I think that, moving forward, this would be the best option. However, it is a lot more complicated since we have to change the UI, the protection backend, etc.

3. Do not show a delete protection UI, but implicitly treat edit protection

as equivalent to delete protection. Edit protection would be conflated,
rather than edit rights. If you have to conflate two things, then I suppose
it makes sense to choose narrower things to conflate, and protection is
narrower than rights. A careful implementation of this option would allow
future migration to option 2, if that is necessary.

^this exactly. I like this option, especially with the option of a future migration to option 2 if we decide.

(In reply to rschen7754.wiki from comment #2)

This had the effect of making it impossible for global sysops to delete
anything on login.wikimedia.org, since they cannot edit those pages but
loginwiki is in the GS set.

As far as I can tell, no one can create pages on loginwiki, so why would pages ever have to be deleted?

(In reply to Isarra from comment #15)

Requiring being able to edit a page to delete it doesn't make a whole lot of
sense in core: default MediaWiki comes with no situations in which anyone
would be able to delete anything they can't edit, and few MediaWiki installs
will have much reason to change the default structure in such a way.

On the other hand, English Wikipedia's settings come with two interesting situations:

• If you are an oversighter but not an administrator, then you can delete a protected page, but you can't edit protected pages.
• If you are an importer but not an administrator, then you are maybe able to overwrite a protected page by uploading an XML file, but you can't edit the page normally. Has it been tested if this work?

In both situations, you are in effect "editing" the page. If I do not have the right to edit a page, then I do not expect to be able to change what the page looks like at all. I should not be able to turn it into a "404 Not Found", and I should not be able to modify it by creating an XML file in my text editor. If Mediawiki allows such indirect editing, then Mediawiki seems to have a security bug.

Of course, there is no situation where English Wikipedia would grant oversight rights to a non-admin, and the only importers are also admins.

(In reply to Stefan2 from comment #23)

Of course, there is no situation where English Wikipedia would grant
oversight rights to a non-admin, and the only importers are also admins.

Not quite right - if a non-admin ever won ArbCom elections, they could be granted CU/OS, and we had an oversighter temporarily resign as an admin last year.

There are also non-admin oversighters on other wikis, but the permissions are set up slightly differently, if memory serves me right.

(In reply to Brad Jorsch from comment #20)

In a quick glance through the code, it looks like the only side effects
would be for hooks on 'TitleQuickPermissions', 'userCan',
'getUserPermissionsErrors', or 'getUserPermissionsErrorsExpensive' that test
for action=edit rather than just applying for all actions.

So looking at WMF-deployed extensions, I see the following extensions that are explicitly checking for action=edit:

• Translate extension has three hooks. One is explicitly trying to allow delete but not edit, and the other two are likely the same boat.
• CentralAuth has a $wgDisableUnmergedEditing variable; should unmerged deleting be allowed when editing isn't?
• TitleBlacklist, if the user can't edit should they be allowed to delete?
• WikimediaIncubator, denies edits in many code paths but not deletion.

Change 155070 had a related patch set uploaded by Anomie:
$wgDisableUnmergedEditing should also prevent deletion

https://gerrit.wikimedia.org/r/155070

Change 155070 merged by jenkins-bot:
$wgDisableUnmergedEditing should also prevent deletion

https://gerrit.wikimedia.org/r/155070

Change 153619 merged by jenkins-bot:
Test only against protection for deleting

https://gerrit.wikimedia.org/r/153619

What is the status of this bug?

Probably RESOLVED FIXED since comment 28.

Let's just be bold and close it that way, if people disagree they can reopen.