Closed Bug 1054142 Opened 6 years ago Closed 6 years ago

Tests for new permissions model

Categories

(Webtools :: Pulse, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Assigned: sherry.shi, Mentored)

Details

(Whiteboard: [lang=python])

User Story

This is a mentored Pulse bug.  For general information on Pulse, see https://wiki.mozilla.org/Auto-tools/Projects/Pulse, which includes a section on Contributing.
Bug 1016082 updated the mozillapulse package to use the new queue- and exchange-name format.  We should update the tests to use users with the same kind of permissions as we will be using in production instead of using a single user with full read/write/configure access.
Mentor: mcote
User Story: (updated)
Priority: -- → P3
Whiteboard: [lang=python]
Hello, 

I'd like to give this bug a shot.

So from looking at the existing test setup, it looks like I just have to 
1. Edit the test/puppet/manifests/classes/rabbitmq.pp file to create different users with different permissions
2. Edit the test/runtest.py file to:
 - use the new queue- and exchange-formats
 - use a different set of configurations corresponding to different users

Am I missing anything?

Thanks,
Sherry
Hi Mark,

I uploaded my WIP here: https://bitbucket.org/sherryshi/mozillapulse

I did what I described above. Currently, runtest.py runs successfully. I just need to double check that it is doing the right thing. Would you mind taking a look and seeing if I need to change anything else?

Thanks,
Sherry
Hi Sherry, sorry for the delay in getting back to you.  Thanks for working on this!

First, in runtests.py, we actually only test three types of publisher/consumer: Code, Build, and PulseTest.  So you only need to create those three users ("code", "build", and "pulse") in the puppet files.  I'd also just get rid of the whole ${RABBITMQ_USER} variable in the different files and hard-code those three users; it's not that much of a maintenance burden.

Second, your permissions are a bit off; they should be

  \"^(queue/<user>/.*|exchange/.*)\" \"^(queue/<user>/.*|exchange/<user>/.*)\" \"^(queue/<user>/.*|exchange/<user>/.*)\"

replacing <user> with the usernames above ("code", "build", and "pulse").

And then you'll have to change runtests.py to use the appropriate username for each test, instead of them all using "pulse".

Finally, we need a test to make sure users can't publish to exchanges they don't own.  So you could add a simple test that uses the wrong user for the exchange, e.g. try using the "code" user with the PulseTestPublisher, and you should expect an exception when trying to publish (using self.assertRaises).  In this case you don't need to create a consumer, since no message will actually be published.  This means you'll have to create a new unittest.TestCase subclass that doesn't inherit from PulseTestMixin, since PulseTestMixin always expects the message to be published and always waits for the consumer to consume a message, neither of which will be true in this new test case.

That last part is probably confusing; let me know if you need me to explain further. :)
Assignee: nobody → shi.sherry5
Status: NEW → ASSIGNED
Hi Mark,

No problem! And thanks for your feedback!

One question: should we get rid of the "--user" and "--password" options, since specifying them would definitely cause runtest.py to fail? Or should we implement something to support those options such that the test would still pass?

Thanks,
Sherry
Yeah, those were added in case you wanted to run the tests against a different system (not the packaged Vagrant one). But you're right; they won't make much sense now, so you can remove them.  We should also update the docs to indicate that they should be run against the Vagrant box and not on a local system.
Hi Mark,

Another question, regarding the permission settings. The set_permissions command takes form of:

set_permissions [-p <vhostpath>] <user> <conf> <write> <read>

so should the permission settings be:

  \"^(queue/<user>/.*|exchange/<user>/.*)\" \"^(queue/<user>/.*|exchange/<user>/.*)\" \"^(queue/<user>/.*|exchange/.*)\"

i.e. users will have full permissions on "^exchange/<username>/.*$" and "^queue/<username>/.*$" and read permissions to "exchange/*"?

Thanks,
Sherry
Yup!  This is the security model we came up with to isolate users from each other but allow binding to any exchange.  It's presented in more detail on the Pulse wiki, if you're interested (along with other general Pulse info): https://wiki.mozilla.org/Auto-tools/Projects/Pulse#Security_Model

PulseGuardian, which is used to create Pulse users (amongst other functions), enforces these permissions here: https://github.com/mozilla/pulseguardian/blob/master/pulseguardian/model/pulse_user.py#L40-L48

We're emulating them in the mozillapulse tests.

To be honest, more and more I'm thinking these types of tests belong with PulseGuardian, which helps manage the whole Pulse system, and not with mozillapulse, which is a library that interfaces with Pulse.  But we may as well do them here for now and port them over later.
Hey Mark,

