Merge lp:~pfalcon/linaro-license-protection/crowd-auth into lp:~linaro-automation/linaro-license-protection/trunk

[Paul Sokolovsky]

These are changes to switch l-l-p to Crowd API for group access authorization. Expectably, I didn't tear off old "OpenID teams" code, but rather went for adding new Crowd authz code, and allow app to be configured for which of them to use, allowing seamless transition and reverting if needed.

Well, to support switching to another authz method at all, and such "pluggable" architecture in particular, it of course first took some refactoring on existing codebase.

Actually, I submit this MRs with just refactoring existing setup, which doesn't change current authz method. That's to get early comments, while I'm working on Crowd authz "plugin" specifically.

Suggested review mode is to look at the commit individually.

UPDATE 2013-06-03: Finalized patchset re-submitted.

[Milo Casagrande (milo)]

Paul,

thanks for kick-starting this work!
Overall the work looks great, and the refactoring too.

+class ViewHelpersTests(BaseServeViewTest):
+ def test_auth_group_error(self):
+ groups = ["linaro", "batman", "catwoman", "joker"]
+ request = Mock()
+ request.path = "mock_path"
+ response = views.group_auth_failed_response(request, groups)
+ self.assertIsNotNone(response)
+ self.assertTrue(isinstance(response, HttpResponse))
+ self.assertContains(response,
+ "You need to be the member of one of the linaro batman, catwoman "
+ "or joker teams in Launchpad.",
+ status_code=403)

Not something I'm really against, but I would be more interested in testing only
the status code I get, not the kind of "string" I will receive (to me that looks
already more than an unit test). I would test something like that:

self.failUnlessEqual(response.status_code, 403)

(not sure that that is working code though)

+def group_auth_failed_response(request, auth_groups):
+ """Construct a nice response detailing list of auth groups that
+ will allow access to the requested file."""
+ if len(auth_groups) > 1:
+ groups_string = "one of the " + auth_groups.pop(0) + " "
+ if len(auth_groups) > 1:
+ groups_string += ", ".join(auth_groups[0:-1])
+
+ groups_string += " or " + auth_groups[-1] + " teams"
+ else:
+ groups_string = "the " + auth_groups[0] + " team"

I personally try to avoid concatenating strings in that way. Why not something like:

if len(auth_group) > 1:
    groups_string = "one of the %s" % auth_groups.pop(0)
    if len(auth_groups) > 1:
        groups_string += (" %s" % ", ".join(auth_groups[0:-1])).rstrip()
    groups_string += " or %s teams" % auth_groups[-1]
else:
    groups_string = "the %s team" % auth_groups[0]

I prefer it since it is clearer what you are trying to achieve and you can easily spot spurious white spaces.

I'm approving since what I listed are not really blockers, and functionalities are OK, but if you feel to apply them, do while merging.

Another thing I noticed, but not related to this code review: we are missing a license for all the code in linaro-license-protection, and all license header too. We should probably apply one at one point.

review: Approve

[Georgy Redkozubov (gesha)]

Looks good.
+1 to Milo's comment on tests, afaics, you've missed a ',' in comparison string on line 202. Should be
202 + "You need to be the member of one of the linaro, batman, catwoman "

Otherwise approved.

review: Approve

[Paul Sokolovsky (pfalcon)]

> Not something I'm really against, but I would be more interested in testing only
the status code I get, not the kind of "string" I will receive (to me that looks
already more than an unit test). I would test something like that:

Well, if looking in more detail, what I did is factored out that string construction out of OpenIdAuth class. The was also test_openid_auth which exactly did that string test. Consequently, I moved that string test where its new routine belongs. I didn't add anything, I don't feel comfortable removing anything either - my idea was to preserve original semantics and tests which cover it.

> I personally try to avoid concatenating strings in that way.

Again, routine wasn't written by me ;-)

[Paul Sokolovsky (pfalcon)]

> afaics, you've missed a ',' in comparison string on line

Ok, let me fire up bzr blame. Well, Englishman can't be wrong with English usage - it was done by James ;-).

And of course, I'm in principle all for fixing side issues, but: 1) They may need more consideration (in this case, both test "simplification" and grammar), and 2) They should be done separately, to not skew topic branch context.

Re: 2), if we used git, I could just apply quick fix to main branch, and rebase mine, but with bzr, it just better wait in queue (to not introduce twisted merges).

[Milo Casagrande (milo)]

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

[Georgy Redkozubov (gesha)]

Paul, thanks for working on this and a good job.

Here are my 5cents:

One thought about versions, I think it makes sense to add warning that user is using "old" version of BUILD-INFO.txt with OpenID-Launchpad-Teams which will be deprecated sooner or later.

Another is about tests. May be it is worth renaming test 'test_apply_to_dir(self)' to something like 'test_apply_to_dir_openid_launchpad_teams_field(self)' to better reflect what it does after you've added backwards compatibility with renaming of fields.

And the last one is in license_protected_downloads/buildinfo.py
There is a function that does write BUILD-INFO.txt from an array, it's used in splicing, it writes out Format-Version: 0.1, I guess we need to update it to write latest version (currently = 0.5) since it will be updated during read.

    def write_from_array(cls, build_info_array, file_path):
        with open(file_path, "w") as outfile:
            outfile.write("Format-Version: 0.1\n\n")

[Paul Sokolovsky (pfalcon)]

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

Fixed.

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

Yep, I paused at that too when wrote that, but somehow did it like that (to emphasize that members are of group, not of names?). Let it be just "List of groups".

> Can you please split this on two lines?

Done.

> I know there is nothing wrong with the for loop, but just a suggestion to avoid it:
> return user_groups.isdisjoint(set(required_groups))

Gotta remember (those who will later look at the code too) that Python now has pretty complete set implementation! Another problem is readability and understandability. In particular, in this case, it should be "return not". All in all, it now requires a comment where previously it didn't. Well, ok, let's go for it.

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

Makes sense.

[lp:~pfalcon/linaro-license-protection/crowd-auth]

206. By Paul Sokolovsky on 2013-06-03

Typos/grammar.

207. By Paul Sokolovsky on 2013-06-03

Improve docstring for _set().

208. By Paul Sokolovsky on 2013-06-03

We use Auth-Groups directive in this file, so it's format 0.5 .

209. By Paul Sokolovsky on 2013-06-03

Applied isdisjoint() "optimization", with credit to Milo.

[Paul Sokolovsky (pfalcon)]

> I think it makes sense to add warning that user is using "old" version of BUILD-INFO.txt with OpenID-Launchpad-Teams which will be deprecated sooner or later.

Yes, makes sense, I added warning this may happen in the future to README. But when it should happen? It should happen during publishing stage, not in webapp. Or do you hint about updating scripts/publish_to_snapshots.py? Well, that's not directly related with the aim of this patch, which is "adding Crowd API support to l-l-p webapp", and would be just scope creep even further. So, let's leave this for separate patch.

> Another is about tests. May be it is worth renaming test 'test_apply_to_dir(self)' to something like 'test_apply_to_dir_openid_launchpad_teams_field(self)' to better reflect what it does after you've added backwards compatibility with renaming of fields.

Well, that test actually tests how BUILD-INFO.txt applies to dirs, nothing else. It's just that was the only test in testsuite which deals with group field, so I selected it as a template for Auth-Groups field test. It's not ideal, but the whole testsuite is not ideal (well, I'm not sure it's the aim - it just should provide test coverage). Improving that should be a separate effort again.

> There is a function that does write BUILD-INFO.txt from an array, it's used in splicing, it writes out Format-Version: 0.1

Fixed, thanks.

[lp:~pfalcon/linaro-license-protection/crowd-auth]

210. By Paul Sokolovsky on 2013-06-03

write_from_array(): Write 0.5 format files.

[Milo Casagrande (milo)]

Thanks Paul! Looks good to me.

review: Approve

[lp:~pfalcon/linaro-license-protection/crowd-auth]

211. By Paul Sokolovsky on 2013-06-04

Handle errors during group auth properly.

212. By Paul Sokolovsky on 2013-06-04

First step toward normal logging setup.

213. By Paul Sokolovsky on 2013-06-04

Fix exception logging.

214. By Paul Sokolovsky on 2013-06-04

More test fixes for 0.5 format version.

215. By Paul Sokolovsky on 2013-06-04

Pyflakes fixes.

=== modified file 'README'
--- README	2013-02-15 09:45:08 +0000
+++ README	2013-06-04 18:22:27 +0000
@@ -16,7 +16,9 @@
  * makes use of the regular directory structure on disk
  * click through licensing with each file potentially having a different
    license/theme
- * openid restrictions (by Launchpad teams)
+ * group-based access authorization restrictions (using group information
+   from external services, currently OpenID with team extensions
+   (as used by Launchpad.net) and Atlassian Crowd are supported).
  * post-processing of all uploads (a script that manages uploads)
  * per-IP pass-through (for automatic services like a test framework)
 
