Skip to content
This repository has been archived by the owner on Feb 25, 2021. It is now read-only.

22 add types to relativepathresolver #23

Merged
merged 2 commits into from
Jul 10, 2020

Conversation

h-artzi
Copy link
Contributor

@h-artzi h-artzi commented Jun 25, 2020

Issue: #22

I was unsure regarding the relative paths for members in revoke and grant statements. It seems that members either have a role.id or an id. So currently, I transform both of those into relative paths. (Not sure if that was the right decision)

The next step is to bump the version!

@h-artzi h-artzi self-assigned this Jun 25, 2020
@micahlee
Copy link
Contributor

micahlee commented Jun 25, 2020

We should add tests cases for grant/revoke and permit/deny with absolute and relative paths in the test fixtures: https://github.com/cyberark/conjur-policy-parser/tree/master/spec/resolver-fixtures

There should at least be test case that would fail without this before making the change.

@h-artzi
Copy link
Contributor Author

h-artzi commented Jun 25, 2020

Yes, I will add a test case. This PR was put on hold until I update master to match the current tag (v3.0.4)

@h-artzi h-artzi force-pushed the 22-add-types-to-relativepathresolver branch 3 times, most recently from 3812964 to 9385661 Compare June 26, 2020 20:53
@h-artzi h-artzi requested a review from micahlee June 26, 2020 20:54
@h-artzi h-artzi force-pushed the 22-add-types-to-relativepathresolver branch from 9385661 to ca4b82b Compare June 26, 2020 20:57
- !revoke
member: !member
account: the-account
id: myapp/../the-group
Copy link
Contributor

@micahlee micahlee Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you expect this .. to be expanded/resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not, I forgot to change this from an earlier test I was running.

Array(record.member).each do |member|
member.role.id = absolute_path_of(member.role.id)
# Member has either role.id or id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be expanded a bit more. It's not clear to me what each of these cases means.

@h-artzi h-artzi force-pushed the 22-add-types-to-relativepathresolver branch from ca4b82b to adb3738 Compare July 9, 2020 20:56
h-artzi added 2 commits July 9, 2020 17:33
This should fail when running `./test.sh` because the fix has not been implemented yet.
@h-artzi h-artzi force-pushed the 22-add-types-to-relativepathresolver branch from adb3738 to f832ca2 Compare July 9, 2020 21:33
@h-artzi h-artzi requested review from a team and micahlee July 9, 2020 21:34
@h-artzi h-artzi merged commit 2f958c8 into master Jul 10, 2020
@h-artzi h-artzi deleted the 22-add-types-to-relativepathresolver branch July 10, 2020 13:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants