Page MenuHomePhabricator
Create Task
Maniphest T102244

D8. Make it possible to figure out root-permissions of things we have yet to insert
Closed, ResolvedPublic
Actions

Description

RevisionActionPermissions::isAllowed() accepts null to figure out permissions for things that are not yet an existing revision; to figure out if we are actually allowed to create a new post/topic/summary/...

However, part of the permissions check is also looking at the root (topic), and now also board level.
If the topic locked/moderated, we shouldn't allow anything new to be added.
Currently, for new items (null), we can't trace back the root post, so we can't check those permissions.

It's possible to create a summary for a locked topic, for example. But once it's been created, you can't edit it anymore (because then we can figure out that thing's root)

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/Flowmaster+88 -66Stop using the general isAllowed() method for null values
Customize query in gerrit

Related Objects

Event Timeline

matthiasmullie created this task.Jun 12 2015, 3:05 PM
matthiasmullie claimed this task.
matthiasmullie raised the priority of this task from to Needs Triage.
matthiasmullie updated the task description. (Show Details)
matthiasmullie added projects: Collaboration-Team-Sprint-C-2015-06-17, StructuredDiscussions.
matthiasmullie added a subscriber: matthiasmullie.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptJun 12 2015, 3:05 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
matthiasmullie moved this task from Sprint C - ends June 15 to In Development on the Collaboration-Team-Sprint-C-2015-06-17 board.Jun 12 2015, 3:09 PM
Catrope added a subscriber: Catrope.Jun 12 2015, 5:44 PM

Is https://gerrit.wikimedia.org/r/#/c/217841/ related to this?

matthiasmullie added a subscriber: SBisson.Jun 13 2015, 6:10 PM

It isn't - that one is related to T98367.
This ticket is somewhat associated with T98929: C1. "Mark as resolved" for Flow topics - while working on the new lock/unlock, @SBisson discovered this permissions-related issue.

DannyH added a project: Collaboration-Team-Sprint-D-2015-06-30.Jun 16 2015, 7:00 PM
DannyH set Security to None.
DannyH moved this task from Sprint D - ends July 1 to In Development on the Collaboration-Team-Sprint-D-2015-06-30 board.
DannyH renamed this task from Make it possible to figure out root-permissions of things we have yet to insert to D8. Make it possible to figure out root-permissions of things we have yet to insert.Jun 16 2015, 7:23 PM
gerritbot added a subscriber: gerritbot.Jun 18 2015, 11:40 AM

Change 218929 had a related patch set uploaded (by Matthias Mullie):
Stop using the general isAllowed() method for null values

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

gerritbot added a project: Patch-For-Review.Jun 18 2015, 11:40 AM
matthiasmullie moved this task from In Development to Needs Review on the Collaboration-Team-Sprint-D-2015-06-30 board.Jun 18 2015, 11:40 AM
Catrope moved this task from Needs Review to Code Review Started on the Collaboration-Team-Sprint-D-2015-06-30 board.Jun 19 2015, 5:40 PM
DannyH added a project: Collaboration-Team-Sprint-E-Everywhere-2015-07-14.Jun 30 2015, 7:41 PM
DannyH moved this task from Sprint E - ends July 14 to Code Review Started on the Collaboration-Team-Sprint-E-Everywhere-2015-07-14 board.
DannyH triaged this task as Medium priority.Jul 1 2015, 11:24 PM
SBisson moved this task from Code Review Started to QA Review on the Collaboration-Team-Sprint-E-Everywhere-2015-07-14 board.Jul 8 2015, 5:35 PM
gerritbot added a comment.Jul 8 2015, 5:37 PM

Change 218929 merged by jenkins-bot:
Stop using the general isAllowed() method for null values

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

SBisson mentioned this in rEFLWdf7b58a4e7e2: Stop using the general isAllowed() method for null values.Jul 8 2015, 5:37 PM
matthiasmullie mentioned this in rMEXTc88f0d0bf770: Updated mediawiki/extensions Project: mediawiki/extensions/Flow….Jul 8 2015, 5:37 PM
Forrestbot added a project: WMF-deploy-2015-07-14_(1.26wmf14).Jul 8 2015, 6:00 PM
Etonkovidova added a subscriber: Etonkovidova.Jul 15 2015, 11:07 PM

If the topic locked/moderated, we shouldn't allow anything new to be added.
It's possible to create a summary for a locked topic, for example. But once it's been created, you can't edit it anymore

So, a summary can be added to a closed topic or not?

SBisson added a comment.Jul 16 2015, 1:26 PM

So, a summary can be added to a closed topic or not?

It used to be forbidden but possible because of this bug. Now that lock is presented as resolve, it is allowed.

Etonkovidova added a comment.Jul 17 2015, 2:07 AM

To summarize - to make sure that everything is as it's supposed to be:

  • not-logged user cannot Mark a topic as resolved
  • any logged user can Mark the topic as resolved
  • 'Edit Summary' can be done by any user resolved topics
  • 'Edit Summary' can be done by a user who is different from the user who Mark the topic as resolved
  • a summary can be added to a topic that is marked as resolved by any user
Etonkovidova moved this task from QA Review to Product Review on the Collaboration-Team-Sprint-E-Everywhere-2015-07-14 board.Jul 17 2015, 2:07 AM
SBisson added a subscriber: DannyH.Jul 17 2015, 12:37 PM

That's my understanding but I would let @DannyH confirm.

DannyH closed this task as Resolved.Jul 22 2015, 4:50 PM

Yes, that's right.

DannyH moved this task from Product Review to Done on the Collaboration-Team-Sprint-E-Everywhere-2015-07-14 board.Jul 22 2015, 4:50 PM
Quiddity moved this task from Untriaged to Resolved on the Collaboration-Team-Triage board.Jul 29 2015, 11:56 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