@@ -46,12 +48,12 @@
 syntax).
 
 BUILD-INFO.txt file will allow one to specify the access restrictions
-(such as click-through licensing and OpenID-restrictions) and influence
+(such as click-through licensing and required groups) and influence
 the display (such as license-theme) of a particular build.
 
-WARNING: if you want a build type to be protected by OpenID, you need to
-ensure that the appropriate team is added to the list of django groups
-in the database.  Only django admins can do that.
+WARNING: if you want a build to be protected by OpenID-based groups, you
+need to ensure that the appropriate team is added to the list of django
+groups in the database. Only django admins can do that.
 
 Next step is to ensure that
 
@@ -108,15 +110,15 @@
     a placeholder and will be ignored. To be added later by build services?
  * License-Type: (required)
     open - Open builds. No license page is displayed.
-    protected - EULA protected builds. If 'OpenID-Launchpad-Teams' is defined
-      then OpenID protection is used, otherwise simple Accept/Decline license
+    protected - EULA protected builds. If 'Auth-Groups' is defined
+      then group authorization is used, otherwise simple Accept/Decline license
       page is displayed before accessing protected files.
  * Theme: (required only if License-Type is 'protected')
     Acceptable values are: stericsson, samsung.
     Theme name for selecting proper theming on download and license pages.
- * OpenID-Launchpad-Teams: (optional)
-    LP team names, members of which are allowed to access protected files. No
-    OpenID protection if absent.
+ * Auth-Groups: (optional)
+    List of groups, members of which are allowed to access protected files.
+    No group-based protection if absent.
  * Collect-User-Data: (optional)
     Acceptable values are: yes, no.
     Defaults to 'no' if not present. If the field is set to 'yes' then
@@ -131,10 +133,20 @@
 - Field names are case insensitive.
 - Fields order doesn't matter, except 'Files-Pattern'
 
+History of BUILD-INFO.txt format changes
+........................................
+
+Changes from format version 0.1 to 0.5:
+
+1. Field "OpenID-Launchpad-Teams" is deprecated and renamed to
+"Auth-Groups". Old name is supported for archived builds, however
+it may lead to warnings, and at later time to errors, during
+publishing new builds, so client usage should be upgraded.
+
 BUILD-INFO.txt example:
 .......................
 
-Format-Version: 0.1
+Format-Version: 0.5
 
 Files-Pattern: *.img, *.tar.bz2
 Build-Name: landing-snowball

=== modified file 'license_protected_downloads/buildinfo.py'
--- license_protected_downloads/buildinfo.py	2013-05-29 13:41:12 +0000
+++ license_protected_downloads/buildinfo.py	2013-06-04 18:22:27 +0000
@@ -14,8 +14,11 @@
         self.file_info_array = [{}]
         self.fields_defined = [
                 "format-version", "files-pattern", "build-name", "theme",
-                "license-type", "openid-launchpad-teams", "collect-user-data",
-                "license-text"]
+                "license-type", "auth-groups", "collect-user-data",
+                "license-text",
+                # Deprecated
+                "openid-launchpad-teams",
+                ]
         self.full_file_name = fn
         self.search_path = self.get_search_path(fn)
         self.fname = os.path.basename(fn)
@@ -40,6 +43,9 @@
                 os.path.join(cls.get_search_path(path), "BUILD-INFO.txt"))
 
     def _set(self, key, value):
+        """Record set of directives applying to a file pattern
+        key: file pattern
+        value: list of dicts of field/val pairs"""
         if key in self.build_info_array[self.index]:
             # A repeated key indicates we have found another chunk of
             # build-info
@@ -91,10 +97,7 @@
         if index > self.max_index:
             return False
         block = self.file_info_array[index]
-        for key in block:
-            if field == key:
-                return block[field]
-        return False
+        return block.get(field, False)
 
     def parseLine(self, line):
         values = line.split(":", 1)
@@ -108,6 +111,9 @@
                 raise IncorrectDataFormatException(
                         "Field '%s' not allowed." % field)
             else:
+                # Rename any deprecated field names to new names
+                field_renames = {"openid-launchpad-teams": "auth-groups"}
+                field = field_renames.get(field, field)
                 return {field: value}
 
     def isValidField(self, field_name):
@@ -189,7 +195,7 @@
     @classmethod
     def write_from_array(cls, build_info_array, file_path):
         with open(file_path, "w") as outfile:
