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.

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

MSC3575 (Sliding Sync) add well-known proxy support #12307

Merged
merged 108 commits into from
Apr 30, 2024

Conversation

EdGeraghty
Copy link
Contributor

@EdGeraghty EdGeraghty commented Mar 4, 2024

Adds MSC3575 (Sliding Sync) proxy support from well-known. Removes the ability for adding custom sliding sync servers through Labs.

Signed-off-by: Ed Geraghty ed@geraghty.family

@EdGeraghty EdGeraghty requested a review from a team as a code owner March 4, 2024 19:52
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Mar 4, 2024
@t3chguy t3chguy added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Mar 5, 2024
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

This seems like it wouldn't work if the value in the well-known was to be updated, as it would only ever be fetched when you enable Sliding Sync and you would not be able to update it AFAICT. This seems like a problem

src/components/views/dialogs/SlidingSyncOptionsDialog.tsx Outdated Show resolved Hide resolved
src/components/views/dialogs/SlidingSyncOptionsDialog.tsx Outdated Show resolved Hide resolved
@EdGeraghty EdGeraghty changed the title MSC3575 (Sliding Sync) "native" (proxy) support MSC3575 (Sliding Sync) add proxy support Mar 5, 2024
@EdGeraghty EdGeraghty marked this pull request as draft March 5, 2024 20:03
@EdGeraghty
Copy link
Contributor Author

This seems like it wouldn't work if the value in the well-known was to be updated, as it would only ever be fetched when you enable Sliding Sync and you would not be able to update it AFAICT. This seems like a problem

Yes. Perhaps adding a setting that it's a proxy and needs to get the client well-known on launch?

@t3chguy
Copy link
Member

t3chguy commented Mar 6, 2024

Yes. Perhaps adding a setting that it's a proxy and needs to get the client well-known on launch?

Yeah this would be a better option, happy for the proxy & "native" support to be removed in favour of the well-known check

@EdGeraghty
Copy link
Contributor Author

EdGeraghty commented Mar 8, 2024

Yes. Perhaps adding a setting that it's a proxy and needs to get the client well-known on launch?

Yeah this would be a better option, happy for the proxy & "native" support to be removed in favour of the well-known check

So should I do away with the dialog altogether, allowing it to be enabled from labs iff ( proxySlidingSyncSupportCheck() || nativeSlidingSyncSupportCheck() ) && slidingSyncHealthCheck()?

I guess I could then add two settings at device level, slidingSyncEnabled and isProxy or similar so it knows whether to do the well-known check on start?

ETA:

Perhaps just set whether or not sliding sync has been enabled, then on open (if there isn't already a proxy url set) have a fail-through from "native" support on the unstable endpoint to a well-known check for the proxy? 🤔

I don't really know how all this stuff is handled in the views and dialogs...

ETA2: Thanks to feedback in the relevant matrix #, I'll:

  1. check if the labs flag is enabled; if not bail: no sliding sync
  2. check if the labs flag proxy url is set; if so: use sliding sync with that proxy url
  3. check if the well-known contains a proxy url; if so: use sliding sync with that proxy url
  4. fallback to "native" mode

Falling back in this order means that people with "native" support provided by their homeserver can still choose to use a proxy url...

Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
Signed-off-by: Ed Geraghty <ed@geraghty.family>
@EdGeraghty
Copy link
Contributor Author

🎉

Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

LGTM otherwise, thanks for persevering!!

src/SlidingSyncManager.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Apr 30, 2024

Needs sign-off as per https://github.com/element-hq/element-web/blob/develop/CONTRIBUTING.md#sign-off

EdGeraghty and others added 2 commits April 30, 2024 18:42
@t3chguy t3chguy enabled auto-merge April 30, 2024 17:51
@t3chguy t3chguy added this pull request to the merge queue Apr 30, 2024
Merged via the queue into matrix-org:develop with commit bb4f575 Apr 30, 2024
27 checks passed
@EdGeraghty EdGeraghty deleted the edg/msc3575-native-support branch April 30, 2024 18:35
Comment on lines +381 to +384
await client.http.authedRequest<void>(Method.Post, "/sync", undefined, undefined, {
localTimeoutMs: 10 * 1000, // 10s
prefix: "/_matrix/client/unstable/org.matrix.msc3575",
});
Copy link
Member

Choose a reason for hiding this comment

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

this is unsafe. It will cause the Sliding Sync proxy to start polling with our access token, which means that the SS proxy will receive our to-device messages, causing decryption errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants