For background, all of the consumers of the eventbus are trusted systems (inside the WMF cluster in our case), right?

In T120212#1851239, @csteipp wrote:

For background, all of the consumers of the eventbus are trusted systems (inside the WMF cluster in our case), right?

Yes, at this stage all of the producers (including this extension) and consumers are internal.

Yes, at this stage all of the producers (including this extension) and

consumers are internal.

So.. plans to have external consumers in the future? Or is the future
undecided at this point? It makes a big difference from a security
perspective, so I think that needs to be established soon, in either case.

If there are external consumers in the future, they will be similar to RCStream, and made for specific use cases. There may be external consumers built off of the data that will be generated by this extension, but they will just be consumers of data in Kafka.

Additionally, the extension itself will never directly talk to external clients, only to the service validating and proxying event messages to Kafka (all internal).

Can someone link to the proposed consumers and a system-level design for this?

"system-level design" = what systems will the data be flowing into, and for (roughly) what purposes?

In T120212#1852816, @csteipp wrote:

Can someone link to the proposed consumers and a system-level design for this?

In T120212#1852817, @csteipp wrote:

"system-level design" = what systems will the data be flowing into, and for (roughly) what purposes?

RESTBase is the only current use-case/consumer for these events, and initially this will only cover the same transitions in state that RestbaseUpdateJobs currently provides. For changes to revision visibility, this is encapsulated in this event schema definition.

My understanding is that username suppressions will be added at some point in the future, when there is a use/need for them on the RESTBase end of things (or elsewhere). @mobrovac or @GWicke can probably elaborate more on this though.

For changes to revision visibility, this is encapsulated in this event schema definition.

So the issue I'm worried about is in this schema, you have,

user:
   type: boolean
   description: "whether the author of the revision's text is available"

If by this, you're saying, "whether the author of the revision's text should be shown", in MediaWiki this is determined by 2 factors-- if the username has been delete/suppressed on a particular revision, or if the username has been suppressed on the entire wiki (using a suppression block). If Restbase is going to cache the username, then it needs to be aware of both. The patch that @mobrovac merged only accounts for the first.

In T120212#1853089, @csteipp wrote:

If by this, you're saying, "whether the author of the revision's text should be shown", in MediaWiki this is determined by 2 factors-- if the username has been delete/suppressed on a particular revision, or if the username has been suppressed on the entire wiki (using a suppression block). If Restbase is going to cache the username, then it needs to be aware of both. The patch that @mobrovac merged only accounts for the first.

Aha! Thanks for your patience, I think I get it now. :)