-            outfile.write("Format-Version: 0.1\n\n")
+            outfile.write("Format-Version: 0.5\n\n")
             for key in build_info_array[0]:
                 if key != "format-version":
                     outfile.write("Files-Pattern: %s\n" % key)

=== added file 'license_protected_downloads/group_auth_common.py'
--- license_protected_downloads/group_auth_common.py	1970-01-01 00:00:00 +0000
+++ license_protected_downloads/group_auth_common.py	2013-06-04 18:22:27 +0000
@@ -0,0 +1,3 @@
+class GroupAuthError(Exception):
+    "Unexpected (infastructure) error during group authorization check."
+    pass

=== 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-04 18:22:27 +0000
@@ -0,0 +1,48 @@
+import logging
+
+from django.conf import settings
+from django.shortcuts import redirect
+import requests
+
+from group_auth_common import GroupAuthError
+
+
+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()
+
+
+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"]])
+
+    # If groups don't intersect, access denied
+    return not user_groups.isdisjoint(required_groups)

=== renamed file 'license_protected_downloads/openid_auth.py' => 'license_protected_downloads/group_auth_openid.py'
--- license_protected_downloads/openid_auth.py	2012-08-23 13:37:29 +0000
+++ license_protected_downloads/group_auth_openid.py	2013-06-04 18:22:27 +0000
@@ -1,16 +1,21 @@
+import logging
+
 from django.conf import settings
-from django.shortcuts import redirect, render_to_response
+from django.shortcuts import redirect
 from django.contrib.auth.models import Group
-import bzr_version
-
-
-class OpenIDAuth:
-
-    @classmethod
-    def process_openid_auth(cls, request, openid_teams):
+
+
+log = logging.getLogger(__file__)
+
+
+def process_group_auth(request, openid_teams):
+        """Returns True if access granted, False if denied and Response
+        object if not enough authentication information available and
+        user should authenticate first (by following that Response).
+        """
 
         if not openid_teams:
-            return None
+            return True
 
         for openid_team in openid_teams:
             Group.objects.get_or_create(name=openid_team)
@@ -19,28 +24,10 @@
             # Force OpenID login
             return redirect(settings.LOGIN_URL + "?next=" + request.path)
 
+        log.warn("Authenticating using Launchpad OpenID Teams: %s",
+                 request.user.username)
         for group in request.user.groups.all():
             if group.name in openid_teams:
-                return None
-
-        # Construct a nice string of openid teams that will allow access to
-        # the requested file
-        if len(openid_teams) > 1:
-            teams_string = "one of the " + openid_teams.pop(0) + " "
-            if len(openid_teams) > 1:
-                teams_string += ", ".join(openid_teams[0:-1])
-
-            teams_string += " or " + openid_teams[-1] + " teams"
-        else:
-            teams_string = "the " + openid_teams[0] + " team"
-
-        response = render_to_response(
-            'openid_forbidden_template.html',
-            {'login': settings.LOGIN_URL + "?next=" + request.path,
-             'authenticated': request.user.is_authenticated(),
-             'openid_teams': teams_string,
-             'revno': bzr_version.get_my_bzr_revno(),
-             })
-
-        response.status_code = 403
-        return response
+                return True
+
+        return False

=== modified file 'license_protected_downloads/tests/BUILD-INFO.txt'
--- license_protected_downloads/tests/BUILD-INFO.txt	2012-06-26 18:50:23 +0000
+++ license_protected_downloads/tests/BUILD-INFO.txt	2013-06-04 18:22:27 +0000
@@ -1,4 +1,4 @@
-Format-Version: 0.1
+Format-Version: 0.5
 
 Files-Pattern: *.txt
 Build-Name: landing-protected

=== modified file 'license_protected_downloads/tests/__init__.py'
--- license_protected_downloads/tests/__init__.py	2013-04-19 12:13:03 +0000
+++ license_protected_downloads/tests/__init__.py	2013-06-04 18:22:27 +0000
@@ -11,6 +11,7 @@
     FileViewTests,
     HowtoViewTests,
     ViewTests,
+    ViewHelpersTests,
     )
 from license_protected_downloads.tests.test_openid_auth import TestOpenIDAuth
 from license_protected_downloads.tests.test_custom_commands \
@@ -32,4 +33,5 @@
     'TestPep8': TestPep8,
     'TestPyflakes': TestPyflakes,
     'ViewTests': ViewTests,
+    'ViewHelpersTests': ViewHelpersTests,
 }

=== modified file 'license_protected_downloads/tests/test_buildinfo.py'
--- license_protected_downloads/tests/test_buildinfo.py	2013-04-19 12:13:03 +0000
+++ license_protected_downloads/tests/test_buildinfo.py	2013-06-04 18:22:27 +0000
@@ -27,7 +27,16 @@
         self.assertEquals(build_info.getInfoForFile(),
             [{'build-name': 'landing-protected',
               'license-type': 'protected',
-              'openid-launchpad-teams': 'linaro'}])
+              'auth-groups': 'linaro'}])
+
+    def test_apply_to_dir_auth_groups_field(self):
+        dir_path = THIS_DIRECTORY + \
+            '/testserver_root/build-info/subdir2'
+        build_info = BuildInfo(dir_path)
+        self.assertEquals(build_info.getInfoForFile(),
+            [{'build-name': 'landing-protected',
+              'license-type': 'protected',
+              'auth-groups': 'linaro'}])
 
     def test_apply_to_nonexistent_file(self):
         with self.assertRaises(IOError):
@@ -41,14 +50,14 @@
     def test_getFormatVersion(self):
         build_info = BuildInfo(self.buildinfo_file_path)
 
-        self.assertEqual("0.1", build_info.getFormatVersion())
+        self.assertEqual("0.5", build_info.getFormatVersion())
 
     def test_get_emptyField(self):
         value = "notempty"
         build_info = BuildInfo(self.buildinfo_file_path)
         for pair in build_info.file_info_array:
-            if "openid-launchpad-teams" in pair:
-                value = pair["openid-launchpad-teams"]
+            if "auth-groups" in pair:
+                value = pair["auth-groups"]
 
         self.assertFalse(value)
 

=== modified file 'license_protected_downloads/tests/test_openid_auth.py'
--- license_protected_downloads/tests/test_openid_auth.py	2012-08-23 13:37:29 +0000
+++ license_protected_downloads/tests/test_openid_auth.py	2013-06-04 18:22:27 +0000
@@ -2,7 +2,7 @@
 from django.http import HttpResponse
 from mock import Mock, patch
 
-from license_protected_downloads.openid_auth import OpenIDAuth
+from license_protected_downloads import group_auth_openid as openid_auth
 
 
 class TestOpenIDAuth(TestCase):
@@ -27,14 +27,14 @@
     def test_check_team_membership_no_teams(self):
         mock_request = self.make_mock_request()
         openid_teams = []
-        self.assertIsNone(
-            OpenIDAuth.process_openid_auth(mock_request, openid_teams))
+        self.assertTrue(
+            openid_auth.process_group_auth(mock_request, openid_teams))
 
     def test_check_team_membership_no_authentication(self):
         mock_request = self.make_mock_request()
         mock_request.user.is_authenticated.return_value = False
         openid_teams = ["linaro"]
-        response = OpenIDAuth.process_openid_auth(mock_request, openid_teams)
+        response = openid_auth.process_group_auth(mock_request, openid_teams)
         self.assertIsNotNone(response)
         self.assertTrue(isinstance(response, HttpResponse))
         self.assertEquals(302, response.status_code)
@@ -45,8 +45,8 @@
         mock_request.user.groups.all.return_value = [
             self.make_mock_group("linaro")]
         openid_teams = ["linaro"]
-        response = OpenIDAuth.process_openid_auth(mock_request, openid_teams)
-        self.assertIsNone(response)
+        response = openid_auth.process_group_auth(mock_request, openid_teams)
+        self.assertTrue(response)
 
     def test_check_no_team_membership_authed(self):
         mock_request = self.make_mock_request()
@@ -54,12 +54,8 @@
         mock_request.user.groups.all.return_value = [
             self.make_mock_group("another-group")]
         openid_teams = ["linaro"]
-        response = OpenIDAuth.process_openid_auth(mock_request, openid_teams)
-        self.assertIsNotNone(response)
-        self.assertTrue(isinstance(response, HttpResponse))
-        self.assertContains(response,
-            "You need to be the member of the linaro team in Launchpad.",
-            status_code=403)
+        response = openid_auth.process_group_auth(mock_request, openid_teams)
+        self.assertFalse(response)
 
     def test_check_no_team_membership_authed_multi_teams(self):
         mock_request = self.make_mock_request()
@@ -67,13 +63,8 @@
         mock_request.user.groups.all.return_value = [
             self.make_mock_group("another-group")]
         openid_teams = ["linaro", "batman", "catwoman", "joker"]
