Implement access restrictions in WatchedItemStore
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	daniel
	May 13 2019, 8:25 PM

Description

When replacing User::addWatch and friends with calls to the corresponding methods on WatchedItemStore, the viewmywatchlist and editmywatchlist permissions have to be checked. We don't want all callers to duplicate that logic, but we don't really want WatchedItemStore to worry about user permissions too much either. We could however implement a simplified access check, given the following assumptions:

Users can only access their own watchlist
Users may want to restrict access further when acting through OAuth
Some operations may want to bypass this restriction (e.g. edits via oauth should still automatically add items to the watchlist if this behavior is enabled in the user's preferences)

This can be implemented by having a $readRestriction and a $writeRestriction field in WatchedItemStore. Each restriction field can have three kinds of values:

null: no restrictions apply
true: restricted, access will fail.
a UserIdentity: only this user's watchlist items can be accessed, attempts to access other user's watchlist items will fail.

Checked access will be done via new methods, using names like addWatchChecked or some such (maybe these should not be in WatchedItemStoreInterface).

The restriction levels can be set via the constructor, or a setter in WatchedeItemStore (not WatchedItemStoreInterface). It would have to be set by wiring, when the WatchedeItemStore is constructed, based on information from the current request's session.

NOTE: this is blocked on a decision about how service behavior can vary based on the current user's session, see T218555 and T231930.

Details

	Subject	Repo	Branch	Lines +/-
	Use WatchlistManager rather than accessing WatchedItemStore directly.	mediawiki/extensions/Flow	master	+7 -30

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T208764 Remove cyclic dependency between Title and User classes
Resolved	Vedmaka	T208766 Deprecate User::isWatched, isTempWatched, addWatch, and removeWatch
Resolved	CCicalese_WMF	T223165 Implement access restrictions in WatchedItemStore

Event Timeline

daniel created this task.May 13 2019, 8:25 PM

daniel mentioned this in T208766: Deprecate User::isWatched, isTempWatched, addWatch, and removeWatch.

WDoranWMF moved this task from Later to Mop Column on the Platform Team Legacy board.Jul 5 2019, 2:54 PM

WDoranWMF removed a project: Platform Team Legacy (Later).

It appears Platform Engineering is taking this on so the Growth-Team is moving this to external.

In T223165#5318871, @JTannerWMF wrote:

It appears Platform Engineering is taking this on so the Growth-Team is moving this to external.

It's in scope of the decoupling initiative, but not yet scheduled. If there is any urgency to having this implemented, we should coordinate on who has capacity when.

WDoranWMF moved this task from Decoupling (CDP2) to mop on the Platform Engineering board.Jul 26 2019, 6:55 PM

WDoranWMF edited projects, added Platform Team Initiatives (Decoupling (CDP2)); removed Platform Engineering (Decoupling (CDP2)).

Krinkle subscribed.Sep 3 2019, 2:00 PM

daniel updated the task description. (Show Details)Jan 21 2020, 11:54 AM

DannyS712 subscribed.Feb 26 2020, 8:46 AM

@Vedmaka are you still planning on working on this?

@Vedmaka: Could you please answer the last comment? Thanks! :)

Sorry about the delay in responding. I'll answer on behalf of @Vedmaka. We aren't working on this right now and we don't if/when we will be able to pick it up again.

tosfos added a comment.Apr 29 2020, 3:14 PM

This comment was removed by tosfos.

What about adding a UserWatchlistManager to do this?

Rough outline

public function __construct(
    WatchedItemStore $watchedItemstore,
    PermissionManager $permissionManagerManager
)

public function maybeAddWatch(
    UserIdentity $user,
    Title $title,
    ?string $expiry = null
) {
    // Check user rights, and if the checks pass add to watchlist
}

public function maybeRemoveWatch(
    UserIdentity $user,
    Title $title
) {
    // Check user rights, and if the checks pass remove from watchlist
}

public function maybeCheckWatched(
    UserIdentity $user,
    Title $title
) {
    // Check user rights, and if the checks pass check if the user is watching the page
}

Current calls to User::isWatched, ::addWatch, and ::removeWatch with User::IGNORE_USER_RIGHTS can be replaced with direct access to the WatchedItemStore, while calls using User::CHECK_USER_RIGHTS (the default) would go through the new service.

Thoughts?

MBinder_WMF added a project: Growth-Team-Filtering.Apr 15 2021, 6:51 PM

Krinkle unsubscribed.Apr 15 2021, 8:58 PM