From 871176214307b7c4b8487c1f7754893a8c20cc2f Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 13 Feb 2014 11:42:35 -0500 Subject: [PATCH] Users who have made no more than one post can delete their own accounts from their user preferences page. --- .../javascripts/admin/models/admin_user.js | 4 +- .../controllers/preferences_controller.js | 35 +++++++++ .../javascripts/discourse/models/user.js | 17 ++++- .../templates/user/preferences.js.handlebars | 9 +++ app/assets/stylesheets/desktop/modal.scss | 8 +- app/controllers/users_controller.rb | 9 ++- app/serializers/current_user_serializer.rb | 11 ++- {lib => app/services}/user_destroyer.rb | 14 ++-- config/locales/client.en.yml | 4 + config/routes.rb | 3 +- lib/guardian.rb | 31 +------- lib/guardian/post_guardian.rb | 2 +- lib/guardian/user_guardian.rb | 38 ++++++++++ spec/components/guardian_spec.rb | 31 ++++++-- spec/controllers/users_controller_spec.rb | 31 ++++++++ .../user_destroyer_spec.rb | 75 +++++++++++-------- 16 files changed, 239 insertions(+), 83 deletions(-) rename {lib => app/services}/user_destroyer.rb (82%) create mode 100644 lib/guardian/user_guardian.rb rename spec/{components => services}/user_destroyer_spec.rb (80%) diff --git a/app/assets/javascripts/admin/models/admin_user.js b/app/assets/javascripts/admin/models/admin_user.js index 068eec8aa18..4d6922987d6 100644 --- a/app/assets/javascripts/admin/models/admin_user.js +++ b/app/assets/javascripts/admin/models/admin_user.js @@ -42,7 +42,7 @@ Discourse.AdminUser = Discourse.User.extend({ var message = I18n.t('admin.user.delete_all_posts_confirm', {posts: user.get('post_count'), topics: user.get('topic_count')}); var buttons = [{ "label": I18n.t("composer.cancel"), - "class": "cancel", + "class": "cancel-inline", "link": true, "callback": function() { user.set('can_delete_all_posts', true); @@ -308,7 +308,7 @@ Discourse.AdminUser = Discourse.User.extend({ var message = I18n.t('flagging.delete_confirm', {posts: user.get('post_count'), topics: user.get('topic_count'), email: user.get('email'), ip_address: user.get('ip_address')}); var buttons = [{ "label": I18n.t("composer.cancel"), - "class": "cancel", + "class": "cancel-inline", "link": true }, { "label": ' ' + I18n.t("flagging.yes_delete_spammer"), diff --git a/app/assets/javascripts/discourse/controllers/preferences_controller.js b/app/assets/javascripts/discourse/controllers/preferences_controller.js index b784fefee0a..e8663362964 100644 --- a/app/assets/javascripts/discourse/controllers/preferences_controller.js +++ b/app/assets/javascripts/discourse/controllers/preferences_controller.js @@ -21,6 +21,12 @@ Discourse.PreferencesController = Discourse.ObjectController.extend({ return false; }.property('saving', 'name', 'email'), + deleteDisabled: function() { + if (!this.get('can_delete_account')) return true; + if (this.get('saving')) return true; + if (this.get('deleting')) return true; + }.property('deleting', 'saving', 'can_delete_account'), + canEditName: function() { return Discourse.SiteSettings.enable_names; }.property(), @@ -90,6 +96,35 @@ Discourse.PreferencesController = Discourse.ObjectController.extend({ }); }); } + }, + + delete: function() { + this.set('deleting', true); + var self = this, + message = I18n.t('user.delete_account_confirm'), + model = this.get('model'), + buttons = [{ + "label": I18n.t("cancel"), + "class": "cancel-inline", + "link": true, + "callback": function() { + self.set('deleting', false); + } + }, { + "label": ' ' + I18n.t("user.delete_account"), + "class": "btn btn-danger", + "callback": function() { + model.delete().then(function() { + bootbox.alert(I18n.t('user.deleted_yourself'), function() { + window.location.pathname = Discourse.getURL('/'); + }); + }, function() { + bootbox.alert(I18n.t('user.delete_yourself_not_allowed')); + self.set('deleting', false); + }); + } + }]; + bootbox.dialog(message, buttons, {"classes": "delete-account"}); } } diff --git a/app/assets/javascripts/discourse/models/user.js b/app/assets/javascripts/discourse/models/user.js index c679c9e1955..405f70878b7 100644 --- a/app/assets/javascripts/discourse/models/user.js +++ b/app/assets/javascripts/discourse/models/user.js @@ -375,7 +375,22 @@ Discourse.User = Discourse.Model.extend({ updateWatchedCategories: function() { this.set("watchedCategories", Discourse.Category.findByIds(this.watched_category_ids)); - }.observes("watched_category_ids") + }.observes("watched_category_ids"), + + canDeleteAccount: function() { + return this.get('can_delete_account') && ((this.get('reply_count')||0) + (this.get('topic_count')||0)) <= 1; + }.property('can_delete_account', 'reply_count', 'topic_count'), + + delete: function() { + if (this.get('can_delete_account')) { + return Discourse.ajax("/users/" + this.get('username'), { + type: 'DELETE', + data: {context: window.location.pathname} + }); + } else { + return Ember.RSVP.reject(I18n.t('user.delete_yourself_not_allowed')); + } + } }); diff --git a/app/assets/javascripts/discourse/templates/user/preferences.js.handlebars b/app/assets/javascripts/discourse/templates/user/preferences.js.handlebars index d2547025795..ed355c3164b 100644 --- a/app/assets/javascripts/discourse/templates/user/preferences.js.handlebars +++ b/app/assets/javascripts/discourse/templates/user/preferences.js.handlebars @@ -165,5 +165,14 @@ + {{#if canDeleteAccount}} +
+
+
+ +
+
+ {{/if}} + diff --git a/app/assets/stylesheets/desktop/modal.scss b/app/assets/stylesheets/desktop/modal.scss index 3431b77a7bc..497639700bc 100644 --- a/app/assets/stylesheets/desktop/modal.scss +++ b/app/assets/stylesheets/desktop/modal.scss @@ -354,11 +354,9 @@ } } -.flagging-delete-spammer, .delete-all-posts { - .modal-footer .cancel { - text-decoration: underline; - margin-left: 10px; - } +.modal-footer .cancel-inline { + text-decoration: underline; + margin-left: 10px; } .permission-list{ diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 86b3e34d99c..05b23328e9f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -7,7 +7,7 @@ class UsersController < ApplicationController skip_before_filter :authorize_mini_profiler, only: [:avatar] skip_before_filter :check_xhr, only: [:show, :password_reset, :update, :activate_account, :authorize_email, :user_preferences_redirect, :avatar] - before_filter :ensure_logged_in, only: [:username, :update, :change_email, :user_preferences_redirect, :upload_avatar, :toggle_avatar] + before_filter :ensure_logged_in, only: [:username, :update, :change_email, :user_preferences_redirect, :upload_avatar, :toggle_avatar, :destroy] before_filter :respond_to_suspicious_request, only: [:create] # we need to allow account creation with bad CSRF tokens, if people are caching, the CSRF token on the @@ -341,6 +341,13 @@ class UsersController < ApplicationController render nothing: true end + def destroy + @user = fetch_user_from_params + guardian.ensure_can_delete_user!(@user) + UserDestroyer.new(current_user).destroy(@user, {delete_posts: true, context: params[:context]}) + render json: success_json + end + private def honeypot_value diff --git a/app/serializers/current_user_serializer.rb b/app/serializers/current_user_serializer.rb index 1ddba2f7160..ba44f82a740 100644 --- a/app/serializers/current_user_serializer.rb +++ b/app/serializers/current_user_serializer.rb @@ -16,7 +16,8 @@ class CurrentUserSerializer < BasicUserSerializer :trust_level, :can_edit, :can_invite_to_forum, - :no_password + :no_password, + :can_delete_account def include_site_flagged_posts_count? object.staff? @@ -54,4 +55,12 @@ class CurrentUserSerializer < BasicUserSerializer !object.has_password? end + def include_can_delete_account? + can_delete_account + end + + def can_delete_account + scope.can_delete_user?(object) + end + end diff --git a/lib/user_destroyer.rb b/app/services/user_destroyer.rb similarity index 82% rename from lib/user_destroyer.rb rename to app/services/user_destroyer.rb index 1df59eef435..57eaf20d3dd 100644 --- a/lib/user_destroyer.rb +++ b/app/services/user_destroyer.rb @@ -3,16 +3,17 @@ class UserDestroyer class PostsExistError < RuntimeError; end - def initialize(staff) - @staff = staff - raise Discourse::InvalidParameters.new('staff user is nil') unless @staff and @staff.is_a?(User) - raise Discourse::InvalidAccess unless @staff.staff? + def initialize(actor) + @actor = actor + raise Discourse::InvalidParameters.new('acting user is nil') unless @actor and @actor.is_a?(User) + @guardian = Guardian.new(actor) end # Returns false if the user failed to be deleted. # Returns a frozen instance of the User if the delete succeeded. def destroy(user, opts={}) raise Discourse::InvalidParameters.new('user is nil') unless user and user.is_a?(User) + @guardian.ensure_can_delete_user!(user) raise PostsExistError if !opts[:delete_posts] && user.post_count != 0 User.transaction do if opts[:delete_posts] @@ -24,12 +25,11 @@ class UserDestroyer end end end - PostDestroyer.new(@staff, post).destroy + x = PostDestroyer.new(@actor.staff? ? @actor : Discourse.system_user, post).destroy if post.topic and post.post_number == 1 Topic.unscoped.where(id: post.topic.id).update_all(user_id: nil) end end - raise PostsExistError if user.reload.post_count != 0 end user.destroy.tap do |u| if u @@ -55,7 +55,7 @@ class UserDestroyer end end - StaffActionLogger.new(@staff).log_user_deletion(user, opts.slice(:context)) + StaffActionLogger.new(@actor == user ? Discourse.system_user : @actor).log_user_deletion(user, opts.slice(:context)) DiscourseHub.unregister_nickname(user.username) if SiteSetting.call_discourse_hub? MessageBus.publish "/file-change", ["refresh"], user_ids: [user.id] end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index eae213f98bd..dab5d43cb15 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -252,6 +252,10 @@ en: tracked_categories_instructions: "You will automatically track all new and existing topics providing you a count of unread and new posts" muted_categories: "Muted" muted_categories_instructions: "You will not be notified of anything about this topic, and it will not appear on your unread tab." + delete_account: "Delete My Account" + delete_account_confirm: "Are you sure you want to permanently delete your account? This action cannot be undone!" + deleted_yourself: "Your account has been deleted successfully." + delete_yourself_not_allowed: "You cannot delete your account right now. Contact an admin to do delete your account for you." messages: all: "All" diff --git a/config/routes.rb b/config/routes.rb index 5e219b0230a..5ade977d0c3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -118,7 +118,7 @@ Discourse::Application.routes.draw do get "session/csrf" => "session#csrf" get "composer-messages" => "composer_messages#index" - resources :users, except: [:show, :update] do + resources :users, except: [:show, :update, :destroy] do collection do get "check_username" get "is_local_username" @@ -157,6 +157,7 @@ Discourse::Application.routes.draw do post "users/:username/send_activation_email" => "users#send_activation_email", constraints: {username: USERNAME_ROUTE_FORMAT} get "users/:username/activity" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT} get "users/:username/activity/:filter" => "users#show", constraints: {username: USERNAME_ROUTE_FORMAT} + delete "users/:username" => "users#destroy", constraints: {username: USERNAME_ROUTE_FORMAT} get "uploads/:site/:id/:sha.:extension" => "uploads#show", constraints: {site: /\w+/, id: /\d+/, sha: /[a-z0-9]{15,16}/i, extension: /\w{2,}/} post "uploads" => "uploads#create" diff --git a/lib/guardian.rb b/lib/guardian.rb index 8867fd7232d..c8faab5e7bf 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -2,12 +2,15 @@ require_dependency 'guardian/category_guardian' require_dependency 'guardian/ensure_magic' require_dependency 'guardian/post_guardian' require_dependency 'guardian/topic_guardian' +require_dependency 'guardian/user_guardian' + # The guardian is responsible for confirming access to various site resources and operations class Guardian include EnsureMagic include CategoryGuardian include PostGuardain include TopicGuardian + include UserGuardian class AnonymousUser def blank?; true; end @@ -141,18 +144,6 @@ class Guardian user && is_staff? end - def can_block_user?(user) - user && is_staff? && not(user.staff?) - end - - def can_unblock_user?(user) - user && is_staff? - end - - def can_delete_user?(user) - user && is_staff? && !user.admin? && user.created_at > SiteSetting.delete_user_max_age.to_i.days.ago - end - # Support sites that have to approve users def can_access_forum? return true unless SiteSetting.must_approve_users? @@ -184,22 +175,6 @@ class Guardian is_admin? || (authenticated? && @user.id == user_id) end - def can_edit_user?(user) - is_me?(user) || is_staff? - end - - def can_edit_username?(user) - return true if is_staff? - return false if SiteSetting.username_change_period <= 0 - is_me?(user) && (user.post_count == 0 || user.created_at > SiteSetting.username_change_period.days.ago) - end - - def can_edit_email?(user) - return true if is_staff? - return false unless SiteSetting.email_editable? - can_edit?(user) - end - def can_send_private_message?(target) (User === target || Group === target) && authenticated? && diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 7c3ad3d2728..c08b6513d9f 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -1,4 +1,4 @@ -#mixin for all guardian methods dealing with post permisions +#mixin for all guardian methods dealing with post permissions module PostGuardain # Can the user act on the post in a particular way. # taken_actions = the list of actions the user has already taken diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb new file mode 100644 index 00000000000..d338a46aa56 --- /dev/null +++ b/lib/guardian/user_guardian.rb @@ -0,0 +1,38 @@ +# mixin for all Guardian methods dealing with user permissions +module UserGuardian + + def can_edit_user?(user) + is_me?(user) || is_staff? + end + + def can_edit_username?(user) + return true if is_staff? + return false if SiteSetting.username_change_period <= 0 + is_me?(user) && (user.post_count == 0 || user.created_at > SiteSetting.username_change_period.days.ago) + end + + def can_edit_email?(user) + return true if is_staff? + return false unless SiteSetting.email_editable? + can_edit?(user) + end + + def can_block_user?(user) + user && is_staff? && not(user.staff?) + end + + def can_unblock_user?(user) + user && is_staff? + end + + def can_delete_user?(user) + return false if user.nil? + return false if user.admin? + if is_me?(user) + user.post_count <= 1 + else + is_staff? && user.created_at > SiteSetting.delete_user_max_age.to_i.days.ago + end + end + +end \ No newline at end of file diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb index 8cb2b8c1246..18f2b44c138 100644 --- a/spec/components/guardian_spec.rb +++ b/spec/components/guardian_spec.rb @@ -1118,31 +1118,50 @@ describe Guardian do describe "can_delete_user?" do it "is false without a logged in user" do - Guardian.new(nil).can_delete_user?(user).should be_false + Guardian.new(nil).can_delete_user?(user).should == false end it "is false without a user to look at" do - Guardian.new(admin).can_delete_user?(nil).should be_false + Guardian.new(admin).can_delete_user?(nil).should == false end it "is false for regular users" do - Guardian.new(user).can_delete_user?(coding_horror).should be_false + Guardian.new(user).can_delete_user?(coding_horror).should == false + end + + context "delete myself" do + let(:myself) { Fabricate.build(:user, created_at: 6.months.ago) } + subject { Guardian.new(myself).can_delete_user?(myself) } + + it "is true to delete myself and I have never made a post" do + subject.should == true + end + + it "is true to delete myself and I have only made 1 post" do + myself.stubs(:post_count).returns(1) + subject.should == true + end + + it "is false to delete myself and I have made 2 posts" do + myself.stubs(:post_count).returns(2) + subject.should == false + end end shared_examples "can_delete_user examples" do let(:deletable_user) { Fabricate.build(:user, created_at: 5.minutes.ago) } it "is true if user is not an admin and is not too old" do - Guardian.new(actor).can_delete_user?(deletable_user).should be_true + Guardian.new(actor).can_delete_user?(deletable_user).should == true end it "is false if user is an admin" do - Guardian.new(actor).can_delete_user?(another_admin).should be_false + Guardian.new(actor).can_delete_user?(another_admin).should == false end it "is false if user is too old" do SiteSetting.stubs(:delete_user_max_age).returns(7) - Guardian.new(actor).can_delete_user?(Fabricate(:user, created_at: 8.days.ago)).should be_false + Guardian.new(actor).can_delete_user?(Fabricate(:user, created_at: 8.days.ago)).should == false end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 2363936e9e6..2751ed39995 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1233,4 +1233,35 @@ describe UsersController do end end + + describe '.destroy' do + it 'raises an error when not logged in' do + lambda { xhr :delete, :destroy, username: 'nobody' }.should raise_error(Discourse::NotLoggedIn) + end + + context 'while logged in' do + let!(:user) { log_in } + + it 'raises an error when you cannot delete your account' do + Guardian.any_instance.stubs(:can_delete_user?).returns(false) + UserDestroyer.any_instance.expects(:destroy).never + xhr :delete, :destroy, username: user.username + response.should be_forbidden + end + + it "raises an error when you try to delete someone else's account" do + UserDestroyer.any_instance.expects(:destroy).never + xhr :delete, :destroy, username: Fabricate(:user).username + response.should be_forbidden + end + + it "deletes your account when you're allowed to" do + Guardian.any_instance.stubs(:can_delete_user?).returns(true) + UserDestroyer.any_instance.expects(:destroy).with(user, anything).returns(user) + xhr :delete, :destroy, username: user.username + response.should be_success + end + end + end + end diff --git a/spec/components/user_destroyer_spec.rb b/spec/services/user_destroyer_spec.rb similarity index 80% rename from spec/components/user_destroyer_spec.rb rename to spec/services/user_destroyer_spec.rb index a0462957949..c50683773dc 100644 --- a/spec/components/user_destroyer_spec.rb +++ b/spec/services/user_destroyer_spec.rb @@ -15,18 +15,6 @@ describe UserDestroyer do it 'raises an error when user is not a User' do expect { UserDestroyer.new(5) }.to raise_error(Discourse::InvalidParameters) end - - it 'raises an error when user is a regular user' do - expect { UserDestroyer.new( Fabricate(:user) ) }.to raise_error(Discourse::InvalidAccess) - end - - it 'returns an instance of UserDestroyer when user is a moderator' do - UserDestroyer.new( Fabricate(:moderator) ).should be_a(UserDestroyer) - end - - it 'returns an instance of UserDestroyer when user is an admin' do - UserDestroyer.new( Fabricate(:admin) ).should be_a(UserDestroyer) - end end describe 'destroy' do @@ -43,6 +31,10 @@ describe UserDestroyer do expect { UserDestroyer.new(@admin).destroy('nothing') }.to raise_error(Discourse::InvalidParameters) end + it 'raises an error when regular user tries to delete another user' do + expect { UserDestroyer.new(@user).destroy(Fabricate(:user)) }.to raise_error(Discourse::InvalidAccess) + end + shared_examples "successfully destroy a user" do it 'should delete the user' do expect { destroy }.to change { User.count }.by(-1) @@ -86,6 +78,13 @@ describe UserDestroyer do end end + context 'user deletes self' do + let(:destroy_opts) { {delete_posts: true} } + subject(:destroy) { UserDestroyer.new(@user).destroy(@user, destroy_opts) } + + include_examples "successfully destroy a user" + end + context 'user has posts' do let!(:topic_starter) { Fabricate(:user) } let!(:topic) { Fabricate(:topic, user: topic_starter) } @@ -116,29 +115,45 @@ describe UserDestroyer do context "delete_posts is true" do let(:destroy_opts) { {delete_posts: true} } - subject(:destroy) { UserDestroyer.new(@admin).destroy(@user, destroy_opts) } - include_examples "successfully destroy a user" - include_examples "email block list" + context "staff deletes user" do + subject(:destroy) { UserDestroyer.new(@admin).destroy(@user, destroy_opts) } - it "deletes the posts" do - destroy - post.reload.deleted_at.should_not be_nil - post.user_id.should be_nil + include_examples "successfully destroy a user" + include_examples "email block list" + + it "deletes the posts" do + destroy + post.reload.deleted_at.should_not be_nil + post.user_id.should be_nil + end + + it "does not delete topics started by others in which the user has replies" do + destroy + topic.reload.deleted_at.should be_nil + topic.user_id.should_not be_nil + end + + it "deletes topics started by the deleted user" do + spammer_topic = Fabricate(:topic, user: @user) + spammer_post = Fabricate(:post, user: @user, topic: spammer_topic) + destroy + spammer_topic.reload.deleted_at.should_not be_nil + spammer_topic.user_id.should be_nil + end end - it "does not delete topics started by others in which the user has replies" do - destroy - topic.reload.deleted_at.should be_nil - topic.user_id.should_not be_nil - end + context "users deletes self" do + subject(:destroy) { UserDestroyer.new(@user).destroy(@user, destroy_opts) } - it "deletes topics started by the deleted user" do - spammer_topic = Fabricate(:topic, user: @user) - spammer_post = Fabricate(:post, user: @user, topic: spammer_topic) - destroy - spammer_topic.reload.deleted_at.should_not be_nil - spammer_topic.user_id.should be_nil + include_examples "successfully destroy a user" + include_examples "email block list" + + it "deletes the posts" do + destroy + post.reload.deleted_at.should_not be_nil + post.user_id.should be_nil + end end end end