add roles when using conjurctl policy load #1510

h-artzi · 2020-04-27T20:20:26Z

What does this PR do?

When using conjurctl to load a policy, it failed to create role credentials. This PR allows the creation of roles.

What ticket does this PR close?

How to test?

Check the ticket and run it locally with the appropriate image. It is expected to successfully run and output the created roles.

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Note: I am not sure that this should be a supported feature in the future.

Follow-on issues

Follow-up issue(s) have been logged (and links included below) to update documentation or related projects, or
No follow-up issues are required

lib/root_loader.rb

CHANGELOG.md

lib/root_loader.rb

jonahx · 2020-04-28T17:44:13Z

lib/root_loader.rb

+          credentials = Credentials[role: role] || Credentials.create(role: role)
+          memo[role.id] = { id: role.id, api_key: credentials.api_key }
+          memo
+        end


I'd split this whole thing up into 2 statements, rather than chaining.

Also, see how the above has the awkward pattern

memo[blah] = stuff..... memo

each_with_object is made for avoiding just this.

So this is exactly what I meant.... buuuuuut

I don't get how it's working because each_with_object reverses the arguments relative to reduce, so that the memo is in the 2nd arg:

https://ruby-doc.org/core-2.7.1/Enumerable.html#method-i-each_with_object

lib/root_loader.rb

jtuttle

Please fix the codeclimate errors. Break that load method up into helpers.

lib/root_loader.rb

codeclimate · 2020-05-05T13:04:23Z

lib/root_loader.rb

+
+      created_roles = accepted_roles.each_with_object({}) do |role, memo|
+        credentials = Credentials[role: role] || Credentials.create(role: role)
+        memo[role.id] = { id: role.id, api_key: credentials.api_key }


RootLoader#create_roles calls 'role.id' 2 times

codeclimate · 2020-05-05T13:04:23Z

lib/root_loader.rb

+      end
+    end
+
+    def create_roles policy_version


RootLoader#create_roles has approx 8 statements

I just noticed this after approving, but per CC you should extract this:

loader = Loader::Orchestrate.new policy_version loader.load accepted_roles = loader.new_roles.select do |role| %w(user host).member?(role.kind) end

into an accepted_roles(policy_version) method.

jonahx · 2020-05-05T14:52:24Z

lib/root_loader.rb

+        policy_version = policy_version(role: admin, policy: root_policy_resource, filename: filename)
+
+        accepted_roles = accepted_roles(policy_version)
+        create_roles(accepted_roles)


To avoid confusion with the method name, we should use something diff for the local var name:

acc_roles = accepted_roles(policy_version) create_roles(acc_roles)

jonahx · 2020-05-05T14:54:28Z

lib/root_loader.rb

@@ -23,19 +25,45 @@ def load account, filename
        end

        root_policy_resource = Loader::Types.find_or_create_root_policy(account)
+        policy_version = policy_version(role: admin, policy: root_policy_resource, filename: filename)


split long line:

policy_version = policy_version( role: admin, policy: root_policy_resource, filename: filename )

and per other comment, use something different from local var name than the method name, policy_ver or something.

jonahx

👍

lib/root_loader.rb

jonahx · 2020-05-05T20:55:40Z

lib/root_loader.rb

+      policy_version.perform_automatic_deletion =
+        policy_version.delete_permitted =
+        policy_version.update_permitted = true
+      policy_version.save


What's happening here? It looks like something went wrong...

Just being naughty and seeing if reek would complain. 🤭

Was trying a multiple assignment to get around the statement limit.

And I would have gotten away with it too if it wasn't for that darn indentation warning! 🐕

hahahahaha

This is a clear case where reek may be ignored. BUT.... the deeper reason it's throwing the warning is because the API for PolicyVersion.new uses the "create and configure" pattern, which is poor and should be avoided. Instead the constructor should take a hash and return an immutable object.

Also, what does the dog emoji mean?

[true] * 3

😆 😆

I think it's time to admit defeat and just ignore the CC error.

Hah yeah I jokingly suggested [true] * 3 to Hadar too. 😂

The dog emoji was a reference to Scooby Doo's "and I would have gotten away with it too if it wasn't for you darn kids!" 😄

codeclimate · 2020-05-06T13:42:53Z

Code Climate has analyzed commit 497873f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 75.8% (50% is the threshold).

This pull request will bring the total coverage in the repository to 85.4% (0.0% change).

View more on Code Climate.

fixed

h-artzi requested a review from a team April 27, 2020 20:20

