Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MSC2190: Allow appservice bots to use /sync #2190

Open
wants to merge 2 commits into
base: old_master
Choose a base branch
from
Open

MSC2190: Allow appservice bots to use /sync #2190

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a0e22e
Allow appservice bots to use /sync
tulir Jul 24, 2019
d67a0d5
Clarify the proposed behavior for /sync
tulir Jul 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions proposals/2190-allow-appservice-sync.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Allow appservice bots to use /sync
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had another loop around this MSC among the spec core team today. I will try to summarise the discussion:

On the last point in particular:

  • This certainly isn't the only way an AS can DoS a homeserver. If somebody chooses to run a poorly-scaling AS against their HS, isn't that their problem?
  • It's relatively trivial to work around the restriction as it currently stands - it feels like we're just putting a low barrier that people will trip over rather than applying any real protection.
  • Generally the situation might be better handled by warnings and advice in the spec (or better, an AS implementation guide).

On the other hand:

  • It is reasonable for the spec to forbid behaviour that cannot be implemented efficiently.
  • We discussed making the behaviour configurable in Synapse (so you would have to consciously enable /sync for appservices), but that basically means that Synapse wouldn't be spec-compliant out of the box and might lead to fragmentation.


In the past, application services could use `/sync` as the appservice bot as an
alternative to having a HTTP server listening for events from the homeserver.
At some point, that feature was deprecated and broke entirely, so appservice
bot `/sync`ing was blocked in synapse entirely (in [matrix-org/synapse#2948]).
Later the limitation was added to the spec in [matrix-org/matrix-doc#1144] and
[matrix-org/matrix-doc#1535].

Currently, the application service spec states

> Application services wishing to use `/sync` or `/events` from the Client-Server
API MUST do so with a virtual user (provide a `user_id` via the query string).
It is expected that the application service use the transactions pushed to
it to handle events rather than syncing with the user implied by
`sender_localpart`.

## Proposal

The limitation is unnecessary, so this proposal suggests removing the special
casing. This would mean calling `/sync` with the appservice token and no
identity assertion parameter would work the same way as any other user calling
`/sync`, i.e. it only returns events for the appservice bot. Note that this
behavior would be different from the original synapse behavior, where `/sync`
returned events for all users in the appservice's namespace.

## Tradeoffs

None.
Comment on lines +27 to +29
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the MSC template removed this section because it was pointless. Suggest doing the same here.

Suggested change
## Tradeoffs
None.


## Potential issues

Old appservices that use `/sync` could run into silent issues rather than the
very clear internal server error currently thrown by synapse.
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The real issue here is that people could write application services that do not scale (since /sync is arguably impossible to implement in an efficient way for a user that is in thousands of rooms).

Whether or not you consider that a real issue, it's unarguably a potential issue, so should be called out here. By omitting it, it suggests you're not even considering it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand on this: it could be considered regrettable that the /sync api was designed the way it was. It doesn't even currently scale well to large user accounts. The horse might have bolted there, but that doesn't necessarily mean we should extend the mistake to ASes.


## Security considerations

None.

[matrix-org/matrix-doc#1144]: https://github.com/matrix-org/matrix-doc/issues/1144
[matrix-org/matrix-doc#1535]: https://github.com/matrix-org/matrix-doc/pull/1535
[matrix-org/synapse#2948]: https://github.com/matrix-org/synapse/pull/2948