Item139: removed oops redirect for error reports. Fixed status header in HTTP responses.

CrawfordCurrie · CrawfordCurrie · commit b75d83052ad9 · 2008-12-02T12:49:15.000Z
diff --git a/EditTablePlugin/lib/Foswiki/Plugins/EditTablePlugin/Core.pm b/EditTablePlugin/lib/Foswiki/Plugins/EditTablePlugin/Core.pm
@@ -1440,7 +1440,7 @@ sub doEnableEdit {
 
         # user has no permission to change the topic
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'topic_access',
             web    => $theWeb,
             topic  => $theTopic,
diff --git a/TWikiCompatibilityPlugin/lib/TWiki/OopsException.pm b/TWikiCompatibilityPlugin/lib/TWiki/OopsException.pm
@@ -1,13 +1,9 @@
 package TWiki::OopsException;
-use Error;
-use Assert;
-use base 'Error';
-
-use Foswiki::OopsException;
+use base 'Foswiki::OopsException';
 
 sub new {
-    shift;
-    return new Foswiki::OopsException(@_);
+    my $class = shift;
+    return $class->SUPER::new(@_);
 }
 
 1;
diff --git a/UnitTestContrib/test/unit/ExceptionTests.pm b/UnitTestContrib/test/unit/ExceptionTests.pm
@@ -1,10 +1,11 @@
 package ExceptionTests;
-use base FoswikiTestCase;
+use base 'FoswikiTestCase';
 
 use strict;
 
 use Error qw( :try );
 use Foswiki::OopsException;
+use Foswiki::AccessControlException;
 use Foswiki::UI::Oops;
 
 # Check an OopsException with one non-array parameter
@@ -63,6 +64,7 @@ sub upchuck {
     $e->redirect($session);
 }
 
+# Test for DEPRECATED redirect
 sub test_redirectOopsException {
     my $this = shift;
     my $t = new Foswiki();
diff --git a/WorkflowPlugin/data/Sandbox/ControlledDocument.txt b/WorkflowPlugin/data/Sandbox/ControlledDocument.txt
@@ -1,3 +1,4 @@
+%META:TOPICINFO{author="SimianApe" date="1228219811" format="1.1" version="1.3"}%
 %WORKFLOWNOTICE%
 
 Some controlled text.
@@ -11,3 +12,7 @@ Some controlled text.
 | Last version in APPROVED state | %WORKFLOWLASTVERSION_APPROVED% |
 
 Workflow history: %WORKFLOWHISTORY%
+
+%META:FORM{name="InProcessForm"}%
+%META:WORKFLOWHISTORY{value="<br>WAITINGFORQM -- 02 Dec 2008 - 12:10"}%
+%META:WORKFLOW{name="WAITINGFORQM" LASTTIME_WAITINGFORQM="02 Dec 2008 - 12:10" LASTVERSION_WAITINGFORQM=""}%
diff --git a/WorkflowPlugin/data/Sandbox/DocumentApprovalWorkflow.txt b/WorkflowPlugin/data/Sandbox/DocumentApprovalWorkflow.txt
@@ -29,10 +29,10 @@ Workflow history: %<nop>WORKFLOWHISTORY%
 | WAITINGFORQM  | nobody       | This document is waiting for approval by the Quality Manager. |
 | WAITINGFORCTO | nobody       | This document is waiting for approval by the CTO.|
 ---++ Transitions
-| *State*        | *Action* | *Next State*  | *Allowed*                        | *Form*        |
-| APPROVED       | revise   | UNDERREVISION |                      | ApprovedForm  |
-| UNDERREVISION  | ready for approval | WAITINGFORQM  |                      | InProcessForm |
-| WAITINGFORQM   | approve  | WAITINGFORCTO |       |               |
-| WAITINGFORQM   | reject   | UNDERREVISION |       |               |
-| WAITINGFORCTO  | approve  | APPROVED      |       | ApprovedForm  |
-| WAITINGFORCTO  | reject   | UNDERREVISION |       |               |
+| *State*        | *Action* | *Next State*  | *Allowed* | *Form*        |
+| APPROVED       | revise   | UNDERREVISION |           | ApprovedForm  |
+| UNDERREVISION  | ready for approval | WAITINGFORQM  | | InProcessForm |
+| WAITINGFORQM   | approve  | WAITINGFORCTO |           |               |
+| WAITINGFORQM   | reject   | UNDERREVISION |           |               |
+| WAITINGFORCTO  | approve  | APPROVED      |           | ApprovedForm  |
+| WAITINGFORCTO  | reject   | UNDERREVISION |           |               |
diff --git a/WorkflowPlugin/lib/TWiki/Plugins/WorkflowPlugin.pm b/WorkflowPlugin/lib/TWiki/Plugins/WorkflowPlugin.pm
@@ -240,7 +240,7 @@ sub beforeEditHandler {
     my $query = TWiki::Func::getCgiQuery();
     if ( !$query->param('INWORKFLOWSEQUENCE') && !$TOPIC->canEdit() ) {
         throw TWiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def   => 'topic_access',
             web   => $_[2],
             topic => $_[1],
@@ -284,35 +284,40 @@ sub _changeState {
         my $newForm = $TOPIC->newForm($action);
 
         try {
-            $query->param('INWORKFLOWSEQUENCE' => 1);
-            $TOPIC->changeState($action);
-            if ($newForm) {
-
-                # If there is a form with the new state, and it's not the same
-                # form as previously, we need to kick into edit mode to support
-                # form field changes.
-                $url = TWiki::Func::getScriptUrl(
-                    $web, $topic, 'edit',
-                    INWORKFLOWSEQUENCE => time());
+            try {
+                $query->param('INWORKFLOWSEQUENCE' => 1);
+                if ($newForm) {
+                    # If there is a form with the new state, and it's not
+                    # the same form as previously, we need to kick into edit
+                    # mode to support form field changes.
+                    $url = TWiki::Func::getScriptUrl(
+                        $web, $topic, 'edit',
+                        INWORKFLOWSEQUENCE => time());
+                }
+                else {
+                    $url = TWiki::Func::getScriptUrl( $web, $topic, 'view' );
+                }
+                # SMELL: don't do this until the edit is over
+                $TOPIC->changeState($action);
+                TWiki::Func::redirectCgiQuery( undef, $url );
+            } catch Error::Simple with {
+                my $error = shift;
+                throw TWiki::OopsException(
+                    'oopssaveerr',
+                    web => $web, topic => $topic,
+                    params => [ $error || '?' ]);
+            };
+        } catch TWiki::OopsException with {
+            my $e = shift;
+            if ($e->can('generate')) {
+                $e->generate( $session );
+            } else {
+                # Deprecated, TWiki compatibility only
+                $e->redirect( $session );
             }
-            else {
-                $url = TWiki::Func::getScriptUrl( $web, $topic, 'view' );
-            }
-        }
-        catch Error::Simple with {
-            my $error = shift;
-            $url = TWiki::Func::getScriptUrl(
-                $web, $topic, 'oops',
-                template => "oopssaveerr",
-                param1   => $error
-            );
-        }
-        catch TWiki::OopsException with {
-            shift->redirect( $session );
 
         };
     }
-    TWiki::Func::redirectCgiQuery( undef, $url );
     return undef;
 }
 
diff --git a/core/lib/Foswiki.pm b/core/lib/Foswiki.pm
@@ -722,7 +722,6 @@ sub generateHTTPHeaders {
 
     # add cookie(s)
     $this->{users}->{loginManager}->modifyHeader($hopts);
-
     $this->{response}->headers($hopts);
 }
 
diff --git a/core/lib/Foswiki/Engine/CGI.pm b/core/lib/Foswiki/Engine/CGI.pm
@@ -176,6 +176,9 @@ sub finalizeHeaders {
     my ( $this, $res, $req ) = @_;
 
     $this->SUPER::finalizeHeaders( $res, $req );
+    # make sure we always generate a status for the response
+    print 'Status: '.$res->status()
+      if ($res->status() && !defined($res->headers->{status}));
     foreach my $header ( keys %{ $res->headers } ) {
         print $header . ': ' . $_ . $this->CRLF
           foreach $res->getHeader($header);
diff --git a/core/lib/Foswiki/OopsException.pm b/core/lib/Foswiki/OopsException.pm
@@ -23,6 +23,7 @@ use Error;
 ...
 
 throw Foswiki::OopsException( 'bathplugin',
+                            status => 418,
                             def => 'toestuck',
                             web => $web,
                             topic => $topic,
@@ -83,7 +84,7 @@ NOTE: parameter values are automatically and unconditionally entity-encoded
 sub new {
     my $class    = shift;
     my $template = shift;
-    my $this     = bless( $class->SUPER::new(), $class );
+    my $this     = $class->SUPER::new();
     $this->{template} = $template;
     ASSERT( scalar(@_) % 2 == 0, join( ";", map { $_ || 'undef' } @_ ) )
       if DEBUG;
@@ -145,22 +146,44 @@ sub stringify {
     }
 }
 
-=begin TML
+# Generate a redirect to an 'oops' script for this exception.
+#
+# If the 'keep' parameter is set in the
+# exception, it saves parameter values into the query as well. This is needed
+# if the query string might get lost during a passthrough, due to a POST
+# being redirected to a GET.
+# This redirect has been replaced by the generate function below and should
+# not be called in new code.
+sub redirect {
+    my ( $this, $session ) = @_;
+    my @p = $this->_prepareResponse($session);
+    my $url =
+      $session->getScriptUrl( 1, 'oops', $this->{web}, $this->{topic}, @p );
+    $session->redirect( $url, 1 );
+}
 
----++ ObjectMethod redirect( $twiki )
+=begin TML
 
-Generate a redirect to an 'oops' script for this exception.
+---++ ObjectMethod generate( $session )
 
-If the 'keep' parameter is set in the
-exception, it saves parameter values into the query as well. This is needed
-if the query string might get lost during a passthrough, due to a POST
-being redirected to a GET.
+Generate an error page for the exception. This will output the error page
+to the browser. The default HTTP Status for an Oops exception is 500. This
+can be overridden using the 'status => ' parameter to the constructor.
 
 =cut
 
-sub redirect {
-    my ( $this, $session ) = @_;
+sub generate {
+    my ($this, $session ) = @_;
+
+    my @p = $this->_prepareResponse( $session );
+    $session->{response}->status( $this->{status} || 500 );
+    require Foswiki::UI::Oops;
+    Foswiki::UI::Oops::oops($session, $this->{web}, $this->{topic},
+                            $session->{request}, 0);
+}
 
+sub _prepareResponse {
+    my ($this, $session ) = @_;
     my @p = ();
 
     $this->{template} = "oops$this->{template}"
@@ -169,12 +192,10 @@ sub redirect {
     push( @p, def => $this->{def} ) if $this->{def};
     my $n = 1;
     push( @p, map { 'param' . ( $n++ ) => $_ } @{ $this->{params} } );
-    my $url =
-      $session->getScriptUrl( 1, 'oops', $this->{web}, $this->{topic}, @p );
     while ( my $p = shift(@p) ) {
         $session->{request}->param( -name => $p, -value => shift(@p) );
     }
-    $session->redirect( $url, 1 );
+    return @p;
 }
 
 1;
diff --git a/core/lib/Foswiki/Store.pm b/core/lib/Foswiki/Store.pm
@@ -1403,12 +1403,14 @@ sub topicExists {
     my ( $this, $web, $topic ) = @_;
     $web =~ s#\.#/#go;
     ASSERT( defined($topic) ) if DEBUG;
-    if (DEBUG) {
-        my ( $webTest, $topicTest ) =
-          $this->{session}->normalizeWebTopicName( $web, $topic );
-        ASSERT( $topic eq $topicTest );
-        ASSERT( $web   eq $webTest );
-    }
+    # This test is invalid. This intent is good, but this function may
+    # be called with a deliberately undef web or topic.
+    #if (DEBUG) {
+    #    my ( $webTest, $topicTest ) =
+    #      $this->{session}->normalizeWebTopicName( $web, $topic );
+    #    ASSERT( $topic eq $topicTest );
+    #    ASSERT( $web   eq $webTest );
+    #}
     return 0 unless $topic;
 
     my $handler = _getHandler( $this, $web, $topic );
diff --git a/core/lib/Foswiki/UI.pm b/core/lib/Foswiki/UI.pm
@@ -182,18 +182,18 @@ sub execute {
                 # Login manager did not want to authenticate, perhaps because
                 # we are already authenticated.
                 my $exception = new Foswiki::OopsException(
-                    'accessdenied',
+                    'accessdenied', status => 403,
                     web    => $e->{web},
                     topic  => $e->{topic},
                     def    => 'topic_access',
                     params => [ $e->{mode}, $e->{reason} ]
                 );
 
-                $exception->redirect($session);
+                $exception->generate($session);
             }
         }
         catch Foswiki::OopsException with {
-            shift->redirect($session);
+            shift->generate($session);
         }
         catch Error::Simple with {
             my $e = shift;
@@ -276,7 +276,7 @@ sub checkWebExists {
 
     unless ( $session->{store}->webExists($webName) ) {
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'no_such_web',
             web    => $webName,
             topic  => $topic,
@@ -300,7 +300,7 @@ sub checkTopicExists {
 
     unless ( $session->{store}->topicExists( $webName, $topic ) ) {
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'no_such_topic',
             web    => $webName,
             topic  => $topic,
@@ -351,7 +351,7 @@ sub checkAccess {
       )
     {
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'topic_access',
             web    => $web,
             topic  => $topic,
diff --git a/core/lib/Foswiki/UI/Edit.pm b/core/lib/Foswiki/UI/Edit.pm
@@ -182,7 +182,7 @@ sub init_edit {
 
     if ( $saveCmd && !$session->{users}->isAdmin( $session->{user} ) ) {
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'only_group',
             web    => $webName,
             topic  => $topic,
@@ -235,7 +235,7 @@ sub init_edit {
 
             unless ( $store->topicExists( $templateWeb, $templateTopic ) ) {
                 throw Foswiki::OopsException(
-                    'accessdenied',
+                    'accessdenied', status => 403,
                     def   => 'no_such_topic_template',
                     web   => $templateWeb,
                     topic => $templateTopic
diff --git a/core/lib/Foswiki/UI/Manage.pm b/core/lib/Foswiki/UI/Manage.pm
@@ -136,7 +136,7 @@ sub _removeUser {
     $users->removeUser($cUID);
 
     throw Foswiki::OopsException(
-        'attention',
+        'attention', status => 200,
         def    => 'remove_user_done',
         web    => $webName,
         topic  => $topic,
@@ -237,7 +237,7 @@ sub _createWeb {
 
     # everything OK, redirect to last message
     throw Foswiki::OopsException(
-        'attention',
+        'attention', status => 200,
         web   => $newWeb,
         topic => $newTopic,
         def   => 'created_web'
@@ -312,7 +312,7 @@ sub rename {
         unless ( $session->{store}->topicExists( $oldWeb, lcfirst $oldTopic ) )
         {
             throw Foswiki::OopsException(
-                'accessdenied',
+                'accessdenied', status => 403,
                 def   => 'no_such_topic_rename',
                 web   => $oldWeb,
                 topic => $oldTopic
@@ -1624,7 +1624,7 @@ sub _restoreRevision {
 
         # user has no permission to change the topic
         throw Foswiki::OopsException(
-            'accessdenied',
+            'accessdenied', status => 403,
             def    => 'topic_access',
             web    => $web,
             topic  => $topic,
diff --git a/core/lib/Foswiki/UI/Register.pm b/core/lib/Foswiki/UI/Register.pm
diff --git a/core/lib/Foswiki/UI/Save.pm b/core/lib/Foswiki/UI/Save.pm
diff --git a/core/lib/Foswiki/UI/Search.pm b/core/lib/Foswiki/UI/Search.pm
diff --git a/core/lib/Foswiki/UI/Upload.pm b/core/lib/Foswiki/UI/Upload.pm

Original file line number	Diff line number	Diff line change
`@@ -722,7 +722,6 @@ sub generateHTTPHeaders {`
`722`	`722`
`723`	`723`	`# add cookie(s)`
`724`	`724`	`$this->{users}->{loginManager}->modifyHeader($hopts);`
`725`		`-`
`726`	`725`	`$this->{response}->headers($hopts);`
`727`	`726`	`}`
`728`	`727`