Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Implemented the "Disable users" feature #240

Merged
merged 7 commits into from
Aug 3, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ pull requests: [#193](https://github.com/SUSE/Portus/pull/193),
PR [#212](https://github.com/SUSE/Portus/pull/212).
- Added the appliance tests. See PR [#208](https://github.com/SUSE/Portus/pull/208).
- Star/Unstar repositories. See PR [#230](https://github.com/SUSE/Portus/pull/230).
- Now users can be disabled. See PR [#240](https://github.com/SUSE/Portus/pull/240).
- And some minor fixes here and there.

## 1.0.1
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/admin/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ class Admin::UsersController < Admin::BaseController
respond_to :html, :js

def index
@users = User.all.page(params[:page])
@users = User.enabled.page(params[:page])
@admin_count = User.admins.count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should filter for enabled users too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore it, I just realized admins are already filtered by status

end

# PATCH/PUT /admin/user/1/toggle_admin
Expand Down
33 changes: 33 additions & 0 deletions app/controllers/auth/registrations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController

before_action :check_admin, only: [:new, :create]
before_action :configure_sign_up_params, only: [:create]
before_action :authenticate_user!, only: [:disable]

# Re-implemented so the template has the auxiliary variables regarding if
# there are more users on the system or this is the first user to be created.
Expand All @@ -25,6 +26,11 @@ def create
end
end

def edit
@admin_count = User.admins.count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, we should count only enabled ones

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore it, I just realized admins are already filtered by status

super
end

def update
success =
if password_update?
Expand All @@ -46,6 +52,19 @@ def update
end
end

# Disable a user.
def disable
user = User.find(params[:id])

if can_disable?(user)
render nothing: true, status: 403
else
user.update_attributes(enabled: false)
sign_out user if current_user == user
render template: 'auth/registrations/disabled', locals: { user: user, path: request.fullpath }
end
end

# Devise does not allow to disable routes on purpose. Ideally, though we
# could still be able to disable the `destroy` method through the
# `routes.rb` file as described in the wiki (by disabling all the routes and
Expand Down Expand Up @@ -76,4 +95,18 @@ def password_update?
!user[:current_password].blank? || !user[:password].blank? ||
!user[:password_confirmation].blank?
end

# Returns whether the given user can be disabled or not. The following rules
# apply:
# 1. A user can disable himself unless it's the last admin on the system.
# 2. The admin user is the only one that can disable other users.
def can_disable?(user)
if current_user == user
# An admin cannot disable himself if he's the only admin in the system.
current_user.admin? && User.admins.count == 1
else
# Only admin users can disable other users.
!current_user.admin?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the branch is useless because we have the check_admin filter in place. So a non admin user is never going to reach this part of the code

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that a regular user should be able to disable himself, therefore non-admin users should be able to perform this action. I'd say to just remove :disable from the check_admin hook (which by the way is not preventing anything, since the test in line 145 of spec/controllers/auth/registrations_controller_spec.rb passes ;) ).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, ignore all my comments about this part of the code 👍

end
end
end
2 changes: 1 addition & 1 deletion app/controllers/teams_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def index
# GET /teams/1.json
def show
authorize @team
@team_users = @team.team_users.page(params[:users_page]).per(10)
@team_users = @team.team_users.enabled.page(params[:users_page]).per(10)
@team_namespaces = @team.namespaces.page(params[:namespaces_page]).per(15)
end

Expand Down
2 changes: 2 additions & 0 deletions app/models/team_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
class TeamUser < ActiveRecord::Base
enum role: [:viewer, :contributor, :owner]

scope :enabled, -> { joins(:user).where('users.enabled' => true) }

validates :team, presence: true
validates :user, presence: true, uniqueness: { scope: :team }

Expand Down
16 changes: 16 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ class User < ActiveRecord::Base
has_many :teams, through: :team_users
has_many :stars

scope :enabled, -> { where enabled: true }
scope :admins, -> { where enabled: true, admin: true }

def private_namespace_available
return unless Namespace.exists?(name: username)
errors.add(:username, 'cannot be used as name for private namespace')
Expand Down Expand Up @@ -51,4 +54,17 @@ def toggle_admin!
team = Registry.first.global_namespace.team
admin ? team.owners << self : team.owners.delete(self)
end

##
# Disabling users.

# This method is picked up by Devise before signing in a user.
def active_for_authentication?
super && enabled?
end

# The flashy message to be shown for disabled users that try to login.
def inactive_message
'Sorry, this account has been disabled.'
end
end
2 changes: 1 addition & 1 deletion app/views/admin/dashboard/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
.col-xs-4
i.fa.fa-user.users.fa-2x
.col-xs-8
span.fa-3x= User.count
span.fa-3x= User.enabled.count

.col-md-3.col-xs-6
.panel-overview
Expand Down
15 changes: 14 additions & 1 deletion app/views/admin/users/index.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,22 @@
col.col-40
col.col-10
col.col-10
col.col-10
col.col-10
thead
tr
th Name
th Email
th Admin
th Namespaces
th Teams
th Enabled
tbody
- @users.each do |user|
tr[id="user_#{user.id}"]
td= user.username
td= user.email
td
td.admin-btn
- if current_user.id == user.id
i.fa.fa-lg[class="fa-toggle-#{user.admin? ? 'on': 'off'}"]
- else
Expand All @@ -34,4 +37,14 @@

td= user.teams.reduce(0){ |sum, t| sum += t.namespaces.count}
td= user.teams.count
td.enabled-btn
- if current_user.id == user.id && @admin_count == 1
i.fa.fa-lg.fa-toggle-on
- else
a[class="btn btn-default"
data-remote="true"
data-method="put"
rel="nofollow"
href=url_for(disable_user_path(user))]
i.fa.fa-lg[class="fa-toggle-#{user.enabled? ? 'on': 'off'}"]
.panel-footer= paginate(@users)
4 changes: 2 additions & 2 deletions app/views/admin/users/toggle_admin.js.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
$('#user_<%= user.id %> td a i').addClass("fa-toggle-<%= user.admin? ? 'on' : 'off' %>");
$('#user_<%= user.id %> td a i').removeClass("fa-toggle-<%= user.admin? ? 'off' : 'on' %>");
$('#user_<%= user.id %> .admin-btn a i').addClass("fa-toggle-<%= user.admin? ? 'on' : 'off' %>");
$('#user_<%= user.id %> .admin-btn a i').removeClass("fa-toggle-<%= user.admin? ? 'off' : 'on' %>");

$('#notice p').html("User '<%= user.username %>' is <%= user.admin? ? 'now' : 'no longer' %> an admin");
$('#notice').fadeIn();
14 changes: 14 additions & 0 deletions app/views/auth/registrations/disabled.js.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

if (window.location.pathname == '/admin/users') {
// We are on the admin panel.
$("#user_<%= user.id %>").fadeOut('normal', function() {
$("#user_<%= user.id %>").remove();
});

$('#alert p').html("User '<%= user.username %>' has been disabled.");
$('#alert').fadeIn();
} else {
// The user profile page.
window.location.pathname = '/';
}

13 changes: 13 additions & 0 deletions app/views/devise/registrations/edit.html.slim
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,16 @@
.form-group
.actions
= f.submit('Change', class: 'btn btn-primary', disabled: true)

- unless current_user.admin? && @admin_count == 1
.panel.panel-default
.panel-heading
h5
' Disable account
.panel-body
= form_tag(disable_user_path(current_user), method: :put, remote: true, id: 'disable-form') do
.form-group
p
| By disabling the account, you won't be able to access Portus with it, and
any affiliations with any team will be lost.
= submit_tag('Disable', class: 'btn btn-primary btn-danger')
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

devise_scope :user do
root to: 'auth/sessions#new'
put 'disable_user/:id', to: 'auth/registrations#disable', as: :disable_user
end

namespace :v2, module: 'api/v2', defaults: { format: :json } do
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20150729153854_add_enabled_to_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddEnabledToUsers < ActiveRecord::Migration
def change
add_column :users, :enabled, :bool, default: true
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20150722194840) do
ActiveRecord::Schema.define(version: 20150729153854) do

create_table "activities", force: :cascade do |t|
t.integer "trackable_id", limit: 4
Expand Down Expand Up @@ -124,6 +124,7 @@
t.datetime "created_at"
t.datetime "updated_at"
t.boolean "admin", limit: 1, default: false
t.boolean "enabled", limit: 1, default: true
end

add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree
Expand Down
12 changes: 12 additions & 0 deletions spec/api/v2/token_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@
end

context 'as invalid user' do
let(:valid_request) do
{
service: registry.hostname,
account: 'account',
scope: 'repository:foo_namespace/me:push'
}
end

it 'denies access when the password is wrong' do
get v2_token_url, { service: registry.hostname, account: 'account', scope: 'repository:foo/me:push' }, invalid_auth_header
Expand All @@ -38,6 +45,11 @@
expect(response.status).to eq 401
end

it 'denies access to a disabled user' do
user.update_attributes(enabled: false)
get v2_token_url, valid_request, valid_auth_header
expect(response.status).to eq 401
end
end

context 'as the special portus user' do
Expand Down
43 changes: 42 additions & 1 deletion spec/controllers/auth/registrations_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@
end

describe 'DELETE #destroy' do

let!(:user) { create(:admin) }

before :each do
Expand All @@ -123,4 +122,46 @@
end
end

describe 'PUT #disable_user' do
let!(:admin) { create(:admin) }
let!(:user) { create(:user) }

before :each do
request.env['devise.mapping'] = Devise.mappings[:user]
end

it 'does not allow to disable the only admin' do
sign_in admin
put :disable, id: admin.id
expect(response.status).to be 403
end

it 'does not allow a regular user to disable another' do
sign_in user
put :disable, id: admin.id
expect(response.status).to be 403
end

it 'allows a user to disable himself' do
sign_in user
put :disable, id: user.id, format: :erb
expect(response.status).to be 200
expect(User.find(user.id).enabled?).to be false
end

it 'allows the admin to disable a regular user' do
sign_in admin
put :disable, id: user.id, format: :erb
expect(response.status).to be 200
expect(User.find(user.id).enabled?).to be false
end

it 'allows an admin to disable another admin' do
admin2 = create(:admin)
sign_in admin
put :disable, id: admin2.id, format: :erb
expect(response.status).to be 200
expect(User.find(admin2.id).enabled?).to be false
end
end
end
12 changes: 12 additions & 0 deletions spec/controllers/teams_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@

expect(response.status).to eq 401
end

it 'does not display disabled users' do
user = create(:user, enabled: false)
TeamUser.create(team: team, user: user, role: TeamUser.roles['viewer'])
sign_in owner

get :show, id: team.id

expect(response.status).to eq 200
expect(TeamUser.count).to be 2
expect(assigns(:team_users).count).to be 1
end
end

describe 'as a portus user' do
Expand Down
2 changes: 0 additions & 2 deletions spec/factories/teams_factory.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
FactoryGirl.define do

factory :team do
sequence(:name) { |n| "team_name#{n}" }
owners { |t| [t.association(:user)] }
end

end
25 changes: 25 additions & 0 deletions spec/features/admin/users_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
require 'rails_helper'

feature 'Admin - Users panel' do
let!(:registry) { create(:registry) }
let!(:admin) { create(:admin) }
let!(:user) { create(:user) }

before do
login_as admin, scope: :user
visit admin_users_path
end

describe 'disable users' do
scenario 'allows the admin to disable other users', js: true do
expect(page).to have_css("#user_#{user.id}")
find("#user_#{user.id} .enabled-btn").click

wait_for_effect_on("#user_#{user.id}")

expect(page).to_not have_css("#user_#{user.id}")
wait_for_effect_on('#alert')
expect(page).to have_content("User '#{user.username}' has been disabled.")
end
end
end
10 changes: 10 additions & 0 deletions spec/features/auth/login_feature_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,14 @@
expect(current_url).to eq root_url
end

scenario 'A disabled user cannot login' do
user.update_attributes(enabled: false)
fill_in 'user_username', with: user.username
fill_in 'user_password', with: user.password
click_button 'Login'

expect(page).to have_content(user.inactive_message)
expect(page).to have_content('Login')
expect(current_url).to eq new_user_session_url
end
end
Loading