Fix for tickets 1009 and 603: Show a themed error page to guests / registered users (not to admins though). And show a login form to guests for 404 (incl. insufficient view permissions) errors.

10 files changed, 97 insertions, 157 deletions

diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index c2b474ee..1cc3b1ec 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -26,19 +26,10 @@ class Albums_Controller extends Items_Controller {
     if (!is_object($album)) {
       // show() must be public because we route to it in url::parse_url(), so make
       // sure that we're actually receiving an object
-      throw new Kohana_404_Exception();
+      Event::run('system.404');
     }
 
-    if (!access::can("view", $album)) {
-      if ($album->id == 1) {
-        // Even show the login page to logged in users.
-        // It's a better user experience than a "Dang" error page.
-        print auth::login_page();
-        return;
-      } else {
-        access::required("view", $album);
-      }
-    }
+    access::required("view", $album);
 
     $page_size = module::get_var("gallery", "page_size", 9);
     $input = Input::instance();
diff --git a/modules/gallery/controllers/items.php b/modules/gallery/controllers/items.php
index f261e3a9..0bd47b2d 100644
--- a/modules/gallery/controllers/items.php
+++ b/modules/gallery/controllers/items.php
@@ -21,7 +21,7 @@ class Items_Controller extends Controller {
   public function __call($function, $args) {
     $item = ORM::factory("item", (int)$function);
     if (!$item->loaded()) {
-      throw new Kohana_404_Exception();
+      Event::run('system.404');
     }
 
     // Redirect to the more specific resource type, since it will render
diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php
index 78a56e81..1dbcb481 100644
--- a/modules/gallery/controllers/movies.php
+++ b/modules/gallery/controllers/movies.php
@@ -22,7 +22,7 @@ class Movies_Controller extends Items_Controller {
     if (!is_object($movie)) {
       // show() must be public because we route to it in url::parse_url(), so make
       // sure that we're actually receiving an object
-      throw new Kohana_404_Exception();
+      Event::run('system.404');
     }
 
     access::required("view", $movie);
diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php
index f2d47eec..2a77aea4 100644
--- a/modules/gallery/controllers/photos.php
+++ b/modules/gallery/controllers/photos.php
@@ -22,7 +22,7 @@ class Photos_Controller extends Items_Controller {
     if (!is_object($photo)) {
       // show() must be public because we route to it in url::parse_url(), so make
       // sure that we're actually receiving an object
-      throw new Kohana_404_Exception();
+      Event::run('system.404');
     }
   
     access::required("view", $photo);
diff --git a/modules/gallery/helpers/access.php b/modules/gallery/helpers/access.php
index 7e8b079a..c4c100ca 100644
--- a/modules/gallery/helpers/access.php
+++ b/modules/gallery/helpers/access.php
@@ -120,7 +120,7 @@ class access_Core {
     if (!self::can($perm_name, $item)) {
       if ($perm_name == "view") {
         // Treat as if the item didn't exist, don't leak any information.
-        throw new Kohana_404_Exception();
+        Event::run('system.404');
       } else {
         self::forbidden();
       }
diff --git a/modules/gallery/helpers/auth.php b/modules/gallery/helpers/auth.php
index 8b0ce470..c3e9e6e9 100644
--- a/modules/gallery/helpers/auth.php
+++ b/modules/gallery/helpers/auth.php
@@ -130,18 +130,4 @@ class auth_Core {
     $session->set("admin_area_activity_timestamp", time());
     return false;
   }
-
-  /**
-   * Returns the themed login page.
-   */
-  static function login_page($continue_url=null) {
-    $view = new Theme_View("page.html", "other", "login");
-    $view->page_title = t("Log in to Gallery");
-    $view->content = new View("login_ajax.html");
-    $view->content->form = auth::get_login_form("login/auth_html");
-    // Avoid anti-phishing protection by passing the url as session variable.
-    $continue_url or $continue_url = url::current(true);
-    Session::instance()->set("continue_url", $continue_url);
-    return $view;
-  }
 }
\ No newline at end of file
diff --git a/modules/gallery/libraries/MY_Kohana_Exception.php b/modules/gallery/libraries/MY_Kohana_Exception.php
index 1c40091a..d6f1f467 100644
--- a/modules/gallery/libraries/MY_Kohana_Exception.php
+++ b/modules/gallery/libraries/MY_Kohana_Exception.php
@@ -33,6 +33,63 @@ class Kohana_Exception extends Kohana_Exception_Core {
     if ($e instanceof ORM_Validation_Exception) {
       Kohana_Log::add("error", "Validation errors: " . print_r($e->validation->errors(), 1));
     }
-    return parent::handle($e);
+    try {
+      $user = identity::active_user();
+      $try_themed_view = $user && !$user->admin;
+    } catch (Exception $e2) {
+      $try_themed_view = false;
+    }
+
+    if ($try_themed_view) {
+      try {
+        return self::_show_themed_error_page($e);
+      } catch (Exception $e3) {
+        Kohana_Log::add("error", "Exception in exception handling code: " . self::text($e3));
+        return parent::handle($e);
+      }
+    } else {
+      return parent::handle($e);
+    }
+  }
+
+  /**
+   * Shows a themed error page.
+   * @see Kohana_Exception::handle
+   */
+  private static function _show_themed_error_page(Exception $e) {
+    // Create a text version of the exception
+    $error = Kohana_Exception::text($e);
+    
+    // Add this exception to the log
+    Kohana_Log::add('error', $error);
+
+    // Manually save logs after exceptions
+    Kohana_Log::save();
+
+    if (!headers_sent()) {
+      if ($e instanceof Kohana_Exception) {
+        $e->sendHeaders();
+      } else {
+        header("HTTP/1.1 500 Internal Server Error");
+      }
+    }
+
+    $view = new Theme_View("page.html", "other", "error");
+    if ($e instanceof Kohana_404_Exception) {
+      $view->page_title = t("Dang...  Page not found!");
+      $view->content = new View("error_404.html");
+      $user = identity::active_user();
+      $view->content->is_guest = $user && $user->guest;
+      if ($view->content->is_guest) {
+        $view->content->login_form = new View("login_ajax.html");
+        $view->content->login_form->form = auth::get_login_form("login/auth_html");
+        // Avoid anti-phishing protection by passing the url as session variable.
+        Session::instance()->set("continue_url", url::current(true));
+      }
+    } else {
+      $view->page_title = t("Dang...  Something went wrong!");
+      $view->content = new View("error.html");
+    }
+    print $view;
   }
 }
\ No newline at end of file
diff --git a/modules/gallery/views/error.html.php b/modules/gallery/views/error.html.php
new file mode 100644
index 00000000..5d81b651
--- /dev/null
+++ b/modules/gallery/views/error.html.php
@@ -0,0 +1,12 @@
+<?php defined("SYSPATH") or die("No direct script access.") ?>
+<div id="g-error">
+  <h1>
+    <?= t("Dang...  Something went wrong!") ?>
+  </h1>
+  <h2>
+    <?= t("We tried really hard, but it's broken.") ?>
+  </h2>
+  <p>
+    <?= t("Talk to your Gallery administrator for help fixing this!") ?>
+  </p>
+</div>
\ No newline at end of file
diff --git a/modules/gallery/views/error_404.html.php b/modules/gallery/views/error_404.html.php
new file mode 100644
index 00000000..e5846e65
--- /dev/null
+++ b/modules/gallery/views/error_404.html.php
@@ -0,0 +1,21 @@
+<?php defined("SYSPATH") or die("No direct script access.") ?>
+<div id="g-error">
+  <h1>
+    <?= t("Dang...  Page not found!") ?>
+  </h1>
+  <? if ($is_guest): ?>
+    <h2>
+      <?= t("Hey wait, you're not signed in yet!") ?>
+    </h2>
+    <p>
+       <?= t("Maybe the page exists, but is only visible to authorized users.") ?>
+       <?= t("Please sign in to find out.") ?>
+    </p>
+    <?= $login_form ?>
+  <? else: ?>
+    <p>
+      <?= t("Maybe the page exists, but is only visible to authorized users.") ?>
+      <?= t("Talk to your Gallery administrator if you think this is an error for help fixing this!") ?>
+    </p>
+ <? endif; ?>
+</div>
\ No newline at end of file
diff --git a/modules/gallery/views/kohana_error_page.php b/modules/gallery/views/kohana_error_page.php
deleted file mode 100644
index b9fdcc19..00000000
--- a/modules/gallery/views/kohana_error_page.php
+++ /dev/null
@@ -1,127 +0,0 @@
-<?php defined("SYSPATH") or die("No direct script access.") ?>
-<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
-  <head>
-    <style type="text/css">
-      body {
-        background: #fff;
-        font-size: 14px;
-        line-height: 130%;
-      }
-
-      div.big_box {
-        padding: 10px;
-        background: #eee;
-        border: solid 1px #ccc;
-        font-family: sans-serif;
-        color: #111;
-        width: 42em;
-        margin: 20px auto;
-      }
-
-      div#framework_error {
-        text-align: center;
-      }
-
-      div#error_details {
-        text-align: left;
-      }
-
-      code {
-        font-family: monospace;
-        font-size: 12px;
-        margin: 20px;
-        color: #333;
-        white-space: pre-wrap;
-        white-space: -moz-pre-wrap;
-        word-wrap: break-word;
-      }
-
-      h3 {
-        font-family: sans-serif;
-        margin: 2px 0px 0px 0px;
-        padding: 8px 0px 0px 0px;
-        border-top: 1px solid #ddd;
-      }
-
-      p {
-        padding: 0px;
-        margin: 0px 0px 10px 0px;
-      }
-
-      li, pre {
-        padding: 0px;
-        margin: 0px;
-      }
-    </style>
-    <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
-    <title><?= t("Something went wrong!") ?></title>
-  </head>
-  <body>
-    <? try { $user = identity::active_user(); } catch (Exception $e) { } ?>
-    <? $admin = php_sapi_name() == "cli" || isset($user) && $user->admin ?>
-    <div class="big_box" id="framework_error">
-      <h1>
-        <?= t("Dang...  Something went wrong!") ?>
-      </h1>
-      <h2>
-        <?= t("We tried really hard, but it's broken.") ?>
-      </h2>
-      <? if (!$admin): ?>
-      <p>
-        <?= t("Talk to your Gallery administrator for help fixing this!") ?>
-      </p>
-      <? endif ?>
-    </div>
-    <? if ($admin): ?>
-    <div class="big_box" id="error_details">
-      <h2>
-        <?= t("Hey wait, you're an admin!  We can tell you stuff.") ?>
-      </h2>
-      <script type="text/javascript">
-        var show_details = function() {
-          document.getElementById("stuff").style.display = "block";
-          document.getElementById("toggle").style.display = "none";
-        }
-      </script>
-      <a id="toggle" href="#" onclick="javascript:show_details(); return false;">
-        <b><?= t("Ok.. tell me stuff!") ?></b>
-      </a>
-      <div id="stuff" style="display: none">
-        <? if (!empty($line) and !empty($file)): ?>
-        <div id="summary">
-          <h3>
-            <?= t("Help!") ?>
-          </h3>
-          <p>
-            <?= t("If this stuff doesn't make any sense to you, <a href=\"%url\">ask for help in the Gallery forums</a>!", array("url" => "http://gallery.menalto.com/forum/96")) ?>
-          </p>
-          <h3>
-            <?= t("So here's the error:") ?>
-          </h3>
-
-          <code class="block"><?= $message ?></code>
-          <p>
-            <?= t("File: <b>%file</b>, line: <b>%line</b>", array("file" => $file, "line" => $line)) ?>
-          </p>
-        </div>
-        <? endif ?>
-
-        <? $trace = $PHP_ERROR ? array_slice(debug_backtrace(), 1) : $exception->getTrace(); ?>
-        <? $trace = Kohana::backtrace($trace); ?>
-        <? if (!empty($trace)): ?>
-        <div id="stack_trace">
-          <h3>
-            <?= t("And here's how we got there:") ?>
-          </h3>
-          <?= $trace ?>
-          <? endif ?>
-        </div>
-      </div>
-      <? else: ?>
-      <? $trace = $PHP_ERROR ? array_slice(debug_backtrace(), 1) : $exception->getTraceAsString(); ?>
-      <? if (!empty($trace)): ?>
-      <? Kohana_Log::add("error", print_r($trace, 1)); ?>
-      <? endif ?>
-      <? endif ?>
-  </body>
-</html>