Skip to content

permission api: use _keystore.get_* functions#194

Merged
kyrofa merged 4 commits intoros2:masterfrom
kyrofa:bugfix/get_keystore
Apr 9, 2020
Merged

permission api: use _keystore.get_* functions#194
kyrofa merged 4 commits intoros2:masterfrom
kyrofa:bugfix/get_keystore

Conversation

@kyrofa
Copy link
Member

@kyrofa kyrofa commented Apr 8, 2020 •
edited
Loading

Proof that we're still completely missing test coverage of some verbs (logged as #193) 😢 . Added some here.

@kyrofa kyrofa force-pushed the bugfix/get_keystore branch from fb8f0bb to 4005906 Compare April 8, 2020 04:11
@kyrofa kyrofa changed the title permission: use _keystore.get_* functions permission api: use _keystore.get_* functions Apr 8, 2020
@codecov
Copy link

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

Merging #194 into master will increase coverage by 4.95%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #194      +/-   ##
==========================================
+ Coverage   55.38%   60.34%   +4.95%     
==========================================
  Files          17       17              
  Lines         585      585              
  Branches       52       52              
==========================================
+ Hits          324      353      +29     
+ Misses        247      218      -29     
  Partials       14       14
Flag Coverage Δ
#unittests 60.34% <100.00%> (+4.95%) ⬆️
Impacted Files Coverage Δ
sros2/sros2/api/_permission.py 92.30% <100.00%> (+30.76%) ⬆️
sros2/sros2/verb/create_permission.py 68.00% <0.00%> (+68.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b41a2a...e7156ce. Read the comment docs.

mikaelarguedas
mikaelarguedas reviewed Apr 8, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

This changes the AttributeError to a segfaults for me. Can you detail how to test this ?

(Or provide a test for it :p)

@kyrofa
Copy link
Member Author

kyrofa commented Apr 8, 2020 •
edited
Loading

It changes it to a... segfault? What the heck? Any chance that's Focal/SSL-related? I just ran ros2 security create_permission for the basic talker/listener example and got AttributeErrors without this patch, and success with it (on Bionic).

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Apr 8, 2020

I just ran ros2 security create_permission for the basic talker/listener example and got AttributeErrors without this patch, and success with it.

I confirmed I have a segfault on Focal with the repo state before all the refactoring so not related to this PR. Not great but at least clears this ...

As mentionned offline, committing a simple test along this PR would be great, full coverage can be done at another time.

@kyrofa kyrofa force-pushed the bugfix/get_keystore branch from 4005906 to 6b9c1ef Compare April 8, 2020 18:58
@kyrofa
Copy link
Member Author

kyrofa commented Apr 8, 2020

committing a simple test along this PR would be great, full coverage can be done at another time.

Done. Couldn't let codecov own me like that. I'm afraid that I rebased instead of merged though, I'm sorry. At least the diff is tiny.

@kyrofa
permission api: use _keystore.get_* functions
e6aaaa3 
Proof that we're still completely missing test coverage of some verbs.
Add a basic `create_permissions` test as well.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@kyrofa kyrofa force-pushed the bugfix/get_keystore branch from 6b9c1ef to e6aaaa3 Compare April 8, 2020 19:59
@kyrofa
Support multiple RMWs in test
06754f8 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
@kyrofa kyrofa force-pushed the bugfix/get_keystore branch from 298c9c2 to 06754f8 Compare April 8, 2020 20:28
@kyrofa
Remove superfluous __init__.py
cd0ee0d 
Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
mikaelarguedas
mikaelarguedas previously approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

thanks for adding a smoke test !

Couple comments below but non blocking

@kyrofa
Refactor test and validate permissions
e7156ce 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
ruffsl
ruffsl approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@ruffsl ruffsl left a comment

Choose a reason for hiding this comment

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

LGTM

@ruffsl
Copy link
Member

ruffsl commented Apr 8, 2020

Later on, it might be easier to just compare the permission.xml tree to a reference xml file, and just pre-processes steps to remove/sanitize known diffs, like <validity>, or <domains> matches the ros domain env, or exceptional rules for logging and discovery. Once you filter out those tags, the rest of the tree for the _test_identity grant name your checking should be the same when pretty printed.

sros2/sros2/test/policies/permissions/talker_listener/permissions.xml

Lines 2 to 4 in 1984821

<permissions>
<grant name="/talker_listener/talker">
<subject_name>CN=/talker_listener/talker</subject_name>

@kyrofa kyrofa merged commit 84dc386 into ros2:master Apr 9, 2020
@kyrofa kyrofa deleted the bugfix/get_keystore branch April 9, 2020 01:28
@mikaelarguedas mikaelarguedas mentioned this pull request Jun 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ruffsl ruffsl ruffsl approved these changes

@mikaelarguedas mikaelarguedas mikaelarguedas left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kyrofa @mikaelarguedas @ruffsl