I made the changes you specified above: https://bitbucket.org/sherryshi/mozillapulse
Please let me know if I need to make more changes.

Thanks!
Sherry
Thanks Sherry, this is great!  The only thing that's missing is updating test/README.md, which is still documenting the old system (one user with full read/write permissions).  Could you fix that up?
Oh right... done! :)
Great!  There are only two little issues, which I commented in the code.  However they are tiny so I think I'll just fix them myself when I commit the code (in a few minutes).

After I commit it, do you want to do a follow-up patch for this bug?  There are other things we can test, and I think one that we should add is to verify that a consumer cannot connect to another user's queue.  This is slightly trickier, since the queue name is defined within the GenericConsumer class (the queue_name() property function).  You could perhaps subclass the consumer and override that function, or just create a kombu Queue directly like we do in GenericConsumer._create_queue().  I leave it up to you. :)
Hi Mark,

I'm having a little trouble getting the follow-up patch to work.

When I have the following:

        self.proc = ConsumerSubprocess(self.consumer, consumer_cfg, True)
        self.assertRaises(AccessRefused, self.proc.start)

The test fails with an AccessRefused exception on the first line, and the test fails with "AssertionError: AccessRefused not raised".

However, if I have: 

        self.assertRaises(AccessRefused, ConsumerSubprocess, self.consumer, consumer_cfg, True)

then the test fails in the same way ("AssertionError: AccessRefused not raised"), suggesting that the ConsumerSubprocess isn't getting initialised the same way when called by assertRaises.

Do you have any suggestions on a solution to this?

Thanks,
Sherry
That's strange.  In the current code, initializing the ConsumerSubprocess doesn't really do anything.  It's the call to start() that actually creates the consumer instance, which then (by default) connects.  Something is strange here; I would look at the tracebacks to try to see what's going on.  Maybe, since you are using two processes, the error is actually coming from the publisher?

I should mention two other things:

- You don't really need a subprocess for this test.  If you change the queue name to fall outside of its user's permissions (e.g. user "foo" tries to access queue/bar/test), then just creating the consumer instance should fail.  You actually don't need a publisher at all for this test, since you don't need to publish anything.

- You will probably get a SocketError instead of AccessRefused.  I'm not sure why; see bug 1079515 for details.
Hi Mark,

I fixed the issue by calling self.assertRaises on consumer.listen() instead. It did raise an "AccessRefused" exception. Here's the traceback:

Traceback (most recent call last):
  File "runtests.py", line 257, in test_wrong_queue
    consumer.listen()
  File "/usr/local/lib/python2.7/dist-packages/MozillaPulse-1.0-py2.7.egg/mozillapulse/consumers.py", line 161, in listen
    consumer.queues[0].queue_declare()
  File "/usr/local/lib/python2.7/dist-packages/kombu/entity.py", line 531, in queue_declare
    nowait=nowait)
  File "/usr/local/lib/python2.7/dist-packages/amqp/channel.py", line 1258, in queue_declare
    (50, 11),  # Channel.queue_declare_ok
  File "/usr/local/lib/python2.7/dist-packages/amqp/abstract_channel.py", line 69, in wait
    return self.dispatch_method(method_sig, args, content)
  File "/usr/local/lib/python2.7/dist-packages/amqp/abstract_channel.py", line 87, in dispatch_method
    return amqp_method(self, args)
  File "/usr/local/lib/python2.7/dist-packages/amqp/channel.py", line 241, in _close
    reply_code, reply_text, (class_id, method_id), ChannelError,
AccessRefused: Queue.declare: (403) ACCESS_REFUSED - access to queue 'queue/foo/test' in vhost '/' refused for user 'code'

Any way, my fix is uploaded in the usual location.

Let me know if that's okay =]

Hope you had a happy New Year's!
Sherry
Thanks!  Looks good; I just added a few comments to your commits.

As for the AccessRefused error, I just realized that only happens on wrong credentials (invalid username/password), not wrong permissions, so never mind. :)
All fixed! =]
Thanks!  There was just one little problem with indentation, and I noticed a couple other small style problems (lines with more than 80 characters, and some trailing whitespace).  Very minor, though, so I've cleaned them up in a follow-up commit.

I think this is good for now for this bug.  Thanks very much, and feel free to take a look at other Pulse bugs!  And Happy New Year! :)

https://hg.mozilla.org/automation/mozillapulse/rev/70224206ac64
https://hg.mozilla.org/automation/mozillapulse/rev/c6d8ca0c5150
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Thank you for being a great mentor!

Happy New Year to you too!
You need to log in before you can comment on or make changes to this bug.