Bug 21031

Summary: Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration
Product: Koha Reporter: David Cook <dcook>
Component: REST APIAssignee: David Cook <dcook>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: ere.maijala, fridolin.somers, J.P.Knight, jonathan.druart, m.de.rooy, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18175
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration
Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration
Apache_rewrite_log.txt
Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration

Description David Cook 2018-07-03 08:35:19 UTC
If you install Koha using the Debian packages and you enable Plack, you should be able to reach the API using the following URL: "/api/v1/"

If you install Koha using the Debian packages and you don't enable Plack (or you install from source or you're using a git install or whatever else), you can only get the same API response by visiting "/api/v1/app.pl/api/v1/spec"

It appears to me that the Rewrite rules in etc/koha-httpd.conf, debian/templates/apache-shared-opac.conf, and debian/templates/apache-shared-intranet.conf don't work. 

Fixing this doesn't seem quite as easy as just copying over the Apache configuration for Plack-enabled instances, unfortunately, but I'm looking at this...
Comment 1 David Cook 2018-07-03 09:08:25 UTC
Created attachment 76645 [details] [review]
Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration

The Rewrite rules for Apache don't work unless you're using
debian/templates/apache-shared-opac-plack.conf or
debian/templates/apache-shared-intranet-plack.conf.

This patch fixes the Rewrite rules for the non-Plack Debian
Apache configuration templates as well as the standard
Apache configuration file that comes with Koha.

__BEFORE APPLYING__
1. Visit /api/v1/app.pl/api/v1/spec on your git dev install
2. This should display a large page of JSON
3. Visit /api/v1/spec on your git dev install
4. This should generate a 404 error

__APPLY PATCH__

__AFTER APPLYING__
5. Visit /api/v1/app.pl/api/v1/spec on your git dev install
6. This should display a large page of JSON
7. Visit /api/v1/spec on your git dev install
8. This should display a large page of JSON (identical to
the one from earlier steps)
Comment 2 Jon Knight 2018-07-03 17:34:41 UTC
On my kohadevbox I seem to get 404 errors for all the URLs in the test plan? Do I need to turn something on first?  Going to an known API end point seems to work - for example

http://localhost:8080/api/v1/app.pl/api/v1/patrons/50

or

http://localhost:8080/api/v1/patrons/50

I'd sort of assumed that kohadevbox is a git install, so should be able to test this?
Comment 3 David Cook 2018-07-04 04:09:50 UTC
(In reply to Jon Knight from comment #2)
> On my kohadevbox I seem to get 404 errors for all the URLs in the test plan?
> Do I need to turn something on first?  Going to an known API end point seems
> to work - for example
> 
> http://localhost:8080/api/v1/app.pl/api/v1/patrons/50
> 
> or
> 
> http://localhost:8080/api/v1/patrons/50
> 
> I'd sort of assumed that kohadevbox is a git install, so should be able to
> test this?

Hi Jon,

Thanks for taking the time to try and test this out!

I think the kohadevbox can't be used for testing this patch though. The kohadevbox is actually a package install which gets converted into using git, but it's not a git install per se.

kohadevbox uses koha-gitify which re-writes the installed stable package Apache configuration, so using kohadevbox you're not actually testing these patches, unfortunately.
Comment 4 Ere Maijala 2018-07-05 06:15:44 UTC
Is there a reason for only accepting v0 or v1 in the RewriteCond? Especially since the rule uses 0-9?
Comment 5 David Cook 2018-07-05 08:17:51 UTC
(In reply to Ere Maijala from comment #4)
> Is there a reason for only accepting v0 or v1 in the RewriteCond? Especially
> since the rule uses 0-9?

Very good question. I was just keeping it consistent with what's in "debian/templates/apache-shared-opac-plack.conf" and "debian/templates/apache-shared-intranet-plack.conf".

I imagine they all should be updated to 0-9 for the RewriteCond too.
Comment 6 Ere Maijala 2018-07-06 11:05:22 UTC
Created attachment 76725 [details] [review]
Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration

The Rewrite rules for Apache don't work unless you're using
debian/templates/apache-shared-opac-plack.conf or
debian/templates/apache-shared-intranet-plack.conf.

This patch fixes the Rewrite rules for the non-Plack Debian
Apache configuration templates as well as the standard
Apache configuration file that comes with Koha.

__BEFORE APPLYING__
1. Visit /api/v1/app.pl/api/v1/spec on your git dev install
2. This should display a large page of JSON
3. Visit /api/v1/spec on your git dev install
4. This should generate a 404 error

__APPLY PATCH__

__AFTER APPLYING__
5. Visit /api/v1/app.pl/api/v1/spec on your git dev install
6. This should display a large page of JSON
7. Visit /api/v1/spec on your git dev install
8. This should display a large page of JSON (identical to
the one from earlier steps)

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>
Comment 7 David Cook 2018-07-12 07:43:55 UTC
Thanks for the sign off!
Comment 8 Marcel de Rooy 2018-07-13 09:30:07 UTC
From the plack counterpart:
        RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
        RewriteRule ^/api/(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L,PT]

From your patch:
    RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
    RewriteRule ^(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L]

Not the same. You start with v[0-9] which seems to be wrong ;)
How did you test?

Note btw that v[0-9]+ may not be what we want. It covers v987654321 for instance. We do not have v0, but only v1. Hoping for v2 :)
Comment 9 David Cook 2018-07-16 00:22:28 UTC
(In reply to Marcel de Rooy from comment #8)
> From the plack counterpart:
>         RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
>         RewriteRule ^/api/(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L,PT]
> 
> From your patch:
>     RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
>     RewriteRule ^(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L]
> 
> Not the same. You start with v[0-9] which seems to be wrong ;)
> How did you test?
> 

It might seem wrong but it's not wrong ;). 

The reason is that these directives are happening in a Directory block with a RewriteBase of /api/ (note that in Apache 2.4 without an explicit RewriteBase an implicit one will be created anyway), so Apache strips off the "/api/" part after it matches the RewriteCond. You can check this using a logging directive like "LogLevel alert rewrite:trace3".

I tested by putting in the Apache config with the directives I've included in these patches along with the logging I mention above and I observed that my test plan worked. 

> Note btw that v[0-9]+ may not be what we want. It covers v987654321 for
> instance. We do not have v0, but only v1. Hoping for v2 :)

That may be true but I don't think that's within the scope of this bug report. This patch is just trying to bring the CGI config in line with the Plack config. 

I'd like to reset this status back to Signed Off. I can provide some logging output if that would be helpful in regards to the RewriteRule?
Comment 10 Marcel de Rooy 2018-07-16 07:40:22 UTC
(In reply to David Cook from comment #9)
> (In reply to Marcel de Rooy from comment #8)
> > From the plack counterpart:
> >         RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
> >         RewriteRule ^/api/(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L,PT]
> > 
> > From your patch:
> >     RewriteCond %{REQUEST_URI} !^/api/v[0-1]+/app.pl
> >     RewriteRule ^(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2 [L]
> > 
> > Not the same. You start with v[0-9] which seems to be wrong ;)
> > How did you test?
> > 
> 
> It might seem wrong but it's not wrong ;). 
> 
> The reason is that these directives are happening in a Directory block with
> a RewriteBase of /api/ (note that in Apache 2.4 without an explicit
> RewriteBase an implicit one will be created anyway), so Apache strips off
> the "/api/" part after it matches the RewriteCond. You can check this using
> a logging directive like "LogLevel alert rewrite:trace3".
> 
> I tested by putting in the Apache config with the directives I've included
> in these patches along with the logging I mention above and I observed that
> my test plan worked. 
> 
> > Note btw that v[0-9]+ may not be what we want. It covers v987654321 for
> > instance. We do not have v0, but only v1. Hoping for v2 :)
> 
> That may be true but I don't think that's within the scope of this bug
> report. This patch is just trying to bring the CGI config in line with the
> Plack config. 
> 
> I'd like to reset this status back to Signed Off. I can provide some logging
> output if that would be helpful in regards to the RewriteRule?
I did it for you now. Yeah, your log might be helpful to prove :)
Comment 11 David Cook 2018-07-16 08:30:05 UTC
(In reply to Marcel de Rooy from comment #10)
> I did it for you now. Yeah, your log might be helpful to prove :)

Thanks, Marcel. I'll redo my tests in the morning and copy some of the log output here.
Comment 12 David Cook 2018-07-19 07:59:17 UTC
I haven't forgotten but I've been totally snowed under by other things. Will get to this when I get a chance...
Comment 13 David Cook 2018-07-20 03:53:06 UTC
Created attachment 77148 [details]
Apache_rewrite_log.txt

Apologies for the delay on getting this out. 

As you can see, because we're in a directory ending in "api", the "api" gets stripped out of the uri:

