Merge groupfinder() into effective_principals()

No longer necessary to have separate functions.

Also changed effective_principals() to just work with a list instead of
building up a set and then turning it into a list before returning. Since we
never actually add duplicate principals using a set achieves nothing except to
make testing awkward by making the order ot the items in the returned list
undefined.

h/auth.py

@@ -54,48 +54,37 @@ def auth_domain(request):
     return request.registry.settings.get('h.auth_domain', request.domain)
 
 
-def groupfinder(userid, request):
+def effective_principals(userid, request):
     """
-    Return the list of additional groups of which userid is a member.
+    Return the list of effective principals for the passed userid.
 
-    Returns a list of group principals of which the passed userid is a member,
-    or None if the userid is not known by this application.
+    Usually, we can leave the computation of the full set of effective
+    principals to the pyramid authentication policy. Sometimes, however, it can
+    be useful to discover the full set of effective principals for a userid
+    other than the current authenticated userid. This function replicates the
+    normal behaviour of a pyramid authentication policy and can be used for
+    that purpose.
     """
-    principals = set()
+    principals = [security.Everyone]
 
     user = accounts.get_user(userid, request)
+
     if user is None:
-        return
+        return principals
 
     if user.admin:
-        principals.add('group:__admin__')
-    if user.staff:
-        principals.add('group:__staff__')
-    principals.update(groups.group_principals(user))
-
-    return list(principals)
+        principals.append('group:__admin__')
 
+    if user.staff:
+        principals.append('group:__staff__')
 
-def effective_principals(userid, request, groupfinder=groupfinder):
-    """
-    Return the list of effective principals for the passed userid.
+    principals.extend(groups.group_principals(user))
 
-    Usually, we can leave the computation of the full set of effective
-    principals to the pyramid authentication policy. Sometimes, however, it can
-    be useful to discover the full set of effective principals for a userid
-    other than the current authenticated userid. This function replicates the
-    normal behaviour of a pyramid authentication policy and can be used for
-    that purpose.
-    """
-    principals = set([security.Everyone])
+    principals.append(security.Authenticated)
 
-    groups = groupfinder(userid, request)
-    if groups is not None:
-        principals.add(security.Authenticated)
-        principals.add(userid)
-        principals.update(groups_)
+    principals.append(userid)
 
-    return list(principals)
+    return principals
 
 
 def is_api_request(request):

h/test/auth_test.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 import pytest
 
-from mock import MagicMock, patch, Mock
+import mock
 from pyramid import testing
 from pyramid import security
 
@@ -12,156 +12,102 @@
 SECRET = 'somesecret'
 
 
-# The fixtures required to mock all of groupfinder()'s dependencies.
-groupfinder_fixtures = pytest.mark.usefixtures('accounts', 'groups')
+effective_principals_fixtures = pytest.mark.usefixtures('accounts', 'groups')
 
 
-@groupfinder_fixtures
-def test_groupfinder_returns_no_principals(accounts):
-    """It should return only [] by default.
+@effective_principals_fixtures
+def test_effective_principals_when_user_is_None(accounts):
+    accounts.get_user.return_value = None
 
-    If the request has no client and the user is not an admin or staff member
-    nor a member of any group, it should return no additional principals.
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
-    """
-    accounts.get_user.return_value = MagicMock(admin=False, staff=False)
+    assert principals == [security.Everyone]
 
-    assert auth.groupfinder("acct:jiji@hypothes.is", Mock()) == []
 
+@effective_principals_fixtures
+def test_effective_principals_when_user_authenticated(accounts, groups):
+    # User is authenticated but is not an admin or staff or a member of any
+    # groups.
+    accounts.get_user.return_value = mock.Mock(admin=False, staff=False)
+    groups.group_principals.return_value = []
 
-@groupfinder_fixtures
-def test_groupfinder_with_admin_user(accounts):
-    """If the user is an admin it should return "group:__admin__"."""
-    accounts.get_user.return_value = MagicMock(admin=True, staff=False)
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
-    assert "group:__admin__" in auth.groupfinder(
-        "acct:jiji@hypothes.is", Mock())
+    assert principals == [
+        security.Everyone, security.Authenticated, 'acct:jiji@hypothes.is']
 
 
-@groupfinder_fixtures
-def test_groupfinder_with_staff_user(accounts):
-    """If the user is staff it should return a "group:__staff__" principal."""
-    accounts.get_user.return_value = MagicMock(admin=False, staff=True)
+@effective_principals_fixtures
+def test_effective_principals_when_user_is_admin(accounts, groups):
+    accounts.get_user.return_value = mock.Mock(admin=True, staff=False)
+    groups.group_principals.return_value = []
 
-    assert "group:__staff__" in auth.groupfinder(
-        "acct:jiji@hypothes.is", Mock())
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
+    assert principals == [security.Everyone,
+                          'group:__admin__',
+                          security.Authenticated,
+                          'acct:jiji@hypothes.is']
 
-@groupfinder_fixtures
-def test_groupfinder_admin_and_staff(accounts):
-    accounts.get_user.return_value = MagicMock(admin=True, staff=True)
 
-    principals = auth.groupfinder("acct:jiji@hypothes.is", Mock())
+@effective_principals_fixtures
+def test_effective_principals_when_user_is_staff(accounts, groups):
+    accounts.get_user.return_value = mock.Mock(admin=False, staff=True)
+    groups.group_principals.return_value = []
 
-    assert "group:__admin__" in principals
-    assert "group:__staff__" in principals
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
+    assert principals == [security.Everyone,
+                          'group:__staff__',
+                          security.Authenticated,
+                          'acct:jiji@hypothes.is']
 
-@groupfinder_fixtures
-def test_groupfinder_calls_group_principals(accounts, groups):
-    auth.groupfinder("acct:jiji@hypothes.is", Mock())
 
-    groups.group_principals.assert_called_once_with(
-        accounts.get_user.return_value)
+@effective_principals_fixtures
+def test_effective_principals_when_user_has_groups(accounts, groups):
+    accounts.get_user.return_value = mock.Mock(admin=False, staff=False)
+    groups.group_principals.return_value = ['group:abc123', 'group:def456']
 
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
-@groupfinder_fixtures
-def test_groupfinder_with_one_group(groups):
-    groups.group_principals.return_value = ['group:group-1']
+    assert principals == [security.Everyone,
+                          'group:abc123',
+                          'group:def456',
+                          security.Authenticated,
+                          'acct:jiji@hypothes.is']
 
-    additional_principals = auth.groupfinder("acct:jiji@hypothes.is", Mock())
 
-    assert 'group:group-1' in additional_principals
+@effective_principals_fixtures
+def test_effective_principals_with_staff_admin_and_groups(accounts, groups):
+    accounts.get_user.return_value = mock.Mock(admin=True, staff=True)
+    groups.group_principals.return_value = ['group:abc123', 'group:def456']
 
+    principals = auth.effective_principals('acct:jiji@hypothes.is',
+                                           testing.DummyRequest())
 
-@groupfinder_fixtures
-def test_groupfinder_with_three_groups(groups):
-    groups.group_principals.return_value = [
-        'group:group-1',
-        'group:group-2',
-        'group:group-3'
-    ]
-
-    additional_principals = auth.groupfinder("acct:jiji@Hypothes.is", Mock())
-
-    assert 'group:group-1' in additional_principals
-    assert 'group:group-2' in additional_principals
-    assert 'group:group-3' in additional_principals
-
-
-def test_effective_principals_includes_everyone():
-    """
-    Even if the groupfinder returns None, implying that the userid is not
-    recognised, `security.Everyone` should be included in the list of effective
-    principals.
-    """
-    groupfinder = lambda userid, request: None
-    request = testing.DummyRequest()
-
-    result = auth.effective_principals('acct:elina@example.com',
-                                       request,
-                                       groupfinder=groupfinder)
-
-    assert result == [security.Everyone]
-
-
-def test_effective_principals_includes_authenticated_and_userid():
-    """
-    If the groupfinder returns the empty list, implying that the userid is
-    recognised but is a member of no groups, `security.Authenticated` and the
-    passed userid should be included in the list of effective principals.
-    """
-    groupfinder = lambda userid, request: []
-    request = testing.DummyRequest()
-
-    result = auth.effective_principals('acct:elina@example.com',
-                                       request,
-                                       groupfinder=groupfinder)
-
-    assert set(result) == set([security.Everyone,
-                               security.Authenticated,
-                               'acct:elina@example.com'])
-
-
-def test_effective_principals_includes_returned_groupfinder_principals():
-    """
-    If the groupfinder returns groups, these should be included in the list of
-    effective principals.
-    """
-    groupfinder = lambda userid, request: ['group:foo', 'group:bar']
-    request = testing.DummyRequest()
-
-    result = auth.effective_principals('acct:elina@example.com',
-                                       request,
-                                       groupfinder=groupfinder)
-
-    assert set(result) == set([security.Everyone,
-                               security.Authenticated,
-                               'acct:elina@example.com',
-                               'group:foo',
-                               'group:bar'])
-
-def test_effective_principals_calls_groupfinder_with_userid_and_request():
-    groupfinder = Mock()
-    groupfinder.return_value = []
-    request = testing.DummyRequest()
-
-    auth.effective_principals('acct:elina@example.com',
-                              request,
-                              groupfinder=groupfinder)
-
-    groupfinder.assert_called_with('acct:elina@example.com', request)
+    assert principals == [security.Everyone,
+                          'group:__admin__',
+                          'group:__staff__',
+                          'group:abc123',
+                          'group:def456',
+                          security.Authenticated,
+                          'acct:jiji@hypothes.is']
 
 
 @pytest.fixture
 def accounts(request):
-    patcher = patch('h.auth.accounts', autospec=True)
+    patcher = mock.patch('h.auth.accounts', autospec=True)
     request.addfinalizer(patcher.stop)
     return patcher.start()
 
 
 @pytest.fixture
 def groups(request):
-    patcher = patch('h.auth.groups', autospec=True)
+    patcher = mock.patch('h.auth.groups', autospec=True)
     request.addfinalizer(patcher.stop)
     return patcher.start()