From 00ee91837faf4807fb17dde3272ca8248a9dcd94 Mon Sep 17 00:00:00 2001
From: Tim Almdal <tnalmdal@shaw.ca>
Date: Mon, 5 Oct 2009 14:04:27 -0700
Subject: Convert direct lookups for the user table using ORM to using the
 user::lookup_by_name and user_lookup API methods. Convert the Admin_User
 controller Convert the login and password change controller Change the item
 model to call user::lookup to get the owner. On the log model, delete the
 relationship between the log and user table, and replace with a call to
 user::lookup (cherry picked from commit
 194cc3b27a73afe5119da9f09407c1e068dc6fa3) Create the get_user_list,
 lookup_by_name, lookup_by_hash and get_group_list api functions

---
 modules/gallery/models/item.php          |  2 +-
 modules/gallery/models/log.php           | 17 ++++++++-
 modules/user/controllers/admin_users.php | 65 +++++++++++++++-----------------
 modules/user/controllers/login.php       |  5 +--
 modules/user/controllers/password.php    | 15 +++-----
 modules/user/helpers/group.php           | 14 +++++++
 modules/user/helpers/user.php            | 55 ++++++++++++++++++++++-----
 7 files changed, 114 insertions(+), 59 deletions(-)

(limited to 'modules')