[Fri Jul 20 13:43:01.358964 2018] [rewrite:trace1] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] pass through /api/v1/cities
[Fri Jul 20 13:43:01.359158 2018] [rewrite:trace3] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] strip per-dir prefix: /home/dcook/koha/api/v1/cities -> v1/cities
[Fri Jul 20 13:43:01.359189 2018] [rewrite:trace3] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] applying pattern '^(v[0-9]+)/(.*)$' to uri 'v1/cities'
[Fri Jul 20 13:43:01.359271 2018] [rewrite:trace2] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] rewrite 'v1/cities' -> '/api/v1/app.pl/api/v1/cities'
[Fri Jul 20 13:43:01.359302 2018] [rewrite:trace2] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] trying to replace prefix /home/dcook/koha/api/ with /api/
[Fri Jul 20 13:43:01.359334 2018] [rewrite:trace2] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] trying to replace context docroot /home/dcook/koha/api with context prefix /api
[Fri Jul 20 13:43:01.359353 2018] [rewrite:trace1] [pid 11341] mod_rewrite.c(477): [client 192.168.1.100:15501] 192.168.1.100 - - [192.168.1.200/sid#7f1ca31258b0][rid#7f1c9d3a50a0/initial] [perdir /home/dcook/koha/api/] internal redirect with /api/v1/app.pl/api/v1/cities [INTERNAL REDIRECT]

I admit that I don't 100% understand some of the later lines. I get the "strip per-dir prefix", "applying pattern", "rewrite" and "internal redirect", but I don't understand why "trying to replace prefix" and "trying to replace context docroot" happen. Maybe I need to increase the trace level to 5 from 3 - I don't know.

But yeah the important thing is that the RewriteRule for non-Plack Apache configuration shouldn't include the "api" path since the directory-level configuration means "api" gets stripped out of the incoming uri.
Comment 14 Marcel de Rooy 2018-07-27 09:29:35 UTC
Created attachment 77300 [details] [review]
Bug 21031 - Apache Rewrite rules don't work for API when using anything but Debian package Plack configuration

The Rewrite rules for Apache don't work unless you're using
debian/templates/apache-shared-opac-plack.conf or
debian/templates/apache-shared-intranet-plack.conf.

This patch fixes the Rewrite rules for the non-Plack Debian
Apache configuration templates as well as the standard
Apache configuration file that comes with Koha.

__BEFORE APPLYING__
1. Visit /api/v1/app.pl/api/v1/spec on your git dev install
2. This should display a large page of JSON
3. Visit /api/v1/spec on your git dev install
4. This should generate a 404 error

__APPLY PATCH__

__AFTER APPLYING__
5. Visit /api/v1/app.pl/api/v1/spec on your git dev install
6. This should display a large page of JSON
7. Visit /api/v1/spec on your git dev install
8. This should display a large page of JSON (identical to
the one from earlier steps)

Signed-off-by: Ere Maijala <ere.maijala@helsinki.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Passed QA with few notes posted separately to Bugzilla.
Comment 15 Marcel de Rooy 2018-07-27 09:39:52 UTC
QA Comment:
Thanks David. Improving parts not touched by most users (Debian/Plack).

Just a few remarks (close to nitpicking):
[1] A difference with the rule in shared-opac-plack is that you do not include the PT flag. Note that this is fine. You do not need the PT flag since you have the rule in a <Directory> section. (The PT flag is implied in per-directory contexts.)
[2] The implicit PT may explain why you got the two additional lines in the apache trace. Might be possible to finetune that, but needs more Apache background.
[3] The rule is a bit hybrid. As you explained, it is in the context of a RewriteBase with /api/. So paths are relative. But the replacement is not relative but absolute. No reason to change, but a relative replace without /api in front would have looked good to me too.
Comment 16 David Cook 2018-07-30 06:22:07 UTC
(In reply to Marcel de Rooy from comment #15)
> QA Comment:
> Thanks David. Improving parts not touched by most users (Debian/Plack).
> 
> Just a few remarks (close to nitpicking):
> [1] A difference with the rule in shared-opac-plack is that you do not
> include the PT flag. Note that this is fine. You do not need the PT flag
> since you have the rule in a <Directory> section. (The PT flag is implied in
> per-directory contexts.)
> [2] The implicit PT may explain why you got the two additional lines in the
> apache trace. Might be possible to finetune that, but needs more Apache
> background.
> [3] The rule is a bit hybrid. As you explained, it is in the context of a
> RewriteBase with /api/. So paths are relative. But the replacement is not
> relative but absolute. No reason to change, but a relative replace without
> /api in front would have looked good to me too.

Thanks for the notes! I appreciate the feedback and it gives me food for thought!
Comment 17 Tomás Cohen Arazi 2018-08-02 13:24:15 UTC
Patch pushed to master. Thanks!
Comment 18 Martin Renvoize 2018-08-03 14:25:00 UTC
Pushed to 18.05.x for 18.05.03.
Comment 19 Fridolin Somers 2018-09-21 10:02:08 UTC
Pushed to 17.11.x for 17.11.10

BTW here is the Nginx configuration for this rewrite :
rewrite ^/api/(v[0-9]+)/(.*)$ /api/$1/app.pl/api/$1/$2;