Fix duplicate members validation in policy yaml

[oburstein-hub]

What does this PR do? Is there any background context you want to provide?

This PR added validation on policy yaml file upon loading it to conjur Now members with same name under each hierarchy level are disallowed

Reconstruction Steps:

1. Run Conjur
2. Enter Conjur CLI
3. Create policy.yaml file with the following content:

   
   - !host host1
   - !host host2

• !variable secret

• !group secret-consumers

• !permit  role: !group secret-consumers  resource: !variable secret  privilege: [ read, execute ]

• !grant  role: !group secret-consumers  member: !host host1  member: !host host2

4. Load policy to the system using the following command: conjur policy load root policy.yaml

5. The expected result should be failure of loading policy with an appropriate error message describing that there are duplicate entries in policy and mentioning which of them The error message is: "Duplicate attribute on type member"

What ticket does this PR close?

https://github.com/cyberark/conjur/issues/1263

Where should the reviewer start? How should this functionality be validated?

The reviewer should look on the changes in handler.rb file - they implement a new validation

[orenbm]

@oburstein-hub few comments before i can review this:

1. can you please fix the PR description? It still has some template leftovers. Is this connected to an issue? I guess it's fixing a bug so it will really help me to see the bug that this PR is fixing.
2. I don't really understand what is printed to the user and when is it printed. What is the value of value in the printed message. Please write a use-case in the description (with an example policy) and what will be printed when it will be loaded.
3. Please fix the commit message to start with the word "Fix" instead of "fixing".

[orenbm]

Also - if this error is raised to the user we need the message to be reviewed by a TW.

[oburstein-hub]

Hi Yivon Can you please tell if the following error message is good enough for the test case above: The error message is: "Duplicate attribute on type member"

[orenbm]

@oburstein-hub thanks for addressing my requests!

Small nit - can you change the word "fix" to "Fix" in the commit message? We want capital letters in the beginning of commits.

[orenbm]

@yserota please provide a message that will be sent to the user in case of the error described here.

Please note that another use-case is the following policy:

- !host host1
- !host host2

- !variable secret
- !group secret-consumers

- !permit
  role: !group secret-consumers
  role: !variable secret

In this case the role attribute is duplicated and thus the policy load would fail.

@oburstein-hub can you please elaborate on the parameters of the scalar method:

def scalar value, tag, quoted, anchor

It will help us to know what we can output to the user.

I think that Duplicated attribute <attribute> in <section> section (e.g Duplicated attribute "member" in "permit" section`) will be a good start.

[yserota]

Hi @oburstein-hub I think I need more context to comment.

[orenbm]

@yserota is the issue description not enough? You can go to the link stated there if it can help. i think it will give enough context

[yserota]

Hi @oburstein-hub , I'm just not sure what you want me to review. As I am not working on this feature, I need more context. Sorry to be a big pain.

[oburstein-hub]

Regarding parameters is scalar function: 'value' is the name of parameter (like 'member' in our case) 'tag' is a type of simple parameter (like '!host'). In case of 'member' it is empty 'quoted' means that the parameter is quoted - irrelevant for us

There is no simple way to retrieve the hierarchy under which 'member' is placed. From the the other side the generic error handling of conjur issues the line number in file where the problem is - and I think its good enough

[orenbm]

Regarding parameters is scalar function: 'value' is the name of parameter (like 'member' in our case) 'tag' is a type of simple parameter (like '!host'). In case of 'member' it is empty

Thanks @oburstein-hub . In this case I think we can have only the value in the log message.