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 all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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
21 changes: 21 additions & 0 deletions app/controllers/concerns/current_stage.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# 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
return unless current_project
return unless stage_param = params[:stage_id] || params[:id]
@stage = Stage.where(project_id: current_project.id).find_by_param!(stage_param)
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 && respond_to?(:current_stage)
current_stage
elsif respond_to?(:current_project)
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
8 changes: 5 additions & 3 deletions app/models/access_control.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ def can?(user, action, resource_namespace, scope = nil)
case action
when :read then true
when :write
if scope
user.deployer_for?(scope) # stage locks
return user.admin? if scope.nil? # global locks
return false unless scope.is_a?(Stage) # stage locks
if ENV['PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN'] && scope.production
user.admin_for?(scope.project)
else
user.admin? # global locks
user.deployer_for?(scope.project)
end
else raise ArgumentError, "Unsupported action #{action}"
end
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
48 changes: 46 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 @@ -116,6 +117,29 @@ def create_lock(resource = nil, options = {})
assert_response :success
JSON.parse(response.body).fetch("lock").fetch("id").must_equal Lock.last.id
end

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

it 'cannot create a stage lock for a production stage' do
create_lock prod_stage

assert_response :unauthorized
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
end

describe '#destroy' do
Expand All @@ -136,6 +160,26 @@ 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.update_column(:production, true)
delete :destroy, params: {id: lock.id}

assert_response :unauthorized
end
end
end

as_an_admin do
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