Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

remove obsolete webclient #2601

Closed
wants to merge 16 commits into from

Conversation

krombel
Copy link
Contributor

@krombel krombel commented Oct 28, 2017

This got requested several times as in #1527

This introduces a homepage which is located at /_matrix/static/ and redirects the old webclient-path (/_matrix/client) to that path as well if the webclient is enabled
Why /_matrix/static/ is used is described in the commit-description of ba0c18e

This would close #1527, #1883, #2113 and indirectly #1250 and #1353

Signed-Off-by: Matthias Kesler [email protected]

@matrixbot
Copy link
Member

Can one of the admins verify this patch?

1 similar comment
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@maxidorius
Copy link
Contributor

@krombel What about the following concerns:

  • This leaks HS domains to a 3rd-party organization by default
  • Whatever website is used might be blocked or become obsolete

What about showing a page to the user instead, explaining it has been removed and why?

@turt2live
Copy link
Member

or as per #1527 give some kind of page that says "Congratulations! You have synapse running!"

@krombel
Copy link
Contributor Author

krombel commented Oct 30, 2017

So you would prefer to

  • use a "Congratulations" page for /
  • show a descriptive page for the "old" webclient URL instead of redirecting?

@maxidorius
Copy link
Contributor

/ and old webclient URL should show the same thing, which would ideally be a "Get started" page, pointing to clients, useful rooms for synapse, support channels, etc.

Ideally, the oldwebclient URL would redirect to /

@krombel
Copy link
Contributor Author

krombel commented Oct 30, 2017

So some page that mixes matrix.to and the homepage of riot (for the rooms), correct?

But: I do not know if it is good to introduce another site that would need updates from time to time.
Especially as synapse is going to be replaced by dendrite and the focus will move away from synapse.

So I think it might be enough to have a page that just links to matrix.org and matrix.to instead of a page that contains those information

@maxidorius
Copy link
Contributor

So some page that mixes matrix.to and the homepage of riot (for the rooms), correct?

No, just links to helpful pages, like the Try Matrix one. As you pointed out, you don't want to have to update this often.

So I think it might be enough to have a page that just links to matrix.org and matrix.to instead of a page that contains those information

Yes, definitely.

somehow this introduced the bug that synapse tried to handle all URL's
to be relative to STATIC_PREFIX so all endpoints got not handled
any longer. This commit introduces a redirect to `/_matrix/static` where
the homepage gets automatically served to circumvent this bug.
@turt2live
Copy link
Member

An interesting problem is that the spec actually says there needs to be a web client: https://matrix.org/docs/spec/client_server/r0.3.0.html#login-fallback

It feels kinda weird to say in the spec that a small webapp has to be published. Instead, the spec should be updated to get rid of that in my opinion.

@krombel
Copy link
Contributor Author

krombel commented Jan 16, 2018

related Spec-PR: matrix-org/matrix-spec-proposals#1105

<p>For questions about synapse or matrix, please visit <br />
<a href="https://matrix.to/#/#synapse-community:matrix.org">#synapse-community:matrix.org</a> or
<a href="https://matrix.to/#/#matrix:matrix.org">#matrix:matrix.org</a>.</p>
<p>Welcome in the matrix-universe :)</p>
Copy link
Member

Choose a reason for hiding this comment

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

s/in/to/

<h1>Synapse is running</h1>
<p>Congratulations!</p>
<p>Your Synapse server is listening on this port and is ready for messages.</p>
<p>To use this matrix-server you'll need a client to use this server - e.g. one of
Copy link
Member

Choose a reason for hiding this comment

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

seems duplicative, """to use this server you need a client to use this server"""

dbkr
dbkr previously requested changes Jan 18, 2018
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

This also removes the login fallback which is unrelated to the angular client.

@krombel
Copy link
Contributor Author

krombel commented Jan 18, 2018

