[PW-2409] Policies implementation #123

toydestroyer · 2019-10-29T06:26:28Z

Refer to the issue id if any. Include the link to the issue. E.g:

ApplicationPolicy should make use of the actions assigned to a user

Describe the changes made. What does this PR changes that might be critical. If any critical decisions have been made, make sure you explain the rationale for these decisions.

Introducing new (almost) concept of Policy Actions represented by Action model on IAM service.

Action model has these attributes:

name represented action name and could be new, create, index, show, destroy, edit, update or * for wildcard. Action name points to actual controller action so if you have a custom one like publish you can create policy action with this name.
effect — allow or deny. Everything is denied. deny effect more valuable than any amount of allow effects for the resource.
target_resource — resource represented by urn (including wildcarded ones):
- urn:ros:iam::222222222:credential
- urn:ros:iam::222222222:*
- urn:ros:*
segment — scope of resources described by action. Points to actual scope on model. scope MUST accept user context as a parameter (user context is instance of PolicyUser object).

There's predefined scopes everything that just alias for all but with user context and owned, that initially also pointed to all and should be redefined if necessary, for example iam/user model:

  def self.owned(user_context)
    where(id: user_context.iam_user.id)
  end

Actions belongs to Policies and Policy has many Actions. IAM User has field attached_actions with all cached actions for this User, User Groups or User Roles. Combined and deduplicated*.

[
  {
    "name": "*",
    "effect": "allow",
    "segment": "everything",
    "resources": [
      "urn:perx:cognito::222222222:*",
      "urn:perx:comm::222222222:*",
      "urn:perx:iam::platform:tenant",
      "urn:perx:iam::222222222:*"
    ]
  }
]

How does the implementation addresses the problem

User can manage policies and actions now to set permissions for User, Group and Role.

lib/core/app/policies/ros/application_policy.rb

rpbaltazar

On going review. but first set of comments to be addressed. I'm currently looking at the URN logic that is quite complex and I believe it can be simplified.

rpbaltazar · 2019-11-13T07:57:53Z

lib/core/app/models/concerns/ros/urn_concern.rb


      def urn_match?(urn_to_compare)
        params = %i[txt partition_name service_name region account_id resource]
-        record_urn = Ros::Urn.from_urn(self.class.to_urn)
+        record_urn = Ros::Urn.from_urn(self.to_urn)


What do we need the class urn_match? method for?

rpbaltazar · 2019-11-13T08:31:00Z

lib/core/app/policies/ros/application_policy.rb

-      check_action(:create?)
-    end
+      actions = if user.attached_actions.is_a?(String)
+                  JSON.parse(user.attached_actions)


I think that this should be handled by the PolicyUser and not the ApplicationPolicy itself, meaning that once we ask the PolicyUser for its actions, we should always get the same data type back.

rpbaltazar · 2019-11-14T06:24:57Z

lib/core/app/models/concerns/ros/scopable_concern.rb

+    extend ActiveSupport::Concern
+
+    included do
+      scope :everything, ->(_user_context) { all }


I'm not sure we should be sending the whole context to the scope. This would mean that all the models now all of the sudden know how to handle PolicyUser and know what to expect inside of it. I don't think that is the right way of doing it.
The context should most likely receive only the cognito_user_id and use it when needed, ignore it when they don't care about it

rpbaltazar · 2019-11-14T06:26:50Z

lib/core/app/models/concerns/ros/urn_concern.rb

@@ -16,22 +16,24 @@ def find_by_urn(value); find_by(urn_id => value) end

      # NOTE: Override in model to provide a custom id
      def urn_id; :id end
-    end
-
-    included do


Do we need to have urn_match? method both in the class level and instance level?

rpbaltazar · 2019-11-14T06:32:11Z

lib/core/app/policies/ros/application_policy.rb

-    def edit?
-      update?
-    end
+      actions.select! { |i| i['effect'] == 'allow' && (i['name'] == action.to_s || i['name'] == '*') }


This should also probably be the responsibility of the PolicyUser instead of the application policy.
E.g.
user.can?(action, record) which returns true/false

rpbaltazar · 2019-11-14T06:38:51Z

lib/core/app/policies/ros/application_policy.rb

+
+        scopes = []
+
+        actions.select! { |i| i['effect'] == 'allow' && (i['name'] == user.params['action'] || i['name'] == '*') }


This could probably become

user.scopes_for?(action, scope) which would return the scopes to be sent to the original scope (line 61)

rpbaltazar · 2019-11-14T08:03:14Z

services/cognito/app/models/user.rb

