Page MenuHomePhabricator

Consistently check permission for entity creation
Open, HighPublic

Description

Permission for entity creation is currently not checked consistently. We do:

  • require 'property-create' for SpecialNewProperty, by specifying it as required permission in the parent constructor.
  • EditEntity uses EntityPermissionChecker to check the "edit" permission.
    • EditEntity::addRequiredPermission() is never called
  • EntityContentFactory also requires 'createpage' if the Entity's ID is null (!)
  • Api/EditPage::getRequiredPermissions() returns 'edit'; If the entity ID is null (new entity), it also returns 'createpage' and 'property-create'

This shows that permission checks are distributed all over the code, and partially inconsistent.

Also, some API modules that allow the creation of entities may not check all necessary permission.

Necessary permissions should be determined and checked in a central place. EditEntity seems to be the right place for this, since all entity edits go through there, and it has enough information about the user.

NOTE: this does not currently pose a security risk on wikidata.org, since a) the 'edit' and 'createpage' permissions are always checked and b) only Properties require an extra permission and c) property creation is only possible via wbeditentity, which does check 'property-create'.

Patch-For-Review:

Event Timeline

daniel created this task.May 30 2017, 4:56 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 30 2017, 4:56 PM
daniel triaged this task as High priority.May 30 2017, 4:57 PM
daniel added a project: User-Daniel.
WMDE-leszek moved this task from Backlog to Doing on the Wikidata-Former-Sprint-Board board.

Change 361693 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] [WIP] Add WikiPageEntityStorePermissionChecker for consistent permission checks

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

Change 361889 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Use WikiPageEntityStorePermissionChecker as WikibaseRepo permission checker

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

Change 362239 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Remove entity creation permission logic checks from Api\EditEntity

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

Change 363303 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Remove user permission logic from RedirectCreationInteractor

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

Change 363304 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Remove user permission logic from ItemMergeInteractor

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

Change 363305 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Use EntityPermissionChecker for checking term modify permissions on relevant Special pages

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

Change 361693 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add WikiPageEntityStorePermissionChecker for consistent permission checks

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

Change 361889 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use WikiPageEntityStorePermissionChecker as WikibaseRepo permission checker

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

Change 362239 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove permission check logic from some Api classes

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

Change 365582 had a related patch set uploaded (by WMDE-leszek; owner: WMDE-leszek):
[mediawiki/extensions/Wikibase@master] Consider create permissions when entity is creating with API action for editing entity terms

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

Change 363303 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove user permission logic from RedirectCreationInteractor

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

Change 363304 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove user permission logic from ItemMergeInteractor

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

Change 365582 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Consider create permissions when entity is created using the API action for editing entity terms

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

Change 363305 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use EntityPermissionChecker for checking term modify permissions on relevant Special pages

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

thiemowmde updated the task description. (Show Details)
daniel closed this task as Resolved.Jul 24 2017, 2:25 PM

Seems like all the patches are merged now

Restricted Application added a subscriber: PokestarFan. · View Herald TranscriptJul 24 2017, 2:25 PM
Aleksey_WMDE reopened this task as Open.EditedJul 24 2017, 3:31 PM

Not all patches are merged in subtask

WMDE-leszek added a subscriber: Aleksey_WMDE.

What's still left here now? I can't find any remaining patches in the open subtasks.

Aklapper removed WMDE-leszek as the assignee of this task.Jun 19 2020, 4:17 PM

This task has been assigned to the same task owner for more than two years. Resetting task assignee due to inactivity, to decrease task cookie-licking and to get a slightly more realistic overview of plans. Please feel free to assign this task to yourself again if you still realistically work or plan to work on this task - it would be welcome!

For tips how to manage individual work in Phabricator (noisy notifications, lists of task, etc.), see https://phabricator.wikimedia.org/T228575#6237124 for available options.
(For the records, two emails were sent to assignee addresses before resetting assignees. See T228575 for more info and for potential feedback. Thanks!)