Document justification for ApiZeroConfigPrinter and mark modules using it as internal
Open, Needs TriagePublic

Description

I see in the commit message for Gerrit change 102850 this class was apparently added because varnish cannot handle receiving API warnings.

There should be a comment documenting this, and since ApiZeroBanner and ApiZeroPortal are apparently intended for internal use by varnish rather than general public consumption they should implement isInternal() to return true.

Anomie created this task.Mar 3 2015, 10:08 PM
Anomie updated the task description. (Show Details)
Anomie raised the priority of this task from to Low.
Anomie moved this task to Non-core-API stuff on the MediaWiki-API board.
Anomie added a subscriber: Anomie.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 3 2015, 10:08 PM
jhobs assigned this task to Yurik.Mar 3 2015, 10:27 PM
jhobs set Security to None.
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 17 2016, 6:35 PM

@Yurik: This issue has been assigned to you 20 months ago.
Could you please share a status update? Are you still working (or still plan to work) on this issue? Is there anything that others could help with?
Only in case you do not plan to work on this issue anymore, should you be removed as assignee (via 'Assign / Claim' in the 'Actions' dropdown menu) to avoid cookie-licking?

Jdlrobson removed Yurik as the assignee of this task.Jun 5 2017, 6:10 PM
Jdlrobson lowered the priority of this task from Low to Lowest.
Jdlrobson added subscribers: Yurik, Jdlrobson.

Still relevant...?

Anomie added a comment.Jun 5 2017, 6:29 PM

Yes. Although the class is apparently now named ZeroBanner\ApiRawJsonPrinter rather than ApiZeroConfigPrinter.

Jdlrobson moved this task from Backlog to Blocked on the Readers-Web-Backlog (Tracking) board.

Web team is not familiar with the Zero extension. Will need onboarding/knowledge share before we can understand and work on this bug.

Say, I noticed that calls to, for example, http://php5.local.wmftest.net:8080/w/api.php?action=zeroconfig&type=message&agent=<someUA> on my Vagrant instance with header forging (which would be equivalent to https://en.wikipedia.org/w/api.php?action=zeroconfig&type=message&agent=<someUA> with VCL determining an X-CS header independently or https://en.wikipedia.org/w/api.php?action=zeroconfig&type=message&agent=<someUA>&X-CS=TEST1 if using the parameter debugging technique...which gets send to the inbound origin) exercise ApiZeroBanner and ApiRawJsonPrinter. This type of URL is consumed from an internet context by native apps (and historically the FFOS web app).

@Anomie I noticed in ApiBase.php's documentation for isInternal() that "Internal API modules are not (yet) intended for 3rd party use and may be unstable." It's true that, at least to our knowledge, nobody's tried to re-use this code in a third party hosting context, and likely won't do so. Given this, does it make sense to add the isInternal() method to ApiRawJsonPrinter (ApiBase is an ancestor class) as you suggest given this, or was this more about server-to-server calls in fact?

As you say ApiZeroPortal (directly extends ApiBase as well) is for internal server-to-server Action API goodness - the netmapper routine and portal DSL connects to it, for example (CC @BBlack, no action required, just making you aware of this).

I can easily submit a patch for one or both of these things, but with impending time off, I'd be afraid to post the patch and have it shipped to the cluster without being around to test. So I think we could patch one class or both classes depending on your input here. But could we set up a time to SWAT this stuff so that @DFoy and, if I'm around, me can verify no adverse effects? I'm pretty sure this will be safe, but better to be truly safe than sorry.

Heads up, @DFoy (Dan's the Senior Product Manager on Partnerships and Global Reach - our technical PrdM for Zero), @Mholloway (technical POC on W0 while I'm on leave), @Fjalapeno (Reading Infrastructure product owner).

