Minor refactor on role/group code. (#54)

samlauthenticator/samlauthenticator.py

@@ -217,7 +217,7 @@ class SAMLAuthenticator(Authenticator):
  config=True,
  help='''
  The nameId format to set in the Jupyter SAML Metadata.
- Detaults to transient nameid-format, but other values such as
+ Defaults to transient nameid-format, but other values such as
  urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress or
  urn:oasis:names:tc:SAML:2.0:nameid-format:persistent
  are available. See section 8.3 of the spec
@@ -314,7 +314,10 @@ class SAMLAuthenticator(Authenticator):
  Comma-separated list of roles. SAMLAuthenticator will restrict access to
  jupyterhub to these roles if specified.
  '''
- ) 
+ )
+ _const_warn_explain = 'Because no user would be allowed to log in via roles, role check disabled.'
+ _const_warn_no_role_xpath = 'Allowed roles set while role location XPath is not set.'
+ _const_warn_no_roles = 'Allowed roles not set while role location XPath is set.'
 
  def _get_metadata_from_file(self):
  with open(self.metadata_filepath, 'r') as saml_metadata:
@@ -588,8 +591,6 @@ def _get_roles_from_saml_etree(self, signed_xml):
  return xpath_result
 
  self.log.warning('Could not find role from role XPath')
- else:
- self.log.warning('Role XPath not set')
 
  return []
 
@@ -626,18 +627,48 @@ def _check_username_and_add_user(self, username):
  self.check_blacklist(username) and \
  self.check_whitelist(username):
  if self.create_system_users:
- return self._optional_user_add(username)
+ if self._optional_user_add(username):
+ # Successfully added user
+ return username
+ else:
+ # Failed to add user
+ self.log.error('Failed to add user by calling add user')
+ return None
 
- return True
+ # Didn't try to add user
+ return username
 
- return False
+ # Failed to validate username or failed list check
+ self.log.error('Failed to validate username or failed list check')
+ return None
 
  def _check_role(self, user_roles):
- if self.allowed_roles:
- allowed_roles = [x.strip() for x in self.allowed_roles.split(',')]
+ allowed_roles = [x.strip() for x in self.allowed_roles.split(',')]
+
+ return any(elem in allowed_roles for elem in user_roles)
 
- return any(elem in allowed_roles for elem in user_roles)
+ def _valid_roles_in_assertion(self, signed_xml, saml_doc_etree):
+ user_roles = self._get_roles_from_saml_doc(signed_xml, saml_doc_etree)
 
+ user_roles_result = self._check_role(user_roles)
+ if not user_roles_result:
+ self.log.error('User role not authorized')
+ return user_roles_result
+
+ def _valid_config_and_roles(self, signed_xml, saml_doc_etree):
+ if self.allowed_roles and self.xpath_role_location:
+ return self._valid_roles_in_assertion(signed_xml, saml_doc_etree)
+
+ if (not self.allowed_roles) and self.xpath_role_location:
+ self.log.warning(self._const_warn_no_roles)
+ self.log.warning(self._const_warn_explain)
+
+ if self.allowed_roles and (not self.xpath_role_location):
+ self.log.warning(self._const_warn_no_role_xpath)
+ self.log.warning(self._const_warn_explain)
+
+ # This technically skips the "neither set" case, but since that's expected-ish, I think we can let
+ # that slide.
  return True
 
  def _authenticate(self, handler, data):
@@ -659,19 +690,12 @@ def _authenticate(self, handler, data):
  self.log.debug('Authenticated user using SAML')
  username = self._get_username_from_saml_doc(signed_xml, saml_doc_etree)
  username = self.normalize_username(username)
- user_roles = self._get_roles_from_saml_doc(signed_xml, saml_doc_etree)
-
- user_roles_result = self._check_role(user_roles)
- if not user_roles_result:
- self.log.error('User role not authorized')
- return None
 
- self.log.debug('Optionally create and return user: ' + username)
- username_add_result = self._check_username_and_add_user(username)
- if username_add_result:
- return username
+ if self._valid_config_and_roles(signed_xml, saml_doc_etree):
+ self.log.debug('Optionally create and return user: ' + username)
+ return self._check_username_and_add_user(username)
 
- self.log.error('Failed to add user')
+ self.log.error('Assertion did not have appropriate roles')
  return None
 
  self.log.error('Error validating SAML response')

tests/test_authenticator.py

@@ -467,6 +467,7 @@ def test_check_role(self):
  a.allowed_roles = 'group1'
 
  assert a._check_role(['group1'])
+ assert a._check_role(['group1', 'group2'])
 
  def test_check_roles(self):
  a = SAMLAuthenticator()
@@ -476,28 +477,73 @@ def test_check_roles(self):
  assert a._check_role(['group2', 'group3'])
  assert a._check_role(['group1', 'nogroup1'])
 
- def test_check_role_allow_all(self):
+ def test_check_role_fails(self):
  a = SAMLAuthenticator()
+ a.allowed_roles='group1,group2,group3'
+
+ assert not a._check_role([])
+ assert not a._check_role(['nogroup1'])
+ assert not a._check_role(['nogroup1', 'nogroup2'])
 
- assert a._check_role([])
- assert a._check_role(['group1'])
- assert a._check_role(['group1', 'group2'])
 
- def test_check_role_empty_allow_all(self):
+class TestValidRolesConfig(unittest.TestCase):
+
+ def test_no_xpath_no_roles_run_default(self):
  a = SAMLAuthenticator()
- a.allowed_roles=''
+ a._valid_roles_in_assertion = unittest.mock.create_autospec(MagicMock(name='_valid_roles_in_assertion'))
+ a.log.warning = MagicMock(name='warning')
 
- assert a._check_role([])
- assert a._check_role(['group1'])
- assert a._check_role(['group1', 'group2'])
+ assert a._valid_config_and_roles(None, None)
+ a._valid_roles_in_assertion.assert_not_called()
+ a.log.warning.assert_not_called()
 
- def test_check_role_fails(self):
+ def test_xpath_roles_call_methods_true_return(self):
  a = SAMLAuthenticator()
- a.allowed_roles='group1,group2,group3'
+ a._valid_roles_in_assertion = MagicMock(name='_valid_roles_in_assertion', return_value=True)
+ a.log.warning = MagicMock(name='warning')
+ a.allowed_roles = 'group1'
+ a.xpath_role_location = 'value'
 
- assert not a._check_role([])
- assert not a._check_role(['nogroup1'])
- assert not a._check_role(['nogroup1', 'nogroup2'])
+ assert a._valid_config_and_roles(None, None) == True
+ a._valid_roles_in_assertion.assert_called_once_with(None, None)
+ a.log.warning.assert_not_called()
+
+ def test_xpath_roles_call_methods_false_return(self):
+ a = SAMLAuthenticator()
+ a._valid_roles_in_assertion = MagicMock(name='_valid_roles_in_assertion', return_value=False)
+ a.log.warning = MagicMock(name='warning')
+ a.allowed_roles = 'group1'
+ a.xpath_role_location = 'value'
+
+ assert a._valid_config_and_roles(None, None) == False
+ a._valid_roles_in_assertion.assert_called_once_with(None, None)
+ a.log.warning.assert_not_called()
+
+ def test_xpath_no_roles(self):
+ a = SAMLAuthenticator()
+ a.xpath_role_location = 'value'
+ a._valid_roles_in_assertion = unittest.mock.create_autospec(MagicMock(name='_valid_roles_in_assertion'))
+ a.log.warning = MagicMock(name='warning')
+
+ assert a._valid_config_and_roles(None, None)
+ a._valid_roles_in_assertion.assert_not_called()
+ print(a.log.warning.call_args_list)
+ a.log.warning.assert_called()
+ a.log.warning.assert_any_call(a._const_warn_explain)
+ a.log.warning.assert_any_call(a._const_warn_no_roles)
+
+ def test_no_xpath_roles(self):
+ a = SAMLAuthenticator()
+ a.allowed_roles = 'value'
+ a._valid_roles_in_assertion = unittest.mock.create_autospec(MagicMock(name='_valid_roles_in_assertion'))
+ a.log.warning = MagicMock(name='warning')
+
+ assert a._valid_config_and_roles(None, None)
+ a._valid_roles_in_assertion.assert_not_called()
+ print(a.log.warning.call_args_list)
+ a.log.warning.assert_called()
+ a.log.warning.assert_any_call(a._const_warn_explain)
+ a.log.warning.assert_any_call(a._const_warn_no_role_xpath)
 
 
 class TestCreateUser(unittest.TestCase):