Page MenuHomePhabricator

Implement access restrictions in WatchedItemStore
Open, Needs TriagePublic

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.

Event Timeline

JTannerWMF added a subscriber: JTannerWMF.

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

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.

@Vedmaka are you still planning on working on this?

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

tosfos added a subscriber: Vedmaka.

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.

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?