diff --git a/modules/gallery/models/item.php b/modules/gallery/models/item.php
index 6499fd2d..6f0e3525 100644
--- a/modules/gallery/models/item.php
+++ b/modules/gallery/models/item.php
@@ -333,7 +333,7 @@ class Item_Model extends ORM_MPTT {
       // This relationship depends on an outside module, which may not be present so handle
       // failures gracefully.
       try {
-        return model_cache::get("user", $this->owner_id);
+        return user::lookup($this->owner_id);
       } catch (Exception $e) {
         return null;
       }
diff --git a/modules/gallery/models/log.php b/modules/gallery/models/log.php
index 6734afb8..d143d7bd 100644
--- a/modules/gallery/models/log.php
+++ b/modules/gallery/models/log.php
@@ -18,5 +18,20 @@
  * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA  02110-1301, USA.
  */
 class Log_Model extends ORM {
-  protected $has_one = array("user");
+  /**
+   * @see ORM::__get()
+   */
+  public function __get($column) {
+    if ($column == "user") {
+      // This relationship depends on an outside module, which may not be present so handle
+      // failures gracefully.
+      try {
+        return user::lookup($this->user_id);
+      } catch (Exception $e) {
+        return null;
+      }
+    } else {
+      return parent::__get($column);
+    }
+  }
 }
diff --git a/modules/user/controllers/admin_users.php b/modules/user/controllers/admin_users.php
index 0b748955..a8a8cd95 100644
--- a/modules/user/controllers/admin_users.php
+++ b/modules/user/controllers/admin_users.php
@@ -21,8 +21,8 @@ class Admin_Users_Controller extends Admin_Controller {
   public function index() {
     $view = new Admin_View("admin.html");
     $view->content = new View("admin_users.html");
-    $view->content->users = ORM::factory("user")->orderby("name")->find_all();
-    $view->content->groups = ORM::factory("group")->orderby("name")->find_all();
+    $view->content->users = user::get_user_list(array("orderby" => array("name" => "ASC")));
+    $view->content->groups = group::get_group_list(array("orderby" => array("name" => "ASC")));
     print $view;
   }
 
@@ -32,8 +32,7 @@ class Admin_Users_Controller extends Admin_Controller {
     $form = user::get_add_form_admin();
     $valid = $form->validate();
     $name = $form->add_user->inputs["name"]->value;
-    $user = ORM::factory("user")->where("name", $name)->find();
-    if ($user->loaded) {
+    if ($user = user::lookup_by_name($name)) {
       $form->add_user->inputs["name"]->add_error("in_use", 1);
       $valid = false;
     }
@@ -70,8 +69,8 @@ class Admin_Users_Controller extends Admin_Controller {
       access::forbidden();
     }
 
-    $user = ORM::factory("user", $id);
-    if (!$user->loaded) {
+    $user = user::lookup($id);
+    if (empty($user)) {
       kohana::show_404();
     }
 
@@ -91,8 +90,8 @@ class Admin_Users_Controller extends Admin_Controller {
   }
 
   public function delete_user_form($id) {
-    $user = ORM::factory("user", $id);
-    if (!$user->loaded) {
+    $user = user::lookup($id);
+    if (empty($user)) {
       kohana::show_404();
     }
     print user::get_delete_form_admin($user);
@@ -101,8 +100,8 @@ class Admin_Users_Controller extends Admin_Controller {
   public function edit_user($id) {
     access::verify_csrf();
 
-    $user = ORM::factory("user", $id);
-    if (!$user->loaded) {
+    $user = user::lookup($id);
+    if (empty($user)) {
       kohana::show_404();
     }
 
@@ -110,12 +109,9 @@ class Admin_Users_Controller extends Admin_Controller {
     $valid = $form->validate();
     if ($valid) {
       $new_name = $form->edit_user->inputs["name"]->value;
+      $temp_user = user::lookup_by_name($new_name);
       if ($new_name != $user->name &&
-          ORM::factory("user")
-          ->where("name", $new_name)
-          ->where("id !=", $user->id)
-          ->find()
-          ->loaded) {
+          ($temp_user && $temp_user->id != $user->id)) {
         $form->edit_user->inputs["name"]->add_error("in_use", 1);
         $valid = false;
       } else {
@@ -151,8 +147,8 @@ class Admin_Users_Controller extends Admin_Controller {
   }
 
   public function edit_user_form($id) {
-    $user = ORM::factory("user", $id);
-    if (!$user->loaded) {
+    $user = user::lookup($id);
+    if (empty($user)) {
       kohana::show_404();
     }
 
@@ -166,23 +162,23 @@ class Admin_Users_Controller extends Admin_Controller {
 
   public function add_user_to_group($user_id, $group_id) {
     access::verify_csrf();
-    $group = ORM::factory("group", $group_id);
-    $user = ORM::factory("user", $user_id);
+    $group = group::lookup($group_id);
+    $user = user::lookup($user_id);
     $group->add($user);
     $group->save();
   }
 
   public function remove_user_from_group($user_id, $group_id) {
     access::verify_csrf();
-    $group = ORM::factory("group", $group_id);
-    $user = ORM::factory("user", $user_id);
+    $group = group::lookup($group_id);
+    $user = user::lookup($user_id);
     $group->remove($user);
     $group->save();
   }
 
   public function group($group_id) {
     $view = new View("admin_users_group.html");
-    $view->group = ORM::factory("group", $group_id);
+    $view->group = group::lookup($group_id);
     print $view;
   }
 
@@ -193,8 +189,8 @@ class Admin_Users_Controller extends Admin_Controller {
     $valid = $form->validate();
     if ($valid) {
       $new_name = $form->add_group->inputs["name"]->value;
-      $group = ORM::factory("group")->where("name", $new_name)->find();
-      if ($group->loaded) {
+      $group = group::lookup_by_name($new_name);
+      if (!empty($group)) {
         $form->add_group->inputs["name"]->add_error("in_use", 1);
         $valid = false;
       }
@@ -219,8 +215,8 @@ class Admin_Users_Controller extends Admin_Controller {
   public function delete_group($id) {
     access::verify_csrf();
 
-    $group = ORM::factory("group", $id);
-    if (!$group->loaded) {
+    $group = group::lookup($id);
+    if (empty($group)) {
       kohana::show_404();
     }
 
@@ -240,19 +236,20 @@ class Admin_Users_Controller extends Admin_Controller {
   }
 
   public function delete_group_form($id) {
-    $group = ORM::factory("group", $id);
-    if (!$group->loaded) {
+    $group = group::lookup($id);
+    if (empty($group)) {
       kohana::show_404();
     }
+
     print group::get_delete_form_admin($group);
   }
 
   public function edit_group($id) {
     access::verify_csrf();
 
-    $group = ORM::factory("group", $id);
-    if (!$group->loaded) {
-      kohana::show_404();
+    $group = group::lookup($id);
+    if (empty($group)) {
+       kohana::show_404();
     }
 
     $form = group::get_edit_form_admin($group);
@@ -260,7 +257,7 @@ class Admin_Users_Controller extends Admin_Controller {
 
     if ($valid) {
       $new_name = $form->edit_group->inputs["name"]->value;
-      $group = ORM::factory("group")->where("name", $new_name)->find();
+      $group = group::lookup_by_name($name);
       if ($group->loaded) {
         $form->edit_group->inputs["name"]->add_error("in_use", 1);
         $valid = false;
@@ -282,8 +279,8 @@ class Admin_Users_Controller extends Admin_Controller {
   }
 
   public function edit_group_form($id) {
-    $group = ORM::factory("group", $id);
-    if (!$group->loaded) {
+    $group = group::lookup($id);
+    if (empty($group)) {
       kohana::show_404();
     }
 
diff --git a/modules/user/controllers/login.php b/modules/user/controllers/login.php
index 8bee7db5..2c4bd557 100644
--- a/modules/user/controllers/login.php
+++ b/modules/user/controllers/login.php
@@ -53,13 +53,12 @@ class Login_Controller extends Controller {
       print $form;
     }
   }
-
   private function _auth($url) {
     $form = user::get_login_form($url);
     $valid = $form->validate();
     if ($valid) {
-      $user = ORM::factory("user")->where("name", $form->login->inputs["name"]->value)->find();
-      if (!$user->loaded || !user::is_correct_password($user, $form->login->password->value)) {
+      $user = user::lookup_by_name($form->login->inputs["name"]->value);
+      if (empty($user) || !user::is_correct_password($user, $form->login->password->value)) {
         log::warning(
           "user",
           t("Failed login for %name",
diff --git a/modules/user/controllers/password.php b/modules/user/controllers/password.php
index 4629bbf2..e8b08960 100644
--- a/modules/user/controllers/password.php
+++ b/modules/user/controllers/password.php
@@ -32,10 +32,8 @@ class Password_Controller extends Controller {
     if (request::method() == "post") {
       $this->_change_password();
     } else {
-      $user = ORM::factory("user")
-        ->where("hash", Input::instance()->get("key"))
-        ->find();
-      if ($user->loaded) {
+      $user = user::lookup_by_hash(Input::instance()->get("key"));
+      if (!empty($user)) {
         print $this->_new_password_form($user->hash);
       } else {
         throw new Exception("@todo FORBIDDEN", 503);
@@ -48,7 +46,7 @@ class Password_Controller extends Controller {
 
     $valid = $form->validate();
     if ($valid) {
-      $user = ORM::factory("user")->where("name", $form->reset->inputs["name"]->value)->find();
+      $user = user::lockup_by_name($form->reset->inputs["name"]->value);
       if (!$user->loaded || empty($user->email)) {
         $form->reset->inputs["name"]->add_error("no_email", 1);
         $valid = false;
@@ -118,11 +116,8 @@ class Password_Controller extends Controller {
   private function _change_password() {
     $view = $this->_new_password_form();
     if ($view->content->validate()) {
-      $user = ORM::factory("user")
-        ->where("hash", $view->content->reset->hash->value)
-        ->find();
-
-      if (!$user->loaded) {
+      $user = user::lookup_by_hash(Input::instance()->get("key"));
+      if (empty($user)) {
         throw new Exception("@todo FORBIDDEN", 503);
       }
 
diff --git a/modules/user/helpers/group.php b/modules/user/helpers/group.php
index b13895bc..2590c7d1 100644
--- a/modules/user/helpers/group.php
+++ b/modules/user/helpers/group.php
@@ -73,6 +73,20 @@ class group_Core {
     return null;
   }
 
+  /**
+   * List the users
+   * @param mixed      filters (@see Database.php
+   * @return array     the group list.
+   */
+  static function get_group_list($filter=array()) {
+    $group = ORM::factory("group");
+
+    foreach($filter as $method => $args) {
+      $group->$method($args);
+    }
+    return $group->find_all();
+  }
+
   static function get_edit_form_admin($group) {
     $form = new Forge("admin/users/edit_group/$group->id", "", "post", array("id" => "g-edit-group-form"));
     $form_group = $form->group("edit_group")->label(t("Edit Group"));
diff --git a/modules/user/helpers/user.php b/modules/user/helpers/user.php
index 432994d6..6a155768 100644
--- a/modules/user/helpers/user.php
+++ b/modules/user/helpers/user.php
@@ -304,27 +304,62 @@ class user_Core {
    * @return User_Model  the user object, or null if the id was invalid.
    */
   static function lookup($id) {
-    $user = model_cache::get("user", $id);
-    if ($user->loaded) {
-      return $user;
-    }
-    return null;
+    return self::_lookup_user_by_field("id", $id);
   }
 
   /**
    * Look up a user by name.
-   * @param integer      $id the user name
+   * @param integer      $name the user name
    * @return User_Model  the user object, or null if the name was invalid.
    */
   static function lookup_by_name($name) {
-    $user = model_cache::get("user", $name, "name");
-    if ($user->loaded) {
-      return $user;
+    return self::_lookup_user_by_field("name", $name);
+  }
+
+  /**
+   * Look up a user by hash.
+   * @param integer      $hash the user hash value
+   * @return User_Model  the user object, or null if the name was invalid.
+   */
+  static function lookup_by_hash($hash) {
+    return self::_lookup_user_by_field("hash", $hash);
+  }
+
+  /**
+   * List the users
+   * @param mixed      filters (@see Database.php
+   * @return array     the user list.
+   */
+  static function get_user_list($filter=array()) {
+    $user = ORM::factory("user");
+
+    foreach($filter as $method => $args) {
+      $user->$method($args);
     }
-    return null;
+    return $user->find_all();
   }
 
   /**
+   * Look up a user by field value.
+   * @param string      search field
+   * @param string      search value
+   * @return User_Core  the user object, or null if the name was invalid.
+   */
+ private static function _lookup_user_by_field($field_name, $value) {
+    try {
+      $user = model_cache::get("user", $value, $field_name);
+      if ($user->loaded) {
+        return $user;
+      }
+    } catch (Exception $e) {
+      if (strpos($e->getMessage(), "MISSING_MODEL") === false) {
+       throw $e;
+      }
+    }
+    return null;
+  }
+
+ /**
    * Create a hashed password using md5 plus salt.
    * @param string $password plaintext password
    * @param string $salt (optional) salt or hash containing salt (randomly generated if omitted)
-- 
cgit v1.2.3


From 1ee7d24766a3055d4199251f6ce990714b9c3641 Mon Sep 17 00:00:00 2001
From: Tim Almdal <tnalmdal@shaw.ca>
Date: Fri, 9 Oct 2009 01:12:05 -0700
Subject: Add support for the in filter to get_user_list and get_group_list.
 Convert the notifications to use user::get_user_list instead of accessing the
 table directly.

---
 modules/notification/helpers/notification.php | 21 +++++++++++++--------
 modules/user/helpers/group.php                |  8 +++++++-
 modules/user/helpers/user.php                 |  8 +++++++-
 3 files changed, 27 insertions(+), 10 deletions(-)

(limited to 'modules')

diff --git a/modules/notification/helpers/notification.php b/modules/notification/helpers/notification.php
index 88d92b16..150616ab 100644
--- a/modules/notification/helpers/notification.php
+++ b/modules/notification/helpers/notification.php
@@ -67,15 +67,20 @@ class notification {
   }
 
   static function get_subscribers($item) {
-    // @todo don't access the user table directly
     // @todo only return distinct email addresses
-    $users = ORM::factory("user")
-      ->join("subscriptions", "users.id", "subscriptions.user_id")
-      ->join("items", "subscriptions.item_id", "items.id")
-      ->where("email IS NOT", null)
-      ->where("items.left_ptr <=", $item->left_ptr)
-      ->where("items.right_ptr >", $item->right_ptr)
-      ->find_all();
+    $subscriber_ids = array();
+    foreach (ORM::factory("subscription")
+             ->select("user_id")
+             ->join("items", "subscriptions.item_id", "items.id")
+             ->where("items.left_ptr <=", $item->left_ptr)
+             ->where("items.right_ptr >", $item->right_ptr)
+             ->find_all()
+             ->as_array() as $subscriber) {
+      $subscriber_ids[] = $subscriber->user_id;
+    }
+
+    $users = user::get_user_list(array("in" => array("id", $subscriber_ids),
+                                       "where" => array("email IS NOT" => null)));
 
     $subscribers = array();
     foreach ($users as $user) {
diff --git a/modules/user/helpers/group.php b/modules/user/helpers/group.php
index 2590c7d1..fbc5157d 100644
--- a/modules/user/helpers/group.php
+++ b/modules/user/helpers/group.php
@@ -82,7 +82,13 @@ class group_Core {
     $group = ORM::factory("group");
 
     foreach($filter as $method => $args) {
-      $group->$method($args);
+      switch ($method) {
+      case "in":
+        $group->in($args[0], $args[1]);
+        break;
+      default:
+        $group->$method($args);
+      }
     }
     return $group->find_all();
   }
diff --git a/modules/user/helpers/user.php b/modules/user/helpers/user.php
index 6a155768..c6a7735b 100644
--- a/modules/user/helpers/user.php
+++ b/modules/user/helpers/user.php
@@ -334,7 +334,13 @@ class user_Core {
     $user = ORM::factory("user");
 
     foreach($filter as $method => $args) {
-      $user->$method($args);
+      switch ($method) {
+      case "in":
+        $user->in($args[0], $args[1]);
+        break;
+      default:
+        $user->$method($args);
+      }
     }
     return $user->find_all();
   }
-- 
cgit v1.2.3


From 79b4b8bdc606ecd46b258e32b3973511a43d38b0 Mon Sep 17 00:00:00 2001
From: Tim Almdal <tnalmdal@shaw.ca>
Date: Fri, 9 Oct 2009 01:25:03 -0700
Subject: update the Access_Helper_Test to use the user::lookup_by_name API
 method.

---
 modules/gallery/tests/Access_Helper_Test.php | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'modules')

diff --git a/modules/gallery/tests/Access_Helper_Test.php b/modules/gallery/tests/Access_Helper_Test.php
index 59cec453..72d7e04c 100644
--- a/modules/gallery/tests/Access_Helper_Test.php
+++ b/modules/gallery/tests/Access_Helper_Test.php
@@ -33,7 +33,7 @@ class Access_Helper_Test extends Unit_Test_Case {
     } catch (Exception $e) { }
 
     try {
-      $user = ORM::factory("user")->where("name", "access_test")->find();
+      $user = user::lookup_by_name("access_test");
       if ($user->loaded) {
         $user->delete();
       }
@@ -307,7 +307,7 @@ class Access_Helper_Test extends Unit_Test_Case {
     $group->save();
     access::allow($group, "edit", $root);
 
-    $user = ORM::factory("user", $user->id);  // reload() does not flush related columns
+    $user = user::lookup($user->id);  // reload() does not flush related columns
     user::set_active($user);
 
     // And verify that the user can edit.
-- 
cgit v1.2.3


From 0a66ef9cc785fa5fb3614e7664c424d13ff09728 Mon Sep 17 00:00:00 2001
From: Bharat Mediratta <bharat@menalto.com>
Date: Tue, 13 Oct 2009 10:36:50 -0700
Subject: Don't allow users to change the file extension of photos/movies

If you can change the extension, then you can alter the way the server
handles the file, which is a security problem.  So for example, you
can change a .JPG to a .PHP and then if you put some malicious PHP
code in the EXIF data, you can get the server to execute
it. Vulnerability is low because only users who have edit permissions
could do this.

Fixes ticket #846
---
 modules/gallery/controllers/movies.php | 13 ++++++++++++-
 modules/gallery/controllers/photos.php | 12 +++++++++++-
 modules/gallery/helpers/movie.php      |  3 ++-
 modules/gallery/helpers/photo.php      |  3 ++-
 4 files changed, 27 insertions(+), 4 deletions(-)

(limited to 'modules')

diff --git a/modules/gallery/controllers/movies.php b/modules/gallery/controllers/movies.php
index 2a917c58..01a9fc8b 100644
--- a/modules/gallery/controllers/movies.php
+++ b/modules/gallery/controllers/movies.php
@@ -61,7 +61,18 @@ class Movies_Controller extends Items_Controller {
     access::required("edit", $movie);
 
     $form = movie::get_edit_form($movie);
-    if ($valid = $form->validate()) {
+    $valid = $form->validate();
+
+    if ($valid) {
+      $new_ext = pathinfo($form->edit_item->filename->value, PATHINFO_EXTENSION);
+      $old_ext = pathinfo($photo->name, PATHINFO_EXTENSION);
+      if (strcasecmp($new_ext, $old_ext)) {
+        $form->edit_item->filename->add_error("illegal_extension", 1);
+        $valid = false;
+      }
+    }
+
+    if ($valid) {
       if ($form->edit_item->filename->value != $movie->name ||
           $form->edit_item->slug->value != $movie->slug) {
         // Make sure that there's not a name or slug conflict
diff --git a/modules/gallery/controllers/photos.php b/modules/gallery/controllers/photos.php
index 81e7519e..fbff53ce 100644
--- a/modules/gallery/controllers/photos.php
+++ b/modules/gallery/controllers/photos.php
@@ -63,7 +63,17 @@ class Photos_Controller extends Items_Controller {
 
     $form = photo::get_edit_form($photo);
     $valid = $form->validate();
-    if ($valid = $form->validate()) {
+
+    if ($valid) {
+      $new_ext = pathinfo($form->edit_item->filename->value, PATHINFO_EXTENSION);
+      $old_ext = pathinfo($photo->name, PATHINFO_EXTENSION);
+      if (strcasecmp($new_ext, $old_ext)) {
+        $form->edit_item->filename->add_error("illegal_extension", 1);
+        $valid = false;
+      }
+    }
+
+    if ($valid) {
       if ($form->edit_item->filename->value != $photo->name ||
           $form->edit_item->slug->value != $photo->slug) {
         // Make sure that there's not a name or slug conflict
diff --git a/modules/gallery/helpers/movie.php b/modules/gallery/helpers/movie.php
index 98419387..9ca28fe6 100644
--- a/modules/gallery/helpers/movie.php
+++ b/modules/gallery/helpers/movie.php
@@ -141,7 +141,8 @@ class movie_Core {
       ->callback("item::validate_no_slashes")
       ->error_messages("no_slashes", t("The movie name can't contain a \"/\""))
       ->callback("item::validate_no_trailing_period")
-      ->error_messages("no_trailing_period", t("The movie name can't end in \".\""));
+      ->error_messages("no_trailing_period", t("The movie name can't end in \".\""))
+      ->error_messages("illegal_extension", t("You cannot change the filename extension"));
     $group->input("slug")->label(t("Internet Address"))->value($movie->slug)
       ->callback("item::validate_url_safe")
       ->error_messages(
diff --git a/modules/gallery/helpers/photo.php b/modules/gallery/helpers/photo.php
index 99f28753..6677ddc9 100644
--- a/modules/gallery/helpers/photo.php
+++ b/modules/gallery/helpers/photo.php
@@ -169,7 +169,8 @@ class photo_Core {
       ->callback("item::validate_no_slashes")
       ->error_messages("no_slashes", t("The photo name can't contain a \"/\""))
       ->callback("item::validate_no_trailing_period")
-      ->error_messages("no_trailing_period", t("The photo name can't end in \".\""));
+      ->error_messages("no_trailing_period", t("The photo name can't end in \".\""))
+      ->error_messages("illegal_extension", t("You cannot change the filename extension"));
     $group->input("slug")->label(t("Internet Address"))->value($photo->slug)
       ->callback("item::validate_url_safe")
       ->error_messages(
-- 
cgit v1.2.3