-        response = OpenIDAuth.process_openid_auth(mock_request, openid_teams)
-        self.assertIsNotNone(response)
-        self.assertTrue(isinstance(response, HttpResponse))
-        self.assertContains(response,
-            "You need to be the member of one of the linaro batman, catwoman "
-            "or joker teams in Launchpad.",
-            status_code=403)
+        response = openid_auth.process_group_auth(mock_request, openid_teams)
+        self.assertFalse(response)
 
     @patch("django.contrib.auth.models.Group.objects.get_or_create")
     def test_auto_adding_groups(self, get_or_create_mock):
@@ -83,7 +74,7 @@
             self.make_mock_group("another-group")]
 
         openid_teams = ["linaro", "linaro-infrastructure"]
-        OpenIDAuth.process_openid_auth(mock_request, openid_teams)
+        openid_auth.process_group_auth(mock_request, openid_teams)
 
         expected = [
             ((), {'name': 'linaro'}), ((), {'name': 'linaro-infrastructure'})]

=== modified file 'license_protected_downloads/tests/test_splicebuildinfos.py'
--- license_protected_downloads/tests/test_splicebuildinfos.py	2013-04-19 12:13:03 +0000
+++ license_protected_downloads/tests/test_splicebuildinfos.py	2013-06-04 18:22:27 +0000
@@ -20,16 +20,16 @@
             'test-protected.txt':
             [{'license-type': 'protected',
               'build-name': 'landing-protected',
-              'openid-launchpad-teams': 'linaro'}],
+              'auth-groups': 'linaro'}],
             'test-protected-2.txt':
             [{'license-type': 'protected',
               'build-name': 'landing-protected',
-              'openid-launchpad-teams': 'linaro'}]}
+              'auth-groups': 'linaro'}]}
 
         result = {'test-protected.txt, test-protected-2.txt':
                   [{'license-type': 'protected',
                     'build-name': 'landing-protected',
-                    'openid-launchpad-teams': 'linaro'}]}
+                    'auth-groups': 'linaro'}]}
 
         build_info_res = SpliceBuildInfos.merge_duplicates(build_info_dict)
         self.assertEquals(build_info_res, result)

=== modified file 'license_protected_downloads/tests/test_views.py'
--- license_protected_downloads/tests/test_views.py	2013-04-02 09:58:42 +0000
+++ license_protected_downloads/tests/test_views.py	2013-06-04 18:22:27 +0000
@@ -2,6 +2,7 @@
 
 from django.conf import settings
 from django.test import Client, TestCase
+from django.http import HttpResponse
 import hashlib
 import os
 import tempfile
@@ -10,6 +11,8 @@
 import urlparse
 import json
 
+from mock import Mock
+
 from license_protected_downloads import bzr_version
 from license_protected_downloads.buildinfo import BuildInfo
 from license_protected_downloads.config import INTERNAL_HOSTS
@@ -19,6 +22,7 @@
 from license_protected_downloads.views import _process_include_tags
 from license_protected_downloads.views import _sizeof_fmt
 from license_protected_downloads.views import is_same_parent_dir
+from license_protected_downloads import views
 
 THIS_DIRECTORY = os.path.dirname(os.path.abspath(__file__))
 TESTSERVER_ROOT = os.path.join(THIS_DIRECTORY, "testserver_root")
@@ -903,5 +907,19 @@
             self.assertEqual(response.status_code, 200)
 
 
+class ViewHelpersTests(BaseServeViewTest):
+    def test_auth_group_error(self):
+        groups = ["linaro", "batman", "catwoman", "joker"]
+        request = Mock()
+        request.path = "mock_path"
+        response = views.group_auth_failed_response(request, groups)
+        self.assertIsNotNone(response)
+        self.assertTrue(isinstance(response, HttpResponse))
+        self.assertContains(response,
+            "You need to be the member of one of the linaro batman, catwoman "
+            "or joker teams in Launchpad.",
+            status_code=403)
+
+
 if __name__ == '__main__':
     unittest.main()

=== 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-04 18:22:27 +0000
@@ -0,0 +1,7 @@
+Format-Version: 0.5
+
+
+Files-Pattern: *
+Build-Name: landing-protected
+License-Type: protected
+Auth-Groups: linaro

=== added file 'license_protected_downloads/tests/testserver_root/build-info/subdir2/testfile.txt'
=== removed file 'license_protected_downloads/tests/testserver_root/build-info/write-test/BUILD-INFO.txt'
--- license_protected_downloads/tests/testserver_root/build-info/write-test/BUILD-INFO.txt	2013-04-19 12:13:03 +0000
+++ license_protected_downloads/tests/testserver_root/build-info/write-test/BUILD-INFO.txt	1970-01-01 00:00:00 +0000
@@ -1,42 +0,0 @@
-Format-Version: 0.1
-
-Files-Pattern: *openid*
-license-type: protected
-build-name: landing-protected
-openid-launchpad-teams: linaro
-
-Files-Pattern: *panda*
-license-type: open
-build-name: landing-panda
-
-Files-Pattern: *.txt
-license-type: protected
-build-name: landing-protected
-openid-launchpad-teams: 
-theme: stericsson
-collect-user-data: yes
-license-text: <p>IMPORTANT — PLEASE READ THE FOLLOWING AGREEMENT CAREFULLY.</p>
- <p>
-   THIS IS A LEGALLY BINDING AGREEMENT
- </p>
-
-Files-Pattern: *origen*
-theme: samsung
-license-type: protected
-build-name: landing-origen
-license-text: <p>IMPORTANT — PLEASE READ THE FOLLOWING AGREEMENT CAREFULLY.</p>
- <p>
-  THIS IS A LEGALLY BINDING AGREEMENT BETWEEN YOU, an individual or a
-  legal entity, (“LICENSEE”) AND HAL 1000.
- </p>
-
-Files-Pattern: *snowball*
-theme: stericsson
-license-type: protected
-build-name: landing-snowball
-license-text: <p>IMPORTANT — PLEASE READ THE FOLLOWING AGREEMENT CAREFULLY.</p>
- <p>
-  THIS IS A LEGALLY BINDING AGREEMENT BETWEEN YOU, an individual or a
-  legal entity, (“LICENSEE”) AND ME, THE TEST SERVER.
- </p>
-

=== modified file 'license_protected_downloads/views.py'
--- license_protected_downloads/views.py	2013-05-09 11:15:33 +0000
+++ license_protected_downloads/views.py	2013-06-04 18:22:27 +0000
@@ -1,3 +1,4 @@
+import logging
 import glob
 import hashlib
 import json
@@ -23,9 +24,12 @@
 from buildinfo import BuildInfo, IncorrectDataFormatException
 from render_text_files import RenderTextFiles
 from models import License
-from openid_auth import OpenIDAuth
+# Load group auth "plugin" dynamically
+import importlib
+group_auth = importlib.import_module(settings.GROUP_AUTH_MODULE)
 from BeautifulSoup import BeautifulSoup
 import config
+from group_auth_common import GroupAuthError
 
 
 LINARO_INCLUDE_FILE_RE = re.compile(
@@ -33,6 +37,8 @@
 LINARO_INCLUDE_FILE_RE1 = re.compile(
     r'<linaro:include file="(?P<file_name>.*)">(.*)</linaro:include>')
 
+log = logging.getLogger(__file__)
+
 
 def _hidden_file(file_name):
     hidden_files = ["BUILD-INFO.txt", "EULA.txt", r"^\.", "HEADER.html"]
@@ -242,7 +248,7 @@
         license_type = build_info.get("license-type")
         license_text = build_info.get("license-text")
         theme = build_info.get("theme")
-        openid_teams = build_info.get("openid-launchpad-teams")
+        auth_groups = build_info.get("auth-groups")
         max_index = build_info.max_index
     elif os.path.isfile(open_eula_path):
         return "OPEN"
@@ -256,7 +262,7 @@
         license_type = "protected"
         license_file = os.path.join(settings.PROJECT_ROOT,
                                     'templates/licenses/' + theme + '.txt')
-        openid_teams = False
+        auth_groups = False
         with open(license_file, "r") as infile:
             license_text = infile.read()
     elif _check_special_eula(path):
@@ -264,7 +270,7 @@
         license_type = "protected"
         license_file = os.path.join(settings.PROJECT_ROOT,
                                     'templates/licenses/' + theme + '.txt')
-        openid_teams = False
+        auth_groups = False
         with open(license_file, "r") as infile:
             license_text = infile.read()
     elif _check_special_eula(base_path + "/*"):
@@ -279,7 +285,7 @@
             return "OPEN"
 
         # File matches a license, isn't open.
-        if openid_teams:
+        if auth_groups:
             return "OPEN"
         elif license_text:
             for i in range(max_index):
@@ -392,6 +398,30 @@
     return response
 
 
+def group_auth_failed_response(request, auth_groups):
+    """Construct a nice response detailing list of auth groups that
+    will allow access to the requested file."""
+    if len(auth_groups) > 1:
+        groups_string = "one of the " + auth_groups.pop(0) + " "
+        if len(auth_groups) > 1:
+            groups_string += ", ".join(auth_groups[0:-1])
+
+        groups_string += " or " + auth_groups[-1] + " teams"
+    else:
+        groups_string = "the " + auth_groups[0] + " team"
+
+    response = render_to_response(
+        'openid_forbidden_template.html',
+        {'login': settings.LOGIN_URL + "?next=" + request.path,
+         'authenticated': request.user.is_authenticated(),
+         'openid_teams': groups_string,
+         'revno': bzr_version.get_my_bzr_revno(),
+         })
+
+    response.status_code = 403
+    return response
+
+
 def file_server(request, path):
     """Serve up a file / directory listing or license page as required"""
     path = iri_to_uri(path)
@@ -415,16 +445,25 @@
             return HttpResponseForbidden(
                 "Error parsing BUILD-INFO.txt")
 
-        launchpad_teams = build_info.get("openid-launchpad-teams")
-        if launchpad_teams:
-            launchpad_teams = launchpad_teams.split(",")
-            launchpad_teams = [team.strip() for team in launchpad_teams]
-            # TODO: use logging!
-            print "Checking membership in OpenID groups:", launchpad_teams
-            openid_response = OpenIDAuth.process_openid_auth(
-                request, launchpad_teams)
-            if openid_response:
-                return openid_response
+        auth_groups = build_info.get("auth-groups")
+        if auth_groups:
+            auth_groups = auth_groups.split(",")
+            auth_groups = [g.strip() for g in auth_groups]
+            log.info("Checking membership in auth groups: %s", auth_groups)
+            try:
+                response = group_auth.process_group_auth(request, auth_groups)
+            except GroupAuthError:
+                log.exception("GroupAuthError")
+                response = render_to_response('group_auth_failure.html')
+                response.status_code = 500
+                return response
+
+            if response == False:
+                return group_auth_failed_response(request, auth_groups)
+            elif response == True:
+                pass
+            else:
+                return response
 
     if target_type == "dir":
         # Generate a link to the parent directory (if one exists)

=== modified file 'settings.py'
--- settings.py	2013-02-26 19:38:30 +0000
+++ settings.py	2013-06-04 18:22:27 +0000
@@ -124,6 +124,10 @@
 LOGIN_URL = '/linaro-openid/login/'
 LOGIN_REDIRECT_URL = '/'
 
+# Name of "plugin" module to use for group authentication
+GROUP_AUTH_MODULE = 'license_protected_downloads.group_auth_openid'
+
+# Config for django_openid_auth.auth.OpenIDBackend
 OPENID_CREATE_USERS = True
 OPENID_SSO_SERVER_URL = 'https://login.launchpad.net/'
 OPENID_UPDATE_DETAILS_FROM_SREG = True
@@ -132,6 +136,11 @@
 OPENID_USE_AS_ADMIN_LOGIN = True
 OPENID_USE_EMAIL_FOR_USERNAME = True
 
+ATLASSIAN_CROWD_API_URL = \
+    "https://login.linaro.org:8443/crowd/rest/usermanagement/1"
+ATLASSIAN_CROWD_API_USERNAME = None
+ATLASSIAN_CROWD_API_PASSWORD = None
+
 # A sample logging configuration. The only tangible logging
 # performed by this configuration is to send an email to
 # the site admins on every HTTP 500 error.
@@ -144,7 +153,11 @@
         'mail_admins': {
             'level': 'ERROR',
             'class': 'django.utils.log.AdminEmailHandler'
-        }
+        },
+        'console': {
+            'level': 'DEBUG',
+            'class': 'logging.StreamHandler',
+        },
     },
     'loggers': {
         'django.request': {
@@ -152,6 +165,11 @@
             'level': 'ERROR',
             'propagate': True,
         },
+        # Root logger
+        '': {
+            'level': 'DEBUG',
+            'handlers': ['console'],
+        },
     }
 }
 

=== added file 'templates/group_auth_failure.html'
--- templates/group_auth_failure.html	1970-01-01 00:00:00 +0000
+++ templates/group_auth_failure.html	2013-06-04 18:22:27 +0000
@@ -0,0 +1,10 @@
+{% extends "header.html" %}
+
+{% block content %}
+  <h1>Group Authentication Error</h1>
+  <p>
+    Unexpected group authentication error happened. Error details were logged.
+    Please contact administrator for resolution.
+  </p>
+</table>
+{% endblock %}