From 8bd183ee65b9e2bea29df9f2a36a200bad67ea40 Mon Sep 17 00:00:00 2001 From: PrahlM93 Date: Sun, 5 Jun 2016 12:40:21 -0400 Subject: [PATCH 1/3] Remove unused functions --- postmaster/utils.py | 51 ----------------------------- tests/utils/test_utils_functions.py | 14 -------- 2 files changed, 65 deletions(-) diff --git a/postmaster/utils.py b/postmaster/utils.py index 403c75a..56912d1 100644 --- a/postmaster/utils.py +++ b/postmaster/utils.py @@ -15,57 +15,6 @@ from postmaster.errors import ValidationError -def row2dict(row): - """ Return a dictionary from a sqlalchemy row We use this so - we don't have to deal with sa_instance_state stuff - """ - d = {} - for column in row.__table__.columns: - d[column.name] = str(getattr(row, column.name)) - return d - - -def getAllFromTable(table): - """ Returns the entire table that is specified - """ - # Dynamically get the model to query based on the table variable - if getattr(models, table): - table = (getattr(models, table)).query.all() - listItemsDict = [row2dict(row) for row in table] - return listItemsDict - - return None - - -def getDomain(name): - """ Returns a domain from VirtualDomains - """ - queriedDomain = models.VirtualDomains.query.filter_by(name=name).first() - if queriedDomain: - return row2dict(queriedDomain) - return None - - -def getUser(email): - """ Returns a user from VirtualUsers - """ - queriedUser = models.VirtualUsers.query.filter_by(email=email).first() - - if queriedUser: - return row2dict(queriedUser) - return None - - -def getAlias(source): - """ Returns alias from VirtualAliases - """ - queriedAlias = models.VirtualAliases.query.filter_by(source=source).first() - - if queriedAlias: - return row2dict(queriedAlias) - return None - - def maildb_auditing_enabled(): """ Returns a bool based on if mail db auditing is enabled """ diff --git a/tests/utils/test_utils_functions.py b/tests/utils/test_utils_functions.py index edb8aec..88b0b1f 100644 --- a/tests/utils/test_utils_functions.py +++ b/tests/utils/test_utils_functions.py @@ -8,20 +8,6 @@ class TestUtilsFunctions: - def test_getDomain(self): - result = getDomain('postmaster.com') - assert (result['name'] == 'postmaster.com') and ('id' in result) - - def test_getUser(self): - result = getUser('email@postmaster.com') - assert (result['email'] == 'email@postmaster.com') and ( - 'id' in result) and ('password' in result) - - def test_getAlias(self): - result = getAlias('aliasemail@postmaster.com') - assert (result['source'] == 'aliasemail@postmaster.com') and ( - result['destination'] == 'email@postmaster.com') - def test_maildb_auditing_enabled(self): result = maildb_auditing_enabled() assert isinstance(result, bool) From c18838bb2b1d9e3adece83fb9b5dbc55d007cedf Mon Sep 17 00:00:00 2001 From: PrahlM93 Date: Sun, 5 Jun 2016 15:21:33 -0400 Subject: [PATCH 2/3] Changed get_nested_group_members function to check_nested_group_membership * Changed get_nested_group_members function to check_nested_group_membership to make it more efficient, as it now queries the user's nested membership and validates if the user is part of that group. --- postmaster/utils.py | 40 ++++++-------- tests/utils/test_utils_functions.py | 81 +++++++++++++++-------------- 2 files changed, 57 insertions(+), 64 deletions(-) diff --git a/postmaster/utils.py b/postmaster/utils.py index 56912d1..9a56590 100644 --- a/postmaster/utils.py +++ b/postmaster/utils.py @@ -350,24 +350,25 @@ def get_distinguished_name(self, sAMAccountName): else: raise ADException('You must be logged into LDAP to search') - def get_nested_group_members(self, group_distinguishedName): - """ Gets the members and nested members of a group by supplying a distinguishedName for the group. - A list with the members will be returned + def check_nested_group_membership(self, group_sAMAccountName, user_sAMAccountName): + """ Checks the nested group membership of a user by supplying the sAMAccountName, and verifies if the user is a + part of that supplied group. A list with the groups the user is a member of will be returned """ # Check if the ldap_connection is in a logged in state if self.ldap_connection.whoami_s(): - group_members = list() + group_dn = self.get_distinguished_name(group_sAMAccountName) + user_dn = self.get_distinguished_name(user_sAMAccountName) - if group_distinguishedName: + if group_dn and user_dn: # Get the base distinguished name based on the domain name base_dn = 'dc=' + (self.domain.replace('.', ',dc=')) - search_filter = '(memberOf:1.2.840.113556.1.4.1941:={0})'.format(group_distinguishedName) + search_filter = '(member:1.2.840.113556.1.4.1941:={0})'.format(user_dn) search_result = self.ldap_connection.search_s(base_dn, ldap.SCOPE_SUBTREE, search_filter, ['distinguishedName']) - for member in search_result: - if member[0]: - group_members.append(member[0]) - return group_members + for group in search_result: + if group[0] == group_dn: + return True + return False else: raise ADException('You must be logged into LDAP to search') @@ -421,14 +422,6 @@ def check_group_membership(self): if self.ldap_connection.whoami_s(): # AD returns the username as DOMAIN\username, so this gets the sAMAccountName username = sub(r'(^.*(?<=\\))', '', self.ldap_connection.whoami_s()) - # Get the distinguished name of the logged in user - user_distinguished_name = self.get_distinguished_name(username) - - if not user_distinguished_name: - json_logger( - 'error', username, - 'The LDAP user "{0}" authenticated but could not be found'.format(username)) - raise ADException('There was an error searching LDAP. Please try again.') # Get the distinguished name of the admin group in the database group_distinguished_name = self.get_distinguished_name(self.ldap_admin_group) @@ -438,10 +431,8 @@ def check_group_membership(self): 'The PostMaster Admin group "{0}" could not be found'.format(self.ldap_admin_group)) raise ADException('There was an error searching LDAP. Please try again.') - group_members = self.get_nested_group_members(group_distinguished_name) - for member in group_members: - if user_distinguished_name == member: - return True + if self.check_nested_group_membership(self.ldap_admin_group, username): + return True # If the user was not a member of the group, check to see if the admin group is the primary group # of the user which is not included in memberOf (this is typically Domain Users) @@ -451,9 +442,8 @@ def check_group_membership(self): json_logger( 'auth', username, - 'The LDAP user "{0}" authenticated but the login failed \ - because they weren\'t a PostMaster administrator'.format( - username)) + ('The LDAP user "{0}" authenticated but the login failed because they weren\'t ' + 'a PostMaster administrator').format(username)) raise ADException('The user account is not authorized to login to PostMaster') else: raise ADException('You must be logged into LDAP to search') diff --git a/tests/utils/test_utils_functions.py b/tests/utils/test_utils_functions.py index 88b0b1f..f78f3b2 100644 --- a/tests/utils/test_utils_functions.py +++ b/tests/utils/test_utils_functions.py @@ -1,4 +1,5 @@ import functools +import pytest from mockldap import MockLdap from mock import patch from postmaster import app @@ -89,16 +90,21 @@ def wrapped(self, *args, **kwargs): return wrapped -def mocked_nested_group_members_query(): - """ Returns mocked output of the memberOf:1.2.840.113556.1.4.1941 LDAP query +def mocked_nested_group_membership_query(): + """ Returns mocked output of the member:1.2.840.113556.1.4.1941 LDAP query """ - return [('cn=testuser,cn=users,dc=postmaster,dc=local', {}), - ('cn=someuser,cn=users,dc=postmaster,dc=local', {}), - (None, ['ldaps://ForestDnsZones.postmaster.local/DC=ForestDnsZones,DC=postmaster,DC=local']), - (None, ['ldaps://DomainDnsZones.postmaster.local/DC=DomainDnsZones,DC=postmaster,DC=local']), + return [('CN=Some Group,OU=Groups,DC=postmaster,DC=local', + {'distinguishedName': ['CN=Some Group,OU=Groups,DC=postmaster,DC=local']}), + ('CN=PostMaster Admins,OU=Groups,DC=postmaster,DC=local', + {'distinguishedName': ['CN=ADReset Users,OU=Groups,DC=postmaster,DC=local']}), + (None, + ['ldaps://ForestDnsZones.postmaster.local/DC=ForestDnsZones,DC=postmaster,DC=local']), + (None, + ['ldaps://DomainDnsZones.postmaster.local/DC=DomainDnsZones,DC=postmaster,DC=local']), (None, ['ldaps://postmaster.local/CN=Configuration,DC=postmaster,DC=local'])] + def mocked_get_nested_group_members(): """ Returns mocked output of the get_nested_group_members function """ @@ -293,62 +299,58 @@ def test_get_distinguished_name(self, mock_whoami_s): # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') - @patch('mockldap.ldapobject.LDAPObject.search_s', return_value=mocked_nested_group_members_query()) + # This must be patched as this type of query is only valid in Active Directory + @patch('mockldap.ldapobject.LDAPObject.search_s', return_value=mocked_nested_group_membership_query()) @manage_mock_ldap - def test_get_nested_group_members(self, mock_nested_group_members_query, mock_whoami_s): - """ Tests the get_nested_group_members function and expects the return value + def test_check_nested_group_membership(self, mock_nested_group_membership_query, mock_whoami_s): + """ Tests the check_nested_group_membership function and expects the return value of the users's distinguished name """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - nested_groups = self.ad_obj.get_nested_group_members(self.post_master_admins_group[1]['distinguishedName'][0]) - assert nested_groups == mocked_get_nested_group_members() + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True + assert self.ad_obj.check_nested_group_membership(self.post_master_admins_group[1]['distinguishedName'][0], + self.test_user[1]['distinguishedName'][0]) is True # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') - @patch('postmaster.utils.AD.get_nested_group_members', return_value=mocked_get_nested_group_members()) + # Simpler to patch this in order to reduce the overall amount of patches + @patch('postmaster.utils.AD.check_nested_group_membership', return_value=True) @manage_mock_ldap - def test_check_group_membership_pass_memberof(self, mock_get_nested_group_members, mock_whoami_s): + def test_check_group_membership_pass_memberof(self, mock_check_nested_group_membership, mock_whoami_s): """ Tests the check_group_membership function and that the user's group membership matches the administrative LDAP group specified in the database. A return value of True is expected """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - group_membership_result = self.ad_obj.check_group_membership() - assert group_membership_result is True + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True + assert self.ad_obj.check_group_membership() is True # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser2') - @patch('postmaster.utils.AD.get_nested_group_members', return_value=mocked_get_nested_group_members()) + # Simpler to patch this in order to reduce the overall amount of patches + @patch('postmaster.utils.AD.check_nested_group_membership', return_value=False) @manage_mock_ldap - def test_check_group_membership_pass_primary_group(self, mock_get_nested_group_members, mock_whoami_s): + def test_check_group_membership_pass_primary_group(self, mock_check_nested_group_membership, mock_whoami_s): """ Tests the check_group_membership function and that the user's primaryGroupID matches the administrative LDAP group specified in the database. A return value of True is expected """ - login_result = self.ad_obj.login(self.test_user2[1]['distinguishedName'][0], - self.test_user2[1]['userPassword'][0]) - assert login_result is True - group_membership_result = self.ad_obj.check_group_membership() - assert group_membership_result is True + assert self.ad_obj.login(self.test_user2[1]['distinguishedName'][0], + self.test_user2[1]['userPassword'][0]) is True + assert self.ad_obj.check_group_membership() is True # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser3') - @patch('postmaster.utils.AD.get_nested_group_members', return_value=mocked_get_nested_group_members()) + # Simpler to patch this in order to reduce the overall amount of patches + @patch('postmaster.utils.AD.check_nested_group_membership', return_value=False) @manage_mock_ldap - def test_check_group_membership_fail(self, mock_get_nested_group_members, mock_whoami_s): + def test_check_group_membership_fail(self, mock_check_nested_group_membership, mock_whoami_s): """ Tests the check_group_membership function and that the user is not authorized to use Post Master. A return value of ADException is expected """ - login_result = self.ad_obj.login(self.test_user3[1]['distinguishedName'][0], - self.test_user3[1]['userPassword'][0]) - assert login_result is True - try: + assert self.ad_obj.login(self.test_user3[1]['distinguishedName'][0], + self.test_user3[1]['userPassword'][0]) is True + with pytest.raises(ADException) as excinfo: self.ad_obj.check_group_membership() - assert False, 'The check_group_membership function did not throw the expected exception' - except ADException as e: - assert e.message == 'The user account is not authorized to login to PostMaster' + assert excinfo.value.message == 'The user account is not authorized to login to PostMaster' def test_add_ldap_user_to_db(self): """ Tests the add_ldap_user_to_db function and expects that the database @@ -363,9 +365,10 @@ def test_add_ldap_user_to_db(self): # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') @patch('postmaster.utils.AD') - @patch('postmaster.utils.AD.get_nested_group_members', return_value=mocked_get_nested_group_members()) + # Simpler to patch this in order to reduce the overall amount of patches + @patch('postmaster.utils.AD.check_nested_group_membership', return_value=True) @manage_mock_ldap - def test_validate_wtforms_password(self, mock_get_nested_group_members, mock_ad, mock_whoami_s): + def test_validate_wtforms_password(self, mock_check_nested_group_membership, mock_ad, mock_whoami_s): """ Tests the validate_wtforms_password function by logging in with an authorized LDAP user, and expects the Dashboard page (view when logged in) to be returned """ From 59b4a125f8e2ae6026f9e5005f35d117a1d9b009 Mon Sep 17 00:00:00 2001 From: PrahlM93 Date: Sun, 5 Jun 2016 15:27:29 -0400 Subject: [PATCH 3/3] Reduced the number of lines in the tests --- tests/utils/test_utils_functions.py | 54 +++++++++++------------------ 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/tests/utils/test_utils_functions.py b/tests/utils/test_utils_functions.py index f78f3b2..3ab7b5e 100644 --- a/tests/utils/test_utils_functions.py +++ b/tests/utils/test_utils_functions.py @@ -206,19 +206,16 @@ class TestAdFunctions: def test_login_pass(self): """ Tests the login function and expects a return value of True """ - result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert result is True + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True @manage_mock_ldap def test_login_fail(self): """ Tests the login function with a wrong password and expects a return value of ADException """ - try: + with pytest.raises(ADException) as excinfo: self.ad_obj.login(self.test_user[1]['distinguishedName'][0], 'WrongPassword') - assert False, 'The login function did not throw the expected exception' - except ADException as e: - assert e.message == 'The username or password was incorrect' + assert excinfo.value.message == 'The username or password was incorrect' # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') @@ -226,11 +223,9 @@ def test_login_fail(self): def test_get_loggedin_user(self, mock_whoami_s): """ Tests the get_loggedin_user function and expects the return value to be testUser """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - loggedin_user_result = self.ad_obj.get_loggedin_user() - assert loggedin_user_result == 'testUser' + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True + assert self.ad_obj.get_loggedin_user() == 'testUser' # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') @@ -239,11 +234,9 @@ def test_get_loggedin_user_display_name(self, mock_whoami_s): """ Tests the get_loggedin_user_display_name function which expects the return value of Test User """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - loggedin_user_display_name_result = self.ad_obj.get_loggedin_user_display_name() - assert loggedin_user_display_name_result == 'Test User' + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True + assert self.ad_obj.get_loggedin_user_display_name() == 'Test User' # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser2') @@ -252,11 +245,9 @@ def test_get_loggedin_user_display_name_when_none(self, mock_whoami_s): """ Tests the get_loggedin_user_display_name function on a user which does not have the displayName attribute specified. It expects the return of the name attribute which is testUser2 """ - login_result = self.ad_obj.login(self.test_user2[1]['distinguishedName'][0], - self.test_user2[1]['userPassword'][0]) - assert login_result is True - loggedin_user_display_name_result = self.ad_obj.get_loggedin_user_display_name() - assert loggedin_user_display_name_result == 'testUser2' + assert self.ad_obj.login(self.test_user2[1]['distinguishedName'][0], + self.test_user2[1]['userPassword'][0]) == True + assert self.ad_obj.get_loggedin_user_display_name() == 'testUser2' @manage_mock_ldap def test_sid2str(self): @@ -275,13 +266,11 @@ def test_get_primary_group_dn_of_user(self, mock_whoami_s): distinguished name can be found when passing the sAMAccountName of testUser. The distinguished name of Domain Users is expected as the return value """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - domain_users_primary_group_dn = self.ad_obj.get_primary_group_dn_of_user( - self.test_user[1]['sAMAccountName'][0]) + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True # MockLdap returns the result as lowercase which is why upper is needed for the assert - assert domain_users_primary_group_dn.upper() == 'CN=Domain Users,CN=Users,DC=postmaster,DC=local'.upper() + assert self.ad_obj.get_primary_group_dn_of_user(self.test_user[1]['sAMAccountName'][0]).upper() == \ + 'CN=Domain Users,CN=Users,DC=postmaster,DC=local'.upper() # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser') @@ -290,12 +279,11 @@ def test_get_distinguished_name(self, mock_whoami_s): """ Tests the get_distinguished_name function and expects the return value of the users's distinguished name """ - login_result = self.ad_obj.login(self.test_user[1]['distinguishedName'][0], - self.test_user[1]['userPassword'][0]) - assert login_result is True - distinguished_name = self.ad_obj.get_distinguished_name(self.test_user[1]['sAMAccountName'][0]) + assert self.ad_obj.login(self.test_user[1]['distinguishedName'][0], + self.test_user[1]['userPassword'][0]) is True # MockLdap returns the result as lowercase which is why upper is needed for the assert - assert distinguished_name.upper() == self.test_user[1]['distinguishedName'][0].upper() + assert self.ad_obj.get_distinguished_name(self.test_user[1]['sAMAccountName'][0]).upper() == \ + self.test_user[1]['distinguishedName'][0].upper() # Mocks the actual value returned from AD versus another LDAP directory @patch('mockldap.ldapobject.LDAPObject.whoami_s', return_value='POSTMASTER\\testUser')