diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index d8173b8..8a64d4a 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -60,4 +60,6 @@ Set up your environment 5. Push the topic branch to your personal fork -6. Create a pull request to the django-auth-adfs repository with a detailed explanation +6. Test your code by running the test suite ``poetry run coverage run manage.py test -v 2`` + +7. Create a pull request to the django-auth-adfs repository with a detailed explanation diff --git a/django_auth_adfs/config.py b/django_auth_adfs/config.py index 3beb64c..4a4c07d 100644 --- a/django_auth_adfs/config.py +++ b/django_auth_adfs/config.py @@ -27,6 +27,13 @@ AZURE_AD_SERVER = "login.microsoftonline.com" DEFAULT_SETTINGS_CLASS = 'django_auth_adfs.config.Settings' +REQUIRED_SETTINGS = [ + "AUDIENCE", + "CLIENT_ID", + "RELYING_PARTY_ID", + "USERNAME_CLAIM", +] + class ConfigLoadError(Exception): pass @@ -78,13 +85,6 @@ def __init__(self): self.VERSION = 'v1.0' - required_settings = [ - "AUDIENCE", - "CLIENT_ID", - "RELYING_PARTY_ID", - "USERNAME_CLAIM", - ] - deprecated_settings = { "AUTHORIZE_PATH": "This setting is automatically loaded from ADFS.", "ISSUER": "This setting is automatically loaded from ADFS.", @@ -102,72 +102,91 @@ def __init__(self): if "SETTINGS_CLASS" in _settings: del _settings["SETTINGS_CLASS"] - # Handle deprecated settings - for setting, message in deprecated_settings.items(): - if setting in _settings: - warnings.warn("Setting {} is deprecated and it's value was ignored. {}".format(setting, message), + if "IDPS" in _settings: + if any(item in REQUIRED_SETTINGS for item in _settings): + raise ImproperlyConfigured( + "The IDPS configuration cannot be set when any of these {} are set".format(REQUIRED_SETTINGS)) + + if not len(_settings['IDPS']): + raise ImproperlyConfigured( + "The IDPS configuration must have at least one configuration defined.") + + # Deprecated settings are not supported in IDPS + + for idp_name, idp_settings in _settings['IDPS'].items(): + for setting in REQUIRED_SETTINGS: + if setting not in idp_settings: + raise ImproperlyConfigured( + "django_auth_adfs setting '{}' has not been set for IDP key '{}'".format(setting, idp_name) + ) + + else: + # Handle deprecated settings + for setting, message in deprecated_settings.items(): + if setting in _settings: + warnings.warn("Setting {} is deprecated and it's value was ignored. {}".format(setting, message), + DeprecationWarning) + del _settings[setting] + + if "CERT_MAX_AGE" in _settings: + _settings["CONFIG_RELOAD_INTERVAL"] = _settings["CERT_MAX_AGE"] + warnings.warn('Setting CERT_MAX_AGE has been renamed to CONFIG_RELOAD_INTERVAL. The value was copied.', DeprecationWarning) - del _settings[setting] - - if "CERT_MAX_AGE" in _settings: - _settings["CONFIG_RELOAD_INTERVAL"] = _settings["CERT_MAX_AGE"] - warnings.warn('Setting CERT_MAX_AGE has been renamed to CONFIG_RELOAD_INTERVAL. The value was copied.', - DeprecationWarning) - del _settings["CERT_MAX_AGE"] - - if "GROUP_CLAIM" in _settings: - _settings["GROUPS_CLAIM"] = _settings["GROUP_CLAIM"] - warnings.warn('Setting GROUP_CLAIM has been renamed to GROUPS_CLAIM. The value was copied.', - DeprecationWarning) - del _settings["GROUP_CLAIM"] - - if "RESOURCE" in _settings: - _settings["RELYING_PARTY_ID"] = _settings["RESOURCE"] - del _settings["RESOURCE"] - if "TENANT_ID" in _settings: - # If a tenant ID was set, switch to Azure AD mode - if "SERVER" in _settings: - raise ImproperlyConfigured("The SERVER cannot be set when TENANT_ID is set.") - self.SERVER = AZURE_AD_SERVER - self.USERNAME_CLAIM = "upn" - self.GROUPS_CLAIM = "groups" - self.CLAIM_MAPPING = {"first_name": "given_name", - "last_name": "family_name", - "email": "email"} - elif "VERSION" in _settings: - raise ImproperlyConfigured("The VERSION cannot be set when TENANT_ID is set.") - - # Overwrite defaults with user settings - for setting, value in _settings.items(): - if hasattr(self, setting): - setattr(self, setting, value) - else: - msg = "'{0}' is not a valid configuration directive for django_auth_adfs." - raise ImproperlyConfigured(msg.format(setting)) + del _settings["CERT_MAX_AGE"] + + if "GROUP_CLAIM" in _settings: + _settings["GROUPS_CLAIM"] = _settings["GROUP_CLAIM"] + warnings.warn('Setting GROUP_CLAIM has been renamed to GROUPS_CLAIM. The value was copied.', + DeprecationWarning) + del _settings["GROUP_CLAIM"] + + if "RESOURCE" in _settings: + _settings["RELYING_PARTY_ID"] = _settings["RESOURCE"] + del _settings["RESOURCE"] + + if "TENANT_ID" in _settings: + # If a tenant ID was set, switch to Azure AD mode + if "SERVER" in _settings: + raise ImproperlyConfigured("The SERVER cannot be set when TENANT_ID is set.") + self.SERVER = AZURE_AD_SERVER + self.USERNAME_CLAIM = "upn" + self.GROUPS_CLAIM = "groups" + self.CLAIM_MAPPING = {"first_name": "given_name", + "last_name": "family_name", + "email": "email"} + elif "VERSION" in _settings: + raise ImproperlyConfigured("The VERSION cannot be set when TENANT_ID is set.") + + # Overwrite defaults with user settings + for setting, value in _settings.items(): + if hasattr(self, setting): + setattr(self, setting, value) + else: + msg = "'{0}' is not a valid configuration directive for django_auth_adfs." + raise ImproperlyConfigured(msg.format(setting)) + + if self.SERVER != AZURE_AD_SERVER and self.BLOCK_GUEST_USERS: + raise ImproperlyConfigured("You can only set BLOCK_GUEST_USERS when self.TENANT_ID is set") - if self.SERVER != AZURE_AD_SERVER and self.BLOCK_GUEST_USERS: - raise ImproperlyConfigured("You can only set BLOCK_GUEST_USERS when self.TENANT_ID is set") + if self.TENANT_ID is None: + # For on premises ADFS, the tenant ID is set to adfs + # On AzureAD the adfs part in the URL happens to be replace by the tenant ID. + self.TENANT_ID = "adfs" - if self.TENANT_ID is None: - # For on premises ADFS, the tenant ID is set to adfs - # On AzureAD the adfs part in the URL happens to be replace by the tenant ID. - self.TENANT_ID = "adfs" + for setting in REQUIRED_SETTINGS: + if not getattr(self, setting): + raise ImproperlyConfigured("django_auth_adfs setting '{0}' has not been set".format(setting)) - for setting in required_settings: - if not getattr(self, setting): - msg = "django_auth_adfs setting '{0}' has not been set".format(setting) - raise ImproperlyConfigured(msg) + # Validate setting conflicts + usermodel = get_user_model() + if usermodel.USERNAME_FIELD in self.CLAIM_MAPPING: + raise ImproperlyConfigured("You cannot set the username field of the user model from " + "the CLAIM_MAPPING setting. Instead use the USERNAME_CLAIM setting.") # Setup dynamic settings if not callable(self.CUSTOM_FAILED_RESPONSE_VIEW): self.CUSTOM_FAILED_RESPONSE_VIEW = import_string(self.CUSTOM_FAILED_RESPONSE_VIEW) - # Validate setting conflicts - usermodel = get_user_model() - if usermodel.USERNAME_FIELD in self.CLAIM_MAPPING: - raise ImproperlyConfigured("You cannot set the username field of the user model from " - "the CLAIM_MAPPING setting. Instead use the USERNAME_CLAIM setting.") - class ProviderConfig(object): def __init__(self): diff --git a/tests/test_settings.py b/tests/test_settings.py index 96a8b98..a1ea58a 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -5,7 +5,7 @@ from django.test import TestCase, SimpleTestCase, override_settings from mock import patch from django_auth_adfs.config import django_settings -from django_auth_adfs.config import Settings +from django_auth_adfs.config import Settings, REQUIRED_SETTINGS from .custom_config import Settings as CustomSettings @@ -24,6 +24,47 @@ def test_claim_mapping_overlapping_username_field(self): with self.assertRaises(ImproperlyConfigured): Settings() + def test_idps_as_mutually_exclusive(self): + settings = deepcopy(django_settings) + settings.AUTH_ADFS["IDPS"] = {} + with patch("django_auth_adfs.config.django_settings", settings): + with self.assertRaises(ImproperlyConfigured): + Settings() + + def test_idps_empty_entries(self): + settings = deepcopy(django_settings) + for setting in REQUIRED_SETTINGS: + if setting in settings.AUTH_ADFS: + del settings.AUTH_ADFS[setting] + settings.AUTH_ADFS["IDPS"] = {} + with patch("django_auth_adfs.config.django_settings", settings): + with self.assertRaises(ImproperlyConfigured) as cm: + Settings() + + self.assertEqual( + str(cm.exception), + "The IDPS configuration must have at least one configuration defined." + ) + + def test_idps_missing_required_settings_in_entry(self): + settings = deepcopy(django_settings) + for setting in REQUIRED_SETTINGS: + if setting in settings.AUTH_ADFS: + del settings.AUTH_ADFS[setting] + settings.AUTH_ADFS["IDPS"] = { + "adfs": { + "CLIENT_ID": "abc" + } + } + with patch("django_auth_adfs.config.django_settings", settings): + with self.assertRaises(ImproperlyConfigured) as cm: + Settings() + + self.assertEqual( + str(cm.exception), + "django_auth_adfs setting 'AUDIENCE' has not been set for IDP key 'adfs'" + ) + def test_tenant_and_server(self): settings = deepcopy(django_settings) settings.AUTH_ADFS["TENANT_ID"] = "abc"