@Anomie I noticed in ApiBase.php's documentation for isInternal() that "Internal API modules are not (yet) intended for 3rd party use and may be unstable." It's true that, at least to our knowledge, nobody's tried to re-use this code in a third party hosting context, and likely won't do so. Given this, does it make sense to add the isInternal() method to ApiRawJsonPrinter (ApiBase is an ancestor class) as you suggest given this, or was this more about server-to-server calls in fact?

ApiRawJsonPrinter is never going to be reported in a manner where isInternal() would have an effect, so it doesn't make much difference there. The suggestion for implementing isInternal() is directed towards ZeroPortal\ApiZeroPortal and ZeroBanner\ApiZeroBanner, which do show up in API help (e.g. here).

I can easily submit a patch for one or both of these things, but with impending time off, I'd be afraid to post the patch and have it shipped to the cluster without being around to test.

It would be extremely hard to break anything while fixing this bug. ;)

The only thing implementing isInternal() does is adds a note to the right-floated box on the API help page (see here for example) and a similar item in the action=paraminfo output (see here, the "internal" flag).

The other part of the bug is just to add a doc comment here and/or here and here explaining why you need to override it.

Thanks, @Anomie ! Do https://gerrit.wikimedia.org/r/#/c/386014/ and https://gerrit.wikimedia.org/r/#/c/386012/ capture the sort of detail you had in mind? Thanks for the reassurance about the non-breakage.

@JoeWalsh, @Dbrant , @BBlack added you as courtesy reviewers - the contract of request-response shouldn't change with this per the comment preceding this one.

@Mholloway you're on both patches now as well. Hoping you can test the experience on your VM to ensure it's working (it's working on mine); recall that to access action=zeroportal&type=carriers you should be auth'd with the MediaWiki-Vagrant login.

Change 386012 had a related patch set uploaded (by Jforrester; owner: Dr0ptp4kt):
[mediawiki/extensions/ZeroBanner@master] Document why ApiBase is overridden and add isInternal member

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

Change 386014 had a related patch set uploaded (by Jforrester; owner: Dr0ptp4kt):
[mediawiki/extensions/ZeroPortal@master] Document why ApiBase is overridden and add isInternal member

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

Change 386012 merged by jenkins-bot:
[mediawiki/extensions/ZeroBanner@master] Document why ApiBase is overridden and add isInternal member

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

Change 386014 merged by jenkins-bot:
[mediawiki/extensions/ZeroPortal@master] Document why ApiBase is overridden and add isInternal member

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

@Mholloway you're on both patches now as well. Hoping you can test the experience on your VM to ensure it's working (it's working on mine); recall that to access action=zeroportal&type=carriers you should be auth'd with the MediaWiki-Vagrant login.

Looks good on my end.

Thanks @Mholloway, thanks also @Yurik for code review and Brad for guidance along the way.

Thanks, @Anomie ! Do https://gerrit.wikimedia.org/r/#/c/386014/ and https://gerrit.wikimedia.org/r/#/c/386012/ capture the sort of detail you had in mind?

I see isInternal() is implemented in the right places (see Beta), so that part is good.

The added descriptions don't seem adequate to me though.

Heads up for those following the ticket: @Anomie and I talked and I have this on my list for a follow on patches to clarify more of the why in the comments. Regarding the comment on the class and not the method, I'll fix that - thanks for spotting that @Anomie. As a reminder, my impending time off could arrive any moment - although documentation changes are simple I want to get it right, so please bear with me in case it takes some time...possibly until after returning.

https://gerrit.wikimedia.org/r/#/c/387628 and https://gerrit.wikimedia.org/r/#/c/387629 posted. @Anomie would you please review? Note, I pointed ApiZeroPortal:getCustomPrinter() documentation readers back to the documentation concerning ApiRawJsonPrinter.

Those look like decent explanations.

Thanks. Okay to close this task? Looks like @Yurik +2'd.

Framawiki moved this task from Backlog to Doing on the Easy board.Dec 2 2017, 1:41 PM