Page MenuHomePhabricator
Create Task
Maniphest T257278

Command::restrict( Shell::RESTRICT_NONE ) doesn't actually work
Closed, ResolvedPublic
Actions

Description

Command::restrict() is supposed to add bitfields to $restrictions, not overwrite them. So ->restrict( Shell:RESTRICT_NONE ), which has a value of 0, will do absolutely nothing, leaving the default shell restrictions in place.

My proposal is to make restrict() overwrite the existing restrictions. This would mean that anyone setting a restriction would need to always do something like ->restrict( Shell:RESTRICT_DEFAULT | Shell::NO_FOO ).

Given that codesearch shows every non-test caller already doing this, I believe this is a safe and useful breaking change to make.

Details

ProjectBranchLines +/-Subject
mediawiki/coreREL1_34+18 -3shell: Make ->restrict( RESTRICT_NONE ) actually work
mediawiki/coreREL1_34+14 -0shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken
mediawiki/coreREL1_31+18 -3shell: Make ->restrict( RESTRICT_NONE ) actually work
mediawiki/coreREL1_31+14 -0shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken
mediawiki/coreREL1_35+18 -3shell: Make ->restrict( RESTRICT_NONE ) actually work
mediawiki/coremaster+18 -3shell: Make ->restrict( RESTRICT_NONE ) actually work
mediawiki/coremaster+14 -0shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken
Customize query in gerrit

Event Timeline

Legoktm created this task.Jul 7 2020, 12:08 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 7 2020, 12:08 AM
Legoktm claimed this task.Jul 7 2020, 12:15 AM
gerritbot added a comment.Jul 7 2020, 12:18 AM

Change 609869 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

gerritbot added a project: Patch-For-Review.Jul 7 2020, 12:18 AM

Change 609870 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

gerritbot added a comment.Jul 7 2020, 9:09 AM

Change 609869 merged by jenkins-bot:
[mediawiki/core@master] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.41; 2020-07-14).Jul 7 2020, 10:00 AM
Tgr added a subscriber: Tgr.Jul 8 2020, 2:21 PM

Deprecate and rename to setRestrictions? (Or just add that method and leave restrict as it is.) The current name with the new behavior isn't really intuitive, IMO.

Krinkle added a project: Platform Engineering.Jul 29 2020, 2:40 AM
Krinkle added a subscriber: tstarling.
tstarling added a comment.Jul 29 2020, 3:16 AM

Ideally I think it would be called restrictions(), following the convention that chaining mutators take the name of the thing they are mutating. But the fact that there are already callers expecting this behaviour makes the proposed patch good enough for me.

gerritbot added a comment.Jul 29 2020, 3:41 AM

Change 609870 merged by jenkins-bot:
[mediawiki/core@master] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.3; 2020-08-04).Jul 29 2020, 4:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jul 29 2020, 4:10 AM
Legoktm closed this task as Resolved.Jul 29 2020, 5:23 AM

Sorry, I forgot to reply earlier.

In T257278#6289606, @Tgr wrote:

Deprecate and rename to setRestrictions? (Or just add that method and leave restrict as it is.) The current name with the new behavior isn't really intuitive, IMO.

In T257278#6343400, @tstarling wrote:

Ideally I think it would be called restrictions(), following the convention that chaining mutators take the name of the thing they are mutating. But the fact that there are already callers expecting this behaviour makes the proposed patch good enough for me.

Right, changing the name would've just been extra work in that we'd have to update all the uses just to change the name, not change any behavior. Even though this was technically a breaking change, it only did fixing, not really breaking.

gerritbot added a comment.Jul 29 2020, 5:49 AM

Change 616977 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@REL1_35] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

gerritbot added a project: Patch-For-Review.Jul 29 2020, 5:49 AM
gerritbot added a comment.Jul 29 2020, 5:57 AM

Change 616977 merged by jenkins-bot:
[mediawiki/core@REL1_35] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

ReleaseTaggerBot edited projects, added MW-1.35-notes; removed MW-1.35-notes (1.35.0-wmf.41; 2020-07-14).Jul 29 2020, 6:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jul 29 2020, 6:10 AM
Tgr added a comment.Jul 29 2020, 1:42 PM
In T257278#6343472, @Legoktm wrote:

Right, changing the name would've just been extra work in that we'd have to update all the uses just to change the name, not change any behavior. Even though this was technically a breaking change, it only did fixing, not really breaking.

It did break expectations created by the name, though. It doesn't sound like $command->restrict( Shell::NO_NETWORK ) would make your code less safe, but it actually does. Intuitive naming is especially important for security-sensitive code.

gerritbot added a comment.Aug 29 2020, 6:15 PM

Change 622785 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_34] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

gerritbot added a project: Patch-For-Review.Aug 29 2020, 6:15 PM
gerritbot added a comment.Aug 29 2020, 6:21 PM

Change 623121 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_34] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

gerritbot added a comment.Aug 29 2020, 6:33 PM

Change 623122 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_31] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

gerritbot added a comment.Aug 29 2020, 6:33 PM

Change 623123 had a related patch set uploaded (by Reedy; owner: Legoktm):
[mediawiki/core@REL1_31] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

gerritbot added a comment.Aug 29 2020, 6:41 PM

Change 623122 merged by jenkins-bot:
[mediawiki/core@REL1_31] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

gerritbot added a comment.Aug 29 2020, 6:42 PM

Change 623123 merged by jenkins-bot:
[mediawiki/core@REL1_31] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

gerritbot added a comment.Aug 29 2020, 6:43 PM

Change 622785 merged by jenkins-bot:
[mediawiki/core@REL1_34] shell: Demonstrate that ->restrict( RESTRICT_NONE ) is broken

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

gerritbot added a comment.Aug 29 2020, 6:44 PM

Change 623121 merged by jenkins-bot:
[mediawiki/core@REL1_34] shell: Make ->restrict( RESTRICT_NONE ) actually work

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

ReleaseTaggerBot added projects: MW-1.31-release-notes, MW-1.34-notes.Aug 29 2020, 7:00 PM
Maintenance_bot removed a project: Patch-For-Review.Aug 29 2020, 7:10 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL