From 4722ebfe4e9b40de47a19419154991a12227fe24 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Tue, 1 Mar 2016 12:33:21 +0100 Subject: [PATCH 1/4] Add admin user management interface --- app/controllers/admin/users_controller.rb | 76 +++++++++++++++++ app/models/user.rb | 12 ++- app/views/admin/users/_form.html.erb | 26 ++++++ app/views/admin/users/edit.html.erb | 9 ++ app/views/admin/users/index.html.erb | 48 +++++++++++ app/views/admin/users/new.html.erb | 9 ++ .../_common_registration_fields.html.erb | 28 +++++++ app/views/devise/registrations/new.html.erb | 29 +------ app/views/layouts/_navigation.html.erb | 3 + config/routes.rb | 4 + ...07085545_warn_about_duplicate_usernames.rb | 21 +++++ spec/features/admin_users_spec.rb | 82 +++++++++++++++++++ spec/models/users_spec.rb | 6 ++ 13 files changed, 323 insertions(+), 30 deletions(-) create mode 100644 app/controllers/admin/users_controller.rb create mode 100644 app/views/admin/users/_form.html.erb create mode 100644 app/views/admin/users/edit.html.erb create mode 100644 app/views/admin/users/index.html.erb create mode 100644 app/views/admin/users/new.html.erb create mode 100644 app/views/devise/registrations/_common_registration_fields.html.erb create mode 100644 db/migrate/20160307085545_warn_about_duplicate_usernames.rb create mode 100644 spec/features/admin_users_spec.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb new file mode 100644 index 00000000..87104512 --- /dev/null +++ b/app/controllers/admin/users_controller.rb @@ -0,0 +1,76 @@ +class Admin::UsersController < ApplicationController + before_action :authenticate_admin! + + before_action :find_user, only: [:edit, :destroy, :update] + + helper_method :resource + + def index + @users = User.reorder(:created_at).page(params[:page]) + + respond_to do |format| + format.html + format.json { render json: @users } + end + end + + def new + @user = User.new + end + + def create + admin = params[:user].delete(:admin) + @user = User.new(params[:user]) + @user.requires_no_invitation_code! + @user.admin = admin + + respond_to do |format| + if @user.save + format.html { redirect_to admin_users_path, notice: "User '#{@user.username}' was successfully created." } + format.json { render json: @user, status: :ok, location: admin_users_path(@user) } + else + format.html { render action: 'new' } + format.json { render json: @user.errors, status: :unprocessable_entity } + end + end + end + + def edit + end + + def update + admin = params[:user].delete(:admin) + params[:user].except!(:password, :password_confirmation) if params[:user][:password].blank? + @user.assign_attributes(params[:user]) + @user.admin = admin + + respond_to do |format| + if @user.save + format.html { redirect_to admin_users_path, notice: "User '#{@user.username}' was successfully updated." } + format.json { render json: @user, status: :ok, location: admin_users_path(@user) } + else + format.html { render action: 'edit' } + format.json { render json: @user.errors, status: :unprocessable_entity } + end + end + end + + def destroy + @user.destroy + + respond_to do |format| + format.html { redirect_to admin_users_path, notice: "User '#{@user.username}' was deleted." } + format.json { head :no_content } + end + end + + private + + def find_user + @user = User.find(params[:id]) + end + + def resource + @user + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 9876f69d..ea61376c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -16,9 +16,9 @@ class User < ActiveRecord::Base attr_accessible *(ACCESSIBLE_ATTRIBUTES + [:admin]), :as => :admin validates_presence_of :username - validates_uniqueness_of :username + validates :username, uniqueness: { case_sensitive: false } validates_format_of :username, :with => /\A[a-zA-Z0-9_-]{3,15}\Z/, :message => "can only contain letters, numbers, underscores, and dashes, and must be between 3 and 15 characters in length." - validates_inclusion_of :invitation_code, :on => :create, :in => INVITATION_CODES, :message => "is not valid", if: ->{ User.using_invitation_code? } + validates_inclusion_of :invitation_code, :on => :create, :in => INVITATION_CODES, :message => "is not valid", if: -> { !requires_no_invitation_code? && User.using_invitation_code? } has_many :user_credentials, :dependent => :destroy, :inverse_of => :user has_many :events, -> { order("events.created_at desc") }, :dependent => :delete_all, :inverse_of => :user @@ -44,4 +44,12 @@ class User < ActiveRecord::Base def self.using_invitation_code? ENV['SKIP_INVITATION_CODE'] != 'true' end + + def requires_no_invitation_code! + @requires_no_invitation_code = true + end + + def requires_no_invitation_code? + !!@requires_no_invitation_code + end end diff --git a/app/views/admin/users/_form.html.erb b/app/views/admin/users/_form.html.erb new file mode 100644 index 00000000..d105d1cb --- /dev/null +++ b/app/views/admin/users/_form.html.erb @@ -0,0 +1,26 @@ +<%= form_for([:admin, @user], html: { class: 'form-horizontal' }) do |f| %> + <%= devise_error_messages! %> + <%= render partial: '/devise/registrations/common_registration_fields', locals: { f: f } %> + +
+
+ <%= f.label :admin do %> + <%= f.check_box :admin %> Admin + <% end %> +
+
+ +
+
+ <%= f.submit class: "btn btn-primary" %> +
+
+<% end %> + +
+ +
+
+ <%= link_to icon_tag('glyphicon-chevron-left') + ' Back'.html_safe, admin_users_path, class: "btn btn-default" %> +
+
diff --git a/app/views/admin/users/edit.html.erb b/app/views/admin/users/edit.html.erb new file mode 100644 index 00000000..2f999b38 --- /dev/null +++ b/app/views/admin/users/edit.html.erb @@ -0,0 +1,9 @@ +
+
+
+

Edit User

+ + <%= render partial: 'form' %> +
+
+
diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb new file mode 100644 index 00000000..6f5ec20e --- /dev/null +++ b/app/views/admin/users/index.html.erb @@ -0,0 +1,48 @@ +
+
+
+
+

+ Users +

+
+ +
+ + + + + + + + + + + + <% @users.each do |user| %> + + + + + + + + + + <% end %> +
UsernameEmailStateActive agentsInactive agentsRegistered sinceOptions
<%= link_to user.username, edit_admin_user_path(user) %><%= user.email %>state<%= user.agents.active.count %><%= user.agents.inactive.count %><%= time_ago_in_words user.created_at %> ago +
+ <%= link_to 'Delete', admin_user_path(user), method: :delete, data: { confirm: 'Are you sure? This can not be undone.' }, class: "btn btn-default" %> +
+
+
+ + <%= paginate @users, theme: 'twitter-bootstrap-3' %> + +
+ <%= link_to icon_tag('glyphicon-plus') + ' New User', new_admin_user_path, class: "btn btn-default" %> +
+
+
+
+ diff --git a/app/views/admin/users/new.html.erb b/app/views/admin/users/new.html.erb new file mode 100644 index 00000000..db93f7a3 --- /dev/null +++ b/app/views/admin/users/new.html.erb @@ -0,0 +1,9 @@ +
+
+
+

Create new User

+ + <%= render partial: 'form' %> +
+
+
diff --git a/app/views/devise/registrations/_common_registration_fields.html.erb b/app/views/devise/registrations/_common_registration_fields.html.erb new file mode 100644 index 00000000..488c2855 --- /dev/null +++ b/app/views/devise/registrations/_common_registration_fields.html.erb @@ -0,0 +1,28 @@ +
+ <%= f.label :email, class: 'col-md-4 control-label' %> +
+ <%= f.email_field :email, autofocus: true, class: 'form-control' %> +
+
+ +
+ <%= f.label :username, class: 'col-md-4 control-label' %> +
+ <%= f.text_field :username, class: 'form-control' %> +
+
+ +
+ <%= f.label :password, class: 'col-md-4 control-label' %> +
+ <%= f.password_field :password, autocomplete: "off", class: 'form-control' %> + <% if @validatable %><%= @minimum_password_length %> characters minimum.<% end %> +
+
+ +
+ <%= f.label :password_confirmation, class: 'col-md-4 control-label' %> +
+ <%= f.password_field :password_confirmation, autocomplete: "off", class: 'form-control' %> +
+
diff --git a/app/views/devise/registrations/new.html.erb b/app/views/devise/registrations/new.html.erb index 5af3c7a9..5b36d721 100644 --- a/app/views/devise/registrations/new.html.erb +++ b/app/views/devise/registrations/new.html.erb @@ -41,34 +41,7 @@ bin/setup_heroku <% end %> -
- <%= f.label :email, class: 'col-md-4 control-label' %> -
- <%= f.email_field :email, autofocus: true, class: 'form-control' %> -
-
- -
- <%= f.label :username, class: 'col-md-4 control-label' %> -
- <%= f.text_field :username, class: 'form-control' %> -
-
- -
- <%= f.label :password, class: 'col-md-4 control-label' %> -
- <%= f.password_field :password, autocomplete: "off", class: 'form-control' %> - <% if @validatable %><%= @minimum_password_length %> characters minimum.<% end %> -
-
- -
- <%= f.label :password_confirmation, class: 'col-md-4 control-label' %> -
- <%= f.password_field :password_confirmation, autocomplete: "off", class: 'form-control' %> -
-
+ <%= render partial: 'common_registration_fields', locals: { f: f } %>
diff --git a/app/views/layouts/_navigation.html.erb b/app/views/layouts/_navigation.html.erb index 517cd018..de109005 100644 --- a/app/views/layouts/_navigation.html.erb +++ b/app/views/layouts/_navigation.html.erb @@ -74,6 +74,9 @@
  • <%= link_to 'Job Management', jobs_path, :tabindex => '-1' %>
    • +
  • + <%= link_to 'User Management', admin_users_path, tabindex: '-1' %> +
    • <% end %>
  • <%= link_to 'About', 'https://github.com/cantino/huginn', :tabindex => "-1" %> diff --git a/config/routes.rb b/config/routes.rb index 808bddac..9f67f248 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -66,6 +66,10 @@ Huginn::Application.routes.draw do end end + namespace :admin do + resources :users, except: :show + end + get "/worker_status" => "worker_status#show" match "/users/:user_id/web_requests/:agent_id/:secret" => "web_requests#handle_request", :as => :web_requests, :via => [:get, :post, :put, :delete] diff --git a/db/migrate/20160307085545_warn_about_duplicate_usernames.rb b/db/migrate/20160307085545_warn_about_duplicate_usernames.rb new file mode 100644 index 00000000..966cf6c6 --- /dev/null +++ b/db/migrate/20160307085545_warn_about_duplicate_usernames.rb @@ -0,0 +1,21 @@ +class WarnAboutDuplicateUsernames < ActiveRecord::Migration + def up + names = User.group('LOWER(username)').having('count(*) > 1').pluck('LOWER(username)') + if names.length > 0 + puts "-----------------------------------------------------" + puts "--------------------- WARNiNG -----------------------" + puts "-------- Found users with duplicate usernames -------" + puts "-----------------------------------------------------" + puts "For the users to log in using their username they have to change it to a unique name" + names.each do |name| + puts + puts "'#{name}' is used multiple times:" + User.where(['LOWER(username) = ?', name]).each do |u| + puts "#{u.id}\t#{u.email}" + end + end + puts + puts + end + end +end diff --git a/spec/features/admin_users_spec.rb b/spec/features/admin_users_spec.rb new file mode 100644 index 00000000..689cd38c --- /dev/null +++ b/spec/features/admin_users_spec.rb @@ -0,0 +1,82 @@ +require 'capybara_helper' + +describe Admin::UsersController do + it "requires to be signed in as an admin" do + login_as(users(:bob)) + visit admin_users_path + expect(page).to have_text('Admin access required to view that page.') + end + + context "as an admin" do + before :each do + login_as(users(:jane)) + end + + it "lists all users" do + visit admin_users_path + expect(page).to have_text('bob') + expect(page).to have_text('jane') + end + + it "allows to delete a user" do + visit admin_users_path + find(:css, "a[href='/admin/users/#{users(:bob).id}']").click + expect(page).to have_text("User 'bob' was deleted.") + expect(page).not_to have_text('bob@example.com') + end + + context "creating new users" do + it "follow the 'new user' link" do + visit admin_users_path + click_on('New User') + expect(page).to have_text('Create new User') + end + + it "creates a new user" do + visit new_admin_user_path + fill_in 'Email', with: 'test@test.com' + fill_in 'Username', with: 'usertest' + fill_in 'Password', with: '12345678' + fill_in 'Password confirmation', with: '12345678' + click_on 'Create User' + expect(page).to have_text("User 'usertest' was successfully created.") + expect(page).to have_text('test@test.com') + end + + it "requires the passwords to match" do + visit new_admin_user_path + fill_in 'Email', with: 'test@test.com' + fill_in 'Username', with: 'usertest' + fill_in 'Password', with: '12345678' + fill_in 'Password confirmation', with: 'no_match' + click_on 'Create User' + expect(page).to have_text("Password confirmation doesn't match") + end + end + + context "updating existing users" do + it "follows the edit link" do + visit admin_users_path + click_on('bob') + expect(page).to have_text('Edit User') + end + + it "updates an existing user" do + visit edit_admin_user_path(users(:bob)) + check 'Admin' + click_on 'Update User' + expect(page).to have_text("User 'bob' was successfully updated.") + visit edit_admin_user_path(users(:bob)) + expect(page).to have_checked_field('Admin') + end + + it "requires the passwords to match when changing them" do + visit edit_admin_user_path(users(:bob)) + fill_in 'Password', with: '12345678' + fill_in 'Password confirmation', with: 'no_match' + click_on 'Update User' + expect(page).to have_text("Password confirmation doesn't match") + end + end + end +end diff --git a/spec/models/users_spec.rb b/spec/models/users_spec.rb index 1af1936d..9a1bdb86 100644 --- a/spec/models/users_spec.rb +++ b/spec/models/users_spec.rb @@ -19,6 +19,12 @@ describe User do should_not allow_value(v).for(:invitation_code) end end + + it "requires no authentication code when requires_no_invitation_code! is called" do + u = User.new(username: 'test', email: 'test@test.com', password: '12345678', password_confirmation: '12345678') + u.requires_no_invitation_code! + expect(u).to be_valid + end end context "when configured not to use invitation codes" do From ee1ebea830660b2d35f14b48063677a1b7c961e8 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Tue, 1 Mar 2016 14:05:26 +0100 Subject: [PATCH 2/4] Allow to configure various devise settings in .env --- .env.example | 36 +++++++++++++++++-- app/models/user.rb | 8 +++-- config/initializers/devise.rb | 18 +++++----- ...717_add_confirmable_attributes_to_users.rb | 17 +++++++++ lib/utils.rb | 21 +++++++++++ spec/env.test | 1 + spec/lib/utils_spec.rb | 29 +++++++++++++++ 7 files changed, 115 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20160301113717_add_confirmable_attributes_to_users.rb diff --git a/.env.example b/.env.example index 8afa63cd..43b21f71 100644 --- a/.env.example +++ b/.env.example @@ -40,9 +40,9 @@ DATABASE_PASSWORD="" # Should Rails force all requests to use SSL? FORCE_SSL=false -############################ -# Allowing Signups # -############################ +################################################ +# User authentication and registration # +################################################ # This invitation code will be required for users to signup with your Huginn installation. # You can see its use in user.rb. PLEASE CHANGE THIS! @@ -51,6 +51,36 @@ INVITATION_CODE=try-huginn # If you don't want to require new users to have an invitation code in order to sign up, set this to true. SKIP_INVITATION_CODE=false +# If you'd like to require new users to confirm their email address after sign up, set this to true. +REQUIRE_CONFIRMED_EMAIL=false + +# If REQUIRE_CONFIRMED_EMAIL is true, set this to the duration in which a user needs to confirm their email address. +ALLOW_UNCONFIRMED_ACCESS_FOR=2.days + +# Duration for which the above confirmation token is valid +CONFIRM_WITHIN=3.days + +# Minimum password length +MIN_PASSWORD_LENGTH=8 + +# Duration for which the reset password token is valid +RESET_PASSWORD_WITHIN=6.hours + +# Set to 'failed_attempts' to lock user accounts for the UNLOCK_AFTER period they fail MAX_FAILED_LOGIN_ATTEMPTS login attempts. Set to 'none' to allow unlimited failed login attempts. +LOCK_STRATEGY=failed_attempts + +# After how many failed login attempts the account is locked when LOCK_STRATEGY is set to failed_attempts. +MAX_FAILED_LOGIN_ATTEMPTS=10 + +# Can be set to 'email', 'time', 'both' or 'none'. 'none' requires manual unlocking of your users! +UNLOCK_STRATEGY=both + +# Duration after which the user is unlocked when UNLOCK_STRATEGY is 'both' or 'time' and LOCK_STRATEGY is 'failed_attempts' +UNLOCK_AFTER=1.hour + +# Duration for which the user will be remembered without asking for credentials again. +REMEMBER_FOR=4.weeks + ############################# # Email Configuration # ############################# diff --git a/app/models/user.rb b/app/models/user.rb index ea61376c..6821420e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,8 +1,10 @@ # Huginn is designed to be a multi-User system. Users have many Agents (and Events created by those Agents). class User < ActiveRecord::Base - devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :trackable, :validatable, :lockable, - :omniauthable + DEVISE_MODULES = [:database_authenticatable, :registerable, + :recoverable, :rememberable, :trackable, + :validatable, :lockable, :omniauthable, + (ENV['REQUIRE_CONFIRMED_EMAIL'] == 'true' ? :confirmable : nil)].compact + devise *DEVISE_MODULES INVITATION_CODES = [ENV['INVITATION_CODE'] || 'try-huginn'] diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 50614149..83bf9d81 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -103,7 +103,7 @@ Devise.setup do |config| # able to access the website for two days without confirming their account, # access will be blocked just in the third day. Default is 0.days, meaning # the user cannot access the website without confirming their account. - # config.allow_unconfirmed_access_for = 2.days + config.allow_unconfirmed_access_for = Utils.parse_duration(ENV['ALLOW_UNCONFIRMED_ACCESS_FOR']).presence || 2.days # A period that the user is allowed to confirm their account before their # token becomes invalid. For example, if set to 3.days, the user can confirm @@ -111,7 +111,7 @@ Devise.setup do |config| # their account can't be confirmed with the token any more. # Default is nil, meaning there is no restriction on how long a user can take # before confirming their account. - # config.confirm_within = 3.days + config.confirm_within = Utils.parse_duration(ENV['CONFIRM_WITHIN']).presence || 3.days # If true, requires any email changes to be confirmed (exactly the same way as # initial account confirmation) to be applied. Requires additional unconfirmed_email @@ -124,7 +124,7 @@ Devise.setup do |config| # ==> Configuration for :rememberable # The time the user will be remembered without asking for credentials again. - config.remember_for = 4.weeks + config.remember_for = Utils.parse_duration(ENV['REMEMBER_FOR']).presence || 4.weeks # Invalidates all the remember me tokens when the user signs out. config.expire_all_remember_me_on_sign_out = true @@ -142,7 +142,7 @@ Devise.setup do |config| # ==> Configuration for :validatable # Range for password length. - config.password_length = 8..128 + config.password_length = (Utils.if_present(ENV['MIN_PASSWORD_LENGTH'], :to_i) || 8)..128 # Email regex used to validate email formats. It simply asserts that # one (and only one) @ exists in the given string. This is mainly @@ -158,7 +158,7 @@ Devise.setup do |config| # Defines which strategy will be used to lock an account. # :failed_attempts = Locks an account after a number of failed attempts to sign in. # :none = No lock strategy. You should handle locking by yourself. - config.lock_strategy = :failed_attempts + config.lock_strategy = Utils.if_present(ENV['LOCK_STRATEGY'], :to_sym) || :failed_attempts # Defines which key will be used when locking and unlocking an account config.unlock_keys = [ :email ] @@ -168,14 +168,14 @@ Devise.setup do |config| # :time = Re-enables login after a certain amount of time (see :unlock_in below) # :both = Enables both strategies # :none = No unlock strategy. You should handle unlocking by yourself. - config.unlock_strategy = :both + config.unlock_strategy = Utils.if_present(ENV['UNLOCK_STRATEGY'], :to_sym) || :both # Number of authentication tries before locking an account if lock_strategy # is failed attempts. - config.maximum_attempts = 10 + config.maximum_attempts = Utils.if_present(ENV['MAX_FAILED_LOGIN_ATTEMPTS'], :to_i) || 10 # Time interval to unlock the account if :time is enabled as unlock_strategy. - config.unlock_in = 1.hour + config.unlock_in = Utils.parse_duration(ENV['UNLOCK_AFTER']).presence || 1.hour # Warn on the last attempt before the account is locked. # config.last_attempt_warning = true @@ -188,7 +188,7 @@ Devise.setup do |config| # Time interval you can reset your password with a reset password key. # Don't put a too small interval or your users won't have the time to # change their passwords. - config.reset_password_within = 6.hours + config.reset_password_within = Utils.parse_duration(ENV['RESET_PASSWORD_WITHIN']).presence || 6.hours # ==> Configuration for :encryptable # Allow you to use another encryption algorithm besides bcrypt (default). You can use diff --git a/db/migrate/20160301113717_add_confirmable_attributes_to_users.rb b/db/migrate/20160301113717_add_confirmable_attributes_to_users.rb new file mode 100644 index 00000000..4f63f152 --- /dev/null +++ b/db/migrate/20160301113717_add_confirmable_attributes_to_users.rb @@ -0,0 +1,17 @@ +class AddConfirmableAttributesToUsers < ActiveRecord::Migration + def change + change_table(:users) do |t| + ## Confirmable + t.string :confirmation_token + t.datetime :confirmed_at + t.datetime :confirmation_sent_at + t.string :unconfirmed_email # Only if using reconfirmable + end + + add_index :users, :confirmation_token, unique: true + + if ENV['REQUIRE_CONFIRMED_EMAIL'] != 'true' && ActiveRecord::Base.connection.column_exists?(:users, :confirmed_at) + User.update_all('confirmed_at = NOW()') + end + end +end diff --git a/lib/utils.rb b/lib/utils.rb index bb82d741..797e6857 100644 --- a/lib/utils.rb +++ b/lib/utils.rb @@ -130,4 +130,25 @@ module Utils def self.sort_tuples!(array, orders = []) TupleSorter.sort!(array, orders) end + + def self.parse_duration(string) + return nil if string.blank? + case string.strip + when /\A(\d+)\.(\w+)\z/ + $1.to_i.send($2.to_s) + when /\A(\d+)\z/ + $1.to_i + else + STDERR.puts "WARNING: Invalid duration format: '#{string.strip}'" + nil + end + end + + def self.if_present(string, method) + if string.present? + string.send(method) + else + nil + end + end end diff --git a/spec/env.test b/spec/env.test index ae23d535..c723aab9 100644 --- a/spec/env.test +++ b/spec/env.test @@ -11,3 +11,4 @@ WUNDERLIST_OAUTH_KEY=wunderoauthkey EVERNOTE_OAUTH_KEY=evernoteoauthkey EVERNOTE_OAUTH_SECRET=evernoteoauthsecret FAILED_JOBS_TO_KEEP=2 +REQUIRE_CONFIRMED_EMAIL=false diff --git a/spec/lib/utils_spec.rb b/spec/lib/utils_spec.rb index b2a708ed..6374a5e8 100644 --- a/spec/lib/utils_spec.rb +++ b/spec/lib/utils_spec.rb @@ -172,4 +172,33 @@ describe Utils do expect(tuples).to eq expected end end + + context "#parse_duration" do + it "works with correct arguments" do + expect(Utils.parse_duration('2.days')).to eq(2.days) + expect(Utils.parse_duration('2.seconds')).to eq(2) + expect(Utils.parse_duration('2')).to eq(2) + end + + it "returns nil when passed nil" do + expect(Utils.parse_duration(nil)).to be_nil + end + + it "warns and returns nil when not parseable" do + mock(STDERR).puts("WARNING: Invalid duration format: 'bogus'") + expect(Utils.parse_duration('bogus')).to be_nil + end + end + + context "#if_present" do + it "returns nil when passed nil" do + expect(Utils.if_present(nil, :to_i)).to be_nil + end + + it "calls the specified method when the argument is present" do + argument = mock() + mock(argument).to_i { 1 } + expect(Utils.if_present(argument, :to_i)).to eq(1) + end + end end From 85089289436c1b34980dd6d46323206bd2b857b9 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Wed, 2 Mar 2016 14:16:58 +0100 Subject: [PATCH 3/4] Admins can deactivate user accounts --- app/controllers/admin/users_controller.rb | 20 ++++++++- app/helpers/users_helper.rb | 13 ++++++ app/models/agent.rb | 8 ++-- app/models/user.rb | 26 ++++++++++++ app/views/admin/users/index.html.erb | 9 +++- config/locales/en.yml | 3 ++ config/routes.rb | 7 +++- ...60302095413_add_deactivated_at_to_users.rb | 7 ++++ ...0160307084729_add_deactivated_to_agents.rb | 6 +++ spec/features/admin_users_spec.rb | 20 +++++++++ spec/models/agent_spec.rb | 41 +++++++++++++++++++ spec/models/users_spec.rb | 24 +++++++++++ 12 files changed, 176 insertions(+), 8 deletions(-) create mode 100644 app/helpers/users_helper.rb create mode 100644 db/migrate/20160302095413_add_deactivated_at_to_users.rb create mode 100644 db/migrate/20160307084729_add_deactivated_to_agents.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 87104512..a092a35d 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -1,7 +1,7 @@ class Admin::UsersController < ApplicationController before_action :authenticate_admin! - before_action :find_user, only: [:edit, :destroy, :update] + before_action :find_user, only: [:edit, :destroy, :update, :deactivate, :activate] helper_method :resource @@ -64,6 +64,24 @@ class Admin::UsersController < ApplicationController end end + def deactivate + @user.deactivate! + + respond_to do |format| + format.html { redirect_to admin_users_path, notice: "User '#{@user.username}' was deactivated." } + format.json { render json: @user, status: :ok, location: admin_users_path(@user) } + end + end + + def activate + @user.activate! + + respond_to do |format| + format.html { redirect_to admin_users_path, notice: "User '#{@user.username}' was activated." } + format.json { render json: @user, status: :ok, location: admin_users_path(@user) } + end + end + private def find_user diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb new file mode 100644 index 00000000..7a27591b --- /dev/null +++ b/app/helpers/users_helper.rb @@ -0,0 +1,13 @@ +module UsersHelper + def user_account_state(user) + if !user.active? + content_tag :span, 'inactive', class: 'label label-danger' + elsif user.access_locked? + content_tag :span, 'locked', class: 'label label-danger' + elsif ENV['REQUIRE_CONFIRMED_EMAIL'] == 'true' && !user.confirmed? + content_tag :span, 'unconfirmed', class: 'label label-warning' + else + content_tag :span, 'active', class: 'label label-success' + end + end +end diff --git a/app/models/agent.rb b/app/models/agent.rb index 2e6420dd..54f9a17c 100644 --- a/app/models/agent.rb +++ b/app/models/agent.rb @@ -61,8 +61,8 @@ class Agent < ActiveRecord::Base has_many :scenario_memberships, :dependent => :destroy, :inverse_of => :agent has_many :scenarios, :through => :scenario_memberships, :inverse_of => :agents - scope :active, -> { where(disabled: false) } - scope :inactive, -> { where(disabled: true) } + scope :active, -> { where(disabled: false, deactivated: false) } + scope :inactive, -> { where(['disabled = ? OR deactivated = ?', true, true]) } scope :of_type, lambda { |type| type = case type @@ -381,7 +381,7 @@ class Agent < ActiveRecord::Base joins("JOIN links ON (links.receiver_id = agents.id)"). joins("JOIN agents AS sources ON (links.source_id = sources.id)"). joins("JOIN events ON (events.agent_id = sources.id AND events.id > links.event_id_at_creation)"). - where("NOT agents.disabled AND (agents.last_checked_event_id IS NULL OR events.id > agents.last_checked_event_id)") + where("NOT agents.disabled AND NOT agents.deactivated AND (agents.last_checked_event_id IS NULL OR events.id > agents.last_checked_event_id)") if options[:only_receivers].present? scope = scope.where("agents.id in (?)", options[:only_receivers]) end @@ -432,7 +432,7 @@ class Agent < ActiveRecord::Base # per type of agent, so you can override this to define custom bulk check behavior for your custom Agent type. def bulk_check(schedule) raise "Call #bulk_check on the appropriate subclass of Agent" if self == Agent - where("agents.schedule = ? and disabled = false", schedule).pluck("agents.id").each do |agent_id| + where("NOT disabled AND NOT deactivated AND schedule = ?", schedule).pluck("agents.id").each do |agent_id| async_check(agent_id) end end diff --git a/app/models/user.rb b/app/models/user.rb index 6821420e..44fc0832 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -43,6 +43,32 @@ class User < ActiveRecord::Base end end + def active? + !deactivated_at + end + + def deactivate! + User.transaction do + agents.update_all(deactivated: true) + update_attribute(:deactivated_at, Time.now) + end + end + + def activate! + User.transaction do + agents.update_all(deactivated: false) + update_attribute(:deactivated_at, nil) + end + end + + def active_for_authentication? + super && active? + end + + def inactive_message + active? ? super : :deactivated_account + end + def self.using_invitation_code? ENV['SKIP_INVITATION_CODE'] != 'true' end diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb index 6f5ec20e..65824228 100644 --- a/app/views/admin/users/index.html.erb +++ b/app/views/admin/users/index.html.erb @@ -14,7 +14,7 @@ Email State Active agents - Inactive agents + Deactivated agents Registered since Options @@ -23,12 +23,17 @@ <%= link_to user.username, edit_admin_user_path(user) %> <%= user.email %> - state + <%= user_account_state(user) %> <%= user.agents.active.count %> <%= user.agents.inactive.count %> <%= time_ago_in_words user.created_at %> ago
    + <% if user.active? %> + <%= link_to 'Deactivate', deactivate_admin_user_path(user), method: :put, class: "btn btn-default" %> + <% else %> + <%= link_to 'Activate', activate_admin_user_path(user), method: :put, class: "btn btn-default" %> + <% end %> <%= link_to 'Delete', admin_user_path(user), method: :delete, data: { confirm: 'Are you sure? This can not be undone.' }, class: "btn btn-default" %>
    diff --git a/config/locales/en.yml b/config/locales/en.yml index 46fc31d7..fa7ce633 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2,6 +2,9 @@ # See https://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points. en: + devise: + failure: + deactivated_account: "Your account has been deactivated by an administrator." datetime: distance_in_words: half_a_minute: "half a minute" diff --git a/config/routes.rb b/config/routes.rb index 9f67f248..0bd99a25 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -67,7 +67,12 @@ Huginn::Application.routes.draw do end namespace :admin do - resources :users, except: :show + resources :users, except: :show do + member do + put :deactivate + put :activate + end + end end get "/worker_status" => "worker_status#show" diff --git a/db/migrate/20160302095413_add_deactivated_at_to_users.rb b/db/migrate/20160302095413_add_deactivated_at_to_users.rb new file mode 100644 index 00000000..da5b352b --- /dev/null +++ b/db/migrate/20160302095413_add_deactivated_at_to_users.rb @@ -0,0 +1,7 @@ +class AddDeactivatedAtToUsers < ActiveRecord::Migration + def change + add_column :users, :deactivated_at, :datetime + + add_index :users, :deactivated_at + end +end diff --git a/db/migrate/20160307084729_add_deactivated_to_agents.rb b/db/migrate/20160307084729_add_deactivated_to_agents.rb new file mode 100644 index 00000000..d2d20b03 --- /dev/null +++ b/db/migrate/20160307084729_add_deactivated_to_agents.rb @@ -0,0 +1,6 @@ +class AddDeactivatedToAgents < ActiveRecord::Migration + def change + add_column :agents, :deactivated, :boolean, default: false + add_index :agents, [:disabled, :deactivated] + end +end diff --git a/spec/features/admin_users_spec.rb b/spec/features/admin_users_spec.rb index 689cd38c..3f2f195e 100644 --- a/spec/features/admin_users_spec.rb +++ b/spec/features/admin_users_spec.rb @@ -78,5 +78,25 @@ describe Admin::UsersController do expect(page).to have_text("Password confirmation doesn't match") end end + + context "(de)activating users" do + it "deactivates an existing user" do + visit admin_users_path + expect(page).not_to have_text('inactive') + find(:css, "a[href='/admin/users/#{users(:bob).id}/deactivate']").click + expect(page).to have_text('inactive') + users(:bob).reload + expect(users(:bob)).not_to be_active + end + + it "deactivates an existing user" do + users(:bob).deactivate! + visit admin_users_path + find(:css, "a[href='/admin/users/#{users(:bob).id}/activate']").click + expect(page).not_to have_text('inactive') + users(:bob).reload + expect(users(:bob)).to be_active + end + end end end diff --git a/spec/models/agent_spec.rb b/spec/models/agent_spec.rb index 2cfc629c..bd76cd47 100644 --- a/spec/models/agent_spec.rb +++ b/spec/models/agent_spec.rb @@ -3,6 +3,34 @@ require 'rails_helper' describe Agent do it_behaves_like WorkingHelpers + describe '.active/inactive' do + let(:agent) { agents(:jane_website_agent) } + + it 'is active per default' do + expect(Agent.active).to include(agent) + expect(Agent.inactive).not_to include(agent) + end + + it 'is not active when disabled' do + agent.update_attribute(:disabled, true) + expect(Agent.active).not_to include(agent) + expect(Agent.inactive).to include(agent) + end + + it 'is not active when deactivated' do + agent.update_attribute(:deactivated, true) + expect(Agent.active).not_to include(agent) + expect(Agent.inactive).to include(agent) + end + + it 'is not active when disabled and deactivated' do + agent.update_attribute(:disabled, true) + agent.update_attribute(:deactivated, true) + expect(Agent.active).not_to include(agent) + expect(Agent.inactive).to include(agent) + end + end + describe ".bulk_check" do before do @weather_agent_count = Agents::WeatherAgent.where(:schedule => "midnight", :disabled => false).count @@ -18,6 +46,12 @@ describe Agent do mock(Agents::WeatherAgent).async_check(anything).times(@weather_agent_count - 1) Agents::WeatherAgent.bulk_check("midnight") end + + it "should skip agents of deactivated accounts" do + agents(:bob_weather_agent).user.deactivate! + mock(Agents::WeatherAgent).async_check(anything).times(@weather_agent_count - 1) + Agents::WeatherAgent.bulk_check("midnight") + end end describe ".run_schedule" do @@ -335,6 +369,13 @@ describe Agent do Agent.receive! # and we receive it }.to change { agents(:bob_rain_notifier_agent).reload.last_checked_event_id } end + + it "should not run agents of deactivated accounts" do + agents(:bob_weather_agent).user.deactivate! + Agent.async_check(agents(:bob_weather_agent).id) + mock(Agent).async_receive(agents(:bob_rain_notifier_agent).id, anything).times(0) + Agent.receive! + end end describe ".async_receive" do diff --git a/spec/models/users_spec.rb b/spec/models/users_spec.rb index 9a1bdb86..c1009bd9 100644 --- a/spec/models/users_spec.rb +++ b/spec/models/users_spec.rb @@ -40,4 +40,28 @@ describe User do end end end + + context '#deactivate!' do + it "deactivates the user and all her agents" do + agent = agents(:jane_website_agent) + users(:jane).deactivate! + agent.reload + expect(agent.deactivated).to be_truthy + expect(users(:jane).deactivated_at).not_to be_nil + end + end + + context '#activate!' do + before do + users(:bob).deactivate! + end + + it 'activates the user and all his agents' do + agent = agents(:bob_website_agent) + users(:bob).activate! + agent.reload + expect(agent.deactivated).to be_falsy + expect(users(:bob).deactivated_at).to be_nil + end + end end From c0c74113bf943111ec7ef26e577923068ef2f770 Mon Sep 17 00:00:00 2001 From: Dominik Sander Date: Thu, 3 Mar 2016 13:47:09 +0100 Subject: [PATCH 4/4] Admins should not be able to deactivate their own accounts --- app/controllers/admin/users_controller.rb | 2 +- app/views/admin/users/index.html.erb | 12 +++++++----- spec/features/admin_users_spec.rb | 5 +++++ 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index a092a35d..891499e7 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -6,7 +6,7 @@ class Admin::UsersController < ApplicationController helper_method :resource def index - @users = User.reorder(:created_at).page(params[:page]) + @users = User.reorder('created_at DESC').page(params[:page]) respond_to do |format| format.html diff --git a/app/views/admin/users/index.html.erb b/app/views/admin/users/index.html.erb index 65824228..4213d80f 100644 --- a/app/views/admin/users/index.html.erb +++ b/app/views/admin/users/index.html.erb @@ -29,12 +29,14 @@ <%= time_ago_in_words user.created_at %> ago
    - <% if user.active? %> - <%= link_to 'Deactivate', deactivate_admin_user_path(user), method: :put, class: "btn btn-default" %> - <% else %> - <%= link_to 'Activate', activate_admin_user_path(user), method: :put, class: "btn btn-default" %> + <% if user != current_user %> + <% if user.active? %> + <%= link_to 'Deactivate', deactivate_admin_user_path(user), method: :put, class: "btn btn-default" %> + <% else %> + <%= link_to 'Activate', activate_admin_user_path(user), method: :put, class: "btn btn-default" %> + <% end %> + <%= link_to 'Delete', admin_user_path(user), method: :delete, data: { confirm: 'Are you sure? This can not be undone.' }, class: "btn btn-default" %> <% end %> - <%= link_to 'Delete', admin_user_path(user), method: :delete, data: { confirm: 'Are you sure? This can not be undone.' }, class: "btn btn-default" %>
    diff --git a/spec/features/admin_users_spec.rb b/spec/features/admin_users_spec.rb index 3f2f195e..ac19b279 100644 --- a/spec/features/admin_users_spec.rb +++ b/spec/features/admin_users_spec.rb @@ -80,6 +80,11 @@ describe Admin::UsersController do end context "(de)activating users" do + it "does not show deactivation buttons for the current user" do + visit admin_users_path + expect(page).not_to have_css("a[href='/admin/users/#{users(:jane).id}/deactivate']") + end + it "deactivates an existing user" do visit admin_users_path expect(page).not_to have_text('inactive')