Page MenuHomePhabricator

Update requireLogin() for temp users on ReadingLists extension
Closed, ResolvedPublic

Description

In T344276, we want to make sure the SpecialPage::requireLogin() function is used in accordance with the introduction of temporary users. As for now, we want to treat temporary users just like anonymous or IP users.

Request:
Should requireLogin() be available to temporary users?
If yes, no update is needed.
If not, requireNamedUser must return true instead.

Notes:
RequireLogin was found in the following files:
src/SpecialReadingLists.php
Code Search Link

Event Timeline

@Jdlrobson Would this be something that the Web Team could take on? Just asking because I saw your name on the line that calls requireLogin, and noticed the pattern was similar to MobileFrontend.

@Tchanders this is officially owned by the ContentTransform team (https://www.mediawiki.org/w/index.php?title=Developers/Maintainers) but Dmitry from the apps team and I have worked on it in the past. I'd be happy to review a patch from your team or provide a patch for your team to review if that's helpful?

@Tchanders thanks for tagging us. Although the Content Transform Team owns this due to historical reasons, no engineer in the team today have enough knowledge about this extension, if it's a major block we can see what can be done to unblock.

For now we are not being able to fit into our bandwidth. Please let me know if you have any questions or concerns.

Change 972448 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/ReadingLists@master] Reading lists should not be available to IP masked users

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

My guess is this page should not be available to IP masked users.

The special page should probably follow whatever decision is taken to allow / disallow temp users from using the reading lists APIs.

My guess is this page should not be available to IP masked users.

Sounds fine - in the absence of an owner, let's disable it. Others can work on whatever is needed to re-enable it if they want to.

The special page should probably follow whatever decision is taken to allow / disallow temp users from using the reading lists APIs.

Makes sense - will make a follow-up for this.

Change 972832 had a related patch set uploaded (by Tchanders; author: Tchanders):

[mediawiki/extensions/ReadingLists@master] Disallow temporary users from using the APIs

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

Change 972832 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Disallow temporary users from using the APIs

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

Change 972448 merged by jenkins-bot:

[mediawiki/extensions/ReadingLists@master] Reading lists should not be available to IP masked users

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

Jdlrobson claimed this task.

@JTannerWMF FYI - the change will prevent temp users from using reading lists (which is the status quo in the sense that anonymous users also can't use them).

In the apps anon users can use reading list and have it stored locally to their device @Tgr

@JTannerWMF @Seddon We made these changes in the absence of an owner to make decisions., so feel free to revisit those decisions

I've opened T351116 to capture the larger product questions and give this more meaningful thought before roll out of IP masking following a chat with @Seddon . For now to reiterate - we're just doing the bare minimum to support roll out of IP masking and this is by no means a final decision and we can revisit it later if T351116 deems it necessary.