codeclimate bot reviewed Apr 27, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

lib/root_loader.rb Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_load_host_and_user branch 3 times, most recently from 11e21a2 to 83e7c29 Compare April 28, 2020 13:22

codeclimate bot reviewed Apr 28, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_load_host_and_user branch from ccd2313 to d3147ed Compare April 28, 2020 14:04

codeclimate bot reviewed Apr 28, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

jtuttle suggested changes Apr 28, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lib/root_loader.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_load_host_and_user branch from a32b4ac to 67c40c5 Compare April 28, 2020 15:29

jonahx reviewed Apr 28, 2020

View reviewed changes

h-artzi force-pushed the conjurctl_load_host_and_user branch from 67c40c5 to 933f74c Compare May 4, 2020 20:23

jonahx reviewed May 4, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

micahlee reviewed May 4, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_load_host_and_user branch 3 times, most recently from b491c48 to 238df9a Compare May 5, 2020 01:21

h-artzi requested review from jtuttle and jonahx May 5, 2020 01:23

jtuttle previously requested changes May 5, 2020

View reviewed changes

lib/root_loader.rb Outdated Show resolved Hide resolved

h-artzi force-pushed the conjurctl_load_host_and_user branch 2 times, most recently from a0f0634 to 8f490bd Compare May 5, 2020 12:48

codeclimate bot reviewed May 5, 2020

View reviewed changes

jonahx previously approved these changes May 5, 2020

View reviewed changes

h-artzi dismissed jonahx’s stale review via ea9caf7 May 5, 2020 14:06

h-artzi force-pushed the conjurctl_load_host_and_user branch from 8f490bd to ea9caf7 Compare May 5, 2020 14:06

h-artzi requested a review from jonahx May 5, 2020 14:09

h-artzi force-pushed the conjurctl_load_host_and_user branch 2 times, most recently from 1b4de2e to ae2c656 Compare May 5, 2020 14:48

jonahx reviewed May 5, 2020

View reviewed changes

h-artzi changed the title ~~add roles when using conjurctl policy load~~ WIP - add roles when using conjurctl policy load May 5, 2020

jonahx previously approved these changes May 5, 2020

View reviewed changes

codeclimate bot reviewed May 5, 2020

View reviewed changes

lib/root_loader.rb Show resolved Hide resolved

h-artzi requested a review from jtuttle May 5, 2020 20:05

h-artzi changed the title ~~WIP - add roles when using conjurctl policy load~~ add roles when using conjurctl policy load May 5, 2020

jtuttle dismissed jonahx’s stale review via 5387caa May 5, 2020 20:52

jonahx reviewed May 5, 2020

View reviewed changes

h-artzi force-pushed the conjurctl_load_host_and_user branch 2 times, most recently from 0cd9b69 to 68a3d37 Compare May 6, 2020 13:23

jonahx previously approved these changes May 6, 2020

View reviewed changes

add roles when using conjurctl load policy

497873f

h-artzi dismissed jonahx’s stale review via 497873f May 6, 2020 13:26

h-artzi force-pushed the conjurctl_load_host_and_user branch from 68a3d37 to 497873f Compare May 6, 2020 13:26

jtuttle approved these changes May 6, 2020

View reviewed changes

jonahx approved these changes May 6, 2020

View reviewed changes

h-artzi merged commit b2a8abb into master May 6, 2020

h-artzi deleted the conjurctl_load_host_and_user branch May 7, 2020 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add roles when using conjurctl policy load #1510

add roles when using conjurctl policy load #1510

h-artzi commented Apr 27, 2020 •

edited

Loading

jonahx Apr 28, 2020

jonahx May 4, 2020

jtuttle left a comment

codeclimate bot May 5, 2020

codeclimate bot May 5, 2020

jonahx May 5, 2020

jonahx May 5, 2020

jonahx May 5, 2020

jonahx left a comment

jonahx May 5, 2020

jtuttle May 5, 2020

jtuttle May 5, 2020

jonahx May 5, 2020

jonahx May 5, 2020

jtuttle May 5, 2020

codeclimate bot commented May 6, 2020

add roles when using conjurctl policy load #1510

add roles when using conjurctl policy load #1510

Conversation

h-artzi commented Apr 27, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

How to test?

Checklists

Change log

Test coverage

Follow-on issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtuttle left a comment

Choose a reason for hiding this comment

codeclimate bot May 5, 2020

Choose a reason for hiding this comment

codeclimate bot May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented May 6, 2020

h-artzi commented Apr 27, 2020 •

edited

Loading