So do I understand it right that the fallback should stay and the angular client be removed?
In other words: Just remove the dependency to matrix_angular_sdk but let the files at
synapse/static/client/login/* (and synapse/static/client/register/?)

@dbkr
Copy link
Member

dbkr commented Jan 18, 2018

Yep, fallback should definitely stay (whether the angular client should go or not seems to be a different debate).

@maxidorius
Copy link
Contributor

@dbkr I don't understand how this should end up then. Should the endpoint still exist but can return a page which is not a client, or no data at all? or should there always be a client, just not the angular client?

@krombel
Copy link
Contributor Author

krombel commented Jan 18, 2018

@dbkr I do not really understand what the purpose of leaving the login client is.
It is commonly advised to not use the buildin client and preferably go for other clients...

@t3chguy
Copy link
Member

t3chguy commented Jan 18, 2018

@krombel the login client is not part of the obsolete client aiui
the login client is a way for clients which don't support password login for whatever reason to still be able to get an access token by just throwing their user at it

@erikjohnston
Copy link
Member

The "login client" isn't really a client at all, its just a web page that can be embedded in a client to do the login flows. This makes it possible to login with any client even if the server has some obscure custom login flows which few clients support.

Without this fallback mechanism clients wouldn't be able to login to servers that require custom login flows that the client doesn't understand.

@dbkr dbkr dismissed their stale review January 18, 2018 15:36

Requested changes made

@dbkr
Copy link
Member

dbkr commented Jan 18, 2018

Yep, what @t3chguy and @erikjohnston said. This looks better. This is becoming much less my area of expertise now, but other potential things which may or may not be worth worrying about:

  • The 'congratulations' page is nice as a first thing the user sees, but making every synapse host it with no way to disable it feels a bit strange to me. It's much like the Apache default page, but of course that would normally get replaced with your content.
  • If there are links to rooms in the software itself, it feels like they ought to be official project rooms

When this setting is set to true we have a redirect from
`/` to ´STATIC_PREFIX`. If set to false a call to / will return with
"No Resource"
@andrewshadura
Copy link
Contributor

andrewshadura commented Jun 7, 2018

This will also solve #1932 and probably also #1561 and #2368.

@andrewshadura
Copy link
Contributor

Is there anything that’s blocking this PR from being merged? I guess non-critical issues can be fixed incrementally in separate PRs.

@richvdh
Copy link
Member

richvdh commented Jun 7, 2018

Just tuits to look at it properly. It's unlikely it will solve the captcha freedom issues, fwiw

@andrewshadura
Copy link
Contributor

Why? The captcha is related to the webclient, if it’s removed, the recaptcha.js will also go away?

@t3chguy
Copy link
Member

t3chguy commented Jun 7, 2018

@andrewshadura its probably used in the fallback login client
https://matrix.org/speculator/spec/HEAD/client_server/unstable.html#fallback
(which is shown by clients which wish not to directly house the recaptcha)

@andrewshadura
Copy link
Contributor

Right, so if reCAPTCHA has to be used, can we dynamically load it and not ship the minified source instead?

@richvdh
Copy link
Member

richvdh commented Jun 7, 2018

Why? The captcha is related to the webclient, if it’s removed, the recaptcha.js will also go away?

recaptcha is part of the spec: https://matrix.org/docs/spec/client_server/r0.3.0.html#google-recaptcha. Removing support from synapse will not fix that, which is the subject of #1561 aiui. (on the other hand, that makes it a spec issue, not a synapse issue: updated accordingly).

Right, so if reCAPTCHA has to be used, can we dynamically load it and not ship the minified source instead?

almost certainly. Definitely an orthogonal problem though, and the subject of #1932, aiui.

richvdh added a commit that referenced this pull request Dec 11, 2018
This is based on the work done by @krombel in #2601.
richvdh added a commit that referenced this pull request Dec 11, 2018
This is largely a precursor for the removal of the bundled webclient. The idea
is to present a page at / which reassures people that something is working, and
to give them some links for next steps.

The welcome page lives at `/_matrix/static/`, so is enabled alongside the other
`static` resources (which, in practice, means the client API is enabled). We'll
redirect to it from `/` if we have nothing better to display there.

It would be nice to have a way to disable it (in the same way that you might
disable the nginx welcome page), but I can't really think of a good way to do
that without a load of ickiness.

It's based on the work done by @krombel for #2601.
@richvdh
Copy link
Member

richvdh commented Dec 11, 2018

superceded by #4289 / #4290. Thank you for your work on this @krombel !

@richvdh richvdh closed this Dec 11, 2018
@krombel
Copy link
Contributor Author

krombel commented Dec 11, 2018

Thanks for finally handling it 😏

@krombel krombel deleted the remove_buildin_webclient branch December 11, 2018 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants