Skip to content

Add request service permissions in generated policies #141

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Jul 9, 2019
Merged

Add request service permissions in generated policies #141

merged 6 commits into from
Jul 9, 2019

Conversation

@skucheria
Copy link
Member

@skucheria skucheria commented Jul 3, 2019

Addresses #94

Requires ros2/rclpy#383

Siddharth Kucheria added 4 commits July 3, 2019 11:05
added request service permissions
e925fb2 
Signed-off-by: Siddharth Kucheria <[email protected]>
added more tests
a9e5767 
Signed-off-by: Siddharth Kucheria <[email protected]>
fixed test
404195c 
Signed-off-by: Siddharth Kucheria <[email protected]>
fixed up tests
90fb399 
Signed-off-by: Siddharth Kucheria <[email protected]>
@skucheria skucheria force-pushed the kucheria/request_permissions branch from f9a24c7 to 90fb399 Compare July 3, 2019 18:05
@skucheria skucheria mentioned this pull request Jul 3, 2019
Merged
@kyrofa
Copy link
Member

kyrofa commented Jul 3, 2019 •
edited
Loading

This looks good, and the test is lovely, thank you! However, have you verified that create_permission can actually handle the policy produced by this? It may require some template changes. If so, that step may need to come before this one so we don't generate broken policies.

@skucheria skucheria requested a review from jacobperron July 3, 2019 18:50
@jacobperron
Copy link
Member

jacobperron commented Jul 8, 2019

I tried passing a generated policy with a service server and service client running. Passing it to create_permission seems to work for me. I believe the add_two_ints.xml policy file used as part of the unit tests covers this.

I think outside the scope of this PR, but it's probably worth adding integration tests to test_security for services (and actions).

jacobperron
jacobperron reviewed Jul 8, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, one suggestion.

@skucheria skucheria requested a review from jacobperron July 8, 2019 21:43
jacobperron
jacobperron approved these changes Jul 8, 2019
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@skucheria
Copy link
Member Author

skucheria commented Jul 8, 2019

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@skucheria skucheria merged commit 1c274b4 into master Jul 9, 2019
@delete-merged-branch delete-merged-branch bot deleted the kucheria/request_permissions branch July 9, 2019 16:32
ruffsl pushed a commit to ruffsl/sros2 that referenced this pull request Aug 6, 2019
@ruffsl
Add request service permissions in generated policies (ros2#141)
7282af4 
* added request service permissions

Signed-off-by: Siddharth Kucheria <[email protected]>
Signed-off-by: ruffsl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@skucheria @kyrofa @jacobperron