|
Description
David Cook
2018-07-03 08:35:19 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) 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? (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. Is there a reason for only accepting v0 or v1 in the RewriteCond? Especially since the rule uses 0-9? (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. 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> Thanks for the sign off! 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 :)
(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? (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 :) (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. I haven't forgotten but I've been totally snowed under by other things. Will get to this when I get a chance... 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.
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. 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. (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! Patch pushed to master. Thanks! Pushed to 18.05.x for 18.05.03. 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; |