Item6095: Registration confirmation page fails to display when user confirmation e-mail can't be sent

Problem with TWiki-4.2.3, Wed, 06 Aug 2008, build 17396, Plugin API version 1.2 (no checkbox, so I checked 4.2.2!)

Summary: Register.pm / Net.pm conspire such that if a registration specifies an invalid e-mail address, the user is registered but the "thanks" page is not displayed. Internet Exploder provides one of its cryptic "can't display page" errors.

Diagnosis: Inspection of the warn*.txt file shows that we have an error from Net:SMTP reporting "User unknown", with a die traceback. Because Net.pm/_sendEmailByNetSMTP prefixes the error with "ERROR:" (line 431), it passes the whole message, including traceback up to Register. (It should be blocked in Net.pm/sendEmail line 324.)

Register.pm/complete tries to report the error at line 830, but the oops page can't deal with all the cybercrud.

Work-around:

Make the code around line 830 in Register look like this (diff below):

 
       if( $status ) {
            $status = $session->i18n->maketext(
                'Warning: Could not send confirmation email')."\n\n$status";
            <font color="#dc143c">$session->writeDebug( "Registration failure:|$status|\n" );
            $status = $session->i18n->maketext(
                'A confirmation e-mail could not be sent to [_1]', $data->{Email} );</font>     
        } else {
            $status = $session->i18n->maketext(
                'A confirmation e-mail has been sent to [_1]', $data->{Email} );
        }

A real fix is more involved - I'll leave that to the code owner.

Issues to be addressed:

1) Policy: If the e-mail can't be sent, the address is quite possibly fake. One could argue that the user should be removed, as registering with an invalid e-mail address is anti-social -- and can cause audit issues for account traceability.

2) The traceback should be suppressed unless debugging is on. It's useless to most people, and provides a quick way for someone to create large logfiles.

3) The "=~ /^ERROR" hack probably should check that each line is prefixed with the ERROR flag -- or _sendEmailByNetSMTP should be more careful

4) _emailRegistrationConfirmations really should be telling the admin that the user supplied an inoperative e-mail. The actual error (but not the traceback) would be useful.

Sample case:

register user FakeTestUser, email address faker@example.com

Traceback from warn.txt*

| 23 Oct 2008 - 07:26 | ERROR: Can't send mail using Net::SMTP. 5.1.1 faker@example.com... User unknown
 at /var/www/twiki/lib/TWiki/Net.pm line 438
        TWiki::Net::_sendEmailByNetSMTP('TWiki::Net=HASH(0x9a826b4)', 'Date: Thu, 23 Oct 2008 11:26:49 GMT\x{a}From: TWiki Administrator...') called at /var/www/twiki/lib/TWiki/Net.pm line 316
        TWiki::Net::__ANON__() called at /usr/lib/perl5/site_perl/5.8.8/Error.pm line 426
        eval {...} called at /usr/lib/perl5/site_perl/5.8.8/Error.pm line 418
        Error::subs::try('CODE(0x9ad1c54)', 'HASH(0x9ba9e8c)') called at /var/www/twiki/lib/TWiki/Net.pm line 332
        TWiki::Net::sendEmail('TWiki::Net=HASH(0x9a826b4)', 'From: TWiki Administrator webmaster@example.com\x{a}To: Fake Test...') called at /var/www/twiki/lib/TWiki/UI/Register.pm line 981
        TWiki::UI::Register::_emailRegistrationConfirmations('TWiki=HASH(0x8ed993c)', 'HASH(0x9460800)') called at /var/www/twiki/lib/TWiki/UI/Register.pm line 819
        TWiki::UI::Register::complete('TWiki=HASH(0x8ed993c)') called at /var/www/twiki/lib/TWiki/UI/Register.pm line 104
        TWiki::UI::Register::register_cgi('TWiki=HASH(0x8ed993c)') called at /var/www/twiki/lib/TWiki/UI.pm line 159
        TWiki::UI::__ANON__() called at /usr/lib/perl5/site_perl/5.8.8/Error.pm line 426
        eval {...} called at /usr/lib/perl5/site_perl/5.8.8/Error.pm line 418
        Error::subs::try('CODE(0x8ed98e8)', 'HASH(0x99285a0)') called at /var/www/twiki/lib/TWiki/UI.pm line 197
        TWiki::UI::run('CODE(0x9060284)', 'register', 1) called at /var/www/twiki/bin/register line 31.

Work-around diff:

 diff -u TWiki/UI/Register.pm~ TWiki/UI/Register.pm
--- TWiki/UI/Register.pm~       2008-09-11 23:41:58.000000000 -0400
+++ TWiki/UI/Register.pm        2008-10-23 08:13:07.000000000 -0400
@@ -828,6 +828,10 @@
         if( $status ) {
             $status = $session->i18n->maketext(
                 'Warning: Could not send confirmation email')."\n\n$status";
+           $session->writeDebug( "Registration email failed:|$status|\n" );
+            $status = $session->i18n->maketext(
+                'A confirmation e-mail could not be sent to [_1]', $data->{Email} );
+
         } else {
             $status = $session->i18n->maketext(
                 'A confirmation e-mail has been sent to [_1]', $data->{Email} );
 

-- TWiki:Main.TimotheLitt - 23 Oct 2008

Great analysis and patch! However please note that I'll leave that to the code owner is not an appropriate thing to do, as this code has no owner. Anybody can modify any code anywhere; if they do something wrong, then someone will pipe up.

-- TWiki:Main.CrawfordCurrie - 24 Oct 2008

First time I've been told that respecting ownership is inappropriate.

Here is a more complete fix. Use it instead of the previous work-around. Perhaps someone with checkin privileges can put it into the codebase. I am leaving item 2 as an exercise for the reader.

--- lib/TWiki/Net.pm.4.2.3      2008-09-11 23:41:58.000000000 -0400
+++ lib/TWiki/Net.pm    2008-10-24 07:09:22.000000000 -0400
@@ -321,6 +321,8 @@
             # be nasty to errors that we didn't throw. They may be
             # caused by SMTP or perl, and give away info about the
             # install that we don't want to share.
+           $e = join( "\n", grep( /^ERROR/, split( /\n/, $e ) ) );
+
             unless( $e =~ /^ERROR/ ) {
                 $e = "Mail could not be sent - see TWiki warning log.";
             }

--- lib/TWiki/UI/Register.pm.4.2.3      2008-09-11 23:41:58.000000000 -0400
+++ lib/TWiki/UI/Register.pm    2008-10-24 07:54:00.000000000 -0400
@@ -803,15 +803,8 @@
                                               $data->{LoginName},
                                               $data );
-    #only change the session's identity _if_ the registration was done by TWikiGuest
-    if ( $session->{user} eq $session->{users}->getCanonicalUserID( $TWiki::cfg{DefaultUserLogin}) ) {
-        # let the client session know that we're logged in. (This probably
-        # eliminates the need for the registrationHandler call above,
-        # but we'll leave them both in here for now.)
-        $users->{loginManager}->userLoggedIn( $data->{LoginName}, $data->{WikiName} );
-    }
-
     my $status;
+    my $safe2login = 1;
     if($TWiki::cfg{EnableEmail}) {
@@ -827,7 +820,9 @@
         if( $status ) {
             $status = $session->i18n->maketext(
-                'Warning: Could not send confirmation email')."\n\n$status";
+                'Warning: Could not send confirmation email')."\n\n$status" .
+               $session->i18n->maketext( '\n\nYou are NOT logged-in and your account is, or soon will be deactivated.\nYour e-mail address must be valid to register successfully.');
+           $safe2login = 0;
         } else {
             $status = $session->i18n->maketext(
                 'A confirmation e-mail has been sent to [_1]', $data->{Email} );
@@ -837,6 +832,15 @@
                 'Warning: Could not send confirmation email, email has been disabled');
     }
