From 098b57bf18112d0a3173dbe28b5ed76782431ff7 Mon Sep 17 00:00:00 2001 From: Tim Almdal Date: Mon, 19 Oct 2009 12:53:44 -0700 Subject: Simplify the user interface by moving the password reset functionality into the user module Bagging the User_Definition and Group_Definition abstract classes and replacing them with interfaces with the same names. Make sure all the unit tests work. --- modules/gallery/controllers/password.php | 133 -------------- modules/gallery/helpers/access.php | 4 +- modules/gallery/libraries/Identity.php | 34 +--- modules/gallery/libraries/drivers/Identity.php | 196 ++------------------- modules/gallery/tests/Albums_Controller_Test.php | 1 + modules/gallery/tests/Photos_Controller_Test.php | 2 +- modules/gallery/views/admin_identity.html.php | 6 +- modules/gallery/views/reset_password.html.php | 17 -- modules/user/controllers/password.php | 133 ++++++++++++++ modules/user/helpers/group.php | 4 +- modules/user/helpers/user.php | 15 +- .../user/libraries/drivers/Identity/Gallery.php | 99 ++--------- modules/user/models/group.php | 2 +- modules/user/models/user.php | 2 +- modules/user/views/admin_users.html.php | 2 +- modules/user/views/reset_password.html.php | 17 ++ 16 files changed, 206 insertions(+), 461 deletions(-) delete mode 100644 modules/gallery/controllers/password.php delete mode 100644 modules/gallery/views/reset_password.html.php create mode 100644 modules/user/controllers/password.php create mode 100644 modules/user/views/reset_password.html.php (limited to 'modules') diff --git a/modules/gallery/controllers/password.php b/modules/gallery/controllers/password.php deleted file mode 100644 index ce6d67b1..00000000 --- a/modules/gallery/controllers/password.php +++ /dev/null @@ -1,133 +0,0 @@ -_send_reset(); - } else { - print $this->_reset_form(); - } - } - - public function do_reset() { - if (request::method() == "post") { - $this->_change_password(); - } else { - $user = Identity::lookup_user_by_hash(Input::instance()->get("key")); - if (!empty($user)) { - print $this->_new_password_form($user->hash); - } else { - throw new Exception("@todo FORBIDDEN", 503); - } - } - } - - private function _send_reset() { - $form = $this->_reset_form(); - - $valid = $form->validate(); - if ($valid) { - $user = Identity::lookup_user_by_name($form->reset->inputs["name"]->value); - if (!$user->loaded || empty($user->email)) { - $form->reset->inputs["name"]->add_error("no_email", 1); - $valid = false; - } - } - - if ($valid) { - $user->hash = md5(rand()); - $user->save(); - $message = new View("reset_password.html"); - $message->confirm_url = url::abs_site("password/do_reset?key=$user->hash"); - $message->user = $user; - - Sendmail::factory() - ->to($user->email) - ->subject(t("Password Reset Request")) - ->header("Mime-Version", "1.0") - ->header("Content-type", "text/html; charset=iso-8859-1") - ->message($message->render()) - ->send(); - - log::success( - "user", - t("Password reset email sent for user %name", array("name" => $user->name))); - } else { - // Don't include the username here until you're sure that it's XSS safe - log::warning( - "user", "Password reset email requested for bogus user"); - } - - message::success(t("Password reset email sent")); - print json_encode( - array("result" => "success")); - } - - private function _reset_form() { - $form = new Forge(url::current(true), "", "post", array("id" => "g-reset-form")); - $group = $form->group("reset")->label(t("Reset Password")); - $group->input("name")->label(t("Username"))->id("g-name")->class(null)->rules("required"); - $group->inputs["name"]->error_messages("no_email", t("No email, unable to reset password")); - $group->submit("")->value(t("Reset")); - - return $form; - } - - private function _new_password_form($hash=null) { - $template = new Theme_View("page.html", "reset"); - - $form = new Forge("password/do_reset", "", "post", array("id" => "g-change-password-form")); - $group = $form->group("reset")->label(t("Change Password")); - $hidden = $group->hidden("hash"); - if (!empty($hash)) { - $hidden->value($hash); - } - $group->password("password")->label(t("Password"))->id("g-password") - ->rules("required|length[1,40]"); - $group->password("password2")->label(t("Confirm Password"))->id("g-password2") - ->matches($group->password); - $group->inputs["password2"]->error_messages( - "mistyped", t("The password and the confirm password must match")); - $group->submit("")->value(t("Update")); - - $template->content = $form; - return $template; - } - - private function _change_password() { - $view = $this->_new_password_form(); - if ($view->content->validate()) { - $user = Identity::lookup_user_by_hash(Input::instance()->get("key")); - if (empty($user)) { - throw new Exception("@todo FORBIDDEN", 503); - } - - $user->password = $view->content->reset->password->value; - $user->hash = null; - $user->save(); - message::success(t("Password reset successfully")); - url::redirect(item::root()->abs_url()); - } else { - print $view; - } - } -} \ No newline at end of file diff --git a/modules/gallery/helpers/access.php b/modules/gallery/helpers/access.php index 21f4de81..fba161e3 100644 --- a/modules/gallery/helpers/access.php +++ b/modules/gallery/helpers/access.php @@ -197,8 +197,8 @@ class access_Core { * @param Item_Model $item * @param boolean $value */ - private static function _set(Group_Model $group, $perm_name, $album, $value) { - if (get_class($group) != "Group_Model") { + private static function _set(Group_Definition $group, $perm_name, $album, $value) { + if (!($group instanceof Group_Definition)) { throw new Exception("@todo PERMISSIONS_ONLY_WORK_ON_GROUPS"); } if (!$album->loaded) { diff --git a/modules/gallery/libraries/Identity.php b/modules/gallery/libraries/Identity.php index 9e5f0bb5..e77fd2d2 100644 --- a/modules/gallery/libraries/Identity.php +++ b/modules/gallery/libraries/Identity.php @@ -133,37 +133,17 @@ class Identity_Core { } /** - * @see Identity_Driver::hash_password. - */ - static function hash_password($password) { - return self::instance()->driver->hash_password($password); - } - - /** - * Look up a user by id. - * @param integer $id the user id - * @return User_Definition the user object, or null if the id was invalid. + * @see Identity_Driver::lookup_user. */ static function lookup_user($id) { - return self::instance()->driver->lookup_user_by_field("id", $id); + return self::instance()->driver->lookup_user($id); } /** - * Look up a user by name. - * @param integer $name the user name - * @return User_Definition the user object, or null if the name was invalid. + * @see Identity_Driver::lookup_user_by_name. */ static function lookup_user_by_name($name) { - return self::instance()->driver->lookup_user_by_field("name", $name); - } - - /** - * Look up a user by hash. - * @param string $name the user name - * @return User_Definition the user object, or null if the name was invalid. - */ - static function lookup_user_by_hash($hash) { - return self::instance()->driver->lookup_user_by_field("hash", $hash); + return self::instance()->driver->lookup_user_by_name($name); } /** @@ -188,12 +168,10 @@ class Identity_Core { } /** - * Look up a group by name. - * @param integer $id the group name - * @return Group_Definition the group object, or null if the name was invalid. + * @see Identity_Driver::lookup_group_by_name. */ static function lookup_group_by_name($name) { - return self::instance()->driver->lookup_group_by_field("name", $name); + return self::instance()->driver->lookup_group_by_name($name); } /** diff --git a/modules/gallery/libraries/drivers/Identity.php b/modules/gallery/libraries/drivers/Identity.php index a9e1a75b..6ab001cb 100644 --- a/modules/gallery/libraries/drivers/Identity.php +++ b/modules/gallery/libraries/drivers/Identity.php @@ -45,19 +45,18 @@ interface Identity_Driver { public function is_correct_password($user, $password); /** - * Create the hashed passwords. - * @param string $password a plaintext password - * @return string hashed password + * Look up a user by id. + * @param integer id + * @return User_Definition the user object, or null if the name was invalid. */ - public function hash_password($password); + public function lookup_user($id); /** - * Look up a user by by search the specified field. - * @param string search field - * @param string search value - * @return User_Definition the user object, or null if the name was invalid. + * Look up a user by name. + * @param string name + * @return User_Definition the user object, or null if the name was invalid. */ - public function lookup_user_by_field($field, $value); + public function lookup_user_by_name($name); /** * Create a new group. @@ -90,181 +89,6 @@ interface Identity_Driver { } // End Identity Driver Definition -/** - * User Data wrapper - */ -abstract class User_Definition { - protected $user; - public function __get($column) { - switch ($column) { - case "id": - case "name": - case "full_name": - case "password": - case "login_count": - case "last_login": - case "email": - case "admin": - case "guest": - case "hash": - case "url": - case "locale": - case "groups": - case "hashed_password": - return $this->user->$column; - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - public function __set($column, $value) { - switch ($column) { - case "id": - case "groups": - throw new Exception("@todo READ ONLY FIELD: $column"); - break; - case "name": - case "full_name": - case "hashed_password": - case "password": - case "login_count": - case "last_login": - case "email": - case "admin": - case "guest": - case "hash": - case "url": - case "locale": - $this->user->$column = $value; - break; - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - public function __isset($column) { - return isset($this->user->$column); - } - - public function __unset($column) { - switch ($column) { - case "id": - case "groups": - throw new Exception("@todo READ ONLY FIELD: $column"); - break; - case "name": - case "full_name": - case "password": - case "login_count": - case "last_login": - case "email": - case "admin": - case "guest": - case "hash": - case "url": - case "locale": - case "hashed_password": - unset($this->user->$column); - break; - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - /** - * Return a url to the user's avatar image. - * @param integer $size the target size of the image (default 80px) - * @return string a url - */ - abstract public function avatar_url($size=80, $default=null); - - /** - * Return the best version of the user's name. Either their specified full name, or fall back - * to the user name. - * @return string - */ - abstract public function display_name(); - - /** - * Return the internal user object without the wrapper. - * This method is used by implementing classes to access the internal user object. - * Consider it pseudo private and only declared public as PHP as not internal or friend modifier - */ - public function _uncloaked() { - return $this->user; - } - - abstract public function save(); - abstract public function delete(); -} - -/** - * Group Data wrapper - */ -abstract class Group_Definition { - protected $group; - - public function __get($column) { - switch ($column) { - case "id": - case "name": - case "special": - case "users": - return $this->group->$column; - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - public function __set($column, $value) { - switch ($column) { - case "id": - case "users": - throw new Exception("@todo READ ONLY FIELD: $column"); - break; - case "name": - case "special": - $this->group->$column = $value; - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - public function __isset($column) { - return isset($this->group->$column); - } - - public function __unset($column) { - switch ($column) { - case "id": - case "users": - throw new Exception("@todo READ ONLY FIELD: $column"); - break; - case "name": - case "special": - unset($this->group->$column); - default: - throw new Exception("@todo UNSUPPORTED FIELD: $column"); - break; - } - } - - /** - * Return the internal group object without the wrapper. - * This method is used by implementing classes to access the internal group object. - * Consider it pseudo private and only declared public as PHP as not internal or friend modifier - */ - public function _uncloaked() { - return $this->group; - } +interface Group_Definition {} - abstract public function save(); - abstract public function delete(); - abstract public function add($user); - abstract public function remove($user); -} +interface User_Definition {} diff --git a/modules/gallery/tests/Albums_Controller_Test.php b/modules/gallery/tests/Albums_Controller_Test.php index 046cb5ad..fa46d924 100644 --- a/modules/gallery/tests/Albums_Controller_Test.php +++ b/modules/gallery/tests/Albums_Controller_Test.php @@ -43,6 +43,7 @@ class Albums_Controller_Test extends Unit_Test_Case { $_POST["column"] = "weight"; $_POST["direction"] = "ASC"; $_POST["csrf"] = access::csrf_token(); + $_POST["slug"] = "new_name"; $_POST["_method"] = "put"; access::allow(Identity::everybody(), "edit", $root); diff --git a/modules/gallery/tests/Photos_Controller_Test.php b/modules/gallery/tests/Photos_Controller_Test.php index cdb4ae4f..59c3f78a 100644 --- a/modules/gallery/tests/Photos_Controller_Test.php +++ b/modules/gallery/tests/Photos_Controller_Test.php @@ -31,7 +31,7 @@ class Photos_Controller_Test extends Unit_Test_Case { $root = ORM::factory("item", 1); $photo = photo::create( $root, MODPATH . "gallery/tests/test.jpg", "test.jpeg", - "test", "test", Session::active_user(), "slug"); + "test", "test", Session::active_user()->id, "slug"); $orig_name = $photo->name; $_POST["filename"] = "test.jpeg"; diff --git a/modules/gallery/views/admin_identity.html.php b/modules/gallery/views/admin_identity.html.php index dcf1dbc1..1405cacb 100644 --- a/modules/gallery/views/admin_identity.html.php +++ b/modules/gallery/views/admin_identity.html.php @@ -15,11 +15,11 @@ height:165, modal: true, overlay: { - backgroundColor: '#000', - opacity: 0.5 + backgroundColor: '#000', + opacity: 0.5 }, buttons: { - "Continue": function() { + "Continue": function() { $("##g-dialog form").submit(); }, Cancel: function() { diff --git a/modules/gallery/views/reset_password.html.php b/modules/gallery/views/reset_password.html.php deleted file mode 100644 index 92ca4917..00000000 --- a/modules/gallery/views/reset_password.html.php +++ /dev/null @@ -1,17 +0,0 @@ - - - - <?= t("Password Reset Request") ?> - - -

-

- $user->full_name ? $user->full_name : $user->name)) ?> -

-

- %site_url. If you made this request, you can confirm it by clicking this link. If you didn't request this password reset, it's ok to ignore this mail.", - array("site_url" => html::mark_clean(url::base(false, "http")), - "confirm_url" => $confirm_url)) ?> -

- - diff --git a/modules/user/controllers/password.php b/modules/user/controllers/password.php new file mode 100644 index 00000000..a8f1c5ca --- /dev/null +++ b/modules/user/controllers/password.php @@ -0,0 +1,133 @@ +_send_reset(); + } else { + print $this->_reset_form(); + } + } + + public function do_reset() { + if (request::method() == "post") { + $this->_change_password(); + } else { + $user = user::lookup_user_by_field("hash", Input::instance()->get("key")); + if (!empty($user)) { + print $this->_new_password_form($user->hash); + } else { + throw new Exception("@todo FORBIDDEN", 503); + } + } + } + + private function _send_reset() { + $form = $this->_reset_form(); + + $valid = $form->validate(); + if ($valid) { + $user = Identity::lookup_user_by_name($form->reset->inputs["name"]->value); + if (!$user->loaded || empty($user->email)) { + $form->reset->inputs["name"]->add_error("no_email", 1); + $valid = false; + } + } + + if ($valid) { + $user->hash = md5(rand()); + $user->save(); + $message = new View("reset_password.html"); + $message->confirm_url = url::abs_site("password/do_reset?key=$user->hash"); + $message->user = $user; + + Sendmail::factory() + ->to($user->email) + ->subject(t("Password Reset Request")) + ->header("Mime-Version", "1.0") + ->header("Content-type", "text/html; charset=iso-8859-1") + ->message($message->render()) + ->send(); + + log::success( + "user", + t("Password reset email sent for user %name", array("name" => $user->name))); + } else { + // Don't include the username here until you're sure that it's XSS safe + log::warning( + "user", "Password reset email requested for bogus user"); + } + + message::success(t("Password reset email sent")); + print json_encode( + array("result" => "success")); + } + + private function _reset_form() { + $form = new Forge(url::current(true), "", "post", array("id" => "g-reset-form")); + $group = $form->group("reset")->label(t("Reset Password")); + $group->input("name")->label(t("Username"))->id("g-name")->class(null)->rules("required"); + $group->inputs["name"]->error_messages("no_email", t("No email, unable to reset password")); + $group->submit("")->value(t("Reset")); + + return $form; + } + + private function _new_password_form($hash=null) { + $template = new Theme_View("page.html", "reset"); + + $form = new Forge("password/do_reset", "", "post", array("id" => "g-change-password-form")); + $group = $form->group("reset")->label(t("Change Password")); + $hidden = $group->hidden("hash"); + if (!empty($hash)) { + $hidden->value($hash); + } + $group->password("password")->label(t("Password"))->id("g-password") + ->rules("required|length[1,40]"); + $group->password("password2")->label(t("Confirm Password"))->id("g-password2") + ->matches($group->password); + $group->inputs["password2"]->error_messages( + "mistyped", t("The password and the confirm password must match")); + $group->submit("")->value(t("Update")); + + $template->content = $form; + return $template; + } + + private function _change_password() { + $view = $this->_new_password_form(); + if ($view->content->validate()) { + $user = user::lookup_user_by_field("hash", Input::instance()->get("key")); + if (empty($user)) { + throw new Exception("@todo FORBIDDEN", 503); + } + + $user->password = $view->content->reset->password->value; + $user->hash = null; + $user->save(); + message::success(t("Password reset successfully")); + url::redirect(item::root()->abs_url()); + } else { + print $view; + } + } +} \ No newline at end of file diff --git a/modules/user/helpers/group.php b/modules/user/helpers/group.php index cf5c050f..8ad52564 100644 --- a/modules/user/helpers/group.php +++ b/modules/user/helpers/group.php @@ -42,14 +42,14 @@ class group_Core { * @see Identity_Driver::everbody. */ static function everybody() { - return Identity::instance()->everybody(); + return model_cache::get("group", 1); } /** * @see Identity_Driver::registered_users. */ static function registered_users() { - return Identity::instance()->everybody(); + return model_cache::get("group", 2); } /** diff --git a/modules/user/helpers/user.php b/modules/user/helpers/user.php index fa7b320f..5ef2b726 100644 --- a/modules/user/helpers/user.php +++ b/modules/user/helpers/user.php @@ -25,14 +25,21 @@ */ class user_Core { /** - * @see Identity_Driver::guest. + * Return the guest user. + * + * @return User_Model the user object */ static function guest() { return model_cache::get("user", 1); } /** - * @see Identity_Driver::create_user. + * Create a new user. + * + * @param string $name + * @param string $full_name + * @param string $password + * @return User_Definition the user object */ static function create($name, $full_name, $password) { $user = ORM::factory("user")->where("name", $name)->find(); @@ -53,7 +60,9 @@ class user_Core { } /** - * @see Identity_Driver::hash_password. + * Hash the password to the internal value + * @param string $password the user password + * @param string The hashed equivalent */ static function hash_password($password) { require_once(MODPATH . "user/lib/PasswordHash.php"); diff --git a/modules/user/libraries/drivers/Identity/Gallery.php b/modules/user/libraries/drivers/Identity/Gallery.php index 77db11a3..f405b710 100644 --- a/modules/user/libraries/drivers/Identity/Gallery.php +++ b/modules/user/libraries/drivers/Identity/Gallery.php @@ -25,14 +25,14 @@ class Identity_Gallery_Driver implements Identity_Driver { * @see Identity_Driver::guest. */ public function guest() { - return new Gallery_User(user::guest()); + return user::guest(); } /** * @see Identity_Driver::create_user. */ public function create_user($name, $full_name, $password) { - return new Gallery_User(user::create($name, $full_name, $password)); + return user::create($name, $full_name, $password); } /** @@ -67,122 +67,55 @@ class Identity_Gallery_Driver implements Identity_Driver { } /** - * @see Identity_Driver::hash_password. + * @see Identity_Driver::lookup_user. */ - public function hash_password($password) { - return user::hash_password($password); + public function lookup_user($id) { + return user::lookup_by_field("id", $id); } /** - * @see Identity_Driver::lookup_user_by_field. + * @see Identity_Driver::lookup_user_by_name. */ - public function lookup_user_by_field($field_name, $value) { - return new Gallery_User(user::lookup_by_field($field_name, $value)); + public function lookup_user_by_name($name) { + return user::lookup_by_field("name", $name); } /** * @see Identity_Driver::create_group. */ public function create_group($name) { - return new Gallery_Group(group::create($name)); + return group::create($name); } /** * @see Identity_Driver::everybody. */ public function everybody() { - return new Gallery_Group(group::everybody()); + return group::everybody(); } /** * @see Identity_Driver::registered_users. */ public function registered_users() { - return new Gallery_Group(group::registered_users()); + return group::registered_users(); } /** - * @see Identity_Driver::lookup_group_by_field. + * @see Identity_Driver::lookup_group_by_name. */ - public function lookup_group_by_field($field_name, $value) { - return new Gallery_Group(group::lookup_by_field($field_name, $value)); + static function lookup_group_by_name($name) { + return group::lookup_by_field("name", $name); } /** * @see Identity_Driver::get_user_list. */ public function get_user_list($ids) { - $results = ORM::factory("user") + return ORM::factory("user") ->in("id", ids) ->find_all() - ->as_array();; - $users = array(); - foreach ($results as $user) { - $users[] = new Gallery_User($user); - } - return $users; + ->as_array(); } } // End Identity Gallery Driver -/** - * User Data wrapper - */ -class Gallery_User extends User_Definition { - /* - * Not for general user, allows the back-end to easily create the interface object - */ - function __construct($user) { - $this->user = $user; - } - - /** - * @see User_Definition::avatar_url - */ - public function avatar_url($size=80, $default=null) { - return $this->user->avatar_url($size, $default); - } - - /** - * @see User_Definition::display_name - */ - public function display_name() { - return $this->user->display_name(); - } - - public function save() { - $this->user->save(); - } - - public function delete() { - $this->user->delete(); - } - -} - -/** - * Group Data wrapper - */ -class Gallery_Group extends Group_Definition { - /* - * Not for general user, allows the back-end to easily create the interface object - */ - function __construct($group) { - $this->group = $group; - } - - public function save() { - $this->group->save(); - } - - public function delete() { - $this->group->delete(); - } - - public function add($user) { - $this->group->add($user->_uncloaked()); - } - - public function remove($user) { - $this->group->remove($user->_uncloaked()); - } -} diff --git a/modules/user/models/group.php b/modules/user/models/group.php index 8af78012..4432fc69 100644 --- a/modules/user/models/group.php +++ b/modules/user/models/group.php @@ -17,7 +17,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ -class Group_Model extends ORM { +class Group_Model extends ORM implements Group_Definition { protected $has_and_belongs_to_many = array("users"); var $rules = array( diff --git a/modules/user/models/user.php b/modules/user/models/user.php index d99603b2..c51fc720 100644 --- a/modules/user/models/user.php +++ b/modules/user/models/user.php @@ -17,7 +17,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ -class User_Model extends ORM { +class User_Model extends ORM implements User_Definition { protected $has_and_belongs_to_many = array("groups"); var $rules = array( diff --git a/modules/user/views/admin_users.html.php b/modules/user/views/admin_users.html.php index 7c54d93d..ee8d413c 100644 --- a/modules/user/views/admin_users.html.php +++ b/modules/user/views/admin_users.html.php @@ -91,7 +91,7 @@ open_text="" class="g-panel-link g-button ui-state-default ui-corner-all ui-icon-left"> - id != $user->id && !$user->guest): ?> + id != $user->id && !$user->guest): ?> id") ?>" class="g-dialog-link g-button ui-state-default ui-corner-all ui-icon-left"> diff --git a/modules/user/views/reset_password.html.php b/modules/user/views/reset_password.html.php new file mode 100644 index 00000000..92ca4917 --- /dev/null +++ b/modules/user/views/reset_password.html.php @@ -0,0 +1,17 @@ + + + + <?= t("Password Reset Request") ?> + + +

+

+ $user->full_name ? $user->full_name : $user->name)) ?> +

+

+ %site_url. If you made this request, you can confirm it by clicking this link. If you didn't request this password reset, it's ok to ignore this mail.", + array("site_url" => html::mark_clean(url::base(false, "http")), + "confirm_url" => $confirm_url)) ?> +

+ + -- cgit v1.2.3