Add support for permissions specific to Local Sites.

Review Request #4880 — Created Nov. 3, 2013 and submitted

Information

Review Board
release-1.7.x

Reviewers

Add support for permissions specific to Local Sites.

Local Sites had no equivalent of our custom permissions, like
can_change_status or can_submit_as_another_user. It also didn't grant
these abilities to administrators of Local Sites.

Now, a LocalSiteProfile defines a permissions dictionary, which contains
all granted permissions for the user. Local Site administrators
automatically have all permissions for a Local Site.

To do this, we make use of an AuthBackend's ability to do custom
permission lookups. The main function for this, has_perm, can take a
parameter for the object permissions are being looked up on.

This parameter is unused by defaut and in fact not allowed on any
built-in backends. Django doesn't make any real use of it. The intent
appears to be to pass an object like a ReviewRequest to it, but there's
no real rules governing this.

For our usage, passing a LocalSite as the object will have it look up
that permission in the user's list of permissions for the LocalSite. All
callers must then do when checking permissions is to pass the LocalSite
(or None) when calling has_perm.

The 'perms' context variable for templates has also been replaced with a
version that will pass the LocalSite for these requests.

Unit tests pass.

Manually tested closing a review request and editing one as a user with
those permissions, and as an administrator.

Description From Last Updated

The commas in these two lines are unnecessary.

daviddavid

I'm a little bit concerned that this isn't implemented for all the auth backends. Maybe we can put it in …

daviddavid

I think if not isinstance(obj, LocalSite), we should return the parent class' implementation (get_all_permissions(user)) rather than asserting.

daviddavid
david
  1. Typo in your change description: "defaut"

  2. reviewboard/accounts/backends.py (Diff revision 1)
     
     

    I'm a little bit concerned that this isn't implemented for all the auth backends. Maybe we can put it in a mixin?

    1. We're fortunately good here. Django's auth system will loop through all auth backend (and StandardAuthBackend will always be in that list), and if any of them have a has_perm method, it'll use that. It'll always call ours, since nothing else implements one.

    2. OK. Maybe comment that?

    3. Sure.

  3. reviewboard/accounts/backends.py (Diff revision 1)
     
     

    I think if not isinstance(obj, LocalSite), we should return the parent class' implementation (get_all_permissions(user)) rather than asserting.

    1. The parent has_perm, while it talks an object parameter, will hard-code an empty result if passed one. There's no warning, meaning it could be confusing if we pass the wrong thing, as we'll just see has_perm return False. The assert is there to better catch cases where we're passing in something we didn't intend to pass.

    2. I'm just worried that asserts will crash things. Can we log and fail closed?

    3. I think we want them to crash. We need to see that this is happening, because it's technically not valid to call the parent functions with an object. If they're ever called with an object other than a LocalSite, there's a programming error somewhere we need to fix.

    4. Very well.

  4. 
      
chipx86
david
  1. Just one trivial comment.

  2. reviewboard/accounts/backends.py (Diff revisions 1 - 2)
     
     
     

    The commas in these two lines are unnecessary.

  3. 
      
chipx86
Review request changed

Status: Closed (submitted)

Loading...