Bug 23075 - Incorrect URL should have a meaningful error message
Summary: Incorrect URL should have a meaningful error message
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Mana-kb (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact:
URL:
Keywords:
Depends on: 22913
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-07 08:31 UTC by Martin Renvoize
Modified: 2019-11-06 17:21 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This enhancement displays a more meaningful error message if an incorrect Mana KB service URL is used in the koha-conf.xml configuration file (for example, if http is used instead of https), rather than the direct output from the failed json parse.
Version(s) released in:
19.11.00


Attachments
Bug 23075: Better error handling in Mana config (2.06 KB, patch)
2019-06-08 18:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: Better error handling in Mana config (2.01 KB, patch)
2019-06-10 15:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: Better error handling in Mana config (2.06 KB, patch)
2019-06-14 11:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: Better error handling in Mana config (2.06 KB, patch)
2019-06-14 11:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: Better error handling in Mana config (2.06 KB, patch)
2019-06-14 11:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: Better error handling in Mana config (2.12 KB, patch)
2019-06-14 17:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23075: Report if mana-kb server is not found (2.89 KB, patch)
2019-06-17 17:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23075: Report if mana-kb server is not found (2.91 KB, patch)
2019-10-14 13:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
screenshot (122.62 KB, image/png)
2019-10-17 08:02 UTC, Martin Renvoize
Details
Bug 23075: Report if mana-kb server is not found (2.89 KB, patch)
2019-11-04 12:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23075: (follow-up) Dissallow transparent redirects (2.29 KB, patch)
2019-11-04 13:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23075: Do not 'Send' if the url is not correct (747 bytes, patch)
2019-11-04 14:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: Report if mana-kb server is not found (2.95 KB, patch)
2019-11-05 19:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: (follow-up) Dissallow transparent redirects (2.33 KB, patch)
2019-11-05 19:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: Do not 'Send' if the url is not correct (796 bytes, patch)
2019-11-05 19:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 23075: Report if mana-kb server is not found (3.02 KB, patch)
2019-11-06 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: (follow-up) Dissallow transparent redirects (2.41 KB, patch)
2019-11-06 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: Do not 'Send' if the url is not correct (870 bytes, patch)
2019-11-06 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23075: Correct indendation (12.14 KB, patch)
2019-11-06 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-06-07 08:31:04 UTC
Currently, if you input an incorrect mana service URL in your config (http rather than https for example) the error message you are given is the direct output of a failed json parse.  We should be able to catch such failures and display a more meaningful error to the end user.
Comment 1 Jonathan Druart 2019-06-08 18:59:31 UTC
Created attachment 90440 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.
Comment 2 David Nind 2019-06-10 09:55:07 UTC
I wanted to test this, but couldn't figure it out! Any hints or direction would be appreciated.

Is this talking about etc/koha-config.xml and the URL in there? For testing is this the URL to configure in koha-config.xml? https://mana-test.koha-community.org/

Or is it once you have it enabled and working, that is you have enabled and added a token?

Up-to-date on master.

I haven't been to get registration to work either, I get these two error messages when on /cgi-bin/koha/admin/share_content.pl:

- An error occurred, mana server returned: URL must be absolute

- Mana configuration is currently empty. This feature will not work. Please contact your site administrator.
Comment 3 Alex Arnaud 2019-06-10 14:08:41 UTC
(In reply to David Nind from comment #2)
> I wanted to test this, but couldn't figure it out! Any hints or direction
> would be appreciated.
> 
> Is this talking about etc/koha-config.xml and the URL in there? For testing
> is this the URL to configure in koha-config.xml?
> https://mana-test.koha-community.org/
> 
> Or is it once you have it enabled and working, that is you have enabled and
> added a token?
David, just enter a non valid URL in mana_config and try to register via cgi-bin/koha/admin/share_content.pl (request a mana token).
> 
> Up-to-date on master.
> 
> I haven't been to get registration to work either, I get these two error
> messages when on /cgi-bin/koha/admin/share_content.pl:
> 
> - An error occurred, mana server returned: URL must be absolute
> 
> - Mana configuration is currently empty. This feature will not work. Please
> contact your site administrator.
As said, mana_config is empty. enter the following in your koha-conf.xml:

<mana_config>https://mana-test.koha-community.org</mana_config>
Comment 4 Alex Arnaud 2019-06-10 14:40:01 UTC
This makes we lost the response code ($result->{code} = $response->code;)
We need it in some place to test the response.

I.e: report search is broken
Comment 5 Jonathan Druart 2019-06-10 14:57:05 UTC
Are you sure Alex? This patch did not remove the line you are referring to, it moved it above.
Comment 6 Jonathan Druart 2019-06-10 14:58:35 UTC
Ha! my bad, got it.
Comment 7 Alex Arnaud 2019-06-10 15:01:40 UTC
My explanation was not very clear, i admit :)
Comment 8 Jonathan Druart 2019-06-10 15:14:31 UTC
Created attachment 90459 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.
Comment 9 David Nind 2019-06-12 22:13:27 UTC
I'm having difficulty getting mana enabled using koha-testing-docker.

I assume I have to update the instance configuration file in /etc/koha/sites/kohadev/koha-conf.xml

I'll try kohadevbox (but last time I tried this I had problems with getting a local git repository and nfs working).
Comment 10 David Nind 2019-06-13 21:05:18 UTC
(In reply to David Nind from comment #9)
> I'm having difficulty getting mana enabled using koha-testing-docker.

Solved my problem!

Needed to go flush_memcached and the restart_all to get changes to /etc/koha/sites/kohadev/koha-conf.xml recognised.
Comment 11 David Nind 2019-06-14 11:46:04 UTC
Created attachment 90594 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.

Test plan:

1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev).
2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all).
3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB.
4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save.
5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB.
6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.'
7. Apply the patch (git so bz apply 23075).
8. Change the Mana config to the correct URL: <mana_config>https://mana-test.koha-community.org</mana_config>
9. Repeat steps 2,3 and 5.
10. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2019-06-14 11:48:11 UTC
Ignore step 8 in the test plan!
Comment 13 David Nind 2019-06-14 11:49:42 UTC
Created attachment 90595 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.

Test plan:

1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev).
2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all).
3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB.
4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save.
5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB.
6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.'
7. Apply the patch (git so bz apply 23075).
8. Change the Mana config to the correct URL: <mana_config>https://mana-test.koha-community.org</mana_config>9. Repeat steps 2,3 and 5.
9. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'.

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2019-06-14 11:51:49 UTC
Created attachment 90596 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.

Test plan:

1. Add <mana_config>http://mana-test.koha-community.org</mana_config> to /etc/koha/sites/<instancename>/koha-conf.xml (see etc/koha-conf.xml in the Koha code repositoty for an example of where to add this; <instancename> if using koha-testing-docker is kohadev).
2. Clear memcached and restart services so that the changes in your Koha instance configuration are recognised (if using koha-testing-docker enter flush_memcached and then restart_all).
3. From the staff client home page go to Koha administration > Additional parameters > Share content with Mana KB.
4. Enable content sharing: change 'Use Mana KB for sharing content' to Yes and press Save.
5. Enter your first name, last name and email address in the Configure Mana KB section of the page and then press Send to Mana KB.
6. An error message is displayed, this may be something like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.'
7. Apply the patch (git so bz apply 23075).
8. Repeat steps 2,3 and 5.
9. A more friendly/human readable error message is now displayed: 'Error trying to connect or read response from the Mana server (Code: 301) Moved Permanently'.

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Nick Clemens 2019-06-14 17:17:42 UTC
Created attachment 90606 [details] [review]
Bug 23075: Better error handling in Mana config

Currently, if you input an incorrect mana service URL in your config
(http rather than https for example) the error message you are given is
the direct output of a failed json parse.  We should be able to catch
such failures and display a more meaningful error to the end user.

This patch makes it display a more friendly message.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Martin Renvoize 2019-06-15 07:03:01 UTC
Nice work!

Pushed to master for 19.11.00
Comment 17 Martin Renvoize 2019-06-17 15:52:58 UTC
Oops, this one slipped passed.

I'm going to revert as I don't think the solution here is tangible unfortunately.

The Mana server itself may return codes that are still meaningful even if they're not 200.. i.e. 201 has a meaning of it's own and as we're short-circuiting anything that's not 200 in this patch we'll be dropping OK responses of kinds.

I have a plan and will post an alternative shortly.
Comment 18 Martin Renvoize 2019-06-17 17:27:46 UTC
Created attachment 90682 [details] [review]
Bug 23075: Report if mana-kb server is not found

This patch adds a status check into the mana configuration page and
prompts the user to ask an adminstrator to check the configured mana
service url.
Comment 19 Martin Renvoize 2019-06-17 17:30:44 UTC
I've submitted an alternative resolution to this bug above.

This alternative checks for a known response for a mana server to identify if the URL appears to be correct.  It relies on bug 22913 at the mana-kb serverside to match this known state..
Comment 20 David Nind 2019-10-03 19:14:25 UTC
Patch now fails to apply.
Comment 21 Jonathan Druart 2019-10-14 13:26:41 UTC
Created attachment 94098 [details] [review]
Bug 23075: Report if mana-kb server is not found

This patch adds a status check into the mana configuration page and
prompts the user to ask an adminstrator to check the configured mana
service url.
Comment 22 David Nind 2019-10-15 10:42:25 UTC
Still getting message like 'malformed JSON string, neither tag, array, object, number, string or atom, at character offset 0 (before "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' when URL is incorrect. (have run flush_memcached and restart_all).
Comment 23 Jonathan Druart 2019-10-15 12:56:05 UTC
Confirmed. Maybe a change server-side?

$result is:

\ {
    code          200,
    description   "Mana KB is a global knowledge base for library-centric data.
                        It has been designed initially to interact with Koha, the Open
                        Source ILS, but can be used by any other software.",
    title         "Mana KB API",
    version       0.1
}
Comment 24 Martin Renvoize 2019-10-17 08:01:16 UTC
(In reply to David Nind from comment #22)
> Still getting message like 'malformed JSON string, neither tag, array,
> object, number, string or atom, at character offset 0 (before
> "<html>\r\n<head><tit...") at /usr/share/perl5/JSON.pm line 171.' when URL
> is incorrect. (have run flush_memcached and restart_all).

Hmm, where are you seeing this exactly.. I'm struggling to replicate it
Comment 25 Martin Renvoize 2019-10-17 08:02:22 UTC
Created attachment 94363 [details]
screenshot

This is what I see with the patch applied.
Comment 26 Jonathan Druart 2019-10-17 08:55:34 UTC
(In reply to Martin Renvoize from comment #25)
> Created attachment 94363 [details]
> screenshot
> 
> This is what I see with the patch applied.

Try with http://mana-test.koha-community.org
Comment 27 Martin Renvoize 2019-11-01 11:42:45 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Martin Renvoize from comment #25)
> > Created attachment 94363 [details]
> > screenshot
> > 
> > This is what I see with the patch applied.
> 
> Try with http://mana-test.koha-community.org

But that is a 'good' URL.. so I don't expect to see any errors..
Comment 28 David Nind 2019-11-01 19:41:52 UTC
The https version works fine, the http version produces errors as it is not a 'valid' URL.
Comment 29 Katrin Fischer 2019-11-03 19:07:14 UTC
Failing because of David's last comment, can you have a look Martin?
Comment 30 Martin Renvoize 2019-11-04 11:21:17 UTC
Still confused.. it should warn you when using http and not https as that's an invalid mana url.

http://neverssl.com/mana-test.koha-community.org
Comment 31 Jonathan Druart 2019-11-04 11:38:19 UTC
What I see https://snipboard.io/bWxT4B.jpg

with <mana_config>http://mana-test.koha-community.org</mana_config>
Comment 32 Martin Renvoize 2019-11-04 12:08:23 UTC
Created attachment 95000 [details] [review]
Bug 23075: Report if mana-kb server is not found

This patch adds a status check into the mana configuration page and
prompts the user to ask an adminstrator to check the configured mana
service url.
Comment 33 Martin Renvoize 2019-11-04 12:14:53 UTC
Pushed yet another rebased version.. but nothing has actually changed in the code..

I can't reproduce your issues, and can't see in the code how you would get that either. :(
Comment 34 Martin Renvoize 2019-11-04 13:16:39 UTC
Created attachment 95003 [details] [review]
Bug 23075: (follow-up) Dissallow transparent redirects

GET request redirects were causing the http URL to 'pass' our 'good url'
test for http => https redirected mana servers. However, on the
subsquent POST request to get a mana security token we would fail as the
POST was not transparently redirected.

This patch changes the request to be a simple_request so we will catch
bad urls earlier and it also prevents submissions for security tokens if
the configured URL is identified as bad.
Comment 35 Martin Renvoize 2019-11-04 13:17:33 UTC
Bad to needs signoff.. after a long chat with Jonathan on IRC I managed to replicate it.
Comment 36 Jonathan Druart 2019-11-04 14:10:55 UTC
Created attachment 95009 [details] [review]
Bug 23075: Do not 'Send' if the url is not correct

Just in case.
Comment 37 David Nind 2019-11-05 19:51:51 UTC
Created attachment 95074 [details] [review]
Bug 23075: Report if mana-kb server is not found

This patch adds a status check into the mana configuration page and
prompts the user to ask an adminstrator to check the configured mana
service url.

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2019-11-05 19:51:54 UTC
Created attachment 95075 [details] [review]
Bug 23075: (follow-up) Dissallow transparent redirects

GET request redirects were causing the http URL to 'pass' our 'good url'
test for http => https redirected mana servers. However, on the
subsquent POST request to get a mana security token we would fail as the
POST was not transparently redirected.

This patch changes the request to be a simple_request so we will catch
bad urls earlier and it also prevents submissions for security tokens if
the configured URL is identified as bad.

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2019-11-05 19:51:58 UTC
Created attachment 95076 [details] [review]
Bug 23075: Do not 'Send' if the url is not correct

Just in case.

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2019-11-05 19:53:48 UTC
Thanks Jonathan and Martin for your perseverance in resolving this!

Message if an incorrect URL in config now says 'Cannot detect mana server at http://mana-test.koha-community.org, please ask an administrator to check your configuration.'
Comment 41 Jonathan Druart 2019-11-06 08:50:47 UTC
Created attachment 95078 [details] [review]
Bug 23075: Report if mana-kb server is not found

This patch adds a status check into the mana configuration page and
prompts the user to ask an adminstrator to check the configured mana
service url.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2019-11-06 08:50:51 UTC
Created attachment 95079 [details] [review]
Bug 23075: (follow-up) Dissallow transparent redirects

GET request redirects were causing the http URL to 'pass' our 'good url'
test for http => https redirected mana servers. However, on the
subsquent POST request to get a mana security token we would fail as the
POST was not transparently redirected.

This patch changes the request to be a simple_request so we will catch
bad urls earlier and it also prevents submissions for security tokens if
the configured URL is identified as bad.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2019-11-06 08:50:55 UTC
Created attachment 95080 [details] [review]
Bug 23075: Do not 'Send' if the url is not correct

Just in case.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2019-11-06 08:50:59 UTC
Created attachment 95081 [details] [review]
Bug 23075: Correct indendation

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Martin Renvoize 2019-11-06 17:10:15 UTC
Nice work!

Pushed to master for 19.11.00