Code review comment for lp:~pfalcon/linaro-license-protection/crowd-auth

Hello Paul!

Thanks for pulling this together!
Following are my findings!

On Mon, Jun 3, 2013 at 1:57 PM, Paul Sokolovsky
<email address hidden> wrote:
>
> === modified file 'README'
> --- README 2013-02-15 09:45:08 +0000
> +++ README 2013-06-03 11:56:29 +0000
> @@ -16,7 +16,9 @@
> - * openid restrictions (by Launchpad teams)
> + * group-based access authorization restrictions (using group information
> + from external services, currently OpenID with team extensions is
> + (as used by Launchpad.net) and Atlassian Crowd are supported).

I guess the "is" on the second changed line should be removed.

> + * Auth-Groups: (optional)
> + Names of groups, members of which are allowed to access protected files.

"Name of groups" should be fine, but better have a
real-English-speaking person review for this one :-)

> -Format-Version: 0.1
> +Format-Version: 0.5

> def _set(self, key, value):
> + "key: file pattern, value: list of dicts of field/val pairs"

Can you please split this on two lines?

> === added file 'license_protected_downloads/group_auth_crowd.py'
> --- license_protected_downloads/group_auth_crowd.py 1970-01-01 00:00:00 +0000
> +++ license_protected_downloads/group_auth_crowd.py 2013-06-03 11:56:29 +0000
> @@ -0,0 +1,52 @@
> +import logging
> +
> +from django.conf import settings
> +from django.shortcuts import redirect
> +
> +import requests
> +
> +
> +log = logging.getLogger(__file__)
> +
> +
> +def upgrade_requests():
> + """Ubuntu 12.04 comes with pretty old requests version. Add convenience
> + methods of newer versions straight to it, to avoid client-side
> + workarounds."""
> + if "json" not in dir(requests.models.Response):
> + def patchy_json(self):
> + import json
> + return json.loads(self.content)
> + requests.models.Response.json = patchy_json
> +
> +# We monkey-patch requests module on first load
> +upgrade_requests()
> +
> +
> +class GroupAuthError(Exception):
> + pass
> +
> +
> +def process_group_auth(request, required_groups):
> + if not required_groups:
> + return True
> + if not request.user.is_authenticated():
> + # Force OpenID login
> + return redirect(settings.LOGIN_URL + "?next=" + request.path)
> +
> + log.warn("Authenticating using Crowd API: %s",
> + request.user.username)
> +
> + auth = (settings.ATLASSIAN_CROWD_API_USERNAME,
> + settings.ATLASSIAN_CROWD_API_PASSWORD)
> + params = {"username": request.user.username}
> + r = requests.get(settings.ATLASSIAN_CROWD_API_URL
> + + "/user/group/nested.json", params=params, auth=auth)
> + if r.status_code != 200:
> + raise GroupAuthError(r.status_code)
> + data = r.json()
> + user_groups = set([x["name"] for x in data["groups"]])
> + for g in required_groups:
> + if g in user_groups:
> + return True
> + return False

I know there is nothing wrong with the for loop, but just a suggestion
to avoid it:

return user_groups.isdisjoint(set(required_groups))

Since you already have a set there in place.

> === added directory 'license_protected_downloads/tests/testserver_root/build-info/subdir2'
> === added file 'license_protected_downloads/tests/testserver_root/build-info/subdir2/BUILD-INFO.txt'
> --- license_protected_downloads/tests/testserver_root/build-info/subdir2/BUILD-INFO.txt 1970-01-01 00:00:00 +0000
> +++ license_protected_downloads/tests/testserver_root/build-info/subdir2/BUILD-INFO.txt 2013-06-03 11:56:29 +0000
> @@ -0,0 +1,7 @@
> +Format-Version: 0.1

Hmmm... shouldn't this be bumped to 0.5?

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs