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

jgoerz · 2018-01-24T21:24:10Z

When set, will require admin role for (un)locking stages with production attribute set.
All other locking is unchanged.

/cc @zendesk/samson

Tasks

👍 from team

References

Jira link:

Risks

Level: Low/Med/High

jgoerz · 2018-01-24T22:56:11Z

Build failure is unrelated to the changes for this pull request. Fails on slow test (didn't see this when I ran it locally)

Error:
JobExecution::builds_in_environment#test_0001_makes builds available via env:
Maxitest::Timeout::TestCaseTimeout: Test took too long to finish, aborting. To use a debugger, set Maxitest.timeout = false at the top of the test file.

And coverage:

app/models/event_streamer.rb has less uncovered lines (2 current vs 3 configured), decrement configured uncovered?

grosser · 2018-01-25T01:39:00Z

hmmm I kinda like the idea of only letting admins unlock ... do you think the same restriction is necessary for locking ? ... I'd rather have anyone able to lock when something goes wrong ...

grosser · 2018-01-25T01:39:24Z

.env.example

@@ -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.
+# FEATURE_ONLY_ADMINS_CAN_LOCK_UNLOCK_PRODUCTION_STAGE=1


ONLY_ADMINS_CAN_UNLOCK_PRODUCTION_STAGE should be long enough :D

... maybe PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN

yeah, naming things is always hard ;-) I like PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN.

grosser · 2018-01-25T01:40:37Z

app/controllers/locks_controller.rb

@@ -60,11 +60,25 @@ def lock
  def require_project
    case action_name
    when 'create' then
-      @project = Stage.find(params[:lock][:resource_id]).project
+      stage = Stage.find(params[:lock][:resource_id])


this should only be handles by access_control.rb and not copied here ...

I agree with this, but it seems that only the project object is passed into access control to make the decision for the locks. I struggled with my debugger configuration, so maybe I overlooked something... but this is the reason for the hacking singleton method below. I was basically trying to attach the stage attribute information to the project. I'll do some more investigating...

So I have a question about this:

# app/controllers/locks_controller.rb line 6 before_action :require_project, if: :for_stage_lock?

I was going to originally change this to

before_action :require_stage, if: :for_stage_lock?

Is there some reason that could not be done? Are there other use cases that use a Project to determine lockability? It seems the locks apply to 2 things, global, and Stages. Why is it that we pass a Project in for the access check?

grosser · 2018-01-25T01:40:55Z

app/controllers/locks_controller.rb

    when 'destroy' then
      @project = lock.resource.project
+      if ENV['FEATURE_ONLY_ADMINS_CAN_LOCK_UNLOCK_PRODUCTION_STAGE'] && lock.resource.production
+        @project.define_singleton_method(:production) { true }


this looks very wrong :D

Yeah, see my other comment above.

grosser · 2018-01-25T16:55:54Z

I think it's just because admin_for etc need a project, should be alright to pass the stage in

…

On Thu, Jan 25, 2018 at 8:48 AM, Jesse Goerz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/controllers/locks_controller.rb <#2492 (comment)>: > @@ -60,11 +60,25 @@ def lock def require_project case action_name when 'create' then - @project = Stage.find(params[:lock][:resource_id]).project + stage = Stage.find(params[:lock][:resource_id]) So I have a question about this: # app/controllers/locks_controller.rb line 6 before_action :require_project, if: :for_stage_lock? I was going to originally change this to before_action :require_stage, if: :for_stage_lock? Is there some reason that could not be done? Are there other use cases that use a Project to determine lockability? It seems the locks apply to 2 things, global, and Stages. Why is it that we pass a Project in for the access check? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2492 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAsZ1HHOo1Z8T2rCftKlAbrKoczBfB8ks5tOLBDgaJpZM4Rr85i> .

jgoerz · 2018-01-25T23:56:33Z

The below is going to fail on the "locks" current_user test (line 245: "renders when authorized via the project"). I'm a little confused by this test. It seems to be testing if a project can be viewed by checking project level authorization... but a lock is only accessed after viewing the project/stage so I don't believe this is a valid test for the locks controller. Let me know what you think.

jgoerz · 2018-01-26T18:44:47Z

grosser commented:
hmmm I kinda like the idea of only letting admins unlock ... do you think the same restriction is necessary for locking ? ... I'd rather have anyone able to lock when something goes wrong ...

For our use case, we just want to prevent accidental production deploys. We plan on locking production stages with non-expiring locks. So letting anyone lock wouldn't be a problem for us. I could see how someone would want to strictly control both (un)lock. The only thing I could see a problem with, is that after someone locked it, because of the role/perms, the Unlock button would be grayed out or not displayed at all... which might be confusing if someone was just pushing buttons (exploring/learning).

If it is implemented... maybe split the lock actions into

:read
:create_or_update (or maybe just :lock)
:destroy or :unlock

If I had a choice, I'd choose to leave it as is (less work for me :-P ). But I'm flexible.

grosser · 2018-01-26T22:59:22Z

app/controllers/concerns/current_stage.rb

+  protected
+
+  def require_stage
+    @stage = (Stage.find_by_param!(params[:stage_id]) if params[:stage_id])


this won't work since stage permalinks are not globally unique, need to find base on project

grosser · 2018-01-26T23:01:49Z

app/controllers/concerns/current_user.rb

@@ -66,7 +66,12 @@ def resource_action
  end

  def can?(action, resource_namespace, scope = nil)
-    scope ||= current_project if respond_to?(:current_project)
+    case resource_namespace


this should be scope ||= to avoid duplication and to avoid the respond_to? calls

grosser · 2018-01-26T23:03:28Z

app/models/access_control.rb

            user.admin? # global locks
+          elsif ENV['PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN'] && scope.production


might be easier to read as:

else # stage locks if PRODUCTION_STAGE_LOCK_REQUIRES_ADMIN else end end

grosser · 2018-01-26T23:04:03Z

test/controllers/concerns/current_user_test.rb

-        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 lines
+      # 22 - 28.


lines change ... prefer names :)

jgoerz · 2018-01-30T15:11:37Z

Ready for review/feedback.

- When set, will require admin role for (un)locking stages with production attribute set. - All other locking is unchanged.

grosser · 2018-01-30T16:09:00Z

app/controllers/concerns/current_user.rb

@@ -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)


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

grosser · 2018-01-30T16:15:09Z

app/models/access_control.rb

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


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

grosser · 2018-01-30T16:17:41Z