From 918717e80d19e52d32c03c4a4bc5345f724f0177 Mon Sep 17 00:00:00 2001
From: Guillaume Rousse <guillaume.rousse@renater.fr>
Date: Thu, 23 May 2024 15:14:21 +0200
Subject: [PATCH] split step3 into two distincts steps: challenge sending and
 challenge validation

---
 lib/AccessCheck/App.pm                        |  3 +-
 .../App/{Step3.pm => SendChallenge.pm}        | 22 +----
 lib/AccessCheck/App/ValidateChallenge.pm      | 66 +++++++++++++++
 lib/Makefile.am                               |  3 +-
 t/app.t                                       | 84 ++++++++++++++++---
 templates/web/edugain/step2.html.tt2          |  2 +-
 ...3.html.tt2 => validate_challenge.html.tt2} |  0
 templates/web/renater/step2.html.tt2          |  2 +-
 ...3.html.tt2 => validate_challenge.html.tt2} |  0
 9 files changed, 148 insertions(+), 34 deletions(-)
 rename lib/AccessCheck/App/{Step3.pm => SendChallenge.pm} (89%)
 create mode 100644 lib/AccessCheck/App/ValidateChallenge.pm
 rename templates/web/edugain/{step3.html.tt2 => validate_challenge.html.tt2} (100%)
 rename templates/web/renater/{step3.html.tt2 => validate_challenge.html.tt2} (100%)

diff --git a/lib/AccessCheck/App.pm b/lib/AccessCheck/App.pm
index 350f4ce..16cfbc1 100644
--- a/lib/AccessCheck/App.pm
+++ b/lib/AccessCheck/App.pm
@@ -90,7 +90,8 @@ sub startup {
     $routes->get('/status')->to(controller => 'status', action => 'run')->name('status');
     $routes->get('/step1')->to(controller => 'step1', action => 'run')->name('step1');
     $routes->get('/step2')->to(controller => 'step2', action => 'run')->name('step2');
-    $routes->get('/step3')->to(controller => 'step3', action => 'run')->name('step3');
+    $routes->get('/send_challenge')->to(controller => 'send_challenge', action => 'run')->name('send_challenge');
+    $routes->get('/validate_challenge')->to(controller => 'validate_challenge', action => 'run')->name('validate_challenge');
     $routes->get('/step4')->to(controller => 'step4', action => 'run')->name('step4');
     $routes->get('/step5')->to(controller => 'step5', action => 'run')->name('step5');
 
diff --git a/lib/AccessCheck/App/Step3.pm b/lib/AccessCheck/App/SendChallenge.pm
similarity index 89%
rename from lib/AccessCheck/App/Step3.pm
rename to lib/AccessCheck/App/SendChallenge.pm
index b97bf07..1803516 100644
--- a/lib/AccessCheck/App/Step3.pm
+++ b/lib/AccessCheck/App/SendChallenge.pm
@@ -1,4 +1,4 @@
-package AccessCheck::App::Step3;
+package AccessCheck::App::SendChallenge;
 
 use Mojo::Base qw(AccessCheck::App::Controller);
 
@@ -119,7 +119,7 @@ sub run {
         sp            => { entityid => $entityid, },
         to            => $email,
         token         => $token->secret(),
-        challenge_url => $self->url_for('step3')->query(entityid => $entityid, email => $email, token => $self->csrf_token())->to_abs(),
+        challenge_url => $self->url_for('validate_challenge')->query(entityid => $entityid, email => $email)->to_abs(),
         lh            => $l10n
     };
     my $text_content;
@@ -173,22 +173,8 @@ sub run {
         )
     );
 
-    my $profiles = $base_templates_dir
-        ->child('accounts')
-        ->list()
-        ->map(sub { m/([^\/]+).tt2$/})
-        ->to_array();
-
-    $self->stash(entityid => $entityid);
-    $self->stash(email    => $email);
-    $self->stash(validity => $config->{service}->{account_validity_period});
-    $self->stash(profiles => $profiles);
-
-    $self->render(
-        status   => 200,
-        template => 'step3',
-        format   => 'html'
-    );
+    $self->redirect_to('validate_challenge', email => $email, entityid => $entityid);
+
 }
 
 1;
