From 14cf90b000f2632c67d82409a2c2e3c1e9d12870 Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Fri, 17 Mar 2023 17:19:00 +0100 Subject: [PATCH 1/4] Factor all Request creations into a sub so that we can log it --- .../lib/Lemonldap/NG/Common/PSGI.pm | 16 ++++++++++++---- .../lib/Lemonldap/NG/Handler/Lib/PSGI.pm | 9 ++++----- .../lib/Lemonldap/NG/Handler/PSGI/Try.pm | 2 +- .../lib/Lemonldap/NG/Handler/Server.pm | 2 +- .../lib/Lemonldap/NG/Handler/Server/Nginx.pm | 6 ++---- .../lib/Lemonldap/NG/Handler/Server/Traefik.pm | 9 ++++----- 6 files changed, 24 insertions(+), 20 deletions(-) diff --git a/lemonldap-ng-common/lib/Lemonldap/NG/Common/PSGI.pm b/lemonldap-ng-common/lib/Lemonldap/NG/Common/PSGI.pm index 7207c4c878..a14839272d 100644 --- a/lemonldap-ng-common/lib/Lemonldap/NG/Common/PSGI.pm +++ b/lemonldap-ng-common/lib/Lemonldap/NG/Common/PSGI.pm @@ -239,8 +239,7 @@ sub abort { my ( $self, $err ) = @_; eval { $self->logger->error($err) }; return sub { - $self->sendError( Lemonldap::NG::Common::PSGI::Request->new( $_[0] ), - $err, 500 ); + $self->sendError( $self->newRequest( $_[0] ), $err, 500 ); }; } @@ -336,6 +335,16 @@ sub sendHtml { # Main method # ############### +sub newRequest { + my ( $self, $env ) = @_; + my $req = Lemonldap::NG::Common::PSGI::Request->new($env); + $self->logger->info( "New request " + . ref($self) . " " + . $req->method . " " + . $req->request_uri ); + return $req; +} + sub run { my ( $self, $args ) = @_; $args //= {}; @@ -349,8 +358,7 @@ sub run { sub _run { my $self = shift; return sub { - $self->_logAndHandle( - Lemonldap::NG::Common::PSGI::Request->new( $_[0] ) ); + $self->_logAndHandle( $self->newRequest( $_[0] ) ); }; } diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Lib/PSGI.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Lib/PSGI.pm index 8fb0cb0983..f1f7c474a3 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Lib/PSGI.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Lib/PSGI.pm @@ -50,8 +50,7 @@ sub _run { # Handle requests # Developers, be careful: Only this part is executed at each request return sub { - return $self->_logAuthTrace( - Lemonldap::NG::Common::PSGI::Request->new( $_[0] ) ); + return $self->_logAuthTrace( $self->newRequest( $_[0] ) ); }; } @@ -67,7 +66,7 @@ sub _run { # Handle unprotected requests return sub { - my $req = Lemonldap::NG::Common::PSGI::Request->new( $_[0] ); + my $req = $self->newRequest( $_[0] ); my $res = $self->_logAndHandle($req); push @{ $res->[1] }, $req->spliceHdrs; return $res; @@ -87,7 +86,7 @@ sub status { $self->logger->error($@) if ($@); } return sub { - my $req = Lemonldap::NG::Common::PSGI::Request->new( $_[0] ); + my $req = $self->newRequest( $_[0] ); $self->api->status($req); return [ 200, [ $req->spliceHdrs ], [ $req->{respBody} ] ]; }; @@ -105,7 +104,7 @@ sub reload { $self->logger->error($@) if ($@); } return sub { - my $req = Lemonldap::NG::Common::PSGI::Request->new( $_[0] ); + my $req = $self->newRequest( $_[0] ); $self->api->reload($req); return [ 200, [ $req->spliceHdrs ], [ $req->{respBody} ] ]; }; diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/PSGI/Try.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/PSGI/Try.pm index a1267ff0fe..393fa55f62 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/PSGI/Try.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/PSGI/Try.pm @@ -84,7 +84,7 @@ sub _run { my $self = shift; return sub { - my $req = Lemonldap::NG::Common::PSGI::Request->new( $_[0] ); + my $req = $self->newRequest( $_[0] ); my $res = $self->_logAuthTrace( $req, 1 ); if ( $res->[0] < 300 ) { $self->routes( $self->authRoutes ); diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server.pm index 0e04c9c0fa..dfc4392468 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server.pm @@ -24,7 +24,7 @@ sub init { sub _run { my ($self) = @_; return sub { - my $req = Lemonldap::NG::Common::PSGI::Request->new( $_[0] ); + my $req = $self->newRequest( $_[0] ); my $res = $self->_logAuthTrace($req); push @{ $res->[1] }, $req->spliceHdrs, Cookie => ( $req->{Cookie} // '' ); diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Nginx.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Nginx.pm index 6d5fa6872d..e059f3a677 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Nginx.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Nginx.pm @@ -29,10 +29,8 @@ sub init { sub _run { my $self = shift; return sub { - my $req = $_[0]; - $self->logger->debug('New request'); - my $res = $self->_logAuthTrace( - Lemonldap::NG::Common::PSGI::Request->new($req) ); + my $env = $_[0]; + my $res = $self->_logAuthTrace( $self->newRequest($env) ); # Transform 302 responses in 401 since Nginx refuse it if ( $res->[0] == 302 or $res->[0] == 303 ) { diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Traefik.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Traefik.pm index 11162a6e48..415ba5a0c9 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Traefik.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Server/Traefik.pm @@ -17,11 +17,10 @@ sub init { sub _run { my $self = shift; return sub { - my $req = $_[0]; - $req->{HTTP_HOST} = $req->{HTTP_X_FORWARDED_HOST}; - $req->{REQUEST_URI} = $req->{HTTP_X_FORWARDED_URI}; - return $self->_logAuthTrace( - Lemonldap::NG::Common::PSGI::Request->new($req) ); + my $env = $_[0]; + $env->{HTTP_HOST} = $env->{HTTP_X_FORWARDED_HOST}; + $env->{REQUEST_URI} = $env->{HTTP_X_FORWARDED_URI}; + return $self->_logAuthTrace( $self->newRequest($env) ); } } -- GitLab From fddd422833b687beb5c50d732a0372b3a30342da Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Fri, 17 Mar 2023 17:19:20 +0100 Subject: [PATCH 2/4] Log new requests on Apache2 handler --- lemonldap-ng-handler/lib/Lemonldap/NG/Handler/ApacheMP2.pm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/ApacheMP2.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/ApacheMP2.pm index af1fb70c99..12c12eb854 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/ApacheMP2.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/ApacheMP2.pm @@ -44,6 +44,9 @@ sub launch { eval "require $class"; die $@ if ($@); + $class->logger->info( + "New request $class " . $req->method . " " . $req->request_uri ); + # register the request object to the logging system if ( ref( $class->logger ) and $class->logger->can('setRequestObj') ) { $class->logger->setRequestObj($req); -- GitLab From 7f52c4eaf6581940d3c0fbdeb02ec0d07366cc7e Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Fri, 17 Mar 2023 17:21:00 +0100 Subject: [PATCH 3/4] Lower priority of "no cookie" message --- lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Main/Run.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Main/Run.pm b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Main/Run.pm index 0a521e9588..27e436cbd3 100644 --- a/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Main/Run.pm +++ b/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Main/Run.pm @@ -241,7 +241,7 @@ sub run { else { # Redirect user to the portal - $class->logger->info("No cookie found") + $class->logger->debug("No cookie found") unless ($id); # if the cookie was fetched, a log is sent by retrieveSession() -- GitLab From 9313b4500dfcda66a5f2d69ef84e5290922b59df Mon Sep 17 00:00:00 2001 From: Maxime Besson Date: Mon, 20 Mar 2023 18:11:51 +0100 Subject: [PATCH 4/4] Add unit tests for base PSGI functions This makes sure every code path that create a PSGI::Request object is tested --- lemonldap-ng-common/.proverc | 1 + lemonldap-ng-common/t/10-PSGI.pm | 28 ++++++++++++++++ lemonldap-ng-common/t/TestPsgi.pm | 32 ++++++++++++++++++ ...60-Lemonldap-NG-Handler-PSGI-unprotected.t | 30 +++++++++++++++++ .../t/61-Lemonldap-NG-Handler-PSGI-Server.t | 14 ++++++++ lemonldap-ng-handler/t/test-psgi-lib.pm | 33 ++++++++++++++----- 6 files changed, 129 insertions(+), 9 deletions(-) create mode 100644 lemonldap-ng-common/t/10-PSGI.pm create mode 100644 lemonldap-ng-common/t/TestPsgi.pm create mode 100644 lemonldap-ng-handler/t/60-Lemonldap-NG-Handler-PSGI-unprotected.t diff --git a/lemonldap-ng-common/.proverc b/lemonldap-ng-common/.proverc index 38e59cf1b7..c87aa36f3f 100644 --- a/lemonldap-ng-common/.proverc +++ b/lemonldap-ng-common/.proverc @@ -1 +1,2 @@ +-I . --blib diff --git a/lemonldap-ng-common/t/10-PSGI.pm b/lemonldap-ng-common/t/10-PSGI.pm new file mode 100644 index 0000000000..4fbd48b2bd --- /dev/null +++ b/lemonldap-ng-common/t/10-PSGI.pm @@ -0,0 +1,28 @@ +use Test::More; +use t::TestPsgi; +use Plack::Test; +use HTTP::Request; + +subtest "Check successful init" => sub { + my $server = Plack::Test->create( t::TestPsgi->run( {} ) ); + + my $req = HTTP::Request->new( GET => "/" ); + $req->header( Accept => "text/html" ); + my $res = $server->request($req); + is( $res->code, 200, "Returned HTTP code 200" ); + like( $res->content, qr/Body lines/, "Found expected message in body" ); +}; + +subtest "Check abort method" => sub { + my $server = Plack::Test->create( + t::TestPsgi->run( { error => "unit_test_failure" } ) ); + + my $req = HTTP::Request->new( GET => "/" ); + $req->header( Accept => "text/html" ); + my $res = $server->request($req); + is( $res->code, 500, "Returned HTTP code 500" ); + like( $res->content, qr/unit_test_failure/, + "Found expected error message in body" ); +}; + +done_testing(); diff --git a/lemonldap-ng-common/t/TestPsgi.pm b/lemonldap-ng-common/t/TestPsgi.pm new file mode 100644 index 0000000000..35ebcca2ff --- /dev/null +++ b/lemonldap-ng-common/t/TestPsgi.pm @@ -0,0 +1,32 @@ +package t::TestPsgi; + +use base Lemonldap::NG::Common::PSGI; + +sub init { + my ( $self, $args ) = @_; + + $args->{logLevel} = "error"; + my $super = $self->SUPER::init($args); + + no warnings 'redefine'; + eval +'sub Lemonldap::NG::Common::Logger::Std::error {return $_[0]->warn($_[1])}'; + + # Return a boolean. If false, then error message has to be stored in + if ( $args->{error} ) { + $self->error( $args->{error} ); + return 0; + } + return $super; +} + +sub handler { + my ( $self, $req ) = @_; + + # Do something and return a PSGI response + # NB: $req is a Lemonldap::NG::Common::PSGI::Request object + + return [ 200, [ 'Content-Type' => 'text/plain' ], ['Body lines'] ]; +} + +1; diff --git a/lemonldap-ng-handler/t/60-Lemonldap-NG-Handler-PSGI-unprotected.t b/lemonldap-ng-handler/t/60-Lemonldap-NG-Handler-PSGI-unprotected.t new file mode 100644 index 0000000000..abbca08b10 --- /dev/null +++ b/lemonldap-ng-handler/t/60-Lemonldap-NG-Handler-PSGI-unprotected.t @@ -0,0 +1,30 @@ +use Test::More; +use JSON; +use MIME::Base64; +use Data::Dumper; +use URI::Escape; + +require 't/test-psgi-lib.pm'; + +init( 'Lemonldap::NG::Handler::PSGI', { protection => 'none' } ); + +my $res; +my $SKIPUSER = 0; + +# Unauthentified query +# -------------------- +ok( $res = $client->_get('/'), 'Unauthentified query' ); +is( $res->[0], 200, "Unprotected request succeeds" ); +is( $res->[2]->[0], "Hello", "Expected content" ); +count(3); +done_testing( count() ); + +clean(); + +sub Lemonldap::NG::Handler::PSGI::handler { + my ( $self, $req ) = @_; + ok( !$req->env->{HTTP_AUTH_USER}, 'No HTTP_AUTH_USER' ) + or explain( $req->env->{HTTP_AUTH_USER}, '' ); + count(1); + return [ 200, [ 'Content-Type', 'text/plain' ], ['Hello'] ]; +} diff --git a/lemonldap-ng-handler/t/61-Lemonldap-NG-Handler-PSGI-Server.t b/lemonldap-ng-handler/t/61-Lemonldap-NG-Handler-PSGI-Server.t index be5865bbb3..680df06530 100644 --- a/lemonldap-ng-handler/t/61-Lemonldap-NG-Handler-PSGI-Server.t +++ b/lemonldap-ng-handler/t/61-Lemonldap-NG-Handler-PSGI-Server.t @@ -7,8 +7,22 @@ require 't/test-psgi-lib.pm'; init('Lemonldap::NG::Handler::Server'); +my $counter = ReloadCounter->new; +Lemonldap::NG::Handler::Main->onReload( $counter, 'doReload' ); + my $res; +is( $counter->counter, 0, "Check intial counter value" ); +ok( $res = $client->_get('/reload'), 'Reload query' ); +is( $res->[0], 200, "Reload returned 200" ); +is( $counter->counter, 1, "Reload handler was called" ); +count(4); + +ok( $res = $client->_get('/status'), 'Status query' ); +is( $res->[0], 200, "Status returned 200" ); +like( $res->[2]->[0], qr/Lemonldap::NG statistics/ ); +count(3); + # Unauthentified query # -------------------- ok( $res = $client->_get('/'), 'Unauthentified query' ); diff --git a/lemonldap-ng-handler/t/test-psgi-lib.pm b/lemonldap-ng-handler/t/test-psgi-lib.pm index ffdece4bdc..508a178532 100644 --- a/lemonldap-ng-handler/t/test-psgi-lib.pm +++ b/lemonldap-ng-handler/t/test-psgi-lib.pm @@ -5,6 +5,7 @@ use 5.10.0; use POSIX 'strftime'; use Data::Dumper; use Time::Fake; +use Plack::Builder; use_ok('Lemonldap::NG::Common::PSGI::Cli::Lib'); our $client; @@ -25,14 +26,15 @@ sub init { } $prms ||= {}; %$prms = ( - configStorage => { type => 'File', dirName => 't' }, - localSessionStorage => '', - logLevel => 'error', - cookieName => 'lemonldap', - securedCookie => 0, - https => 0, - hiddenAttributes => 'cn /^mail/', - logger => 'Lemonldap::NG::Common::Logger::Std', + configStorage => { type => 'File', dirName => 't' }, + localSessionStorage => '', + logLevel => 'error', + cookieName => 'lemonldap', + status => 1, + securedCookie => 0, + https => 0, + hiddenAttributes => 'cn /^mail/', + logger => 'Lemonldap::NG::Common::Logger::Std', %$prms ); ok( @@ -131,7 +133,11 @@ has app => ( is => 'ro', isa => 'CodeRef', builder => sub { - return $module->run( $_[0]->{ini} ); + my $builder = Plack::Builder->new; + $builder->mount( '/reload' => $module->reload( $_[0]->{ini} ) ); + $builder->mount( '/status' => $module->status( $_[0]->{ini} ) ); + $builder->mount( '/' => $module->run( $_[0]->{ini} ) ); + return $builder->to_app; } ); @@ -162,4 +168,13 @@ sub _get { ); } +package ReloadCounter; +use Mouse; +has counter => ( is => 'rw', default => 0 ); + +sub doReload { + my ($self) = @_; + $self->counter( $self->counter + 1 ); +} + 1; -- GitLab