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

Check endpoint to validate WOPI configuration from integrators #4353

Closed

Conversation

juliusknorr
Copy link
Member

  • Target version: master

Summary

The idea of this is that integrators like Nextcloud can easily verify potential setup issues like Collabora not being able to connect back to the storage during configuration, so that we can give the admin more helpful hints on why the setup is not working properly

TODO

This is an early draft PR so there might be additional things

  • See if we can avoid duplicate code between the host checks in the check method and actual wopi handling
  • Cleanup code
  • Proper format for the json response

Checklist

  • Code is properly formatted
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Change-Id: I760ca8d99a6e9bce4371354139b46468f317545f
@juliusknorr
Copy link
Member Author

Let me check this early next week again before reviewing , since i think i already have a rebased and slightly revisited implementation next on my other machine.

@mmeeks
Copy link
Contributor

mmeeks commented Apr 18, 2024

@juliushaertl @Ashod any chance of re-basing this and getting it in - it's a common failure mode I'd love to patch.

@mmeeks
Copy link
Contributor

mmeeks commented May 16, 2024

@meven this is rather an important thing to have included to help people get setup - can you chase it ?

@meven
Copy link
Contributor

meven commented May 16, 2024

Wopi Hosts can already do some validation on their own, check COOL is accessible to them and that the browser can access COOL.
A missing check is the wopi client (COOL) to wopi host access.

A simple design could be:

Having an endpoint in COOL, like /hosting/wopiAccessTest. "wopiAccessTest" name is only a suggestion at this point.
That would take a json input:

{
    "wopiHostUrl": <url-to-wopiHost> 
}

COOL would then try to make a HTTPS request to the wopiHostUrl.
And as long as the answer is HTTP 200, the status would be OK.
Otherwise a number of status would be returned.

status = OK|DNS_RESOLUTION_ERROR|HOST_UNREACHABLE|CERTIFICATE_ISSUE|TIMEOUT|NO_HTTP_RESPONSE|NOT_200_RESPONSE

And then the response would be like:

{
    "status": <status>
    "details": Some text description of the error
}

This would be exposed in Capabilities, so Host can gracefully support different versions with or without this feature.

<app name="Capabilities">
<action ext="" name="wopiAccessTest" urlsrc="https://172.17.0.1:9980/hosting/wopiAccessTest"/>
</app>

Discussed with @juliushaertl

@mmeeks
Copy link
Contributor

mmeeks commented May 16, 2024

Sounds perfect - lets hack it up =)

Copy link
Contributor

@mmeeks mmeeks left a comment

Choose a reason for hiding this comment

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

Code is much better, we still have a synchrous request there - that needs fixing =)

LOG_DBG("Checking remote host: " << addressToCheck);
try
{
if (!StorageBase::allowedWopiHost(addressToCheck) && !net::isLocalhost(addressToCheck))
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not assume that localhost or 127.0.0.x are route-able and reachable locally - this is often not the case in containers - and the main problem we have with setups.

http::Request wopiRequest(remoteServerURI.getPathAndQuery());

const std::shared_ptr<const http::Response> httpResponse =
httpSession->syncRequest(wopiRequest);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sync is synchronous - this will block the server and the main thread until that request completes. We have to do async requests.

@juliusknorr
Copy link
Member Author

@mmeeks What's your take then rather continue this one or #9202 ?

@mmeeks
Copy link
Contributor

mmeeks commented Jul 11, 2024

@mmeeks What's your take then rather continue this one or #9202 ?

I guess we should continue with #9202 - I expect Meven would appreciate some help with getting that finished if you have time.

@mmeeks mmeeks closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Improve setup checks
4 participants