diff --git a/lib/AccessCheck/App/ValidateChallenge.pm b/lib/AccessCheck/App/ValidateChallenge.pm
new file mode 100644
index 0000000..d0295d8
--- /dev/null
+++ b/lib/AccessCheck/App/ValidateChallenge.pm
@@ -0,0 +1,66 @@
+package AccessCheck::App::ValidateChallenge;
+
+use Mojo::Base qw(AccessCheck::App::Controller);
+
+use DateTime;
+use Email::MIME;
+use Email::Sender::Simple;
+use English qw(-no_match_vars);
+use Syntax::Keyword::Try;
+use Template::Constants qw(:chomp);
+
+use AccessCheck::Data::Token;
+use AccessCheck::Regexp;
+use AccessCheck::Tools;
+
+sub run {
+    my $self = shift;
+
+    my $app    = $self->app();
+    my $config = $app->config();
+    my $log    = $app->log();
+
+    my $l10n = $self->init_l10n();
+    my $user = $self->init_user();
+    my $db   = $self->init_db();
+
+    if ($config->{app}->{login_url}) {
+        return if !$self->check_authentication();
+    }
+
+    my $entityid = $self->param('entityid');
+    my $email    = $self->param('email');
+
+    my $sp = $self->get_sp(entityid => $entityid);
+    return if !$sp;
+
+    return $self->abort(
+        log_message  => "Missing parameter: email",
+        user_message => "missing_email"
+    ) if !$email;
+
+    return $self->abort(
+        log_message  => "Invalid parameter: email",
+        user_message => "invalid_email"
+    ) if $email !~ $AccessCheck::Regexp::email;
+
+    my $base_templates_dir = $self->app()->home()->child('templates');
+    my $profiles = $base_templates_dir
+        ->child('accounts')
+        ->list()
+        ->map(sub { m/([^\/]+).tt2$/})
+        ->to_array();
+
+    $self->stash(entityid => $entityid);
+    $self->stash(email    => $email);
+    $self->stash(validity => $config->{service}->{account_validity_period});
+    $self->stash(profiles => $profiles);
+
+    $self->render(
+        status   => 200,
+        template => 'validate_challenge',
+        format   => 'html'
+    );
+}
+
+1;
diff --git a/lib/Makefile.am b/lib/Makefile.am
index cce6d0d..9c90300 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -18,7 +18,8 @@ nobase_applib_DATA = \
 	AccessCheck/App/Status.pm \
 	AccessCheck/App/Step1.pm \
 	AccessCheck/App/Step2.pm \
-	AccessCheck/App/Step3.pm \
+	AccessCheck/App/SendChallenge.pm \
+	AccessCheck/App/ValidateChallenge.pm \
 	AccessCheck/App/Step4.pm \
 	AccessCheck/App/Step5.pm \
 	AccessCheck/Template/Plugin/Quote.pm
diff --git a/t/app.t b/t/app.t
index 1985c77..01fc8c1 100755
--- a/t/app.t
+++ b/t/app.t
@@ -20,7 +20,7 @@ plan(skip_all => 'live database required') unless
     $ENV{TEST_DB_NAME} &&
     $ENV{TEST_DB_TYPE};
 
