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

fix(AdminSettings): test Websocket connection #13973

Merged
merged 2 commits into from
Jan 13, 2025
Merged

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Dec 10, 2024

☑️ Resolves

Allows to test websocket connection with HPB in Admin settings (before users will face it in Talk interface): client attempts to establish new websocket, then:

  • clean close on success
  • throw error if any

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Small screen Wide screen
image image
image image

🚧 Tasks

  • Handle NC <-> HPB servers time sync issue
  • Handle other cases that could be checked?

🏁 Checklist

  • 🌏 Tested with different browsers / clients:
    • Chromium (Chrome / Edge / Opera / Brave)
    • Firefox
    • Safari
    • Talk Desktop
    • Not risky to browser differences / client

@Antreesy Antreesy added this to the 🖤 Next Major (31) milestone Dec 10, 2024
@Antreesy Antreesy self-assigned this Dec 10, 2024
@Antreesy Antreesy changed the title fix(AdminSettings): allow to retry server checks fix(AdminSettings): test Websocket connection Dec 11, 2024
@Antreesy Antreesy force-pushed the fix/noid/signaling-errors branch from 60416d3 to 42c707f Compare December 11, 2024 15:57
@Antreesy
Copy link
Contributor Author

I'm lacking knowledge on how else this connection could fail (and how to reproduce it), so wouldn't mind a second opinion here
cc @danxuliu @fancycode @SystemKeeper

@fancycode
Copy link
Member

Another thing you could test here is sending a hello request after the connection was established. This will verify that the backend url and shared secret are correctly configured in the signaling server.

@fancycode
Copy link
Member

Regarding testing: this will also fail if the signaling server url is incorrect and the request can't be upgraded to a WebSocket. This can be seen in the response (could be 404 for unknown urls or something like 400 for valid urls that don't support WebSocket), but I'm not sure if you get all the details from the JavaScript API.

@fancycode
Copy link
Member

Another thing you could test here is sending a hello request after the connection was established. This will verify that the backend url and shared secret are correctly configured in the signaling server.

As I had a HPB setup support request from a customer yesterday where the backend url was a problem, this would be really great to test from the admin settings. Maybe you could also show the backend url ithat Talk will send to the signaling server in the UI so the user can compare with the configuration of the signaling server (see https://github.com/strukturag/nextcloud-spreed-signaling/blob/0a04ea290969cfe1ebc3ef3aaeeae1bdc49ea552/server.conf.in#L125-L126).

@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 18, 2024

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

In my development environment if the secret is wrong the WebSocket is still successfully connected and no error message is shown.

I guess it is caused by using the hello protocol version 2.0 if available; if I am not mistaken in that case the signaling server does not send a request to the Nextcloud server to verify the authentication parameters, so the Websocket connection can be established even if the secret is wrong (although then an error will be received as soon as the signaling server sends a request to the Nextcloud server, for example, when the user tries to join a conversation).

If my guess is right then it would be better to use the hello protocol version 1.0 for the test, independently of what is supported by the signaling server. @fancycode Would there be any drawbacks with that? And even more important, is my guess right, or am I missing something? :-)

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Works better then before and is blocking an upcoming feature, so fine by me to merge like this and look into secret verification as a follow-up?

@fancycode
Copy link
Member

In my development environment if the secret is wrong the WebSocket is still successfully connected and no error message is shown.

I guess it is caused by using the hello protocol version 2.0 if available; if I am not mistaken in that case the signaling server does not send a request to the Nextcloud server to verify the authentication parameters, so the Websocket connection can be established even if the secret is wrong (although then an error will be received as soon as the signaling server sends a request to the Nextcloud server, for example, when the user tries to join a conversation).

Right, the hello v2 was introduced to get rid of the request to verify the tickets in the hello request. With v2, the public key is fetched from the capabilities (and cached), so the provided ticket can be checked without additional requests.

If my guess is right then it would be better to use the hello protocol version 1.0 for the test, independently of what is supported by the signaling server. @fancycode Would there be any drawbacks with that? And even more important, is my guess right, or am I missing something? :-)

That would be a way to check the secret, however you will not test that v2 also works ;) So the best would be to check a v1 hello to verify the secret and v2 to verify the public key from the capabilities. Note that you will have to establish a new WebSocket connection for each test (sending a second hello through an already established session will not work).

@Antreesy Antreesy merged commit e3be739 into main Jan 13, 2025
50 checks passed
@Antreesy Antreesy deleted the fix/noid/signaling-errors branch January 13, 2025 12:08
@Antreesy
Copy link
Contributor Author

Thanks, @danxuliu @fancycode !

Attempt is made at #14121.
Although I'm unable to catch failed secret:
image
Maybe, there is no actual request made (as in BackendNotifier.php)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants