Apply ACL controls to PUBLISH operations #66
Conversation
This will allow a topic filter to act on both subscription requests and publishing.
We maintain backward compatibility with older configurations by assuming all `PUBLISH` actions are permitted if no ACL is present. Otherwise, we follow the same rules as for `SUBSCRIBE`, with the exception that we read the ACL from the `publish-acl` property instead of `acl`.
If the user forgets to provide a `topic-check` key, we need to initialise `topic_config` to some value or else `topic_filtering` will fail with an `AttributeError`.
sjlongland
force-pushed
the
feature/publish-acl
branch
from
863bc30 to
60f3bee
Compare
This is more of a "demo" plug-in, but let's test it anyway.
Clean up the coding style a bit.
If `BaseTopicPlugin` returns `False`, we should propagate that failed response in `TopicAccessControlListPlugin`.
Yes, too lazy to hold down shift when hitting the `'` key.
|
Hi @sjlongland Thanks for taking this up! Have you seen #42? I guess your work superseeds that MR. |
This uses the real plug-ins to test the broker correctly responds to the return values when publishing and subscribing. The plug-ins themselves are tested elsewhere.
As per review comments.
I had not seen that (should've looked first I guess). Seems there's a bit more to be done on that before it can be merged. |
|
I couldn't figure out an easy way to "unit test" the relevant code paths in |
This is a `py.test` plug-in that allows us to test logging in test cases. Seems like an ideal candidate for a logger fixture.
|
So, rather than making https://pypi.org/project/pytest-logdog/ Have a look, and see what you think. |
|
Hi @sjlongland thanks for continuing the work on this! The amount of tests is amazing :) regarding
|
tests/plugins/passwd
Outdated
| test_user:$6$.c9f9sAzs5YXX2de$GSdOi3iFwHJRCIJn1W63muDFQAL29yoFmU/TXcwDB42F2BZg3zcN5uKBM0.1PjwdMpWHRydbhXWSc3uWKSmKr. | ||
| # Password for these is "test" |
There was a problem hiding this comment.
I think we should have different passwords for different test users - otherwise a bug where user1 can authenticate with password of user2 might not be catched.
While this MR is not about authentication, having it in the example data is a liability in my eyes :)
tests/plugins/test_topic_checking.py
Outdated
| assert authorised is False | ||
|
|
||
| # Should have printed a couple of warnings | ||
| log_records = list(pile.drain(name="testlog")) |
There was a problem hiding this comment.
The example in the documentation interacts with pile after closing the context manager. Not sure how exactly it is intended to be used.
As pointed out, the `logdog` docs do the checks outside of the context manager scope, so we should do so as well.
As pointed out, it's possible to accidentally mix them up if the passwords are the same. Obviously, the passwords used for these tests should _NOT_ be used in production.
|
Yeah, the password thing was me being lazy… but you make a valid point. I've changed those. As for A release in |
There was a problem hiding this comment.
LGTM! Thanks!
@FlorianLudwig your call. Approved from my side.
EDIT: Well, that was a little too early I guess :) Some failures need to be fixed on the pipeline.
|
I think I've got the formatting issues sorted, just working through how to call |
|
There we go, I've ignored the warnings from the |
Seems Python 3.8+ moved it, and I took the reference from there.
|
Is this good to go? LGTM. @sjlongland if this is ready, can you request a review from @FlorianLudwig ? |
Yep, it is… and I have. :-) |
|
The master-branch is now open for |
|
Many thanks. :-) |
This is a work-in-progress, but basically the idea is that the
topic_aclplug-in should be able to also dictate what topics a user may publish to, as well as what topics they may subscribe from.Not fully tested at this stage… still trying to figure out the structure of the test suite.