Page MenuHomePhabricator

Convert PreferencesFactory to Authority [hard]
Open, MediumPublic

Description

Currently DefaultPreferencesFactory and GlobalPreferencesFactory are using PermissionManager to do the permission checks. Both should be converted to using Authority instead.

The permissionManager in both classes is only used for PermissionManager::userHasRight calls, which has a 1-1 replacement as 'Authority::isAllowed'. The authority should be taken from IContextSource passed into the public methods, not from the User.

One complication here is that DefaultPreferencesFactory::saveFormData does not get User object, but instead gets it from the target of the form. Technically, it's correct since the preferences factory target user should always be the current user, but it's ugly. Instead the method signature should be changed to pass in the Authority. The method is overridden in GlobalPreferencesFactory, so we'd need to do some complex dance:

  1. Introduce a new method with authority in DefaultPreferencesFactory
  2. Override the new method in GlobalPreferencesFactory
  3. Switch the call, remove the original method in DefaultPreferencesFactory
  4. Remove the original method in GlobalPreferencesFactory

With the rest of the private/protected methods, User is passed and it probably can just be changed to Authority. The target user in the form object can probably be changed to be UserIdentity.

Event Timeline

daniel raised the priority of this task from Medium to High.Mar 9 2021, 6:31 PM

The PreferencesFactory interface is marked as @stable to implement. If we want to honor that, we need a better plan for this change. One way would be to introduce a new interface, PreferencesFactory2 or PreferencesFactory36 or something, and have the classes implement both. But that would require all methods names to be distinct. And we'd have to expose both interfaces via MediaWikiServices.

Or we go for a breaking change without deprecation, given that PreferencesFactory is not implemented by anything else besides GlobalPreferences... We'll have to decide and announce that, though.

daniel lowered the priority of this task from High to Medium.Mar 10 2021, 6:24 PM
daniel renamed this task from Convert PreferencesFactory to Authority to Convert PreferencesFactory to Authority [hard].Mar 10 2021, 6:32 PM
Aklapper added a subscriber: daniel.

@daniel: Per emails from Sep18 and Oct20 and https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup , I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action...Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!