Code review comment for lp://qastaging/~rvb/maas/maas-security-model

> 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.

> [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.

> [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.

Right.

> [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', mac)

Okay.