Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional Feature: require admin for prod (un)lock. #2492

Merged
merged 3 commits into from
Jan 31, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
3 changes: 3 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,6 @@ PERIODICAL=stop_expired_deploys:60,remove_expired_locks:60,report_system_stats:6
# GCLOUD_OPTIONS=--verbose # optional, options
# GCLOUD_PROJECT=foo-123
# GCLOUD_ACCOUNT=my-account

## Feature: Only admins can (un)lock stages which affect production.
# PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN=1
23 changes: 23 additions & 0 deletions app/controllers/concerns/current_stage.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# frozen_string_literal: true
module CurrentStage
extend ActiveSupport::Concern

included do
before_action :require_stage
helper_method :current_stage
end

def current_stage
@stage
end

protected

def require_stage
if current_project
query = Stage.where(project_id: current_project.id)
stage_param = params[:stage_id] || params[:id]
@stage = query.find_by_param!(stage_param) if stage_param
end
end
end
6 changes: 5 additions & 1 deletion app/controllers/concerns/current_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ def resource_action
end

def can?(action, resource_namespace, scope = nil)
scope ||= current_project if respond_to?(:current_project)
scope ||= if resource_namespace == :locks
try(:current_stage)
Copy link
Contributor

Choose a reason for hiding this comment

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

try does not make sense here, try is for nil prevention like nil.try(:foo?)

else
try(:current_project)
end
AccessControl.can?(current_user, action, resource_namespace, scope)
end
end
11 changes: 6 additions & 5 deletions app/controllers/locks_controller.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
# frozen_string_literal: true
class LocksController < ApplicationController
include CurrentProject
include CurrentStage

before_action :require_project, if: :for_stage_lock?
before_action :require_stage, if: :for_stage_lock?
before_action :authorize_resource!

def index
Expand Down Expand Up @@ -56,13 +57,13 @@ def lock
@lock ||= Lock.find(params[:id])
end

# Overrides CurrentProject#require_project
def require_project
# Overrides CurrentStage#require_stage
def require_stage
case action_name
when 'create' then
@project = Stage.find(params[:lock][:resource_id]).project
@stage = Stage.find(params[:lock][:resource_id])
when 'destroy' then
@project = lock.resource.project
@stage = lock.resource
else
raise 'Unsupported action'
end
Expand Down
10 changes: 7 additions & 3 deletions app/models/access_control.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@ def can?(user, action, resource_namespace, scope = nil)
case action
when :read then true
when :write
if scope
user.deployer_for?(scope) # stage locks

if scope.nil? # global locks
user.admin?
elsif ENV['PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN'] && scope.production
Copy link
Contributor

Choose a reason for hiding this comment

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

might be easier to read as:

else # stage locks
  if PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN
  else
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

prefer to guard this in a elsif scope.is_a?(Stage)so future additions don't require a refactor

user.admin_for?(scope.project) # stage locks
else
user.admin? # global locks
user.deployer_for?(scope.project) # stage locks
end

else raise ArgumentError, "Unsupported action #{action}"
end
when :projects, :build_commands, :stages, :user_project_roles
Expand Down
2 changes: 1 addition & 1 deletion app/views/locks/_button.html.erb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<% if can? :write, :locks, @project %>
<% if can? :write, :locks, @stage %>
<% prefix ||= "" %>
<% warning = {resource: resource, warning: true, icon: warning_icon, text: "#{prefix}Warn", disabled: false} %>
<% locking = {resource: resource, warning: false, icon: lock_icon, text: "#{prefix}Lock", disabled: false} %>
Expand Down
39 changes: 39 additions & 0 deletions test/controllers/concerns/current_stage_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true
require_relative '../../test_helper'

SingleCov.covered!

class CurrentStageConcernTest < ActionController::TestCase
class CurrentStageTestController < ApplicationController
include CurrentProject
include CurrentStage

def show
render inline: '<%= current_stage.class.name %>'
end
end

tests CurrentStageTestController
use_test_routes CurrentStageTestController

let(:project) { projects(:test) }
let(:stage) { stages(:test_staging) }

before { login_as(users(:viewer)) }

it "finds current stage" do
get :show, params: {project_id: project.id, id: stage.id, test_route: true}
response.body.must_equal 'Stage'
end

it "fails with invalid stage id" do
assert_raises ActiveRecord::RecordNotFound do
get :show, params: {project_id: project.id, id: 123456, test_route: true}
end
end

it "does not fail without stage" do
get :show, params: {test_route: true}
response.body.must_equal 'NilClass'
end
end
8 changes: 6 additions & 2 deletions test/controllers/concerns/current_user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ class CurrentUserConcernTest < ActionController::TestCase
class CurrentUserTestController < ApplicationController
include CurrentUser
include CurrentProject
include CurrentStage

def whodunnit
render plain: Audited.store[:current_user].call.name
Expand Down Expand Up @@ -242,8 +243,11 @@ def perform_get(add = {})
assert_response :unauthorized
end

it "renders when authorized via the project" do
perform_get(project_id: projects(:test).id)
# This still authorizes based on Project, but for the lock controller, it needs a Stage object
# passed to access control since locks are per Stage. See app/models/access_control.rb#can? when
# the resource_namespace is :locks
it "renders when authorized via the stage" do
perform_get(project_id: projects(:test).id, id: stages(:test_staging).id)
assert_response :success
end

Expand Down
112 changes: 110 additions & 2 deletions test/controllers/locks_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ def create_lock(resource = nil, options = {})
end

let(:stage) { stages(:test_staging) }
let(:prod_stage) { stages(:test_production) }
let(:environment) { environments(:production) }
let(:lock) { stage.create_lock! user: users(:deployer) }
let(:global_lock) { Lock.create! user: users(:deployer) }
Expand All @@ -27,11 +28,11 @@ def create_lock(resource = nil, options = {})
end
end

describe "#require_project" do
describe "#require_stage" do
it "raises on unsupported action" do
@controller.stubs(action_name: 'show')
assert_raises RuntimeError do
@controller.send(:require_project)
@controller.send(:require_stage)
end
end
end
Expand Down Expand Up @@ -118,6 +119,33 @@ def create_lock(resource = nil, options = {})
end
end

describe '#create with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' do
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's not too complicated I'd prefer this under #create as describe "with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN"

with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true'
before { travel_to Time.now }
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the before and after ... they are no necessary ?

after { travel_back }

it 'cannot create a stage lock for a production stage' do
create_lock prod_stage, delete_in: 3600

assert_response :unauthorized
Copy link
Contributor

Choose a reason for hiding this comment

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

can drop the later assertions ... same in the test below

stage.reload
assert_nil(stage.lock)
end

it 'creates a stage lock' do
create_lock stage, delete_in: 3600
assert_redirected_to '/back'
assert flash[:notice]

stage.reload

lock = stage.lock
lock.warning?.must_equal(false)
lock.description.must_equal 'DESC'
lock.delete_at.must_equal(Time.now + 3600)
end
end

describe '#destroy' do
let(:lock) { stage.create_lock!(user: users(:deployer)) }

Expand All @@ -136,6 +164,31 @@ def create_lock(resource = nil, options = {})
Lock.count.must_equal 0
end
end

describe '#destroy with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' do
with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true'

it 'no change in default behavior for non-production stage lock' do
delete :destroy, params: {id: lock.id}

assert_redirected_to '/back'
assert flash[:notice]

Lock.count.must_equal 0
end

it 'cannot destroy a stage production lock' do
stage.production = true
Copy link
Contributor

Choose a reason for hiding this comment

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

stage.update_column(:production, true)

stage.save
delete :destroy, params: {id: lock.id}

assert_response :unauthorized
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be enough and drop the later assertions ?

stage.reload
Lock.count.must_equal 1
stage.production = false
stage.save
end
end
end

as_an_admin do
Expand All @@ -159,6 +212,48 @@ def create_lock(resource = nil, options = {})
end
end

describe '#create with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' do
Copy link
Contributor

Choose a reason for hiding this comment

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

too much copy-paste for my taste ... the tests above should be sufficient ...

with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true'
before { travel_to Time.now }
after { travel_back }

it 'creates a stage lock for a production stage' do
stage.production = true
stage.save
create_lock stage, delete_in: 3600

assert_redirected_to '/back'
assert flash[:notice]

stage.reload

lock = stage.lock
lock.warning?.must_equal(false)
lock.description.must_equal 'DESC'
lock.delete_at.must_equal(Time.now + 3600)
stage.production = false
stage.save
end

it 'creates a global lock' do
create_lock
assert_redirected_to '/back'
assert flash[:notice]

lock = Lock.global.first
lock.description.must_equal 'DESC'
end

it 'creates an environment lock' do
create_lock environment
assert_redirected_to '/back'
assert flash[:notice]

lock = environment.lock
lock.description.must_equal 'DESC'
end
end

describe '#destroy' do
it 'destroys a global lock' do
delete :destroy, params: {id: global_lock.id}
Expand All @@ -170,6 +265,19 @@ def create_lock(resource = nil, options = {})
end
end

describe '#destroy with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' do
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true'

it 'destroys a global lock' do
delete :destroy, params: {id: global_lock.id}

assert_redirected_to '/back'
assert flash[:notice]

Lock.count.must_equal 0
end
end

describe "#destroy_via_resource" do
before { Lock.create!(user: users(:admin)) }

Expand Down
24 changes: 22 additions & 2 deletions test/models/access_control_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ def can?(user, action, project = nil)
end

let(:project) { projects(:test) }
let(:stage) { stages(:test_staging) }
let(:prod_stage) { stages(:test_production) }
let(:viewer) { users(:viewer) }
let(:project_deployer) { users(:project_deployer) }
let(:deployer) { users(:deployer) }
Expand Down Expand Up @@ -136,11 +138,23 @@ def can?(user, action, project = nil)
describe :write do
describe "stage" do
it "allows deployers to update" do
assert can?(project_deployer, :write, project)
assert can?(project_deployer, :write, stage)
end

it "forbids viewers to update" do
refute can?(viewer, :write, project)
refute can?(viewer, :write, stage)
end

it "allows admins to update with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN" do
with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true' do
assert can?(admin, :write, stage)
end
end

it "forbids deployers to update with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN and prod stage" do
with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true' do
refute can?(deployer, :write, prod_stage)
end
end
end

Expand All @@ -149,6 +163,12 @@ def can?(user, action, project = nil)
assert can?(admin, :write)
end

it "allows admins to update with PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN" do
with_env 'PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN' => 'true' do
assert can?(admin, :write)
end
end

it "forbids deployers to update" do
refute can?(deployer, :write)
end
Expand Down