-plan tests => 11;
+plan tests => 16;
 
 sub named_subtest {
     my ($name, $code, @args) = @_;
@@ -224,10 +224,10 @@ named_subtest "email selection page, valid entityid" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
-named_subtest "challenge page, missing CSRF token" => sub {
+named_subtest "challenge sending page, missing CSRF token" => sub {
     my $t = get_test_object(test => $_[0]);
 
-    $t->get_ok('/step3')
+    $t->get_ok('/send_challenge')
       ->status_is(403)
       ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
       ->content_like(qr/Error:[\n\s]+missing CSRF token/, 'expected error message');
@@ -236,13 +236,13 @@ named_subtest "challenge page, missing CSRF token" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
-named_subtest "challenge page, missing entityid" => sub {
+named_subtest "challenge sending page, missing entityid" => sub {
     my $t = get_test_object(test => $_[0]);
 
     # neutralize CSRF token check, as we short-circuit the form
     $t->app()->hook(before_dispatch => sub { $_[0]->session(csrf_token => 'foo') });
 
-    $t->get_ok('/step3' => form => {token => 'foo'})
+    $t->get_ok('/send_challenge' => form => {token => 'foo'})
       ->status_is(200)
       ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
       ->content_like(qr/Error:[\n\s]+missing parameter 'entityid'/, 'expected error message');
@@ -251,13 +251,13 @@ named_subtest "challenge page, missing entityid" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
-named_subtest "challenge page, invalid entityid" => sub {
+named_subtest "challenge sending page, invalid entityid" => sub {
     my $t = get_test_object(test => $_[0]);
 
     # neutralize CSRF token check, as we short-circuit the form
     $t->app()->hook(before_dispatch => sub { $_[0]->session(csrf_token => 'foo') });
 
-    $t->get_ok('/step3' => form => {token => 'foo', entityid => 'foo'})
+    $t->get_ok('/send_challenge' => form => {token => 'foo', entityid => 'foo'})
       ->status_is(200)
       ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
       ->content_like(qr/Error:[\n\s]+invalid parameter 'entityid'/, 'expected error message');
@@ -266,13 +266,13 @@ named_subtest "challenge page, invalid entityid" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
-named_subtest "challenge page, valid entityid, missing email" => sub {
+named_subtest "challenge sending page, valid entityid, missing email" => sub {
     my $t = get_test_object(test => $_[0]);
 
     # neutralize CSRF token check, as we short-circuit the form
     $t->app()->hook(before_dispatch => sub { $_[0]->session(csrf_token => 'foo') });
 
-    $t->get_ok('/step3' => form => {token => 'foo', entityid => 'https://sp.renater.fr/'})
+    $t->get_ok('/send_challenge' => form => {token => 'foo', entityid => 'https://sp.renater.fr/'})
       ->status_is(200)
       ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
       ->content_like(qr/Error:[\n\s]+missing parameter 'email'/, 'expected error message');
@@ -281,13 +281,13 @@ named_subtest "challenge page, valid entityid, missing email" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
-named_subtest "challenge page, valid entityid, invalid email" => sub {
+named_subtest "challenge sending page, valid entityid, invalid email" => sub {
     my $t = get_test_object(test => $_[0]);
 
     # neutralize CSRF token check, as we short-circuit the form
     $t->app()->hook(before_dispatch => sub { $_[0]->session(csrf_token => 'foo') });
 
-    $t->get_ok('/step3' => form => {token => 'foo', entityid => 'https://sp.renater.fr/', email => 'foo'})
+    $t->get_ok('/send_challenge' => form => {token => 'foo', entityid => 'https://sp.renater.fr/', email => 'foo'})
       ->status_is(200)
       ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
       ->content_like(qr/Error:[\n\s]+invalid parameter 'email'/, 'expected error message');
@@ -296,13 +296,73 @@ named_subtest "challenge page, valid entityid, invalid email" => sub {
     html_ok($res) or diag_file($res, $test_dir);
 };
 
+named_subtest "challenge validation page, missing entityid" => sub {
+    my $t = get_test_object(test => $_[0]);
+
+    $t->get_ok('/validate_challenge')
+      ->status_is(200)
+      ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
+      ->content_like(qr/Error:[\n\s]+missing parameter 'entityid'/, 'expected error message');
+
+    my $res = $t->tx()->res();
+    html_ok($res) or diag_file($res, $test_dir);
+};
+
+named_subtest "challenge validation page, invalid entityid" => sub {
+    my $t = get_test_object(test => $_[0]);
+
+    $t->get_ok('/validate_challenge' => form => {entityid => 'foo'})
+      ->status_is(200)
+      ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
+      ->content_like(qr/Error:[\n\s]+invalid parameter 'entityid'/, 'expected error message');
+
+    my $res = $t->tx()->res();
+    html_ok($res) or diag_file($res, $test_dir);
+};
+
+named_subtest "challenge validation page, valid entityid, missing email" => sub {
+    my $t = get_test_object(test => $_[0]);
+
+    $t->get_ok('/validate_challenge' => form => {entityid => 'https://sp.renater.fr/'})
+      ->status_is(200)
+      ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
+      ->content_like(qr/Error:[\n\s]+missing parameter 'email'/, 'expected error message');
+
+    my $res = $t->tx()->res();
+    html_ok($res) or diag_file($res, $test_dir);
+};
+
+named_subtest "challenge validation page, valid entityid, invalid email" => sub {
+    my $t = get_test_object(test => $_[0]);
+
+    $t->get_ok('/validate_challenge' => form => {entityid => 'https://sp.renater.fr/', email => 'foo'})
+      ->status_is(200)
+      ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
+      ->content_like(qr/Error:[\n\s]+invalid parameter 'email'/, 'expected error message');
+
+    my $res = $t->tx()->res();
+    html_ok($res) or diag_file($res, $test_dir);
+};
+
+named_subtest "challenge validation page, valid entityid, valid email" => sub {
+    my $t = get_test_object(test => $_[0]);
+
+    $t->get_ok('/validate_challenge' => form => {entityid => 'https://sp.renater.fr/', email => 'contact1@renater.fr'})
+      ->status_is(200)
+      ->text_is('html head title' => 'eduGAIN Access Check', 'expected title')
+      ->text_is('div.content h2'  => 'Complete email challenge', 'expected form title');
+
+    my $res = $t->tx()->res();
+    html_ok($res) or diag_file($res, $test_dir);
+};
+
 named_subtest "index page, french version" => sub {
     my $t = get_test_object(test => $_[0], language => 'fr-FR,fr;q=0.9');
 
     $t->get_ok('/')
       ->status_is(200)
       ->text_is('html head title' => 'eduGAIN Access Check')
-      ->text_is('a[href=/step1]'  => 'Commencer', 'get started button');
+      ->text_is('html body a[href=/step1]'  => 'Commencer', 'get started button');
 
     my $res = $t->tx()->res();
     html_ok($res) or diag_file($res, $test_dir);
diff --git a/templates/web/edugain/step2.html.tt2 b/templates/web/edugain/step2.html.tt2
index 862d5e6..c544c15 100644
--- a/templates/web/edugain/step2.html.tt2
+++ b/templates/web/edugain/step2.html.tt2
@@ -1,5 +1,5 @@
 [% WRAPPER index.html.tt2 %]
-<form class="wizard clearfix" action="[% c.url_for('step3') %]" method="get">
+<form class="wizard clearfix" action="[% c.url_for('send_challenge') %]" method="get">
     <input type="hidden" name="token" value="[% c.csrf_token() %]">
     <div class="steps clearfix">
         <ol>
diff --git a/templates/web/edugain/step3.html.tt2 b/templates/web/edugain/validate_challenge.html.tt2
similarity index 100%
rename from templates/web/edugain/step3.html.tt2
rename to templates/web/edugain/validate_challenge.html.tt2
diff --git a/templates/web/renater/step2.html.tt2 b/templates/web/renater/step2.html.tt2
index 12e6860..cd73dfc 100644
--- a/templates/web/renater/step2.html.tt2
+++ b/templates/web/renater/step2.html.tt2
@@ -1,5 +1,5 @@
 [% WRAPPER index.html.tt2 %]
-<form class="wizard clearfix" action="[% c.url_for('step3') %]" method="get">
+<form class="wizard clearfix" action="[% c.url_for('send_challenge') %]" method="get">
     <input type="hidden" name="token" value="[% c.csrf_token() %]">
     <div class="steps clearfix">
         <ol>
diff --git a/templates/web/renater/step3.html.tt2 b/templates/web/renater/validate_challenge.html.tt2
similarity index 100%
rename from templates/web/renater/step3.html.tt2
rename to templates/web/renater/validate_challenge.html.tt2
-- 
GitLab