Ok, so that would be something for a separate event, I assume. And, it's something we don't currently support in RESTBase (i.e. we wouldn't currently hide the author name for any such wiki-wide deletion/suppression). But we should, so I'll open a separate issue to track that.

In T120212#1853124, @Eevans wrote:

... But we should, so I'll open a separate issue to track that.

For reference: T120409: RESTBase should honor wiki-wide deletion/suppression of users

In T120212#1853159, @Eevans wrote:

In T120212#1853124, @Eevans wrote:

... But we should, so I'll open a separate issue to track that.

For reference: T120409: RESTBase should honor wiki-wide deletion/suppression of users

@GWicke - can you let me know what the timeline for implementing this will be? I'm not currently comfortable closing this security review with this gap, unless we have a clear roadmap/timeline for fixing it.

@csteipp: Unless I'm missing something here, (which is possible), this isn't a regression; These wiki-wide suppressions aren't currently covered by RESTBase (presently the only use-case/consumer for these events). And, it's not information missing from the [[https://github.com/wikimedia/mediawiki-event-schemas/blob/master/jsonschema/mediawiki/revision_visibility_set/1.yaml|revision_visibility_set event]], this would require the definition of a separate, distinct event. That event will need to be created in order to complete T120409, (or for any other consumer that needs that it), but doesn't (again, unless I'm missing something), change the implementation here.

@csteipp: We'll prioritize adding support for global user suppression early next quarter.

We are hoping to start moving the extension into production soon; Are we still treating the absence of wiki-wide user suppression as a blocker here?

I don't consider the system meeting our security standards until T120409: RESTBase should honor wiki-wide deletion/suppression of users is fixed. @GWicke says that will be fixed in January.

So as long as restbase is the only consumer of this, and the team is committing to fixing T120409 this quarter, go ahead and deploy it, and we'll close this task when the blocker is fixed.

@csteipp: As mentioned on IRC, we will prioritize this & come up at least with a temporary solution early this quarter. Revision metadata end points are low volume at this point, which lets us consider simple solutions.

[[https://en.wikipedia.org/api/rest_v1/?doc#!/Page_content/get_page_revision_revision|/page/revision/{revision}]] and [[https://en.wikipedia.org/api/rest_v1/?doc#!/Page_content/get_page_title_title|/page/title/{title}]] would seem to be the only places we currently risk a leak. The following PR disables RESTBase caching of revision metadata as a temporary stop-gap, until a more permanent solution is put in place.

https://github.com/wikimedia/restbase/pull/468

In T120212#1933238, @Eevans wrote:

https://github.com/wikimedia/restbase/pull/468

The PR has been merged. To be deployed shortly.

With PR 468 deployed, RESTBase queries the MW API for each content request and hence denies access for edge cases. @csteipp, @dpatrick can we now close this?

In T120212#2075010, @mobrovac wrote:

With PR 468 deployed, RESTBase queries the MW API for each content request and hence denies access for edge cases. @csteipp, @dpatrick can we now close this?

As long as RESTBase is the only consumer of the eventbus, then this solution works. I've heard other teams saying they want to use it, and they would need to account for this also. Is that documented, or is there a check in the process to make sure other teams do that?

Is there an eta on T120409 yet?

As long as RESTBase is the only consumer of the eventbus, then this solution works. I've heard other teams saying they want to use it, and they would need to account for this also.

Per normal policy, any service exposing EventBus (or more generally sensitive) information to the public would need to go through its own security review. This isn't different from RCStream or PHP extensions.

Is there an eta on T120409 yet?

T120409 is resolved for all current / new content.

The only exception is that older, stored Parsoid content still has references to user names in its HTML head, which we will strip in a post-processing step. This isn't really related to EventBus, it just came up in the context of this conversation.

In T120212#2077491, @GWicke wrote:

Is there an eta on T120409 yet?

T120409 is resolved for all current / new content.

The only exception is that older, stored Parsoid content still has references to user names in its HTML head, which we will strip in a post-processing step. This isn't really related to EventBus, it just came up in the context of this conversation.

Sorry, I must have misunderstood the intent of that bug. Since it (initially) had T122079 as a subtask, I assumed that meant the eventbus would push block events, including suppression blocks, so that consumers would get notifications of suppressions. Is there no plan to do that?

Sorry, I must have misunderstood the intent of that bug. Since it (initially) had T122079 as a subtask, I assumed that meant the eventbus would push block events, including suppression blocks, so that consumers would get notifications of suppressions. Is there no plan to do that?

The medium term plan is still to provide user deletion / suppression events in EventBus, which we can then use to safely expose user information from services at a later time.

So, as far as I can tell, all EventBus MVP related work is done. This ticket is still open because T120409 is related, but this has nothing to do with EventBus. I really think we should remove that as a blocking task and close these. @csteipp, objections?

In T120212#2214061, @Ottomata wrote:

So, as far as I can tell, all EventBus MVP related work is done. This ticket is still open because T120409 is related, but this has nothing to do with EventBus. I really think we should remove that as a blocking task and close these. @csteipp, objections?

The real blocker is T122079. Until then, anything using the eventbus has to figure out their own way of handling user suppressions if they store usernames anywhere.

@Ottomata, what is your specific use case?

From my PoV, the EventBus MVP had to do with standing up the service and receiving a minimal set of events from MW. This work was completed 2 quarters ago, so I'd just like to be able to close the ticket.

@Ottomata, I understand the desire. Unfortunately, T122079 is a design flaw (from my understanding of the problem, but happy to discuss that) that should have been addressed before the service was put into production. I'm not going to close the security review until it's fixed. Maybe @GWicke or @Eevans can comment on the timeline?

@csteipp As of 1.27.0-wmf.22 user block events are being emitted to the EventBus. Given that T120409 is effectively resolved for latest content, can we now consider this task as completed?