Bug 17050 - Accessing the REST API through Plack kicks the session out
Summary: Accessing the REST API through Plack kicks the session out
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 17030
Blocks: 14868
  Show dependency treegraph
 
Reported: 2016-08-04 15:03 UTC by Tomás Cohen Arazi
Modified: 2022-11-11 10:32 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17050: Do not kicks the session out when accessing the REST API (3.48 KB, patch)
2016-08-05 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17050: Do not kick the session out when accessing the REST API (3.53 KB, patch)
2016-08-19 18:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17050: (QA followup) Use Mojo::Transaction to get the remote address (2.21 KB, patch)
2016-08-19 18:12 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17050: Do not kick the session out when accessing the REST API (3.60 KB, patch)
2016-08-20 23:20 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17050: (QA followup) Use Mojo::Transaction to get the remote address (2.28 KB, patch)
2016-08-20 23:20 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17050: Do not kick the session out when accessing the REST API (2.66 KB, patch)
2016-08-22 11:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17050: (QA followup) Use Mojo::Transaction to get the remote address (2.35 KB, patch)
2016-08-22 11:35 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 Tomás Cohen Arazi 2016-08-04 15:03:18 UTC
While working on bug 17030 for integrating the REST API on the packages ot make it finally available, I found that the session is lost when Plack is used to access it.

To reproduce:
- Follow the steps for the Plack patch in bug 17030
=> FAIL: Notice no matter what permissions your user has you get:
{ "error": "You don't have the required permission" }
- On a separate tab, notice you've been kicked out from Koha.

This doesn't happen with Plack disabled.
Comment 1 Benjamin Rokseth 2016-08-04 19:17:43 UTC
Just a sidenote: We experienced the same issue in our plack+REST setup, and found out it was due to a syspref: SessionRestrictionByIP

When we disabled this (enabled by default) the problem went away.
Don't know if it is related, though.
Comment 2 Jonathan Druart 2016-08-05 12:26:59 UTC
(In reply to Benjamin Rokseth from comment #1)
> Just a sidenote: We experienced the same issue in our plack+REST setup, and
> found out it was due to a syspref: SessionRestrictionByIP
> 
> When we disabled this (enabled by default) the problem went away.
> Don't know if it is related, though.

Confirmed!
Comment 3 Jonathan Druart 2016-08-05 15:09:08 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-08-05 15:11:46 UTC
Amended test plan:
0/ Switch SessionRestrictionByIP on!
Comment 5 Benjamin Rokseth 2016-08-11 08:28:26 UTC
I tested this patch in our deployment setup (using docker containers), and experienced the following:

Every second request invalidated/deleted the session. Seemingly the syspref cache was emptied also. Enabling/disabling the SessionRestrictedByIp had no effect, as it seemed the Context was gone. Really weird.

I will have to see if I can reproduce this.
Comment 6 Jonathan Druart 2016-08-11 09:08:34 UTC
(In reply to Benjamin Rokseth from comment #5)
> I tested this patch in our deployment setup (using docker containers), and
> experienced the following:
> 
> Every second request invalidated/deleted the session. Seemingly the syspref
> cache was emptied also. Enabling/disabling the SessionRestrictedByIp had no
> effect, as it seemed the Context was gone. Really weird.
> 
> I will have to see if I can reproduce this.

What's the value of SessionStorage?
Comment 7 Benjamin Rokseth 2016-08-11 09:12:58 UTC
I believe it was 'mysql'
Comment 8 Jonathan Druart 2016-08-11 09:28:26 UTC
(In reply to Benjamin Rokseth from comment #7)
> I believe it was 'mysql'

So should be ok. Let me know if you have steps to recreate the problem.
Comment 9 Benjamin Rokseth 2016-08-16 22:24:09 UTC
So I narrowed down the problem:

session is created by a simple auth session module we wrote largely mimicking svc/authentication.

Noticed that the $ip param is missing from the session, and neither ip nor _SESSION_REMOTE_ADDR is present in the mysql session storage.

So I guess svc/authentication and other CGI scripts pass $ip somehow or generates by ENV{REMOTE_ADDR}, and we will have to adjust to that. Or revert to use svc/authentication. Or address some other API authentication bugs.
Comment 10 Tomás Cohen Arazi 2016-08-16 22:44:49 UTC
Jonathan, I inserted my infamous Plack::Middleware::Tomas that just prints env, and interestingly:

 {
    HTTP_ACCEPT                      "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8",
    HTTP_ACCEPT_ENCODING             "gzip, deflate",
    HTTP_ACCEPT_LANGUAGE             "en-US,en;q=0.5",
    HTTP_COOKIE                      "KohaOpacLanguage=en; CGISESSID=59977e155eba3200f39ba4b1cb23efec",
    HTTP_DNT                         1,
    HTTP_HOST                        "localhost:8081",
    HTTP_REFERER                     "http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=51",
    HTTP_UPGRADE_INSECURE_REQUESTS   1,
    HTTP_USER_AGENT                  "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0",
    HTTP_X_FORWARDED_FOR             "10.0.2.2",
    HTTP_X_FORWARDED_HOST            "localhost:8081",
    HTTP_X_FORWARDED_SERVER          "kohadev-intra.myDNSname.org",
    PATH_INFO                        "/intranet/mainpage.pl",
    psgi.errors                      *main::STDERR  (write-only, flags: append, layers: unix perlio),
    psgi.input                       *Starman::Server::$io  (layers: scalar),
    psgi.multiprocess                1,
    psgi.multithread                 "",
    psgi.nonblocking                 "",
    psgi.run_once                    "",
    psgi.streaming                   1,
    psgi.url_scheme                  "http",
    psgi.version                     [
        [0] 1,
        [1] 1
    ],
    psgix.harakiri                   1,
    psgix.input.buffered             1,
    psgix.io                         *Symbol::GEN41  (read/write, layers: unix perlio),
    QUERY_STRING                     "",
    REMOTE_ADDR                      "10.0.2.2",
    REMOTE_HOST                      undef,
    REMOTE_PORT                      0,
    REQUEST_METHOD                   "GET",
    REQUEST_URI                      "/intranet/mainpage.pl",
    SCRIPT_NAME                      "",
    SERVER_NAME                      0,
    SERVER_PORT                      8081,
    SERVER_PROTOCOL                  "HTTP/1.1"
}

so HTTP_X_FORWARDED_FOR and REMOTE_ADDR have the right values. Are you implying that Mojo is messing with ENV?
Comment 11 Jonathan Druart 2016-08-18 13:39:03 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> so HTTP_X_FORWARDED_FOR and REMOTE_ADDR have the right values. Are you
> implying that Mojo is messing with ENV?

ENV is correctly set if you hit mainpage, but not /api/*
Comment 12 Tomás Cohen Arazi 2016-08-19 18:11:54 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2016-08-19 18:12:03 UTC Comment hidden (obsolete)
Comment 14 Benjamin Rokseth 2016-08-20 23:20:41 UTC Comment hidden (obsolete)
Comment 15 Benjamin Rokseth 2016-08-20 23:20:50 UTC Comment hidden (obsolete)
Comment 16 Benjamin Rokseth 2016-08-20 23:23:30 UTC
Good Mojo, Jonathan & Tomas! Working now as intended.
Comment 17 Jonathan Druart 2016-08-22 11:35:10 UTC
Created attachment 54679 [details] [review]
Bug 17050: Do not kick the session out when accessing the REST API

Mojolicious does not set $ENV{REMOTE_ADDR} (neither $ENV{HTTP_*}) as
it may share ENV between different requests.
Fortunately for us, Plack does not!

This is a dirty patch to fix this issue but it seems that there is not
lot of solutions. It adds a remote_addr parameter to
C4::Auth::check_cookie_authin order to send it from
Koha::Rest::V1::startup reading the headers sent by Mojolicious.

Test plan:
Hit /cgi-bin/koha/mainpage.pl
Hit /api/v1/patrons/42
Hit /cgi-bin/koha/mainpage.pl

With this patch applied, everything will be fine and you won't be
logged out.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2016-08-22 11:35:15 UTC
Created attachment 54680 [details] [review]
Bug 17050: (QA followup) Use Mojo::Transaction to get the remote address

While the original patch fixes the issue, reading at Mojolicious source code, revealed
that Mojo::Transaction already has proper processing for detecting the remote address:

sub remote_address {
  my $self = shift;

  return $self->original_remote_address(@_) if @_;
  return $self->original_remote_address unless $self->req->reverse_proxy;

  # Reverse proxy
  return ($self->req->headers->header('X-Forwarded-For') // '') =~ /([^,\s]+)$/
    ? $1
    : $self->original_remote_address;
}

Without this patch, if there's a request without HTTP_X_FORWARDED_FOR, then the remote
address would be empty. Such would be the case of a dev/standard setup without Plack.

This patch makes Koha::REST::V1::startup use tx->remote_address instead of dealing with the
headers on its own.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Kyle M Hall 2016-08-23 13:44:51 UTC
Pushed to master for 16.11, thanks Jonathan, Tomas!
Comment 20 Andrew Isherwood 2019-03-04 15:46:08 UTC
I've been facing this problem, or something very similar, today.

This is on 18.11.03.000 with Plack and memcached enabled and with the SessionRestrictionByIP syspref enabled. I am finding that API requests that require authorisation are causing the session to time out. This manifests itself as the API request returning a 401, with the body {"error":"Session has been expired."}.

After doing some digging, the "timeouts" happen because the condition on this line fails:

https://github.com/PTFS-Europe/koha/blob/18.11.03/C4/Auth.pm#L912

Specifically, $ip is coming back from the session store as 127.0.0.1 and $ENV{'REMOTE_ADDR'} is the true client IP, so they do not match and the condition fails.

So, it seems to me that when the session is created, for some reason 127.0.0.1 is being stored as the remote IP. Doing some research, the "acknowledged" way of ensuring the remote IP passes through an Apache proxy cleanly is:

ProxyPreserveHost On

However, including this in the Apache config doesn't resolve the problem. 

Anyone got any ideas?
Comment 21 Jonathan Druart 2019-06-23 00:08:30 UTC
Andrew, please open a new bug report if the issue remains.
Comment 22 Andrew Isherwood 2019-07-18 11:40:14 UTC
Just in case anybody in the future runs into this problem. I encountered it again today and determined that it was Postman, my REST client, that was at fault. I'm not sure what it was doing that was causing my IP to be set as 127.0.0.1, but switching to Insomnia as a client fixed it for me.