Merge lp://qastaging/~rvb/maas/maas-security-model into lp://qastaging/~maas-committers/maas/trunk

Also, note that proper usage of the security model in view will be done in a followup branch. This branch focuses on the basic infrastructure and the api security.

Looks good.

Let's see if I'm getting this right:

* AccessMiddleware is almost an optimization; it just provides a
  cheap, coarse policy.

* MaaSAuthorizationBackend is more like zope.security in that it
  provides finer control over who can see what, although it's not yet
  fully plumbed in.

[1]

utilities/format-new-and-modified-imports -r submit:

[2]

+ self.public_urls = tuple(
+ re.compile(url) for url in (
+ reverse('login'), reverse('logout'), reverse('api-doc')))

You could compose this into one so you don't have to iterate over it
later in process_request():

        self.public_url = re.compile(
            "|".join(
                (reverse('login'),
                 reverse('logout'),
                 reverse('api-doc'))))

[3]

+ elif self.static_url.match(request.path):
+ return None

Ah, is this how it says "not me"?

[4]

+ return HttpResponseRedirect("%s?next=%s" % (
+ self.login_url, request.path))

There might need to be some escaping in there.

                from urllib import quote_plus
                return HttpResponseRedirect("%s?next=%s" % (
                    self.login_url, quote_plus(request.path)))

[5]

+ status = models.CharField(
+ max_length=10, choices=NODE_STATUS_CHOICES, editable=False,
+ default=DEFAULT_STATUS)

This ought to be an enum, stored as a INT, like in Launchpad.
PostgreSQL does support ENUM fields, but they make upgrading more
complicated. I don't know what the Django way of doing this is.

[6]

+class MaaSAuthorizationBackend(object):

This looks a bit like zope.security.

[7]

+ def test_anon_nodes_GET(self):
+ response = self.client.get('/api/nodes/')
+ self.assertEqual(401, response.status_code)

Comment/docstring missing.

Most of our tests don't explain themselves :-/

Consider using httplib.UNAUTHORIZED instead of 401, or add a trailing
comment explaining what 401 means.

[8]

+ self.user = factory.make_user(username='user', password='test')
+ self.client = Client()
+ self.client.login(username='user', password='test')

This appears twice. I imagine it's going to appear many more
times. Consider extracting this to a get_authenticated_client()
function or something like that, and chucking it in the testing
package somewhere.

[9]

+class AuthTestMixin(object):
+
+ def setUp(self):
+ super(AuthTestMixin, self).setUp()
+ self.backend = MaaSAuthorizationBackend()
...
+class TestMaaSAuthorizationBackend(AuthTestMixin, TestCase):
+
+ def setUp(self):
+ super(TestMaaSAuthorizationBackend, self).setUp()
+ self.backend = MaaSAuthorizationBackend()

One of those self.backend assignments can be removed. If the latter
then TestMaaSAuthorizationBackend.setUp() can be deleted too, though I
suspect you'll be removing the former.

[10]

+ self.assertRaises(
+ NotImplementedError, self.backend.has_perm,
+ *[self.admin, 'access', mac])

Why the *[...]? I think you can just do:

        self.assertRaises(
            NotImplementedError, self.backend.has_perm,
            self.admin, 'access...

> Looks good.
>
> Let's see if I'm getting this right:
>
> * AccessMiddleware is almost an optimization; it just provides a
> cheap, coarse policy.

Well, it's a way to force authentication across the entire web site. This way we won't forget to 'protect' a view or an api call we we had new code. We still can mess up permissions but it will be difficult to expose a view to anon users by mistake.

> * MaaSAuthorizationBackend is more like zope.security in that it
> provides finer control over who can see what, although it's not yet
> fully plumbed in.

Exactly.

> [1]
>
> utilities/format-new-and-modified-imports -r submit:

Done.

> [2]
>
> + self.public_urls = tuple(
> + re.compile(url) for url in (
> + reverse('login'), reverse('logout'), reverse('api-doc')))
>
> You could compose this into one so you don't have to iterate over it
> later in process_request():
>
> self.public_url = re.compile(
> "|".join(
> (reverse('login'),
> reverse('logout'),
> reverse('api-doc'))))

Cool.

>
>
> [3]
>
> + elif self.static_url.match(request.path):
> + return None
>
> Ah, is this how it says "not me"?

Exactly (https://docs.djangoproject.com/en/dev/topics/http/middleware/).

> [4]
>
> + return HttpResponseRedirect("%s?next=%s" % (
> + self.login_url, request.path))
>
> There might need to be some escaping in there.
>
> from urllib import quote_plus
> return HttpResponseRedirect("%s?next=%s" % (
> self.login_url, quote_plus(request.path)))

Good point I've used https://docs.djangoproject.com/en/dev/ref/utils/#django.utils.http.urlquote_plus.

> [5]
>
> + status = models.CharField(
> + max_length=10, choices=NODE_STATUS_CHOICES, editable=False,
> + default=DEFAULT_STATUS)
>
> This ought to be an enum, stored as a INT, like in Launchpad.
> PostgreSQL does support ENUM fields, but they make upgrading more
> complicated. I don't know what the Django way of doing this is.

Django doesn't want to know about this… and python does not provide an enum type so I've changed the status to an IntegerField and did that:

class NODE_STATUS:
    DEFAULT_STATUS = 0
    NEW = 0
    READY = 1
    DEPLOYED = 2
    COMMISSIONED = 3
    DECOMMISSIONED = 4

NODE_STATUS_CHOICES = (
    (NODE_STATUS.NEW, u'New'),
    (NODE_STATUS.READY, u'Ready to Commission'),
    (NODE_STATUS.DEPLOYED, u'Deployed'),
    (NODE_STATUS.COMMISSIONED, u'Commissioned'),
    (NODE_STATUS.DECOMMISSIONED, u'Decommissioned'),
)

> [6]
>
> +class MaaSAuthorizationBackend(object):
>
> This looks a bit like zope.security.

Indeed.

> [7]
>
> + def test_anon_nodes_GET(self):
> + response = self.client.get('/api/nodes/')
> + self.assertEqual(401, response.status_code)
>
> Comment/docstring missing.
>
> Most of our tests don't explain themselves :-/

I'm going to do it because it's you but I think this is rather self-explanatory.

> Consider using httplib.UNAUTHORIZED instead of 401, or add a trailing
> comment explaining what 401 means.

Done...

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

> > [4]
...
>
> Good point I've used https://docs.djangoproject.com/en/dev/ref/utils/#django.u
> tils.http.urlquote_plus.

That's nice. I'll try and remember that one.

> > [5]
> >
> > + status = models.CharField(
> > + max_length=10, choices=NODE_STATUS_CHOICES, editable=False,
> > + default=DEFAULT_STATUS)
> >
> > This ought to be an enum, stored as a INT, like in Launchpad.
> > PostgreSQL does support ENUM fields, but they make upgrading more
> > complicated. I don't know what the Django way of doing this is.
>
> Django doesn't want to know about this… and python does not provide an enum
> type so I've changed the status to an IntegerField and did that:
>
> class NODE_STATUS:
> DEFAULT_STATUS = 0
> NEW = 0
> READY = 1
> DEPLOYED = 2
> COMMISSIONED = 3
> DECOMMISSIONED = 4
>
>
> NODE_STATUS_CHOICES = (
> (NODE_STATUS.NEW, u'New'),
> (NODE_STATUS.READY, u'Ready to Commission'),
> (NODE_STATUS.DEPLOYED, u'Deployed'),
> (NODE_STATUS.COMMISSIONED, u'Commissioned'),
> (NODE_STATUS.DECOMMISSIONED, u'Decommissioned'),
> )

Consider using flufl.enum (in a later branch perhaps). It's one of
Barry Warsaw's libraries. I've never used it but I doubt it's on
crack.

http://packages.python.org/flufl.enum/

> > [7]
> >
> > + def test_anon_nodes_GET(self):
> > + response = self.client.get('/api/nodes/')
> > + self.assertEqual(401, response.status_code)
> >
> > Comment/docstring missing.
> >
> > Most of our tests don't explain themselves :-/
>
> I'm going to do it because it's you but I think this is rather self-
> explanatory.

I agree, but I spent a long time in the Landscape test suite the year
before last. They have tons of small test methods without explanations
and it's actually quite hard going to move around in there. I also
think it's good to write a sentence about what this test is meant to
be doing; I think it helps avoid testing the wrong thing or testing
too many things.

> > [8]
> >
> > + self.user = factory.make_user(username='user', password='test')
> > + self.client = Client()
> > + self.client.login(username='user', password='test')
> >
> > This appears twice. I imagine it's going to appear many more
> > times. Consider extracting this to a get_authenticated_client()
> > function or something like that, and chucking it in the testing
> > package somewhere.
>
> This is really so simple that I'm reluctant to add yet another utility
> function… I've refactored the code avoid the duplication.

I kind of agree, but...

A line of code that reads "client = get_authenticated_client()"
quickly conveys its intent. Also, someone else might want to
parameterize the function to choose a different user name or password,
and what started out as only three lines becomes much more, but
remains short in the test.