+    #only change the session's identity _if_ the registration was done by TWikiGuest & all required notifications happened
+    if ( $safe2login && $session->{user} eq $session->{users}->getCanonicalUserID( $TWiki::cfg{DefaultUserLogin}) ) {
+        # let the client session know that we're logged in. (This probably
+        # eliminates the need for the registrationHandler call above,
+        # but we'll leave them both in here for now.)
+        $users->{loginManager}->userLoggedIn( $data->{LoginName}, $data->{WikiName} );
+    }
+
+
     # and finally display thank you page
     throw TWiki::OopsException( 'attention',
                                 web => $TWiki::cfg{UsersWebName},
@@ -976,8 +980,25 @@
     my $warnings = $session->net->sendEmail( $email);
-    $template =
-      $session->templates->readTemplate( 'registernotifyadmin', $skin );
+    if( $warnings ) {
+       # Email address doesn't work, likely fraudulent registration
+       try {
+           my $users = $session->{users};
+           my $cUID = $users->getCanonicalUserID( $data->{LoginName} );
+
+           if( $session->{users}->removeUser( $cUID ) ) {
+               $template = $session->templates->readTemplate( 'registernotifyadminfailrem', $skin );
+           } else {
+               $template = $session->templates->readTemplate( 'registernotifyadminfail', $skin );
+           }
+       } catch Error::Simple with {
+           # Most Mappng Managers don't support this, unfortunately
+           $template = $session->templates->readTemplate( 'registernotifyadminfail', $skin );
+       };
+    } else {
+       $template =
+           $session->templates->readTemplate( 'registernotifyadmin', $skin );
+    }
     $email =
       _buildConfirmationEmail( $session,
                                $data,

--- /dev/null   2008-04-26 06:27:25.709194980 -0400
+++ templates/registernotifyadminfailrem.tmpl   2008-10-24 07:58:38.000000000 -0400
@@ -0,0 +1,20 @@
+%{ This is a default template }%From: %WIKIWEBMASTERNAME% <%WIKIWEBMASTER%>
+To: %WIKIWEBMASTERNAME% <%WIKIWEBMASTER%>
+Subject: %MAKETEXT{"[_1] - Registration failure for [_2] ([_3])" args="%WIKITOOLNAME%, %WIKINAME%, %EMAILADDRESS%"}%
+MIME-Version: 1.0
+Content-Type: text/plain; charset=%CHARSET%
+Content-Transfer-Encoding: 8bit
+
+%MAKETEXT{"This is an automated e-mail notification of user registration in [_1]." args="%WIKITOOLNAME%"}%
+
+%MAKETEXT{"[_1] has been registered with e-mail [_2]" args="%WIKINAME%, %EMAILADDRESS%"}%
+
+%MAKETEXT{"However, confirmation e-mail to [_2] failed, indicating probable fraudulent registration.  The [_1] was removed automatically, however you must remove the user's topic manually." args="%WIKINAME%, %EMAILADDRESS%"}%
+
+%MAKETEXT{"Submitted content:"}%
+
+%FORMDATA%
+
+%MAKETEXT{"Saved to:"}%
+
+%SCRIPTURL{"view"}%/%USERSWEB%/%WIKINAME%
 

--- /dev/null   2008-04-26 06:27:25.709194980 -0400
+++ templates/registernotifyadminfail.tmpl      2008-10-24 06:37:37.000000000 -0400
@@ -0,0 +1,20 @@
+%{ This is a default template }%From: %WIKIWEBMASTERNAME% <%WIKIWEBMASTER%>
+To: %WIKIWEBMASTERNAME% <%WIKIWEBMASTER%>
+Subject: %MAKETEXT{"[_1] - Registration failure for [_2] ([_3])" args="%WIKITOOLNAME%, %WIKINAME%, %EMAILADDRESS%"}%
+MIME-Version: 1.0
+Content-Type: text/plain; charset=%CHARSET%
+Content-Transfer-Encoding: 8bit
+
+%MAKETEXT{"This is an automated e-mail notification of user registration in [_1]." args="%WIKITOOLNAME%"}%
+
+%MAKETEXT{"[_1] has been registered with e-mail [_2]" args="%WIKINAME%, %EMAILADDRESS%"}%
+
+%MAKETEXT{"However, confirmation e-mail to [_2] failed, indicating probable fraudulent registration.  The [_1] user could not be removed automatically." args="%WIKINAME%, %EMAILADDRESS%"}%
+
+%MAKETEXT{"Submitted content:"}%
+
+%FORMDATA%
+
+%MAKETEXT{"Saved to:"}%
+
+%SCRIPTURL{"view"}%/%USERSWEB%/%WIKINAME%

-- TimotheLitt - 24 Oct 2008

its not that we don't 'respect' code ownership - just that in the core, there is none. Like most core code, its have many authors, and none have consistently enough time to be considered owners.

and after spending all day on what should have been a simple docco change, I can tell you i'm reluctant to 'just commit' your change, because that will actually involve my having to write documenation for it, looking into writing tests for it, and then porting it to trunk too - perhaps i'll have the energy later, but 'exercise for the reader' leaves me with even more.

-- TWiki:Main.SvenDowideit - 27 Oct 2008

I'm looking at this. So far it is going in fairly easily, though the text in the templates needs to be a bit more explicit.

Later: done. I have no way of contacting Timothe, so will have to rely on this note being seen.

• CrawfordCurrie - 02 Nov 2008

mailed him.

• SvenDowideit - 03 Nov 2008

Sven - Thanks for letting me know. Sorry to see the dilution of effort due to the fork, but glad to see work continue. I've registered here so that I can be reached by this group.

Crawford - Thanks for merging the change. Not sure what seemed lacking in the template text; from a quick glance in SVN, it seems that you just changed the template names. Fine with me.

One thing to consider - Since in the failing case, the user never really came into existence, one could make a strong case for deleting his/her topic. removeUser doesn't do this assuming that there could be links/useful history.

• TimotheLitt - 03 Nov 2008

The changes to the templates were purely language; i made them a bit less "it's all your fault" in tone

Removing a topic is a job for the user mapper; it may be worth considering extending the api to signal deletability. That's a subject for another report, though. This one is closed.

-- CrawfordCurrie - 05 Nov 2008 - 14:37