Security pass over all controller code. Mostly adding CSRF checking

and verifying user permissions, but there are several above-the-bar
changes:

1) Server add is now only available to admins.  This is a hard
   requirement because we have to limit server access (eg:
   server_add::children) to a user subset and the current permission
   model doesn't include that.  Easiest fix is to restrict to admins.
   Got rid of the server_add permission.

2) We now know check permissions at every level, which means in
   controllers AND in helpers.  This "belt and suspenders" approach will
   give us defense in depth in case we overlook it in one area.

3) We now do CSRF checking in every controller method that changes the
   code, in addition to the Forge auto-check.  Again, defense in depth
   and it makes scanning the code for security much simpler.

4) Moved Simple_Uploader_Controller::convert_filename_to_title to
   item:convert_filename_to_title

5) Fixed a bug in sending notification emails.

6) Fixed the Organize code to verify that you only have access to your
   own tasks.  In general, added permission checks to organize which had
   pretty much no validation code.

I did my best to verify every feature that I touched.

14 files changed, 83 insertions, 31 deletions

diff --git a/modules/gallery/controllers/admin.php b/modules/gallery/controllers/admin.php
index af0f387a..b92a32cd 100644
--- a/modules/gallery/controllers/admin.php
+++ b/modules/gallery/controllers/admin.php
@@ -22,8 +22,9 @@ class Admin_Controller extends Controller {
 
   public function __construct($theme=null) {
     if (!(user::active()->admin)) {
-      throw new Exception("@todo UNAUTHORIZED", 401);
+      access::forbidden();
     }
+
     parent::__construct();
   }
 
diff --git a/modules/gallery/controllers/admin_dashboard.php b/modules/gallery/controllers/admin_dashboard.php
index a1090a6d..3cb97b14 100644
--- a/modules/gallery/controllers/admin_dashboard.php
+++ b/modules/gallery/controllers/admin_dashboard.php
@@ -29,6 +29,8 @@ class Admin_Dashboard_Controller extends Admin_Controller {
   }
 
   public function add_block() {
+    access::verify_csrf();
+
     $form = gallery_block::get_add_block_form();
     if ($form->validate()) {
       list ($module_name, $id) = explode(":", $form->add_block->id->value);
@@ -51,6 +53,7 @@ class Admin_Dashboard_Controller extends Admin_Controller {
 
   public function remove_block($id) {
     access::verify_csrf();
+
     $blocks_center = block_manager::get_active("dashboard_center");
     $blocks_sidebar = block_manager::get_active("dashboard_sidebar");
 
@@ -73,6 +76,7 @@ class Admin_Dashboard_Controller extends Admin_Controller {
 
   public function reorder() {
     access::verify_csrf();
+
     $active_set = array();
     foreach (array("dashboard_sidebar", "dashboard_center") as $location) {
       foreach (block_manager::get_active($location) as $id => $info) {
diff --git a/modules/gallery/controllers/admin_graphics.php b/modules/gallery/controllers/admin_graphics.php
index 7e8ef47c..72f8d8e1 100644
--- a/modules/gallery/controllers/admin_graphics.php
+++ b/modules/gallery/controllers/admin_graphics.php
@@ -43,6 +43,7 @@ class Admin_Graphics_Controller extends Admin_Controller {
 
   public function choose($toolkit) {
     access::verify_csrf();
+
     if ($toolkit != module::get_var("gallery", "graphics_toolkit")) {
       module::set_var("gallery", "graphics_toolkit", $toolkit);
 
diff --git a/modules/gallery/controllers/admin_languages.php b/modules/gallery/controllers/admin_languages.php
index 1dea733c..4639de89 100644
--- a/modules/gallery/controllers/admin_languages.php
+++ b/modules/gallery/controllers/admin_languages.php
@@ -31,6 +31,8 @@ class Admin_Languages_Controller extends Admin_Controller {
   }
 
   public function save() {
+    access::verify_csrf();
+
     $form = $this->_languages_form();
     if ($form->validate()) {
       module::set_var("gallery", "default_locale", $form->choose_language->locale->value);
@@ -41,6 +43,8 @@ class Admin_Languages_Controller extends Admin_Controller {
   }
 
   public function share() {
+    access::verify_csrf();
+
     $form = $this->_share_translations_form();
     if (!$form->validate()) {
       // Show the page with form errors
diff --git a/modules/gallery/controllers/admin_theme_details.php b/modules/gallery/controllers/admin_theme_details.php
index fec1311b..97696df5 100644
--- a/modules/gallery/controllers/admin_theme_details.php
+++ b/modules/gallery/controllers/admin_theme_details.php
@@ -26,6 +26,8 @@ class Admin_Theme_Details_Controller extends Admin_Controller {
   }
 
   public function save() {
+    access::verify_csrf();
+
     $form = theme::get_edit_form_admin();
     if ($form->validate()) {
       module::set_var("gallery", "page_size", $form->edit_theme->page_size->value);
diff --git a/modules/gallery/controllers/albums.php b/modules/gallery/controllers/albums.php
index 5ccadb37..efde4f09 100644
--- a/modules/gallery/controllers/albums.php
+++ b/modules/gallery/controllers/albums.php
@@ -24,11 +24,11 @@ class Albums_Controller extends Items_Controller {
    */
   public function _show($album) {
     if (!access::can("view", $album)) {
-      if ($album->id != 1) {
-        access::forbidden();
-      } else {
+      if ($album->id == 1) {
         print new Theme_View("login_page.html", "album");
         return;
+      } else {
+        access::forbidden();
       }
     }
 
@@ -77,6 +77,8 @@ class Albums_Controller extends Items_Controller {
    * @see REST_Controller::_create($resource)
    */
   public function _create($album) {
+    access::verify_csrf();
+    access::required("view", $album);
     access::required("add", $album);
 
     switch ($this->input->post("type")) {
@@ -92,6 +94,7 @@ class Albums_Controller extends Items_Controller {
   }
 
   private function _create_album($album) {
+    access::required("view", $album);
     access::required("add", $album);
 
     $form = album::get_add_form($album);
@@ -120,6 +123,7 @@ class Albums_Controller extends Items_Controller {
   }
 
   private function _create_photo($album) {
+    access::required("view", $album);
     access::required("add", $album);
 
     // If we set the content type as JSON, it triggers saving the result as
@@ -153,6 +157,8 @@ class Albums_Controller extends Items_Controller {
    * @see REST_Controller::_update($resource)
    */
   public function _update($album) {
+    access::verify_csrf();
+    access::required("view", $album);
     access::required("edit", $album);
 
     $form = album::get_edit_form($album);
@@ -202,6 +208,7 @@ class Albums_Controller extends Items_Controller {
    */
   public function _form_add($album_id) {
     $album = ORM::factory("item", $album_id);
+    access::required("view", $album);
     access::required("add", $album);
 
     switch ($this->input->get("type")) {
@@ -223,6 +230,7 @@ class Albums_Controller extends Items_Controller {
    *  @see REST_Controller::_form_add($parameters)
    */
   public function _form_edit($album) {
+    access::required("view", $album);
     access::required("edit", $album);
 
     print album::get_edit_form($album);
diff --git a/modules/gallery/controllers/l10n_client.php b/modules/gallery/controllers/l10n_client.php
index 17520051..c3a76659 100644
--- a/modules/gallery/controllers/l10n_client.php
+++ b/modules/gallery/controllers/l10n_client.php
@@ -20,7 +20,9 @@
 class L10n_Client_Controller extends Controller {
   public function save() {
     access::verify_csrf();
-    user::active()->admin or access::forbidden();
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
     $input = Input::instance();
     $message = $input->post("l10n-message-source");
@@ -58,6 +60,9 @@ class L10n_Client_Controller extends Controller {
 
   public function toggle_l10n_mode() {
     access::verify_csrf();
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
 
     $session = Session::instance();
     $session->set("l10n_mode",
@@ -89,6 +94,10 @@ class L10n_Client_Controller extends Controller {
   }
 
   public static function l10n_form() {
+    if (!user::active()->admin) {
+      access::forbidden();
+    }
+
     $calls = I18n::instance()->call_log();
 
     if ($calls) {
diff --git a/modules/gallery/controllers/move.php b/modules/gallery/controllers/move.php
index 130c247f..93ef05a6 100644
--- a/modules/gallery/controllers/move.php
+++ b/modules/gallery/controllers/move.php
@@ -20,6 +20,7 @@
 class Move_Controller extends Controller {
   public function browse($source_id) {
     $source = ORM::factory("item", $source_id);
+    access::required("view", $source);
     access::required("edit", $source);
 
     $view = new View("move_browse.html");
@@ -33,6 +34,11 @@ class Move_Controller extends Controller {
     $source = ORM::factory("item", $source_id);
     $target = ORM::factory("item", $this->input->post("target_id"));
 
+    access::required("view", $source);
+    access::required("edit", $source);
+    access::required("view", $target);
+    access::required("edit", $target);
+
     item::move($source, $target);
 
     print json_encode(
@@ -43,8 +49,11 @@ class Move_Controller extends Controller {
   public function show_sub_tree($source_id, $target_id) {
     $source = ORM::factory("item", $source_id);
     $target = ORM::factory("item", $target_id);
+    access::required("view", $source);
     access::required("edit", $source);
     access::required("view", $target);
+    // show targets even if they're not editable because they may contain children which *are*
+    // editable
 
     print $this->_get_tree_html($source, $target);
   }
diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php
index 55bbb0e5..86b0f177 100644
--- a/modules/gallery/controllers/movies.php
+++ b/modules/gallery/controllers/movies.php
@@ -66,6 +66,8 @@ class Movies_Controller extends Items_Controller {
    * @see REST_Controller::_update($resource)
    */
   public function _update($photo) {
+    access::verify_csrf();
+    access::required("view", $photo);
     access::required("edit", $photo);
 
     $form = photo::get_edit_form($photo);
@@ -108,6 +110,7 @@ class Movies_Controller extends Items_Controller {
    *  @see REST_Controller::_form_edit($resource)
    */
   public function _form_edit($photo) {
+    access::required("view", $photo);
     access::required("edit", $photo);
     print photo::get_edit_form($photo);
   }
diff --git a/modules/gallery/controllers/permissions.php b/modules/gallery/controllers/permissions.php
index b0cee303..c776a0fd 100644
--- a/modules/gallery/controllers/permissions.php
+++ b/modules/gallery/controllers/permissions.php
@@ -20,6 +20,7 @@
 class Permissions_Controller extends Controller {
   function browse($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if (!$item->is_album()) {
@@ -37,6 +38,7 @@ class Permissions_Controller extends Controller {
 
   function form($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if (!$item->is_album()) {
@@ -48,9 +50,11 @@ class Permissions_Controller extends Controller {
 
   function change($command, $group_id, $perm_id, $item_id) {
     access::verify_csrf();
+
     $group = ORM::factory("group", $group_id);
     $perm = ORM::factory("permission", $perm_id);
     $item = ORM::factory("item", $item_id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if ($group->loaded && $perm->loaded && $item->loaded) {
diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php
index 5d4040cf..2de51bc7 100644
--- a/modules/gallery/controllers/photos.php
+++ b/modules/gallery/controllers/photos.php
@@ -62,10 +62,13 @@ class Photos_Controller extends Items_Controller {
     print $template;
   }
 
+
   /**
    * @see REST_Controller::_update($resource)
    */
   public function _update($photo) {
+    access::verify_csrf();
+    access::required("view", $photo);
     access::required("edit", $photo);
 
     $form = photo::get_edit_form($photo);
@@ -110,7 +113,9 @@ class Photos_Controller extends Items_Controller {
    *  @see REST_Controller::_form_edit($resource)
    */
   public function _form_edit($photo) {
+    access::required("view", $photo);
     access::required("edit", $photo);
+
     print photo::get_edit_form($photo);
   }
 }
diff --git a/modules/gallery/controllers/quick.php b/modules/gallery/controllers/quick.php
index 643dce30..6efcb9de 100644
--- a/modules/gallery/controllers/quick.php
+++ b/modules/gallery/controllers/quick.php
@@ -19,8 +19,8 @@
  */
 class Quick_Controller extends Controller {
   public function pane($id) {
-    $item = ORM::factory("item", $id);
-    if (!$item->loaded) {
+    $item = model_cache::get("item", $id);
+    if (!access::can("view", $item) || !access::can("edit", $item)) {
       return "";
     }
 
@@ -32,10 +32,9 @@ class Quick_Controller extends Controller {
 
   public function rotate($id, $dir) {
     access::verify_csrf();
-    $item = ORM::factory("item", $id);
-    if (!$item->loaded) {
-      return "";
-    }
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
+    access::required("edit", $item);
 
     $degrees = 0;
     switch($dir) {
@@ -82,14 +81,21 @@ class Quick_Controller extends Controller {
 
   public function make_album_cover($id) {
     access::verify_csrf();
-    item::make_album_cover(ORM::factory("item", $id));
+
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
+    access::required("view", $item->parent());
+    access::required("edit", $item->parent());
+
+    item::make_album_cover($item);
 
     print json_encode(array("result" => "success"));
   }
 
   public function delete($id) {
     access::verify_csrf();
-    $item = ORM::factory("item", $id);
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
 
     if ($item->is_album()) {
@@ -110,8 +116,10 @@ class Quick_Controller extends Controller {
   }
 
   public function form_edit($id) {
-    $item = ORM::factory("item", $id);
+    $item = model_cache::get("item", $id);
+    access::required("view", $item);
     access::required("edit", $item);
+
     if ($item->is_album()) {
       $form = album::get_edit_form($item);
     } else {
diff --git a/modules/gallery/controllers/rest.php b/modules/gallery/controllers/rest.php
index 11a6bbac..2edf079f 100644
--- a/modules/gallery/controllers/rest.php
+++ b/modules/gallery/controllers/rest.php
@@ -86,21 +86,20 @@ class REST_Controller extends Controller {
       return Kohana::show_404();
     }
 
-    if ($request_method != "get") {
-      access::verify_csrf();
-    }
-
     switch ($request_method) {
     case "get":
       return $this->_show($resource);
 
     case "put":
+      access::verify_csrf();
       return $this->_update($resource);
 
     case "delete":
+      access::verify_csrf();
       return $this->_delete($resource);
 
     case "post":
+      access::verify_csrf();
       return $this->_create($resource);
     }
   }
@@ -111,17 +110,18 @@ class REST_Controller extends Controller {
       throw new Exception("@todo ERROR_MISSING_RESOURCE_TYPE");
     }
 
-    // @todo this needs security checks
     $resource = ORM::factory($this->resource_type, $resource_id);
     if (!$resource->loaded) {
       return Kohana::show_404();
     }
 
+    // Security checks must be performed in _form_edit
     return $this->_form_edit($resource);
   }
 
   /* We're adding a new item, pass along any additional parameters. */
   public function form_add($parameters) {
+    // Security checks must be performed in _form_add
     return $this->_form_add($parameters);
   }
 
diff --git a/modules/gallery/controllers/simple_uploader.php b/modules/gallery/controllers/simple_uploader.php
index ec2a5ab9..dfbd4f17 100644
--- a/modules/gallery/controllers/simple_uploader.php
+++ b/modules/gallery/controllers/simple_uploader.php
@@ -20,6 +20,7 @@
 class Simple_Uploader_Controller extends Controller {
   public function app($id) {
     $item = ORM::factory("item", $id);
+    access::required("view", $item);
     access::required("add", $item);
 
     $v = new View("simple_uploader.html");
@@ -33,13 +34,13 @@ class Simple_Uploader_Controller extends Controller {
 
   public function add_photo($id) {
     $album = ORM::factory("item", $id);
+    access::required("view", $album);
     access::required("add", $album);
     access::verify_csrf();
 
     $file_validation = new Validation($_FILES);
     $file_validation->add_rules("Filedata", "upload::valid", "upload::type[gif,jpg,png,flv,mp4]");
     if ($file_validation->validate()) {
-
       // SimpleUploader.swf does not yet call /start directly, so simulate it here for now.
       if (!batch::in_progress()) {
         batch::start();
@@ -48,7 +49,7 @@ class Simple_Uploader_Controller extends Controller {
       $temp_filename = upload::save("Filedata");
       try {
         $name = substr(basename($temp_filename), 10);  // Skip unique identifier Kohana adds
-        $title = $this->convert_filename_to_title($name);
+        $title = item::convert_filename_to_title($name);
         $path_info = pathinfo($temp_filename);
         if (array_key_exists("extension", $path_info) &&
             in_array(strtolower($path_info["extension"]), array("flv", "mp4"))) {
@@ -69,18 +70,11 @@ class Simple_Uploader_Controller extends Controller {
     print "File Received";
   }
 
-  /**
-   * We should move this into a helper somewhere.. but where is appropriate?
-   */
-  private function convert_filename_to_title($filename) {
-    $title = strtr($filename, "_", " ");
-    $title = preg_replace("/\..*?$/", "", $title);
-    $title = preg_replace("/ +/", " ", $title);
-    return $title;
-  }
-
   public function finish() {
+    access::verify_csrf();
+
     batch::stop();
     print json_encode(array("result" => "success"));
   }
+
 }