@@ -6,6 +6,8 @@ class User < Cognito::ApplicationRecord
  has_many :user_pools
  has_many :pools, through: :user_pools

+  scope :owned, ->(user_context) { where(id: user_context.cognito_user_id) }


So, as mentioned above, I don't think active records should know anything about request contexts

rpbaltazar · 2019-11-14T08:04:33Z

services/cognito/spec/policies/user_policy_spec.rb

@@ -17,7 +17,7 @@
    let(:scope) { Pundit.policy_scope!(policy_user, User) }

    context 'admin iam user' do
-      it 'returns all users' do
+      xit 'returns all users' do


If we don't need these, then we can remove them

rpbaltazar · 2019-11-14T08:34:06Z

services/iam/spec/policies/policy_policy_spec.rb

-    end
-  end
-end
+# RSpec.describe PolicyPolicy do


Once again if this is not needed, then we can delete it.

rpbaltazar · 2019-12-17T04:08:03Z

closing in favor of #170

toydestroyer added the WIP Pull request still in a work in progress. Comments are welcome label Oct 29, 2019

toydestroyer requested a review from rjayroach as a code owner October 29, 2019 06:26

alexeylightit reviewed Oct 29, 2019

View reviewed changes

lib/core/app/policies/ros/application_policy.rb Outdated Show resolved Hide resolved

toydestroyer force-pushed the poc-policies branch 3 times, most recently from 84a48b8 to 5c53e6f Compare November 6, 2019 01:08

toydestroyer force-pushed the poc-policies branch from 7a28bd9 to 326db00 Compare November 11, 2019 09:06

toydestroyer changed the title ~~Policies implementation~~ [PW-2409] Policies implementation Nov 12, 2019

toydestroyer force-pushed the poc-policies branch from 050aefd to 5a0e178 Compare November 13, 2019 01:00

toydestroyer requested a review from rpbaltazar November 13, 2019 02:03

Sergey Toy and others added 20 commits November 13, 2019 15:47

policies WIP

b115e48

WIP on check_action method; remove binding.pry

895e1a7

replace method_missing with define_method

3ca57be

rewrite attached_actions for PolicyUser

23df3a8

WIP

2ff608b

first actual Policies PoC

e8e84c1

WIP - playing with scopes

03f147c

WIP -

17fc2c0

add wildcard * on actions

59baa91

WIP

6ada1a5

Ros::Urn.merge_urns implementation

202b0c8

refactor merge and campare helpers in Ros::Urn

7febb12

add lib/iron_hide to gitignore

393b6cf

wip

28579be

caching attached_actions

6ff6ad0

remove redundant concern

54c55d2

fix request specs for new policies

e1e196b

update policy seeds

382a974

fix rubocop warnings

8b07ff6

temporary skip failed test

dfc575c

Sergey Toy added 2 commits November 13, 2019 15:47

remove redundant comments

32091ec

refactor ScopableConcern

e2ca5f5

toydestroyer force-pushed the poc-policies branch from f7f15f3 to e2ca5f5 Compare November 13, 2019 07:47

Sergey Toy and others added 2 commits November 14, 2019 09:33

fix rubocop warnings

1da74c8

WIP - small line

4eb89de

rpbaltazar requested changes Nov 14, 2019

View reviewed changes

rpbaltazar added 3 commits November 14, 2019 19:25

WIP -

92a6040

WIP -

14ade93

WIP -

d186eab

rpbaltazar closed this Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[PW-2409] Policies implementation #123

[PW-2409] Policies implementation #123

Uh oh!

toydestroyer commented Oct 29, 2019 •

edited

Loading

Uh oh!

Uh oh!

rpbaltazar left a comment

Uh oh!

rpbaltazar Nov 13, 2019

Uh oh!

rpbaltazar Nov 13, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar Nov 14, 2019

Uh oh!

rpbaltazar commented Dec 17, 2019

Uh oh!

Uh oh!


		scopes = []

		actions.select! { \|i\| i['effect'] == 'allow' && (i['name'] == user.params['action'] \|\| i['name'] == '*') }

[PW-2409] Policies implementation #123

[PW-2409] Policies implementation #123

Uh oh!

Conversation

toydestroyer commented Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Refer to the issue id if any. Include the link to the issue. E.g:

Describe the changes made. What does this PR changes that might be critical. If any critical decisions have been made, make sure you explain the rationale for these decisions.

How does the implementation addresses the problem

Uh oh!

Uh oh!

rpbaltazar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpbaltazar commented Dec 17, 2019

Uh oh!

Uh oh!

toydestroyer commented Oct 29, 2019 •

edited

Loading