Conversation
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| ) | ||
| end | ||
| rescue => e | ||
| return @failure.new( |
There was a problem hiding this comment.
Avoid too many return statements within this method.
|
|
||
| require 'base64' | ||
|
|
||
| module Factories |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
|
|
||
| require 'base64' | ||
|
|
||
| module Factories |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| policy: Base64.encode64(policy_template), | ||
| target_policy_namespace: "<%= branch %>", | ||
| target_variable_namespace: "<%= branch %>/<%= id %>", | ||
| schema: { |
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
| version: 1, | ||
| policy: Base64.encode64(policy_template), | ||
| policy_branch: "<%= branch %>", | ||
| schema: { |
There was a problem hiding this comment.
Identical blocks of code found in 2 locations. Consider refactoring.
jvanderhoof
force-pushed
the
policy-factory-v2
branch
2 times, most recently
from
e20cd4b to
3ca23b5
Compare
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def call(factory_template:, request_body:, account:, authorization:) |
There was a problem hiding this comment.
Method call has a Cognitive Complexity of 10 (exceeds 5 allowed). Consider refactoring.
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def call(factory_template:, request_body:, account:, authorization:) |
There was a problem hiding this comment.
Method call has 42 lines of code (exceeds 25 allowed). Consider refactoring.
|
|
||
| private | ||
|
|
||
| def validate_and_transform_request(schema:, params:) |
There was a problem hiding this comment.
Method validate_and_transform_request has 26 lines of code (exceeds 25 allowed). Consider refactoring.
| @failure.new(errors.flatten, status: :bad_request) | ||
| end | ||
|
|
||
| def render_and_apply_policy(policy_load_path:, policy_template:, variables:, account:, authorization:) |
There was a problem hiding this comment.
Method render_and_apply_policy has 41 lines of code (exceeds 25 allowed). Consider refactoring.
| end | ||
| end | ||
|
|
||
| def set_factory_variables(schema_variables:, factory_variables:, variable_path:, authorization:, account:) |
There was a problem hiding this comment.
Method set_factory_variables has 27 lines of code (exceeds 25 allowed). Consider refactoring.
jvanderhoof
force-pushed
the
policy-factory-v2
branch
2 times, most recently
from
3b64fd2 to
4d23821
Compare
john-odonnell
left a comment
john-odonnell
left a comment
There was a problem hiding this comment.
Haven't reviewed the templates or tests yet, but this looks great so far. I'll finish up my review tomorrow 🚀
| @resource.where( | ||
| Sequel.like( | ||
| :resource_id, | ||
| "#{account}:variable:conjur/factories/#{kind}/%" | ||
| ) | ||
| ).all.select { |i| i.resource_id.split('/').last == id }.max { |a, b| a.id <=> b.id } |
There was a problem hiding this comment.
Here's my understanding of this logic:
-
@resource.where(...).allwill return all factory variables of the specified kind:[core/v1/user, core/v2/user, core/v1/group] -
.select {...}reduces the results to only those of the specified ID:[core/v1/user, core/v2/user] -
.max {...}reduces this further to only the resource with the highest version:core/v2/user
I'm not certain of the mechanism that the <=> operator uses, but it seems to leave open some irregularity - for example, "v9" <=> "v10" returns 1, indicating "v9" is greater.
Definitely won't have an effect now, and probably not for a while (if ever), but thought I'd capture it.
There was a problem hiding this comment.
Good catch John. I'll add some tests for this.
| end | ||
| .map do |_, versions| | ||
| # find the most recent version | ||
| versions.max { |a, b| a.id <=> b.id } |
There was a problem hiding this comment.
Same as comment on line 76.
| @failure.new( | ||
| { resource: "#{classification}/#{version}/#{id}", message: 'Requested Policy Factory is not available' }, | ||
| status: :bad_request | ||
| ) |
There was a problem hiding this comment.
Is this failure case truly a :bad_request? Doesn't that imply client error, where this would be a server error (the variable exists, but doesn't contain the information we expect)?
There was a problem hiding this comment.
What do you think about 404 Not Found instead?
There was a problem hiding this comment.
This one is rough because it's dependent on prior actions (as you've pointed out). Micah and I have been talking about phase two of these factories, which makes them first class citizens at the policy level (work here). Much of the pre-population issues will go away with this work.
As this is only an alpha level feature, how do you feel about addressing this work as part of the next phase?
|
|
||
| require 'base64' | ||
|
|
||
| module Factory |
There was a problem hiding this comment.
This PR has both a Factory and Factories module, but they aren't distinct.
There was a problem hiding this comment.
Good catch. I'll standardize on Factories.
| { | ||
| code: @response_codes[@response.status] | ||
| }.tap do |rtn| | ||
| rtn[:error] = format_error_message(@response.message) | ||
| end |
There was a problem hiding this comment.
Do these two code samples have the same result, as .tap returns the object it's called on with modifications?
{
code: @response_codes[@response.status]
}.tap do |rtn|
rtn[:error] = format_error_message(@response.message)
end{
code: @response_codes[@response.status],
error: format_error_message(@response.message)
}There was a problem hiding this comment.
Correct. The tap function is typically used if you need to optionally perform an action on something.
|
|
||
| Setup will follow the following workflow: | ||
|
|
||
| ```plantuml |
There was a problem hiding this comment.
Doesn't look like these are rendering correctly in the final MD.
There was a problem hiding this comment.
Good catch. VSCode does it for me. I'll export the diagrams and include the images.
| - !variable provider-uri | ||
| - !variable client-id | ||
| - !variable client-secret | ||
| - !variable redirect-uri | ||
| - !variable claim-mapping |
There was a problem hiding this comment.
Should we include the optional token-ttl, name & provider-scope?
There was a problem hiding this comment.
I've intentionally left these out as we need a way to create a variable only if the variable has a value. I'm not sure how best to do that yet.
There was a problem hiding this comment.
Would it make more sense to create the variables and leave them empty? Let default behavior ride, and let the user change config if needed.
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def call(factory_template:, request_body:, account:, authorization:) |
There was a problem hiding this comment.
This is the only file where I feel the use of .bind handling the [Success|Failure]Responses has hindered readability & upped complexity.
There was a problem hiding this comment.
This is my least favorite file in the commit. I'll take another look. I'm planning to refactor this in the future, and if it's gnarly, I'll file a ticket and give a comment here.
| - !policy | ||
| id: <%= id %> | ||
| annotations: | ||
| factory: connections/database |
There was a problem hiding this comment.
| factory: connections/database | |
| factory: connections/v1/database |
| "properties": { | ||
| "branch": { | ||
| "description": "Policy branch to load this group into", | ||
| "type": "string" | ||
| }, | ||
| "annotations": { | ||
| "description": "Additional annotations to add to the group", | ||
| "type": "object" | ||
| } | ||
| }, |
There was a problem hiding this comment.
These properties refer to a group, and also seem incomplete - need [member|role]_resource_[id|type]
There was a problem hiding this comment.
This schema is still missing [member|role]_resource_[id|type] properties
|
|
||
| require 'spec_helper' | ||
|
|
||
| describe SuccessResponse do |
There was a problem hiding this comment.
Maybe the unit tests for [Success|Failure]Response could confirm the behavior of .bind. I spent the most time digesting [Success|Failure]Response.bind, and a unit test case or two might've made their behavior plain to see.
There was a problem hiding this comment.
Good call. I'll add some tests. bind isn't the most intuitive action.
spec/app/domain/responses_spec.rb
Outdated
| it "is returned as a hash with the key 'message'" do | ||
| expect(failure.message).to eq('baz') |
There was a problem hiding this comment.
| it "is returned as a hash with the key 'message'" do | |
| expect(failure.message).to eq('baz') | |
| it "is returned as a string" do | |
| expect(failure.message).to eq('baz') |
jvanderhoof
force-pushed
the
policy-factory-v2
branch
2 times, most recently
from
a58be64 to
95a15fe
Compare
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def call(factory_template:, request_body:, account:, authorization:) |
There was a problem hiding this comment.
Method call has a Cognitive Complexity of 13 (exceeds 5 allowed). Consider refactoring.
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def call(factory_template:, request_body:, account:, authorization:) |
There was a problem hiding this comment.
Method call has 43 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 48 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 48 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 26 lines of code (exceeds 25 allowed). Consider refactoring.
jvanderhoof
force-pushed
the
policy-factory-v2
branch
from
95a15fe to
0947884
Compare
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 34 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 34 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 34 lines of code (exceeds 25 allowed). Consider refactoring.
| TEMPLATE | ||
| end | ||
|
|
||
| def data |
There was a problem hiding this comment.
Method data has 38 lines of code (exceeds 25 allowed). Consider refactoring.
| @success.new(factories) | ||
| end | ||
|
|
||
| def find(kind:, id:, account:, role:, version: nil) |
There was a problem hiding this comment.
Method find has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
This commit includes the functional code and tests for the Policy Factory feature.
This commit includes an initial set of Factory templates. These may need some work before the official release.
jvanderhoof
force-pushed
the
policy-factory-v2
branch
from
0947884 to
b30637d
Compare
| @failure = ::FailureResponse | ||
| end | ||
|
|
||
| def find_all(account:, role:) |
There was a problem hiding this comment.
Method find_all has 26 lines of code (exceeds 25 allowed). Consider refactoring.
| @success.new(factories) | ||
| end | ||
|
|
||
| def find(kind:, id:, account:, role:, version: nil) |
There was a problem hiding this comment.
Method find has 26 lines of code (exceeds 25 allowed). Consider refactoring.
| end | ||
|
|
||
| task load: :environment do | ||
| binding.pry |
There was a problem hiding this comment.
Remove debugger entry point binding.pry.
| end | ||
|
|
||
| task test: :environment do | ||
| binding.pry |
There was a problem hiding this comment.
Remove debugger entry point binding.pry.
| "password": { | ||
| "description": "Database Password", | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Avoid comma after the last item of a hash.
|
Code Climate has analyzed commit b30637d and detected 114 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 89.6% (50% is the threshold). This pull request will bring the total coverage in the repository to 88.5% (0.2% change). View more on Code Climate. |
2 participants
This work is an extension of the my Hackathon effort from this past fall, which aimed enable an API for generating a variety of Conjur resources.
This effort:
Overview
Factory Structure
Factories are stored in Conjur Variables as Base64 encoded values. This PR includes the following factories:
Factory variables follow the following convention:
conjur/factories/<classification>/<factory>The following factories will be added as part of a separate pull request:
Tests for these templates will also be part of this upcoming PR.
API
The API selects the desired policy (ex.
coreorauthenticators) as well as the target variable name (group,policy,user,managed-policy, etc), we have an immense amount of flexibility to organize and create factories in the future.Using JSON Schema, each factory defines the inputs it requires and and optionally accepts. This information is available through the factory's
infoendpoint. This allows us to dynamically include these endpoints in CLIs and SDKs in the future.Security
Factory endpoint requests require
executepermission on the Factory variable and appropriate permission on the target Policy. This allows Conjur RBAC to be used to manage who/what can use Factories to extend policy, and all actions are captured in Conjur Audit.Demo
To install the base policies required for the Policy Factory, run the following command on Conjur (Conjur needs to be running):
The above command will install a base policy into the
conjur/factoriesnamespace. This base policy includes factories for creating Conjur (all in thecorenamespace):GroupManagedPolicy- creates a policy with an owner groupPolicyUserWith the above factory templates, the following API endpoints become available:
API Overview
The API is fully documented in the Solution Design.
Connected Issue/Story
N/A
Definition of Done
At least 1 todo must be completed in the sections below for the PR to be
merged.
Changelog
CHANGELOG update
Test coverage
changes, or
Documentation
READMEs) were updated in this PRBehavior
Security