Commit b4904adc by Braden MacDonald

Use ConfigurationModels for third_party_auth, new metadata fetching - PR 8155

parent caca3e1b
...@@ -196,8 +196,9 @@ def auth_pipeline_urls(auth_entry, redirect_url=None): ...@@ -196,8 +196,9 @@ def auth_pipeline_urls(auth_entry, redirect_url=None):
return {} return {}
return { return {
provider.NAME: third_party_auth.pipeline.get_login_url(provider.NAME, auth_entry, redirect_url=redirect_url) provider.provider_id: third_party_auth.pipeline.get_login_url(
for provider in third_party_auth.provider.Registry.enabled() provider.provider_id, auth_entry, redirect_url=redirect_url
) for provider in third_party_auth.provider.Registry.enabled()
} }
......
...@@ -11,12 +11,12 @@ from django.core.urlresolvers import reverse ...@@ -11,12 +11,12 @@ from django.core.urlresolvers import reverse
from util.testing import UrlResetMixin from util.testing import UrlResetMixin
from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.factories import CourseFactory
from student.tests.factories import CourseModeFactory from student.tests.factories import CourseModeFactory
from third_party_auth.tests.testutil import ThirdPartyAuthTestMixin
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
# This relies on third party auth being enabled and configured # This relies on third party auth being enabled in the test
# in the test settings. See the setting `THIRD_PARTY_AUTH` # settings with the feature flag `ENABLE_THIRD_PARTY_AUTH`
# and the feature flag `ENABLE_THIRD_PARTY_AUTH`
THIRD_PARTY_AUTH_BACKENDS = ["google-oauth2", "facebook"] THIRD_PARTY_AUTH_BACKENDS = ["google-oauth2", "facebook"]
THIRD_PARTY_AUTH_PROVIDERS = ["Google", "Facebook"] THIRD_PARTY_AUTH_PROVIDERS = ["Google", "Facebook"]
...@@ -40,7 +40,7 @@ def _finish_auth_url(params): ...@@ -40,7 +40,7 @@ def _finish_auth_url(params):
@ddt.ddt @ddt.ddt
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
class LoginFormTest(UrlResetMixin, ModuleStoreTestCase): class LoginFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleStoreTestCase):
"""Test rendering of the login form. """ """Test rendering of the login form. """
@patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False}) @patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False})
def setUp(self): def setUp(self):
...@@ -50,6 +50,8 @@ class LoginFormTest(UrlResetMixin, ModuleStoreTestCase): ...@@ -50,6 +50,8 @@ class LoginFormTest(UrlResetMixin, ModuleStoreTestCase):
self.course = CourseFactory.create() self.course = CourseFactory.create()
self.course_id = unicode(self.course.id) self.course_id = unicode(self.course.id)
self.courseware_url = reverse("courseware", args=[self.course_id]) self.courseware_url = reverse("courseware", args=[self.course_id])
self.configure_google_provider(enabled=True)
self.configure_facebook_provider(enabled=True)
@patch.dict(settings.FEATURES, {"ENABLE_THIRD_PARTY_AUTH": False}) @patch.dict(settings.FEATURES, {"ENABLE_THIRD_PARTY_AUTH": False})
@ddt.data(THIRD_PARTY_AUTH_PROVIDERS) @ddt.data(THIRD_PARTY_AUTH_PROVIDERS)
...@@ -148,7 +150,7 @@ class LoginFormTest(UrlResetMixin, ModuleStoreTestCase): ...@@ -148,7 +150,7 @@ class LoginFormTest(UrlResetMixin, ModuleStoreTestCase):
@ddt.ddt @ddt.ddt
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
class RegisterFormTest(UrlResetMixin, ModuleStoreTestCase): class RegisterFormTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleStoreTestCase):
"""Test rendering of the registration form. """ """Test rendering of the registration form. """
@patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False}) @patch.dict(settings.FEATURES, {"ENABLE_COMBINED_LOGIN_REGISTRATION": False})
def setUp(self): def setUp(self):
...@@ -157,6 +159,8 @@ class RegisterFormTest(UrlResetMixin, ModuleStoreTestCase): ...@@ -157,6 +159,8 @@ class RegisterFormTest(UrlResetMixin, ModuleStoreTestCase):
self.url = reverse("register_user") self.url = reverse("register_user")
self.course = CourseFactory.create() self.course = CourseFactory.create()
self.course_id = unicode(self.course.id) self.course_id = unicode(self.course.id)
self.configure_google_provider(enabled=True)
self.configure_facebook_provider(enabled=True)
@patch.dict(settings.FEATURES, {"ENABLE_THIRD_PARTY_AUTH": False}) @patch.dict(settings.FEATURES, {"ENABLE_THIRD_PARTY_AUTH": False})
@ddt.data(*THIRD_PARTY_AUTH_PROVIDERS) @ddt.data(*THIRD_PARTY_AUTH_PROVIDERS)
......
...@@ -427,7 +427,7 @@ def register_user(request, extra_context=None): ...@@ -427,7 +427,7 @@ def register_user(request, extra_context=None):
current_provider = provider.Registry.get_from_pipeline(running_pipeline) current_provider = provider.Registry.get_from_pipeline(running_pipeline)
overrides = current_provider.get_register_form_data(running_pipeline.get('kwargs')) overrides = current_provider.get_register_form_data(running_pipeline.get('kwargs'))
overrides['running_pipeline'] = running_pipeline overrides['running_pipeline'] = running_pipeline
overrides['selected_provider'] = current_provider.NAME overrides['selected_provider'] = current_provider.name
context.update(overrides) context.update(overrides)
return render_to_response('register.html', context) return render_to_response('register.html', context)
...@@ -964,12 +964,12 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un ...@@ -964,12 +964,12 @@ def login_user(request, error=""): # pylint: disable-msg=too-many-statements,un
username=username, backend_name=backend_name)) username=username, backend_name=backend_name))
return HttpResponse( return HttpResponse(
_("You've successfully logged into your {provider_name} account, but this account isn't linked with an {platform_name} account yet.").format( _("You've successfully logged into your {provider_name} account, but this account isn't linked with an {platform_name} account yet.").format(
platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.NAME platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.name
) )
+ "<br/><br/>" + + "<br/><br/>" +
_("Use your {platform_name} username and password to log into {platform_name} below, " _("Use your {platform_name} username and password to log into {platform_name} below, "
"and then link your {platform_name} account with {provider_name} from your dashboard.").format( "and then link your {platform_name} account with {provider_name} from your dashboard.").format(
platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.NAME platform_name=settings.PLATFORM_NAME, provider_name=requested_provider.name
) )
+ "<br/><br/>" + + "<br/><br/>" +
_("If you don't have an {platform_name} account yet, click <strong>Register Now</strong> at the top of the page.").format( _("If you don't have an {platform_name} account yet, click <strong>Register Now</strong> at the top of the page.").format(
...@@ -1511,7 +1511,7 @@ def create_account_with_params(request, params): ...@@ -1511,7 +1511,7 @@ def create_account_with_params(request, params):
if third_party_auth.is_enabled() and pipeline.running(request): if third_party_auth.is_enabled() and pipeline.running(request):
running_pipeline = pipeline.get(request) running_pipeline = pipeline.get(request)
current_provider = provider.Registry.get_from_pipeline(running_pipeline) current_provider = provider.Registry.get_from_pipeline(running_pipeline)
provider_name = current_provider.NAME provider_name = current_provider.name
analytics.track( analytics.track(
user.id, user.id,
......
# -*- coding: utf-8 -*-
"""
Admin site configuration for third party authentication
"""
from django.contrib import admin
from config_models.admin import ConfigurationModelAdmin, KeyedConfigurationModelAdmin
from .models import OAuth2ProviderConfig, SAMLProviderConfig, SAMLConfiguration, SAMLProviderData
admin.site.register(OAuth2ProviderConfig, KeyedConfigurationModelAdmin)
class SAMLProviderConfigAdmin(KeyedConfigurationModelAdmin):
""" Django Admin class for SAMLProviderConfig """
def get_list_display(self, request):
""" Don't show every single field in the admin change list """
return (
'name', 'enabled', 'backend_name', 'entity_id', 'metadata_source',
'has_data', 'icon_class', 'change_date', 'changed_by', 'edit_link'
)
def has_data(self, inst):
""" Do we have cached metadata for this SAML provider? """
if not inst.is_active:
return None # N/A
data = SAMLProviderData.current(inst.entity_id)
return bool(data and data.is_valid())
has_data.short_description = u'Metadata Ready'
has_data.boolean = True
admin.site.register(SAMLProviderConfig, SAMLProviderConfigAdmin)
class SAMLConfigurationAdmin(ConfigurationModelAdmin):
""" Django Admin class for SAMLConfiguration """
def get_list_display(self, request):
""" Shorten the public/private keys in the change view """
return (
'change_date', 'changed_by', 'enabled', 'entity_id',
'org_info_str', 'key_summary',
)
def key_summary(self, inst):
""" Short summary of the key pairs configured """
if not inst.public_key or not inst.private_key:
return u'<em>Key pair incomplete/missing</em>'
pub1, pub2 = inst.public_key[0:10], inst.public_key[-10:]
priv1, priv2 = inst.private_key[0:10], inst.private_key[-10:]
return u'Public: {}…{}<br>Private: {}…{}'.format(pub1, pub2, priv1, priv2)
key_summary.allow_tags = True
admin.site.register(SAMLConfiguration, SAMLConfigurationAdmin)
class SAMLProviderDataAdmin(admin.ModelAdmin):
""" Django Admin class for SAMLProviderData """
list_display = ('entity_id', 'is_valid', 'fetched_at', 'expires_at', 'sso_url')
readonly_fields = ('is_valid', )
def get_readonly_fields(self, request, obj=None):
if obj: # editing an existing object
return self.model._meta.get_all_field_names() # pylint: disable=protected-access
return self.readonly_fields
admin.site.register(SAMLProviderData, SAMLProviderDataAdmin)
""" """
DummyProvider: A fake Third Party Auth provider for testing & development purposes. DummyBackend: A fake Third Party Auth provider for testing & development purposes.
""" """
from social.backends.base import BaseAuth from social.backends.oauth import BaseOAuth2
from social.exceptions import AuthFailed from social.exceptions import AuthFailed
from .provider import BaseProvider
class DummyBackend(BaseOAuth2): # pylint: disable=abstract-method
class DummyBackend(BaseAuth): # pylint: disable=abstract-method
""" """
python-social-auth backend that doesn't actually go to any third party site python-social-auth backend that doesn't actually go to any third party site
""" """
...@@ -47,12 +45,3 @@ class DummyBackend(BaseAuth): # pylint: disable=abstract-method ...@@ -47,12 +45,3 @@ class DummyBackend(BaseAuth): # pylint: disable=abstract-method
kwargs.update({'response': response, 'backend': self}) kwargs.update({'response': response, 'backend': self})
return self.strategy.authenticate(*args, **kwargs) return self.strategy.authenticate(*args, **kwargs)
class DummyProvider(BaseProvider):
""" Dummy Provider for testing and development """
BACKEND_CLASS = DummyBackend
ICON_CLASS = 'fa-cube'
NAME = 'Dummy'
SETTINGS = {}
# -*- coding: utf-8 -*-
"""
Management commands for third_party_auth
"""
import datetime
import dateutil.parser
from django.core.management.base import BaseCommand, CommandError
from lxml import etree
import requests
from onelogin.saml2.utils import OneLogin_Saml2_Utils
from third_party_auth.models import SAMLConfiguration, SAMLProviderConfig, SAMLProviderData
#pylint: disable=superfluous-parens,no-member
class MetadataParseError(Exception):
""" An error occurred while parsing the SAML metadata from an IdP """
pass
class Command(BaseCommand):
""" manage.py commands to manage SAML/Shibboleth SSO """
help = '''Configure/maintain/update SAML-based SSO'''
def handle(self, *args, **options):
if len(args) != 1:
raise CommandError("saml requires one argument: pull")
if not SAMLConfiguration.is_enabled():
self.stdout.write("Warning: SAML support is disabled via SAMLConfiguration.\n")
subcommand = args[0]
if subcommand == "pull":
self.cmd_pull()
else:
raise CommandError("Unknown argment: {}".format(subcommand))
@staticmethod
def tag_name(tag_name):
""" Get the namespaced-qualified name for an XML tag """
return '{urn:oasis:names:tc:SAML:2.0:metadata}' + tag_name
def cmd_pull(self):
""" Fetch the metadata for each provider and update the DB """
# First make a list of all the metadata XML URLs:
url_map = {}
for idp_slug in SAMLProviderConfig.key_values('idp_slug', flat=True):
config = SAMLProviderConfig.current(idp_slug)
if not config.enabled:
continue
url = config.metadata_source
if url not in url_map:
url_map[url] = []
if config.entity_id not in url_map[url]:
url_map[url].append(config.entity_id)
# Now fetch the metadata:
for url, entity_ids in url_map.items():
try:
self.stdout.write("\n→ Fetching {}\n".format(url))
if not url.lower().startswith('https'):
self.stdout.write("→ WARNING: This URL is not secure! It should use HTTPS.\n")
response = requests.get(url, verify=True) # May raise HTTPError or SSLError
response.raise_for_status() # May raise an HTTPError
try:
parser = etree.XMLParser(remove_comments=True)
xml = etree.fromstring(response.text, parser)
except etree.XMLSyntaxError:
raise
# TODO: Can use OneLogin_Saml2_Utils to validate signed XML if anyone is using that
for entity_id in entity_ids:
self.stdout.write("→ Processing IdP with entityID {}\n".format(entity_id))
public_key, sso_url, expires_at = self._parse_metadata_xml(xml, entity_id)
self._update_data(entity_id, public_key, sso_url, expires_at)
except Exception as err: # pylint: disable=broad-except
self.stderr.write("→ ERROR: {}\n\n".format(err.message))
@classmethod
def _parse_metadata_xml(cls, xml, entity_id):
"""
Given an XML document containing SAML 2.0 metadata, parse it and return a tuple of
(public_key, sso_url, expires_at) for the specified entityID.
Raises MetadataParseError if anything is wrong.
"""
if xml.tag == cls.tag_name('EntityDescriptor'):
entity_desc = xml
else:
if xml.tag != cls.tag_name('EntitiesDescriptor'):
raise MetadataParseError("Expected root element to be <EntitiesDescriptor>, not {}".format(xml.tag))
entity_desc = xml.find(".//{}[@entityID='{}']".format(cls.tag_name('EntityDescriptor'), entity_id))
if not entity_desc:
raise MetadataParseError("Can't find EntityDescriptor for entityID {}".format(entity_id))
expires_at = None
if "validUntil" in xml.attrib:
expires_at = dateutil.parser.parse(xml.attrib["validUntil"])
if "cacheDuration" in xml.attrib:
cache_expires = OneLogin_Saml2_Utils.parse_duration(xml.attrib["cacheDuration"])
if expires_at is None or cache_expires < expires_at:
expires_at = cache_expires
sso_desc = entity_desc.find(cls.tag_name("IDPSSODescriptor"))
if not sso_desc:
raise MetadataParseError("IDPSSODescriptor missing")
if 'urn:oasis:names:tc:SAML:2.0:protocol' not in sso_desc.get("protocolSupportEnumeration"):
raise MetadataParseError("This IdP does not support SAML 2.0")
# Now we just need to get the public_key and sso_url
public_key = sso_desc.findtext("./{}//{}".format(
cls.tag_name("KeyDescriptor"), "{http://www.w3.org/2000/09/xmldsig#}X509Certificate"
))
if not public_key:
raise MetadataParseError("Public Key missing. Expected an <X509Certificate>")
public_key = public_key.replace(" ", "")
binding_elements = sso_desc.iterfind("./{}".format(cls.tag_name("SingleSignOnService")))
sso_bindings = {element.get('Binding'): element.get('Location') for element in binding_elements}
try:
# The only binding supported by python-saml and python-social-auth is HTTP-Redirect:
sso_url = sso_bindings['urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect']
except KeyError:
raise MetadataParseError("Unable to find SSO URL with HTTP-Redirect binding.")
return public_key, sso_url, expires_at
def _update_data(self, entity_id, public_key, sso_url, expires_at):
"""
Update/Create the SAMLProviderData for the given entity ID.
"""
data_obj = SAMLProviderData.current(entity_id)
fetched_at = datetime.datetime.now()
if data_obj and (data_obj.public_key == public_key and data_obj.sso_url == sso_url):
data_obj.expires_at = expires_at
data_obj.fetched_at = fetched_at
data_obj.save()
self.stdout.write("→ Updated existing SAMLProviderData. Nothing has changed.\n")
else:
SAMLProviderData.objects.create(
entity_id=entity_id,
fetched_at=fetched_at,
expires_at=expires_at,
sso_url=sso_url,
public_key=public_key,
)
self.stdout.write("→ Created new record for SAMLProviderData\n")
...@@ -209,7 +209,7 @@ class ProviderUserState(object): ...@@ -209,7 +209,7 @@ class ProviderUserState(object):
def get_unlink_form_name(self): def get_unlink_form_name(self):
"""Gets the name used in HTML forms that unlink a provider account.""" """Gets the name used in HTML forms that unlink a provider account."""
return self.provider.NAME + '_unlink_form' return self.provider.provider_id + '_unlink_form'
def get(request): def get(request):
...@@ -239,7 +239,7 @@ def get_authenticated_user(auth_provider, username, uid): ...@@ -239,7 +239,7 @@ def get_authenticated_user(auth_provider, username, uid):
user has no social auth associated with the given backend. user has no social auth associated with the given backend.
AssertionError: if the user is not authenticated. AssertionError: if the user is not authenticated.
""" """
match = models.DjangoStorage.user.get_social_auth(provider=auth_provider.BACKEND_CLASS.name, uid=uid) match = models.DjangoStorage.user.get_social_auth(provider=auth_provider.backend_name, uid=uid)
if not match or match.user.username != username: if not match or match.user.username != username:
raise User.DoesNotExist raise User.DoesNotExist
...@@ -249,12 +249,12 @@ def get_authenticated_user(auth_provider, username, uid): ...@@ -249,12 +249,12 @@ def get_authenticated_user(auth_provider, username, uid):
return user return user
def _get_enabled_provider_by_name(provider_name): def _get_enabled_provider(provider_id):
"""Gets an enabled provider by its NAME member or throws.""" """Gets an enabled provider by its provider_id member or throws."""
enabled_provider = provider.Registry.get(provider_name) enabled_provider = provider.Registry.get(provider_id)
if not enabled_provider: if not enabled_provider:
raise ValueError('Provider %s not enabled' % provider_name) raise ValueError('Provider %s not enabled' % provider_id)
return enabled_provider return enabled_provider
...@@ -301,11 +301,11 @@ def get_complete_url(backend_name): ...@@ -301,11 +301,11 @@ def get_complete_url(backend_name):
return _get_url('social:complete', backend_name) return _get_url('social:complete', backend_name)
def get_disconnect_url(provider_name, association_id): def get_disconnect_url(provider_id, association_id):
"""Gets URL for the endpoint that starts the disconnect pipeline. """Gets URL for the endpoint that starts the disconnect pipeline.
Args: Args:
provider_name: string. Name of the provider.BaseProvider child you want provider_id: string identifier of the models.ProviderConfig child you want
to disconnect from. to disconnect from.
association_id: int. Optional ID of a specific row in the UserSocialAuth association_id: int. Optional ID of a specific row in the UserSocialAuth
table to disconnect (useful if multiple providers use a common backend) table to disconnect (useful if multiple providers use a common backend)
...@@ -314,21 +314,21 @@ def get_disconnect_url(provider_name, association_id): ...@@ -314,21 +314,21 @@ def get_disconnect_url(provider_name, association_id):
String. URL that starts the disconnection pipeline. String. URL that starts the disconnection pipeline.
Raises: Raises:
ValueError: if no provider is enabled with the given name. ValueError: if no provider is enabled with the given ID.
""" """
backend_name = _get_enabled_provider_by_name(provider_name).BACKEND_CLASS.name backend_name = _get_enabled_provider(provider_id).backend_name
if association_id: if association_id:
return _get_url('social:disconnect_individual', backend_name, url_params={'association_id': association_id}) return _get_url('social:disconnect_individual', backend_name, url_params={'association_id': association_id})
else: else:
return _get_url('social:disconnect', backend_name) return _get_url('social:disconnect', backend_name)
def get_login_url(provider_name, auth_entry, redirect_url=None): def get_login_url(provider_id, auth_entry, redirect_url=None):
"""Gets the login URL for the endpoint that kicks off auth with a provider. """Gets the login URL for the endpoint that kicks off auth with a provider.
Args: Args:
provider_name: string. The name of the provider.Provider that has been provider_id: string identifier of the models.ProviderConfig child you want
enabled. to disconnect from.
auth_entry: string. Query argument specifying the desired entry point auth_entry: string. Query argument specifying the desired entry point
for the auth pipeline. Used by the pipeline for later branching. for the auth pipeline. Used by the pipeline for later branching.
Must be one of _AUTH_ENTRY_CHOICES. Must be one of _AUTH_ENTRY_CHOICES.
...@@ -341,13 +341,13 @@ def get_login_url(provider_name, auth_entry, redirect_url=None): ...@@ -341,13 +341,13 @@ def get_login_url(provider_name, auth_entry, redirect_url=None):
String. URL that starts the auth pipeline for a provider. String. URL that starts the auth pipeline for a provider.
Raises: Raises:
ValueError: if no provider is enabled with the given provider_name. ValueError: if no provider is enabled with the given provider_id.
""" """
assert auth_entry in _AUTH_ENTRY_CHOICES assert auth_entry in _AUTH_ENTRY_CHOICES
enabled_provider = _get_enabled_provider_by_name(provider_name) enabled_provider = _get_enabled_provider(provider_id)
return _get_url( return _get_url(
'social:begin', 'social:begin',
enabled_provider.BACKEND_CLASS.name, enabled_provider.backend_name,
auth_entry=auth_entry, auth_entry=auth_entry,
redirect_url=redirect_url, redirect_url=redirect_url,
extra_params=enabled_provider.get_url_params(), extra_params=enabled_provider.get_url_params(),
......
""" """
Slightly customized python-social-auth backend for SAML 2.0 support Slightly customized python-social-auth backend for SAML 2.0 support
""" """
import logging
from social.backends.saml import SAMLAuth, OID_EDU_PERSON_ENTITLEMENT
from social.exceptions import AuthForbidden
from social.backends.saml import SAMLIdentityProvider, SAMLAuth log = logging.getLogger(__name__)
class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method
...@@ -14,8 +17,33 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method ...@@ -14,8 +17,33 @@ class SAMLAuthBackend(SAMLAuth): # pylint: disable=abstract-method
def get_idp(self, idp_name): def get_idp(self, idp_name):
""" Given the name of an IdP, get a SAMLIdentityProvider instance """ """ Given the name of an IdP, get a SAMLIdentityProvider instance """
from .provider import Registry # Import here to avoid circular import from .models import SAMLProviderConfig
for provider in Registry.enabled(): return SAMLProviderConfig.current(idp_name).get_config()
if issubclass(provider.BACKEND_CLASS, SAMLAuth) and provider.IDP["id"] == idp_name:
return SAMLIdentityProvider(idp_name, **provider.IDP) def setting(self, name, default=None):
raise KeyError("SAML IdP {} not found.".format(idp_name)) """ Get a setting, from SAMLConfiguration """
if not hasattr(self, '_config'):
from .models import SAMLConfiguration
self._config = SAMLConfiguration.current() # pylint: disable=attribute-defined-outside-init
if not self._config.enabled:
from django.core.exceptions import ImproperlyConfigured
raise ImproperlyConfigured("SAML Authentication is not enabled.")
try:
return self._config.get_setting(name)
except KeyError:
return self.strategy.setting(name, default)
def _check_entitlements(self, idp, attributes):
"""
Check if we require the presence of any specific eduPersonEntitlement.
raise AuthForbidden if the user should not be authenticated, or do nothing
to allow the login pipeline to continue.
"""
if "requiredEntitlements" in idp.conf:
entitlements = attributes.get(OID_EDU_PERSON_ENTITLEMENT, [])
for expected in idp.conf['requiredEntitlements']:
if expected not in entitlements:
log.warning(
"SAML user from IdP %s rejected due to missing eduPersonEntitlement %s", idp.name, expected)
raise AuthForbidden(self)
"""Settings for the third-party auth module. """Settings for the third-party auth module.
Defers configuration of settings so we can inspect the provider registry and
create settings placeholders for only those values actually needed by a given
deployment. Required by Django; consequently, this file must not invoke the
Django armature.
The flow for settings registration is: The flow for settings registration is:
The base settings file contains a boolean, ENABLE_THIRD_PARTY_AUTH, indicating The base settings file contains a boolean, ENABLE_THIRD_PARTY_AUTH, indicating
whether this module is enabled. Ancillary settings files (aws.py, dev.py) put whether this module is enabled. startup.py probes the ENABLE_THIRD_PARTY_AUTH.
options in THIRD_PARTY_SETTINGS. startup.py probes the ENABLE_THIRD_PARTY_AUTH.
If true, it: If true, it:
a) loads this module. a) loads this module.
b) calls apply_settings(), passing in settings.THIRD_PARTY_AUTH. b) calls apply_settings(), passing in the Django settings
THIRD_PARTY AUTH is a dict of the form
'THIRD_PARTY_AUTH': {
'<PROVIDER_NAME>': {
'<PROVIDER_SETTING_NAME>': '<PROVIDER_SETTING_VALUE>',
[...]
},
[...]
}
If you are using a dev settings file, your settings dict starts at the
level of <PROVIDER_NAME> and is a map of provider name string to
settings dict. If you are using an auth.json file, it should contain a
THIRD_PARTY_AUTH entry as above.
c) apply_settings() builds a list of <PROVIDER_NAMES>. These are the
enabled third party auth providers for the deployment. These are enabled
in provider.Registry, the canonical list of enabled providers.
d) then, it sets global, provider-independent settings.
e) then, it sets provider-specific settings. For each enabled provider, we
read its SETTINGS member. These are merged onto the Django settings
object. In most cases these are stubs and the real values are set from
THIRD_PARTY_AUTH. All values that are set from this dict must first be
initialized from SETTINGS. This allows us to validate the dict and
ensure that the values match expected configuration options on the
provider.
f) finally, the (key, value) pairs from the dict file are merged onto the
django settings object.
""" """
from . import provider
_FIELDS_STORED_IN_SESSION = ['auth_entry', 'next'] _FIELDS_STORED_IN_SESSION = ['auth_entry', 'next']
_MIDDLEWARE_CLASSES = ( _MIDDLEWARE_CLASSES = (
'third_party_auth.middleware.ExceptionMiddleware', 'third_party_auth.middleware.ExceptionMiddleware',
...@@ -53,25 +17,7 @@ _MIDDLEWARE_CLASSES = ( ...@@ -53,25 +17,7 @@ _MIDDLEWARE_CLASSES = (
_SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard' _SOCIAL_AUTH_LOGIN_REDIRECT_URL = '/dashboard'
def _merge_auth_info(django_settings, auth_info): def apply_settings(django_settings):
"""Merge auth_info dict onto django_settings module."""
enabled_provider_names = []
to_merge = []
for provider_name, provider_dict in auth_info.items():
enabled_provider_names.append(provider_name)
# Merge iff all settings have been intialized.
for key in provider_dict:
if key not in dir(django_settings):
raise ValueError('Auth setting %s not initialized' % key)
to_merge.append(provider_dict)
for passed_validation in to_merge:
for key, value in passed_validation.iteritems():
setattr(django_settings, key, value)
def _set_global_settings(django_settings):
"""Set provider-independent settings.""" """Set provider-independent settings."""
# Whitelisted URL query parameters retrained in the pipeline session. # Whitelisted URL query parameters retrained in the pipeline session.
...@@ -115,6 +61,9 @@ def _set_global_settings(django_settings): ...@@ -115,6 +61,9 @@ def _set_global_settings(django_settings):
'third_party_auth.pipeline.login_analytics', 'third_party_auth.pipeline.login_analytics',
) )
# Required so that we can use unmodified PSA OAuth2 backends:
django_settings.SOCIAL_AUTH_STRATEGY = 'third_party_auth.strategy.ConfigurationModelStrategy'
# We let the user specify their email address during signup. # We let the user specify their email address during signup.
django_settings.SOCIAL_AUTH_PROTECTED_USER_FIELDS = ['email'] django_settings.SOCIAL_AUTH_PROTECTED_USER_FIELDS = ['email']
...@@ -136,30 +85,3 @@ def _set_global_settings(django_settings): ...@@ -136,30 +85,3 @@ def _set_global_settings(django_settings):
'social.apps.django_app.context_processors.backends', 'social.apps.django_app.context_processors.backends',
'social.apps.django_app.context_processors.login_redirect', 'social.apps.django_app.context_processors.login_redirect',
) )
def _set_provider_settings(django_settings, enabled_providers, auth_info):
"""Sets provider-specific settings."""
# Must prepend here so we get called first.
django_settings.AUTHENTICATION_BACKENDS = (
tuple(enabled_provider.get_authentication_backend() for enabled_provider in enabled_providers) +
django_settings.AUTHENTICATION_BACKENDS)
# Merge settings from provider classes, and configure all placeholders.
for enabled_provider in enabled_providers:
enabled_provider.merge_onto(django_settings)
# Merge settings from <deployment>.auth.json, overwriting placeholders.
_merge_auth_info(django_settings, auth_info)
def apply_settings(auth_info, django_settings):
"""Applies settings from auth_info dict to django_settings module."""
if django_settings.FEATURES.get('ENABLE_DUMMY_THIRD_PARTY_AUTH_PROVIDER'):
# The Dummy provider is handy for testing and development.
from .dummy import DummyProvider # pylint: disable=unused-variable
provider_names = auth_info.keys()
provider.Registry.configure_once(provider_names)
enabled_providers = provider.Registry.enabled()
_set_global_settings(django_settings)
_set_provider_settings(django_settings, enabled_providers, auth_info)
"""
A custom Strategy for python-social-auth that allows us to fetch configuration from
ConfigurationModels rather than django.settings
"""
from .models import OAuth2ProviderConfig
from social.backends.oauth import BaseOAuth2
from social.strategies.django_strategy import DjangoStrategy
class ConfigurationModelStrategy(DjangoStrategy):
"""
A DjangoStrategy customized to load settings from ConfigurationModels
for upstream python-social-auth backends that we cannot otherwise modify.
"""
def setting(self, name, default=None, backend=None):
"""
Load the setting from a ConfigurationModel if possible, or fall back to the normal
Django settings lookup.
BaseOAuth2 subclasses will call this method for every setting they want to look up.
SAMLAuthBackend subclasses will call this method only after first checking if the
setting 'name' is configured via SAMLProviderConfig.
"""
if isinstance(backend, BaseOAuth2):
provider_config = OAuth2ProviderConfig.current(backend.name)
if not provider_config.enabled:
raise Exception("Can't fetch setting of a disabled backend/provider.")
try:
return provider_config.get_setting(name)
except KeyError:
pass
# At this point, we know 'name' is not set in a [OAuth2|SAML]ProviderConfig row.
# It's probably a global Django setting like 'FIELDS_STORED_IN_SESSION':
return super(ConfigurationModelStrategy, self).setting(name, default, backend)
...@@ -7,11 +7,14 @@ from third_party_auth.tests.specs import base ...@@ -7,11 +7,14 @@ from third_party_auth.tests.specs import base
class GoogleOauth2IntegrationTest(base.Oauth2IntegrationTest): class GoogleOauth2IntegrationTest(base.Oauth2IntegrationTest):
"""Integration tests for provider.GoogleOauth2.""" """Integration tests for provider.GoogleOauth2."""
PROVIDER_CLASS = provider.GoogleOauth2 def setUp(self):
PROVIDER_SETTINGS = { super(GoogleOauth2IntegrationTest, self).setUp()
'SOCIAL_AUTH_GOOGLE_OAUTH2_KEY': 'google_oauth2_key', self.provider = self.configure_google_provider(
'SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET': 'google_oauth2_secret', enabled=True,
} key='google_oauth2_key',
secret='google_oauth2_secret',
)
TOKEN_RESPONSE_DATA = { TOKEN_RESPONSE_DATA = {
'access_token': 'access_token_value', 'access_token': 'access_token_value',
'expires_in': 'expires_in_value', 'expires_in': 'expires_in_value',
......
...@@ -7,11 +7,14 @@ from third_party_auth.tests.specs import base ...@@ -7,11 +7,14 @@ from third_party_auth.tests.specs import base
class LinkedInOauth2IntegrationTest(base.Oauth2IntegrationTest): class LinkedInOauth2IntegrationTest(base.Oauth2IntegrationTest):
"""Integration tests for provider.LinkedInOauth2.""" """Integration tests for provider.LinkedInOauth2."""
PROVIDER_CLASS = provider.LinkedInOauth2 def setUp(self):
PROVIDER_SETTINGS = { super(LinkedInOauth2IntegrationTest, self).setUp()
'SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY': 'linkedin_oauth2_key', self.provider = self.configure_linkedin_provider(
'SOCIAL_AUTH_LINKEDIN_OAUTH2_SECRET': 'linkedin_oauth2_secret', enabled=True,
} key='linkedin_oauth2_key',
secret='linkedin_oauth2_secret',
)
TOKEN_RESPONSE_DATA = { TOKEN_RESPONSE_DATA = {
'access_token': 'access_token_value', 'access_token': 'access_token_value',
'expires_in': 'expires_in_value', 'expires_in': 'expires_in_value',
......
...@@ -4,6 +4,7 @@ import random ...@@ -4,6 +4,7 @@ import random
from third_party_auth import pipeline, provider from third_party_auth import pipeline, provider
from third_party_auth.tests import testutil from third_party_auth.tests import testutil
import unittest
# Allow tests access to protected methods (or module-protected methods) under # Allow tests access to protected methods (or module-protected methods) under
...@@ -34,9 +35,11 @@ class MakeRandomPasswordTest(testutil.TestCase): ...@@ -34,9 +35,11 @@ class MakeRandomPasswordTest(testutil.TestCase):
self.assertEqual(expected, pipeline.make_random_password(choice_fn=random_instance.choice)) self.assertEqual(expected, pipeline.make_random_password(choice_fn=random_instance.choice))
@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, 'third_party_auth not enabled')
class ProviderUserStateTestCase(testutil.TestCase): class ProviderUserStateTestCase(testutil.TestCase):
"""Tests ProviderUserState behavior.""" """Tests ProviderUserState behavior."""
def test_get_unlink_form_name(self): def test_get_unlink_form_name(self):
state = pipeline.ProviderUserState(provider.GoogleOauth2, object(), 1000) google_provider = self.configure_google_provider(enabled=True)
self.assertEqual(provider.GoogleOauth2.NAME + '_unlink_form', state.get_unlink_form_name()) state = pipeline.ProviderUserState(google_provider, object(), 1000)
self.assertEqual(google_provider.provider_id + '_unlink_form', state.get_unlink_form_name())
"""Unit tests for provider.py.""" """Unit tests for provider.py."""
from mock import Mock from mock import Mock, patch
from third_party_auth import provider from third_party_auth import provider
from third_party_auth.tests import testutil from third_party_auth.tests import testutil
import unittest
@unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, 'third_party_auth not enabled')
class RegistryTest(testutil.TestCase): class RegistryTest(testutil.TestCase):
"""Tests registry discovery and operation.""" """Tests registry discovery and operation."""
# Allow access to protected methods (or module-protected methods) under
# test. pylint: disable-msg=protected-access
def test_calling_configure_once_twice_raises_value_error(self):
provider.Registry.configure_once([provider.GoogleOauth2.NAME])
with self.assertRaisesRegexp(ValueError, '^.*already configured$'):
provider.Registry.configure_once([provider.GoogleOauth2.NAME])
def test_configure_once_adds_gettable_providers(self): def test_configure_once_adds_gettable_providers(self):
provider.Registry.configure_once([provider.GoogleOauth2.NAME]) facebook_provider = self.configure_facebook_provider(enabled=True)
self.assertIs(provider.GoogleOauth2, provider.Registry.get(provider.GoogleOauth2.NAME)) # pylint: disable=no-member
self.assertEqual(facebook_provider.id, provider.Registry.get(facebook_provider.provider_id).id)
def test_configuring_provider_with_no_implementation_raises_value_error(self):
with self.assertRaisesRegexp(ValueError, '^.*no_implementation$'): def test_no_providers_by_default(self):
provider.Registry.configure_once(['no_implementation']) enabled_providers = provider.Registry.enabled()
self.assertEqual(len(enabled_providers), 0, "By default, no providers are enabled.")
def test_configuring_single_provider_twice_raises_value_error(self):
provider.Registry._enable(provider.GoogleOauth2) def test_runtime_configuration(self):
self.configure_google_provider(enabled=True)
with self.assertRaisesRegexp(ValueError, '^.*already enabled'): enabled_providers = provider.Registry.enabled()
provider.Registry.configure_once([provider.GoogleOauth2.NAME]) self.assertEqual(len(enabled_providers), 1)
self.assertEqual(enabled_providers[0].name, "Google")
def test_custom_provider_can_be_enabled(self): self.assertEqual(enabled_providers[0].secret, "opensesame")
name = 'CustomProvider'
self.configure_google_provider(enabled=False)
with self.assertRaisesRegexp(ValueError, '^No implementation.*$'): enabled_providers = provider.Registry.enabled()
provider.Registry.configure_once([name]) self.assertEqual(len(enabled_providers), 0)
class CustomProvider(provider.BaseProvider): self.configure_google_provider(enabled=True, secret="alohomora")
"""Custom class to ensure BaseProvider children outside provider can be enabled.""" enabled_providers = provider.Registry.enabled()
self.assertEqual(len(enabled_providers), 1)
NAME = name self.assertEqual(enabled_providers[0].secret, "alohomora")
provider.Registry._reset() def test_cannot_load_arbitrary_backends(self):
provider.Registry.configure_once([CustomProvider.NAME]) """ Test that only backend_names listed in settings.AUTHENTICATION_BACKENDS can be used """
self.assertEqual([CustomProvider], provider.Registry.enabled()) self.configure_oauth_provider(enabled=True, name="Disallowed", backend_name="disallowed")
self.enable_saml()
def test_enabled_raises_runtime_error_if_not_configured(self): self.configure_saml_provider(enabled=True, name="Disallowed", idp_slug="test", backend_name="disallowed")
with self.assertRaisesRegexp(RuntimeError, '^.*not configured$'): self.assertEqual(len(provider.Registry.enabled()), 0)
provider.Registry.enabled()
def test_enabled_returns_list_of_enabled_providers_sorted_by_name(self): def test_enabled_returns_list_of_enabled_providers_sorted_by_name(self):
all_providers = provider.Registry._get_all() provider_names = ["Stack Overflow", "Google", "LinkedIn", "GitHub"]
provider.Registry.configure_once(all_providers.keys()) backend_names = []
self.assertEqual( for name in provider_names:
sorted(all_providers.values(), key=lambda provider: provider.NAME), provider.Registry.enabled()) backend_name = name.lower().replace(' ', '')
backend_names.append(backend_name)
self.configure_oauth_provider(enabled=True, name=name, backend_name=backend_name)
def test_get_raises_runtime_error_if_not_configured(self): with patch('third_party_auth.provider._PSA_OAUTH2_BACKENDS', backend_names):
with self.assertRaisesRegexp(RuntimeError, '^.*not configured$'): self.assertEqual(sorted(provider_names), [prov.name for prov in provider.Registry.enabled()])
provider.Registry.get('anything')
def test_get_returns_enabled_provider(self): def test_get_returns_enabled_provider(self):
provider.Registry.configure_once([provider.GoogleOauth2.NAME]) google_provider = self.configure_google_provider(enabled=True)
self.assertIs(provider.GoogleOauth2, provider.Registry.get(provider.GoogleOauth2.NAME)) # pylint: disable=no-member
self.assertEqual(google_provider.id, provider.Registry.get(google_provider.provider_id).id)
def test_get_returns_none_if_provider_not_enabled(self): def test_get_returns_none_if_provider_not_enabled(self):
provider.Registry.configure_once([]) linkedin_provider_id = "oa2-linkedin-oauth2"
self.assertIsNone(provider.Registry.get(provider.LinkedInOauth2.NAME)) # At this point there should be no configuration entries at all so no providers should be enabled
self.assertEqual(provider.Registry.enabled(), [])
self.assertIsNone(provider.Registry.get(linkedin_provider_id))
# Now explicitly disabled this provider:
self.configure_linkedin_provider(enabled=False)
self.assertIsNone(provider.Registry.get(linkedin_provider_id))
self.configure_linkedin_provider(enabled=True)
self.assertEqual(provider.Registry.get(linkedin_provider_id).provider_id, linkedin_provider_id)
def test_get_from_pipeline_returns_none_if_provider_not_enabled(self): def test_get_from_pipeline_returns_none_if_provider_not_enabled(self):
provider.Registry.configure_once([]) self.assertEqual(provider.Registry.enabled(), [], "By default, no providers are enabled.")
self.assertIsNone(provider.Registry.get_from_pipeline(Mock())) self.assertIsNone(provider.Registry.get_from_pipeline(Mock()))
def test_get_enabled_by_backend_name_raises_runtime_error_if_not_configured(self):
with self.assertRaisesRegexp(RuntimeError, '^.*not configured$'):
provider.Registry.get_enabled_by_backend_name('').next()
def test_get_enabled_by_backend_name_returns_enabled_provider(self): def test_get_enabled_by_backend_name_returns_enabled_provider(self):
provider.Registry.configure_once([provider.GoogleOauth2.NAME]) google_provider = self.configure_google_provider(enabled=True)
found = list(provider.Registry.get_enabled_by_backend_name(provider.GoogleOauth2.BACKEND_CLASS.name)) found = list(provider.Registry.get_enabled_by_backend_name(google_provider.backend_name))
self.assertEqual(found, [provider.GoogleOauth2]) self.assertEqual(found, [google_provider])
def test_get_enabled_by_backend_name_returns_none_if_provider_not_enabled(self): def test_get_enabled_by_backend_name_returns_none_if_provider_not_enabled(self):
provider.Registry.configure_once([]) google_provider = self.configure_google_provider(enabled=False)
self.assertEqual( found = list(provider.Registry.get_enabled_by_backend_name(google_provider.backend_name))
[], self.assertEqual(found, [])
list(provider.Registry.get_enabled_by_backend_name(provider.GoogleOauth2.BACKEND_CLASS.name))
)
...@@ -2,6 +2,7 @@ ...@@ -2,6 +2,7 @@
from third_party_auth import provider, settings from third_party_auth import provider, settings
from third_party_auth.tests import testutil from third_party_auth.tests import testutil
import unittest
_ORIGINAL_AUTHENTICATION_BACKENDS = ('first_authentication_backend',) _ORIGINAL_AUTHENTICATION_BACKENDS = ('first_authentication_backend',)
...@@ -30,56 +31,26 @@ class SettingsUnitTest(testutil.TestCase): ...@@ -30,56 +31,26 @@ class SettingsUnitTest(testutil.TestCase):
self.settings = testutil.FakeDjangoSettings(_SETTINGS_MAP) self.settings = testutil.FakeDjangoSettings(_SETTINGS_MAP)
def test_apply_settings_adds_exception_middleware(self): def test_apply_settings_adds_exception_middleware(self):
settings.apply_settings({}, self.settings) settings.apply_settings(self.settings)
for middleware_name in settings._MIDDLEWARE_CLASSES: for middleware_name in settings._MIDDLEWARE_CLASSES:
self.assertIn(middleware_name, self.settings.MIDDLEWARE_CLASSES) self.assertIn(middleware_name, self.settings.MIDDLEWARE_CLASSES)
def test_apply_settings_adds_fields_stored_in_session(self): def test_apply_settings_adds_fields_stored_in_session(self):
settings.apply_settings({}, self.settings) settings.apply_settings(self.settings)
self.assertEqual(settings._FIELDS_STORED_IN_SESSION, self.settings.FIELDS_STORED_IN_SESSION) self.assertEqual(settings._FIELDS_STORED_IN_SESSION, self.settings.FIELDS_STORED_IN_SESSION)
def test_apply_settings_adds_third_party_auth_to_installed_apps(self): def test_apply_settings_adds_third_party_auth_to_installed_apps(self):
settings.apply_settings({}, self.settings) settings.apply_settings(self.settings)
self.assertIn('third_party_auth', self.settings.INSTALLED_APPS) self.assertIn('third_party_auth', self.settings.INSTALLED_APPS)
def test_apply_settings_enables_no_providers_and_completes_when_app_info_empty(self): @unittest.skipUnless(testutil.AUTH_FEATURE_ENABLED, 'third_party_auth not enabled')
settings.apply_settings({}, self.settings) def test_apply_settings_enables_no_providers_by_default(self):
# Providers are only enabled via ConfigurationModels in the database
settings.apply_settings(self.settings)
self.assertEqual([], provider.Registry.enabled()) self.assertEqual([], provider.Registry.enabled())
def test_apply_settings_initializes_stubs_and_merges_settings_from_auth_info(self):
for key in provider.GoogleOauth2.SETTINGS:
self.assertFalse(hasattr(self.settings, key))
auth_info = {
provider.GoogleOauth2.NAME: {
'SOCIAL_AUTH_GOOGLE_OAUTH2_KEY': 'google_oauth2_key',
},
}
settings.apply_settings(auth_info, self.settings)
self.assertEqual('google_oauth2_key', self.settings.SOCIAL_AUTH_GOOGLE_OAUTH2_KEY)
self.assertIsNone(self.settings.SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET)
def test_apply_settings_prepends_auth_backends(self):
self.assertEqual(_ORIGINAL_AUTHENTICATION_BACKENDS, self.settings.AUTHENTICATION_BACKENDS)
settings.apply_settings({provider.GoogleOauth2.NAME: {}, provider.LinkedInOauth2.NAME: {}}, self.settings)
self.assertEqual((
provider.GoogleOauth2.get_authentication_backend(), provider.LinkedInOauth2.get_authentication_backend()) +
_ORIGINAL_AUTHENTICATION_BACKENDS,
self.settings.AUTHENTICATION_BACKENDS)
def test_apply_settings_raises_value_error_if_provider_contains_uninitialized_setting(self):
bad_setting_name = 'bad_setting'
self.assertNotIn('bad_setting_name', provider.GoogleOauth2.SETTINGS)
auth_info = {
provider.GoogleOauth2.NAME: {
bad_setting_name: None,
},
}
with self.assertRaisesRegexp(ValueError, '^.*not initialized$'):
settings.apply_settings(auth_info, self.settings)
def test_apply_settings_turns_off_raising_social_exceptions(self): def test_apply_settings_turns_off_raising_social_exceptions(self):
# Guard against submitting a conf change that's convenient in dev but # Guard against submitting a conf change that's convenient in dev but
# bad in prod. # bad in prod.
settings.apply_settings({}, self.settings) settings.apply_settings(self.settings)
self.assertFalse(self.settings.SOCIAL_AUTH_RAISE_EXCEPTIONS) self.assertFalse(self.settings.SOCIAL_AUTH_RAISE_EXCEPTIONS)
"""Integration tests for settings.py."""
from django.conf import settings
from third_party_auth import provider
from third_party_auth import settings as auth_settings
from third_party_auth.tests import testutil
class SettingsIntegrationTest(testutil.TestCase):
"""Integration tests of auth settings pipeline.
Note that ENABLE_THIRD_PARTY_AUTH is True in lms/envs/test.py and False in
cms/envs/test.py. This implicitly gives us coverage of the full settings
mechanism with both values, so we do not have explicit test methods as they
are superfluous.
"""
def test_can_enable_google_oauth2(self):
auth_settings.apply_settings({'Google': {'SOCIAL_AUTH_GOOGLE_OAUTH2_KEY': 'google_key'}}, settings)
self.assertEqual([provider.GoogleOauth2], provider.Registry.enabled())
self.assertEqual('google_key', settings.SOCIAL_AUTH_GOOGLE_OAUTH2_KEY)
def test_can_enable_linkedin_oauth2(self):
auth_settings.apply_settings({'LinkedIn': {'SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY': 'linkedin_key'}}, settings)
self.assertEqual([provider.LinkedInOauth2], provider.Registry.enabled())
self.assertEqual('linkedin_key', settings.SOCIAL_AUTH_LINKEDIN_OAUTH2_KEY)
...@@ -5,13 +5,15 @@ Used by Django and non-Django tests; must not have Django deps. ...@@ -5,13 +5,15 @@ Used by Django and non-Django tests; must not have Django deps.
""" """
from contextlib import contextmanager from contextlib import contextmanager
import unittest from django.conf import settings
import django.test
import mock import mock
from third_party_auth import provider from third_party_auth.models import OAuth2ProviderConfig, SAMLProviderConfig, SAMLConfiguration, cache as config_cache
AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' AUTH_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH'
AUTH_FEATURE_ENABLED = AUTH_FEATURES_KEY in settings.FEATURES
class FakeDjangoSettings(object): class FakeDjangoSettings(object):
...@@ -23,22 +25,66 @@ class FakeDjangoSettings(object): ...@@ -23,22 +25,66 @@ class FakeDjangoSettings(object):
setattr(self, key, value) setattr(self, key, value)
class TestCase(unittest.TestCase): class ThirdPartyAuthTestMixin(object):
"""Base class for auth test cases.""" """ Helper methods useful for testing third party auth functionality """
# Allow access to protected methods (or module-protected methods) under
# test.
# pylint: disable-msg=protected-access
def setUp(self):
super(TestCase, self).setUp()
self._original_providers = provider.Registry._get_all()
provider.Registry._reset()
def tearDown(self): def tearDown(self):
provider.Registry._reset() config_cache.clear()
provider.Registry.configure_once(self._original_providers) super(ThirdPartyAuthTestMixin, self).tearDown()
super(TestCase, self).tearDown()
def enable_saml(self, **kwargs):
""" Enable SAML support (via SAMLConfiguration, not for any particular provider) """
kwargs.setdefault('enabled', True)
SAMLConfiguration(**kwargs).save()
@staticmethod
def configure_oauth_provider(**kwargs):
""" Update the settings for an OAuth2-based third party auth provider """
obj = OAuth2ProviderConfig(**kwargs)
obj.save()
return obj
def configure_saml_provider(self, **kwargs):
""" Update the settings for a SAML-based third party auth provider """
self.assertTrue(SAMLConfiguration.is_enabled(), "SAML Provider Configuration only works if SAML is enabled.")
obj = SAMLProviderConfig(**kwargs)
obj.save()
return obj
@classmethod
def configure_google_provider(cls, **kwargs):
""" Update the settings for the Google third party auth provider/backend """
kwargs.setdefault("name", "Google")
kwargs.setdefault("backend_name", "google-oauth2")
kwargs.setdefault("icon_class", "fa-google-plus")
kwargs.setdefault("key", "test-fake-key.apps.googleusercontent.com")
kwargs.setdefault("secret", "opensesame")
return cls.configure_oauth_provider(**kwargs)
@classmethod
def configure_facebook_provider(cls, **kwargs):
""" Update the settings for the Facebook third party auth provider/backend """
kwargs.setdefault("name", "Facebook")
kwargs.setdefault("backend_name", "facebook")
kwargs.setdefault("icon_class", "fa-facebook")
kwargs.setdefault("key", "FB_TEST_APP")
kwargs.setdefault("secret", "opensesame")
return cls.configure_oauth_provider(**kwargs)
@classmethod
def configure_linkedin_provider(cls, **kwargs):
""" Update the settings for the LinkedIn third party auth provider/backend """
kwargs.setdefault("name", "LinkedIn")
kwargs.setdefault("backend_name", "linkedin-oauth2")
kwargs.setdefault("icon_class", "fa-linkedin")
kwargs.setdefault("key", "test")
kwargs.setdefault("secret", "test")
return cls.configure_oauth_provider(**kwargs)
class TestCase(ThirdPartyAuthTestMixin, django.test.TestCase):
"""Base class for auth test cases."""
pass
@contextmanager @contextmanager
......
...@@ -9,9 +9,11 @@ from social.apps.django_app.default.models import UserSocialAuth ...@@ -9,9 +9,11 @@ from social.apps.django_app.default.models import UserSocialAuth
from student.tests.factories import UserFactory from student.tests.factories import UserFactory
from .testutil import ThirdPartyAuthTestMixin
@httpretty.activate @httpretty.activate
class ThirdPartyOAuthTestMixin(object): class ThirdPartyOAuthTestMixin(ThirdPartyAuthTestMixin):
""" """
Mixin with tests for third party oauth views. A TestCase that includes Mixin with tests for third party oauth views. A TestCase that includes
this must define the following: this must define the following:
...@@ -32,6 +34,10 @@ class ThirdPartyOAuthTestMixin(object): ...@@ -32,6 +34,10 @@ class ThirdPartyOAuthTestMixin(object):
if create_user: if create_user:
self.user = UserFactory() self.user = UserFactory()
UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid) UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid)
if self.BACKEND == 'google-oauth2':
self.configure_google_provider(enabled=True)
elif self.BACKEND == 'facebook':
self.configure_facebook_provider(enabled=True)
def _setup_provider_response(self, success=False, email=''): def _setup_provider_response(self, success=False, email=''):
""" """
......
...@@ -3,9 +3,10 @@ Extra views required for SSO ...@@ -3,9 +3,10 @@ Extra views required for SSO
""" """
from django.conf import settings from django.conf import settings
from django.core.urlresolvers import reverse from django.core.urlresolvers import reverse
from django.http import HttpResponse, HttpResponseServerError from django.http import HttpResponse, HttpResponseServerError, Http404
from django.shortcuts import redirect from django.shortcuts import redirect
from social.apps.django_app.utils import load_strategy, load_backend from social.apps.django_app.utils import load_strategy, load_backend
from .models import SAMLConfiguration
def inactive_user_view(request): def inactive_user_view(request):
...@@ -24,6 +25,8 @@ def saml_metadata_view(request): ...@@ -24,6 +25,8 @@ def saml_metadata_view(request):
Get the Service Provider metadata for this edx-platform instance. Get the Service Provider metadata for this edx-platform instance.
You must send this XML to any Shibboleth Identity Provider that you wish to use. You must send this XML to any Shibboleth Identity Provider that you wish to use.
""" """
if not SAMLConfiguration.is_enabled():
raise Http404
complete_url = reverse('social:complete', args=("tpa-saml", )) complete_url = reverse('social:complete', args=("tpa-saml", ))
if settings.APPEND_SLASH and not complete_url.endswith('/'): if settings.APPEND_SLASH and not complete_url.endswith('/'):
complete_url = complete_url + '/' # Required for consistency complete_url = complete_url + '/' # Required for consistency
......
...@@ -232,7 +232,7 @@ class CombinedLoginAndRegisterPage(PageObject): ...@@ -232,7 +232,7 @@ class CombinedLoginAndRegisterPage(PageObject):
Only the "Dummy" provider is used for bok choy because it is the only Only the "Dummy" provider is used for bok choy because it is the only
one that doesn't send traffic to external servers. one that doesn't send traffic to external servers.
""" """
self.q(css="button.{}-Dummy".format(self.current_form)).click() self.q(css="button.{}-oa2-dummy".format(self.current_form)).click()
def password_reset(self, email): def password_reset(self, email):
"""Navigates to, fills in, and submits the password reset form. """Navigates to, fills in, and submits the password reset form.
......
...@@ -437,9 +437,10 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest): ...@@ -437,9 +437,10 @@ class AccountSettingsPageTest(AccountSettingsTestMixin, WebAppTest):
Currently there is no way to test the whole authentication process Currently there is no way to test the whole authentication process
because that would require accounts with the providers. because that would require accounts with the providers.
""" """
for field_id, title, link_title in [ providers = (
['auth-facebook', 'Facebook', 'Link'], ['auth-oa2-facebook', 'Facebook', 'Link'],
['auth-google', 'Google', 'Link'], ['auth-oa2-google-oauth2', 'Google', 'Link'],
]: )
for field_id, title, link_title in providers:
self.assertEqual(self.account_settings_page.title_for_field(field_id), title) self.assertEqual(self.account_settings_page.title_for_field(field_id), title)
self.assertEqual(self.account_settings_page.link_title_for_link_field(field_id), link_title) self.assertEqual(self.account_settings_page.link_title_for_link_field(field_id), link_title)
...@@ -166,7 +166,7 @@ class LoginFromCombinedPageTest(UniqueCourseTest): ...@@ -166,7 +166,7 @@ class LoginFromCombinedPageTest(UniqueCourseTest):
# Now unlink the account (To test the account settings view and also to prevent cross-test side effects) # Now unlink the account (To test the account settings view and also to prevent cross-test side effects)
account_settings = AccountSettingsPage(self.browser).visit() account_settings = AccountSettingsPage(self.browser).visit()
field_id = "auth-dummy" field_id = "auth-oa2-dummy"
account_settings.wait_for_field(field_id) account_settings.wait_for_field(field_id)
self.assertEqual("Unlink", account_settings.link_title_for_link_field(field_id)) self.assertEqual("Unlink", account_settings.link_title_for_link_field(field_id))
account_settings.click_on_link_in_link_field(field_id) account_settings.click_on_link_in_link_field(field_id)
...@@ -305,7 +305,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest): ...@@ -305,7 +305,7 @@ class RegisterFromCombinedPageTest(UniqueCourseTest):
# Now unlink the account (To test the account settings view and also to prevent cross-test side effects) # Now unlink the account (To test the account settings view and also to prevent cross-test side effects)
account_settings = AccountSettingsPage(self.browser).visit() account_settings = AccountSettingsPage(self.browser).visit()
field_id = "auth-dummy" field_id = "auth-oa2-dummy"
account_settings.wait_for_field(field_id) account_settings.wait_for_field(field_id)
self.assertEqual("Unlink", account_settings.link_title_for_link_field(field_id)) self.assertEqual("Unlink", account_settings.link_title_for_link_field(field_id))
account_settings.click_on_link_in_link_field(field_id) account_settings.click_on_link_in_link_field(field_id)
......
[
{
"pk": 1,
"model": "third_party_auth.oauth2providerconfig",
"fields": {
"enabled": true,
"change_date": "2001-02-03T04:05:06Z",
"changed_by": null,
"name": "Google",
"icon_class": "fa-google-plus",
"backend_name": "google-oauth2",
"key": "test",
"secret": "test",
"other_settings": "{}"
}
},
{
"pk": 2,
"model": "third_party_auth.oauth2providerconfig",
"fields": {
"enabled": true,
"change_date": "2001-02-03T04:05:06Z",
"changed_by": null,
"name": "Facebook",
"icon_class": "fa-facebook",
"backend_name": "facebook",
"key": "test",
"secret": "test",
"other_settings": "{}"
}
},
{
"pk": 3,
"model": "third_party_auth.oauth2providerconfig",
"fields": {
"enabled": true,
"change_date": "2001-02-03T04:05:06Z",
"changed_by": null,
"name": "Dummy",
"icon_class": "fa-sign-in",
"backend_name": "dummy",
"key": "",
"secret": "",
"other_settings": "{}"
}
}
]
...@@ -23,7 +23,7 @@ from openedx.core.djangoapps.user_api.accounts.api import activate_account, crea ...@@ -23,7 +23,7 @@ from openedx.core.djangoapps.user_api.accounts.api import activate_account, crea
from openedx.core.djangoapps.user_api.accounts import EMAIL_MAX_LENGTH from openedx.core.djangoapps.user_api.accounts import EMAIL_MAX_LENGTH
from student.tests.factories import CourseModeFactory, UserFactory from student.tests.factories import CourseModeFactory, UserFactory
from student_account.views import account_settings_context from student_account.views import account_settings_context
from third_party_auth.tests.testutil import simulate_running_pipeline from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPartyAuthTestMixin
from util.testing import UrlResetMixin from util.testing import UrlResetMixin
from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.factories import CourseFactory
...@@ -204,7 +204,7 @@ class StudentAccountUpdateTest(UrlResetMixin, TestCase): ...@@ -204,7 +204,7 @@ class StudentAccountUpdateTest(UrlResetMixin, TestCase):
@ddt.ddt @ddt.ddt
class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase): class StudentAccountLoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleStoreTestCase):
""" Tests for the student account views that update the user's account information. """ """ Tests for the student account views that update the user's account information. """
USERNAME = "bob" USERNAME = "bob"
...@@ -214,6 +214,9 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase) ...@@ -214,6 +214,9 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase)
@mock.patch.dict(settings.FEATURES, {'EMBARGO': True}) @mock.patch.dict(settings.FEATURES, {'EMBARGO': True})
def setUp(self): def setUp(self):
super(StudentAccountLoginAndRegistrationTest, self).setUp('embargo') super(StudentAccountLoginAndRegistrationTest, self).setUp('embargo')
# For these tests, two third party auth providers are enabled by default:
self.configure_google_provider(enabled=True)
self.configure_facebook_provider(enabled=True)
@ddt.data( @ddt.data(
("account_login", "login"), ("account_login", "login"),
...@@ -290,7 +293,7 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase) ...@@ -290,7 +293,7 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase)
@ddt.unpack @ddt.unpack
def test_third_party_auth(self, url_name, current_backend, current_provider): def test_third_party_auth(self, url_name, current_backend, current_provider):
params = [ params = [
('course_id', 'edX/DemoX/Demo_Course'), ('course_id', 'course-v1:Org+Course+Run'),
('enrollment_action', 'enroll'), ('enrollment_action', 'enroll'),
('course_mode', 'honor'), ('course_mode', 'honor'),
('email_opt_in', 'true'), ('email_opt_in', 'true'),
...@@ -310,12 +313,14 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase) ...@@ -310,12 +313,14 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase)
# This relies on the THIRD_PARTY_AUTH configuration in the test settings # This relies on the THIRD_PARTY_AUTH configuration in the test settings
expected_providers = [ expected_providers = [
{ {
"id": "oa2-facebook",
"name": "Facebook", "name": "Facebook",
"iconClass": "fa-facebook", "iconClass": "fa-facebook",
"loginUrl": self._third_party_login_url("facebook", "login", params), "loginUrl": self._third_party_login_url("facebook", "login", params),
"registerUrl": self._third_party_login_url("facebook", "register", params) "registerUrl": self._third_party_login_url("facebook", "register", params)
}, },
{ {
"id": "oa2-google-oauth2",
"name": "Google", "name": "Google",
"iconClass": "fa-google-plus", "iconClass": "fa-google-plus",
"loginUrl": self._third_party_login_url("google-oauth2", "login", params), "loginUrl": self._third_party_login_url("google-oauth2", "login", params),
...@@ -347,11 +352,14 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase) ...@@ -347,11 +352,14 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase)
def _assert_third_party_auth_data(self, response, current_backend, current_provider, providers): def _assert_third_party_auth_data(self, response, current_backend, current_provider, providers):
"""Verify that third party auth info is rendered correctly in a DOM data attribute. """ """Verify that third party auth info is rendered correctly in a DOM data attribute. """
finish_auth_url = None
if current_backend:
finish_auth_url = reverse("social:complete", kwargs={"backend": current_backend}) + "?"
auth_info = markupsafe.escape( auth_info = markupsafe.escape(
json.dumps({ json.dumps({
"currentProvider": current_provider, "currentProvider": current_provider,
"providers": providers, "providers": providers,
"finishAuthUrl": "/auth/complete/{}?".format(current_backend) if current_backend else None, "finishAuthUrl": finish_auth_url,
"errorMessage": None, "errorMessage": None,
}) })
) )
...@@ -382,7 +390,7 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase) ...@@ -382,7 +390,7 @@ class StudentAccountLoginAndRegistrationTest(UrlResetMixin, ModuleStoreTestCase)
}) })
class AccountSettingsViewTest(TestCase): class AccountSettingsViewTest(ThirdPartyAuthTestMixin, TestCase):
""" Tests for the account settings view. """ """ Tests for the account settings view. """
USERNAME = 'student' USERNAME = 'student'
...@@ -406,6 +414,10 @@ class AccountSettingsViewTest(TestCase): ...@@ -406,6 +414,10 @@ class AccountSettingsViewTest(TestCase):
self.request = RequestFactory() self.request = RequestFactory()
self.request.user = self.user self.request.user = self.user
# For these tests, two third party auth providers are enabled by default:
self.configure_google_provider(enabled=True)
self.configure_facebook_provider(enabled=True)
# Python-social saves auth failure notifcations in Django messages. # Python-social saves auth failure notifcations in Django messages.
# See pipeline.get_duplicate_provider() for details. # See pipeline.get_duplicate_provider() for details.
self.request.COOKIES = {} self.request.COOKIES = {}
......
...@@ -171,15 +171,16 @@ def _third_party_auth_context(request, redirect_to): ...@@ -171,15 +171,16 @@ def _third_party_auth_context(request, redirect_to):
if third_party_auth.is_enabled(): if third_party_auth.is_enabled():
context["providers"] = [ context["providers"] = [
{ {
"name": enabled.NAME, "id": enabled.provider_id,
"iconClass": enabled.ICON_CLASS, "name": enabled.name,
"iconClass": enabled.icon_class,
"loginUrl": pipeline.get_login_url( "loginUrl": pipeline.get_login_url(
enabled.NAME, enabled.provider_id,
pipeline.AUTH_ENTRY_LOGIN, pipeline.AUTH_ENTRY_LOGIN,
redirect_url=redirect_to, redirect_url=redirect_to,
), ),
"registerUrl": pipeline.get_login_url( "registerUrl": pipeline.get_login_url(
enabled.NAME, enabled.provider_id,
pipeline.AUTH_ENTRY_REGISTER, pipeline.AUTH_ENTRY_REGISTER,
redirect_url=redirect_to, redirect_url=redirect_to,
), ),
...@@ -190,13 +191,14 @@ def _third_party_auth_context(request, redirect_to): ...@@ -190,13 +191,14 @@ def _third_party_auth_context(request, redirect_to):
running_pipeline = pipeline.get(request) running_pipeline = pipeline.get(request)
if running_pipeline is not None: if running_pipeline is not None:
current_provider = third_party_auth.provider.Registry.get_from_pipeline(running_pipeline) current_provider = third_party_auth.provider.Registry.get_from_pipeline(running_pipeline)
context["currentProvider"] = current_provider.NAME context["currentProvider"] = current_provider.name
context["finishAuthUrl"] = pipeline.get_complete_url(current_provider.BACKEND_CLASS.name) context["finishAuthUrl"] = pipeline.get_complete_url(current_provider.backend_name)
# Check for any error messages we may want to display: # Check for any error messages we may want to display:
for msg in messages.get_messages(request): for msg in messages.get_messages(request):
if msg.extra_tags.split()[0] == "social-auth": if msg.extra_tags.split()[0] == "social-auth":
context['errorMessage'] = unicode(msg) # msg may or may not be translated. Try translating [again] in case we are able to:
context['errorMessage'] = _(msg) # pylint: disable=translation-of-non-string
break break
return context return context
...@@ -368,19 +370,20 @@ def account_settings_context(request): ...@@ -368,19 +370,20 @@ def account_settings_context(request):
auth_states = pipeline.get_provider_user_states(user) auth_states = pipeline.get_provider_user_states(user)
context['auth']['providers'] = [{ context['auth']['providers'] = [{
'name': state.provider.NAME, # The name of the provider e.g. Facebook 'id': state.provider.provider_id,
'name': state.provider.name, # The name of the provider e.g. Facebook
'connected': state.has_account, # Whether the user's edX account is connected with the provider. 'connected': state.has_account, # Whether the user's edX account is connected with the provider.
# If the user is not connected, they should be directed to this page to authenticate # If the user is not connected, they should be directed to this page to authenticate
# with the particular provider. # with the particular provider.
'connect_url': pipeline.get_login_url( 'connect_url': pipeline.get_login_url(
state.provider.NAME, state.provider.provider_id,
pipeline.AUTH_ENTRY_ACCOUNT_SETTINGS, pipeline.AUTH_ENTRY_ACCOUNT_SETTINGS,
# The url the user should be directed to after the auth process has completed. # The url the user should be directed to after the auth process has completed.
redirect_url=reverse('account_settings'), redirect_url=reverse('account_settings'),
), ),
# If the user is connected, sending a POST request to this url removes the connection # If the user is connected, sending a POST request to this url removes the connection
# information for this provider from their edX account. # information for this provider from their edX account.
'disconnect_url': pipeline.get_disconnect_url(state.provider.NAME, state.association_id), 'disconnect_url': pipeline.get_disconnect_url(state.provider.provider_id, state.association_id),
} for state in auth_states] } for state in auth_states]
return context return context
...@@ -536,29 +536,21 @@ TIME_ZONE_DISPLAYED_FOR_DEADLINES = ENV_TOKENS.get("TIME_ZONE_DISPLAYED_FOR_DEAD ...@@ -536,29 +536,21 @@ TIME_ZONE_DISPLAYED_FOR_DEADLINES = ENV_TOKENS.get("TIME_ZONE_DISPLAYED_FOR_DEAD
X_FRAME_OPTIONS = ENV_TOKENS.get('X_FRAME_OPTIONS', X_FRAME_OPTIONS) X_FRAME_OPTIONS = ENV_TOKENS.get('X_FRAME_OPTIONS', X_FRAME_OPTIONS)
##### Third-party auth options ################################################ ##### Third-party auth options ################################################
THIRD_PARTY_AUTH = AUTH_TOKENS.get('THIRD_PARTY_AUTH', THIRD_PARTY_AUTH) if FEATURES.get('ENABLE_THIRD_PARTY_AUTH'):
AUTHENTICATION_BACKENDS = (
# The reduced session expiry time during the third party login pipeline. (Value in seconds) ENV_TOKENS.get('THIRD_PARTY_AUTH_BACKENDS', [
SOCIAL_AUTH_PIPELINE_TIMEOUT = ENV_TOKENS.get('SOCIAL_AUTH_PIPELINE_TIMEOUT', 600) 'social.backends.google.GoogleOAuth2',
'social.backends.linkedin.LinkedinOAuth2',
'social.backends.facebook.FacebookOAuth2',
'third_party_auth.saml.SAMLAuthBackend',
]) + list(AUTHENTICATION_BACKENDS)
)
##### SAML configuration for third_party_auth ##### # The reduced session expiry time during the third party login pipeline. (Value in seconds)
SOCIAL_AUTH_PIPELINE_TIMEOUT = ENV_TOKENS.get('SOCIAL_AUTH_PIPELINE_TIMEOUT', 600)
if 'SOCIAL_AUTH_TPA_SAML_SP_ENTITY_ID' in ENV_TOKENS: # third_party_auth config moved to ConfigurationModels. This is for data migration only:
SOCIAL_AUTH_TPA_SAML_SP_ENTITY_ID = ENV_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SP_ENTITY_ID') THIRD_PARTY_AUTH_OLD_CONFIG = AUTH_TOKENS.get('THIRD_PARTY_AUTH', None)
SOCIAL_AUTH_TPA_SAML_SP_NAMEID_FORMAT = ENV_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SP_NAMEID_FORMAT', 'unspecified')
SOCIAL_AUTH_TPA_SAML_SP_EXTRA = ENV_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SP_EXTRA', {})
SOCIAL_AUTH_TPA_SAML_ORG_INFO = ENV_TOKENS.get('SOCIAL_AUTH_TPA_SAML_ORG_INFO')
SOCIAL_AUTH_TPA_SAML_TECHNICAL_CONTACT = ENV_TOKENS.get(
'SOCIAL_AUTH_TPA_SAML_TECHNICAL_CONTACT',
{"givenName": "Technical Support", "emailAddress": TECH_SUPPORT_EMAIL}
)
SOCIAL_AUTH_TPA_SAML_SUPPORT_CONTACT = ENV_TOKENS.get(
'SOCIAL_AUTH_TPA_SAML_SUPPORT_CONTACT',
{"givenName": "Support", "emailAddress": TECH_SUPPORT_EMAIL}
)
SOCIAL_AUTH_TPA_SAML_SECURITY_CONFIG = ENV_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SECURITY_CONFIG', {})
SOCIAL_AUTH_TPA_SAML_SP_PUBLIC_CERT = AUTH_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SP_PUBLIC_CERT')
SOCIAL_AUTH_TPA_SAML_SP_PRIVATE_KEY = AUTH_TOKENS.get('SOCIAL_AUTH_TPA_SAML_SP_PRIVATE_KEY')
##### OAUTH2 Provider ############## ##### OAUTH2 Provider ##############
if FEATURES.get('ENABLE_OAUTH2_PROVIDER'): if FEATURES.get('ENABLE_OAUTH2_PROVIDER'):
......
...@@ -117,17 +117,6 @@ ...@@ -117,17 +117,6 @@
"username": "lms" "username": "lms"
}, },
"SECRET_KEY": "", "SECRET_KEY": "",
"THIRD_PARTY_AUTH": {
"Dummy": {},
"Google": {
"SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "test",
"SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "test"
},
"Facebook": {
"SOCIAL_AUTH_FACEBOOK_KEY": "test",
"SOCIAL_AUTH_FACEBOOK_SECRET": "test"
}
},
"DJFS": { "DJFS": {
"type": "s3fs", "type": "s3fs",
"bucket": "test", "bucket": "test",
......
...@@ -79,7 +79,6 @@ ...@@ -79,7 +79,6 @@
"ENABLE_INSTRUCTOR_ANALYTICS": true, "ENABLE_INSTRUCTOR_ANALYTICS": true,
"ENABLE_S3_GRADE_DOWNLOADS": true, "ENABLE_S3_GRADE_DOWNLOADS": true,
"ENABLE_THIRD_PARTY_AUTH": true, "ENABLE_THIRD_PARTY_AUTH": true,
"ENABLE_DUMMY_THIRD_PARTY_AUTH_PROVIDER": true,
"ENABLE_COMBINED_LOGIN_REGISTRATION": true, "ENABLE_COMBINED_LOGIN_REGISTRATION": true,
"PREVIEW_LMS_BASE": "localhost:8003", "PREVIEW_LMS_BASE": "localhost:8003",
"SUBDOMAIN_BRANDING": false, "SUBDOMAIN_BRANDING": false,
...@@ -119,6 +118,13 @@ ...@@ -119,6 +118,13 @@
"SYSLOG_SERVER": "", "SYSLOG_SERVER": "",
"TECH_SUPPORT_EMAIL": "technical@example.com", "TECH_SUPPORT_EMAIL": "technical@example.com",
"THEME_NAME": "", "THEME_NAME": "",
"THIRD_PARTY_AUTH_BACKENDS": [
"social.backends.google.GoogleOAuth2",
"social.backends.linkedin.LinkedinOAuth2",
"social.backends.facebook.FacebookOAuth2",
"third_party_auth.dummy.DummyBackend",
"third_party_auth.saml.SAMLAuthBackend"
],
"TIME_ZONE": "America/New_York", "TIME_ZONE": "America/New_York",
"WIKI_ENABLED": true "WIKI_ENABLED": true
} }
...@@ -2385,10 +2385,6 @@ for app_name in OPTIONAL_APPS: ...@@ -2385,10 +2385,6 @@ for app_name in OPTIONAL_APPS:
continue continue
INSTALLED_APPS += (app_name,) INSTALLED_APPS += (app_name,)
# Stub for third_party_auth options.
# See common/djangoapps/third_party_auth/settings.py for configuration details.
THIRD_PARTY_AUTH = {}
### ADVANCED_SECURITY_CONFIG ### ADVANCED_SECURITY_CONFIG
# Empty by default # Empty by default
ADVANCED_SECURITY_CONFIG = {} ADVANCED_SECURITY_CONFIG = {}
......
...@@ -170,6 +170,10 @@ FEATURES['STORE_BILLING_INFO'] = True ...@@ -170,6 +170,10 @@ FEATURES['STORE_BILLING_INFO'] = True
FEATURES['ENABLE_PAID_COURSE_REGISTRATION'] = True FEATURES['ENABLE_PAID_COURSE_REGISTRATION'] = True
FEATURES['ENABLE_COSMETIC_DISPLAY_PRICE'] = True FEATURES['ENABLE_COSMETIC_DISPLAY_PRICE'] = True
########################## Third Party Auth #######################
if FEATURES.get('ENABLE_THIRD_PARTY_AUTH') and 'third_party_auth.dummy.DummyBackend' not in AUTHENTICATION_BACKENDS:
AUTHENTICATION_BACKENDS = ['third_party_auth.dummy.DummyBackend'] + list(AUTHENTICATION_BACKENDS)
##################################################################### #####################################################################
# See if the developer has any local overrides. # See if the developer has any local overrides.
......
...@@ -238,18 +238,13 @@ PASSWORD_COMPLEXITY = {} ...@@ -238,18 +238,13 @@ PASSWORD_COMPLEXITY = {}
######### Third-party auth ########## ######### Third-party auth ##########
FEATURES['ENABLE_THIRD_PARTY_AUTH'] = True FEATURES['ENABLE_THIRD_PARTY_AUTH'] = True
THIRD_PARTY_AUTH = { AUTHENTICATION_BACKENDS = (
"Google": { 'social.backends.google.GoogleOAuth2',
"SOCIAL_AUTH_GOOGLE_OAUTH2_KEY": "test", 'social.backends.linkedin.LinkedinOAuth2',
"SOCIAL_AUTH_GOOGLE_OAUTH2_SECRET": "test", 'social.backends.facebook.FacebookOAuth2',
}, 'third_party_auth.dummy.DummyBackend',
"Facebook": { 'third_party_auth.saml.SAMLAuthBackend',
"SOCIAL_AUTH_FACEBOOK_KEY": "test", ) + AUTHENTICATION_BACKENDS
"SOCIAL_AUTH_FACEBOOK_SECRET": "test",
},
}
FEATURES['ENABLE_DUMMY_THIRD_PARTY_AUTH_PROVIDER'] = True
################################## OPENID ##################################### ################################## OPENID #####################################
FEATURES['AUTH_USE_OPENID'] = True FEATURES['AUTH_USE_OPENID'] = True
......
...@@ -141,4 +141,4 @@ def enable_third_party_auth(): ...@@ -141,4 +141,4 @@ def enable_third_party_auth():
""" """
from third_party_auth import settings as auth_settings from third_party_auth import settings as auth_settings
auth_settings.apply_settings(settings.THIRD_PARTY_AUTH, settings) auth_settings.apply_settings(settings)
...@@ -32,12 +32,14 @@ define(['backbone', 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers ...@@ -32,12 +32,14 @@ define(['backbone', 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers
var AUTH_DATA = { var AUTH_DATA = {
'providers': [ 'providers': [
{ {
'id': 'oa2-network1',
'name': "Network1", 'name': "Network1",
'connected': true, 'connected': true,
'connect_url': 'yetanother1.com/auth/connect', 'connect_url': 'yetanother1.com/auth/connect',
'disconnect_url': 'yetanother1.com/auth/disconnect' 'disconnect_url': 'yetanother1.com/auth/disconnect'
}, },
{ {
'id': 'oa2-network2',
'name': "Network2", 'name': "Network2",
'connected': true, 'connected': true,
'connect_url': 'yetanother2.com/auth/connect', 'connect_url': 'yetanother2.com/auth/connect',
......
...@@ -25,12 +25,14 @@ define([ ...@@ -25,12 +25,14 @@ define([
currentProvider: null, currentProvider: null,
providers: [ providers: [
{ {
id: 'oa2-google-oauth2',
name: 'Google', name: 'Google',
iconClass: 'fa-google-plus', iconClass: 'fa-google-plus',
loginUrl: '/auth/login/google-oauth2/?auth_entry=account_login', loginUrl: '/auth/login/google-oauth2/?auth_entry=account_login',
registerUrl: '/auth/login/google-oauth2/?auth_entry=account_register' registerUrl: '/auth/login/google-oauth2/?auth_entry=account_register'
}, },
{ {
id: 'oa2-facebook',
name: 'Facebook', name: 'Facebook',
iconClass: 'fa-facebook', iconClass: 'fa-facebook',
loginUrl: '/auth/login/facebook/?auth_entry=account_login', loginUrl: '/auth/login/facebook/?auth_entry=account_login',
...@@ -195,8 +197,8 @@ define([ ...@@ -195,8 +197,8 @@ define([
createLoginView(this); createLoginView(this);
// Verify that Google and Facebook registration buttons are displayed // Verify that Google and Facebook registration buttons are displayed
expect($('.button-Google')).toBeVisible(); expect($('.button-oa2-google-oauth2')).toBeVisible();
expect($('.button-Facebook')).toBeVisible(); expect($('.button-oa2-facebook')).toBeVisible();
}); });
it('displays a link to the password reset form', function() { it('displays a link to the password reset form', function() {
......
...@@ -32,12 +32,14 @@ define([ ...@@ -32,12 +32,14 @@ define([
currentProvider: null, currentProvider: null,
providers: [ providers: [
{ {
id: 'oa2-google-oauth2',
name: 'Google', name: 'Google',
iconClass: 'fa-google-plus', iconClass: 'fa-google-plus',
loginUrl: '/auth/login/google-oauth2/?auth_entry=account_login', loginUrl: '/auth/login/google-oauth2/?auth_entry=account_login',
registerUrl: '/auth/login/google-oauth2/?auth_entry=account_register' registerUrl: '/auth/login/google-oauth2/?auth_entry=account_register'
}, },
{ {
id: 'oa2-facebook',
name: 'Facebook', name: 'Facebook',
iconClass: 'fa-facebook', iconClass: 'fa-facebook',
loginUrl: '/auth/login/facebook/?auth_entry=account_login', loginUrl: '/auth/login/facebook/?auth_entry=account_login',
...@@ -284,8 +286,8 @@ define([ ...@@ -284,8 +286,8 @@ define([
createRegisterView(this); createRegisterView(this);
// Verify that Google and Facebook registration buttons are displayed // Verify that Google and Facebook registration buttons are displayed
expect($('.button-Google')).toBeVisible(); expect($('.button-oa2-google-oauth2')).toBeVisible();
expect($('.button-Facebook')).toBeVisible(); expect($('.button-oa2-facebook')).toBeVisible();
}); });
it('validates registration form fields', function() { it('validates registration form fields', function() {
......
...@@ -137,7 +137,7 @@ ...@@ -137,7 +137,7 @@
screenReaderTitle: interpolate_text( screenReaderTitle: interpolate_text(
gettext("Connect your {accountName} account"), {accountName: provider['name']} gettext("Connect your {accountName} account"), {accountName: provider['name']}
), ),
valueAttribute: 'auth-' + provider.name.toLowerCase(), valueAttribute: 'auth-' + provider.id,
helpMessage: '', helpMessage: '',
connected: provider.connected, connected: provider.connected,
connectUrl: provider.connect_url, connectUrl: provider.connect_url,
......
...@@ -532,30 +532,30 @@ ...@@ -532,30 +532,30 @@
margin-right: 0; margin-right: 0;
} }
&.button-Google:hover, &.button-Google:focus { &.button-oa2-google-oauth2:hover, &.button-oa2-google-oauth2:focus {
background-color: #dd4b39; background-color: #dd4b39;
border: 1px solid #A5382B; border: 1px solid #A5382B;
} }
&.button-Google:hover { &.button-oa2-google-oauth2:hover {
box-shadow: 0 2px 1px 0 #8D3024; box-shadow: 0 2px 1px 0 #8D3024;
} }
&.button-Facebook:hover, &.button-Facebook:focus { &.button-oa2-facebook:hover, &.button-oa2-facebook:focus {
background-color: #3b5998; background-color: #3b5998;
border: 1px solid #263A62; border: 1px solid #263A62;
} }
&.button-Facebook:hover { &.button-oa2-facebook:hover {
box-shadow: 0 2px 1px 0 #30487C; box-shadow: 0 2px 1px 0 #30487C;
} }
&.button-LinkedIn:hover , &.button-LinkedIn:focus { &.button-oa2-linkedin-oauth2:hover , &.button-oa2-linkedin-oauth2:focus {
background-color: #0077b5; background-color: #0077b5;
border: 1px solid #06527D; border: 1px solid #06527D;
} }
&.button-LinkedIn:hover { &.button-oa2-linkedin-oauth2:hover {
box-shadow: 0 2px 1px 0 #005D8E; box-shadow: 0 2px 1px 0 #005D8E;
} }
......
...@@ -388,7 +388,7 @@ $sm-btn-linkedin: #0077b5; ...@@ -388,7 +388,7 @@ $sm-btn-linkedin: #0077b5;
margin-bottom: $baseline; margin-bottom: $baseline;
} }
&.button-Google { &.button-oa2-google-oauth2 {
color: $sm-btn-google; color: $sm-btn-google;
.icon { .icon {
...@@ -407,7 +407,7 @@ $sm-btn-linkedin: #0077b5; ...@@ -407,7 +407,7 @@ $sm-btn-linkedin: #0077b5;
} }
} }
&.button-Facebook { &.button-oa2-facebook {
color: $sm-btn-facebook; color: $sm-btn-facebook;
.icon { .icon {
...@@ -426,7 +426,7 @@ $sm-btn-linkedin: #0077b5; ...@@ -426,7 +426,7 @@ $sm-btn-linkedin: #0077b5;
} }
} }
&.button-LinkedIn { &.button-oa2-linkedin-oauth2 {
color: $sm-btn-linkedin; color: $sm-btn-linkedin;
.icon { .icon {
......
...@@ -221,7 +221,7 @@ from microsite_configuration import microsite ...@@ -221,7 +221,7 @@ from microsite_configuration import microsite
% for enabled in provider.Registry.enabled(): % for enabled in provider.Registry.enabled():
## Translators: provider_name is the name of an external, third-party user authentication provider (like Google or LinkedIn). ## Translators: provider_name is the name of an external, third-party user authentication provider (like Google or LinkedIn).
<button type="submit" class="button button-primary button-${enabled.NAME} login-${enabled.NAME}" onclick="thirdPartySignin(event, '${pipeline_url[enabled.NAME]}');"><span class="icon fa ${enabled.ICON_CLASS}"></span>${_('Sign in with {provider_name}').format(provider_name=enabled.NAME)}</button> <button type="submit" class="button button-primary button-${enabled.provider_id} login-${enabled.provider_id}" onclick="thirdPartySignin(event, '${pipeline_url[enabled.provider_id]}');"><span class="icon fa ${enabled.icon_class}"></span>${_('Sign in with {provider_name}').format(provider_name=enabled.name)}</button>
% endfor % endfor
</div> </div>
......
...@@ -132,7 +132,7 @@ import calendar ...@@ -132,7 +132,7 @@ import calendar
% for enabled in provider.Registry.enabled(): % for enabled in provider.Registry.enabled():
## Translators: provider_name is the name of an external, third-party user authentication service (like Google or LinkedIn). ## Translators: provider_name is the name of an external, third-party user authentication service (like Google or LinkedIn).
<button type="submit" class="button button-primary button-${enabled.NAME} register-${enabled.NAME}" onclick="thirdPartySignin(event, '${pipeline_urls[enabled.NAME]}');"><span class="icon fa ${enabled.ICON_CLASS}"></span>${_('Sign up with {provider_name}').format(provider_name=enabled.NAME)}</button> <button type="submit" class="button button-primary button-${enabled.provider_id} register-${enabled.provider_id}" onclick="thirdPartySignin(event, '${pipeline_urls[enabled.provider_id]}');"><span class="icon fa ${enabled.icon_class}"></span>${_('Sign up with {provider_name}').format(provider_name=enabled.name)}</button>
% endfor % endfor
</div> </div>
......
...@@ -49,7 +49,7 @@ ...@@ -49,7 +49,7 @@
<% _.each( context.providers, function( provider ) { <% _.each( context.providers, function( provider ) {
if ( provider.loginUrl ) { %> if ( provider.loginUrl ) { %>
<button type="button" class="button button-primary button-<%- provider.name %> login-provider login-<%- provider.name %>" data-provider-url="<%- provider.loginUrl %>"> <button type="button" class="button button-primary button-<%- provider.id %> login-provider login-<%- provider.id %>" data-provider-url="<%- provider.loginUrl %>">
<div class="icon fa <%- provider.iconClass %>" aria-hidden="true"></div> <div class="icon fa <%- provider.iconClass %>" aria-hidden="true"></div>
<%- provider.name %> <%- provider.name %>
</button> </button>
......
...@@ -29,7 +29,7 @@ ...@@ -29,7 +29,7 @@
<% <%
_.each( context.providers, function( provider) { _.each( context.providers, function( provider) {
if ( provider.registerUrl ) { %> if ( provider.registerUrl ) { %>
<button type="button" class="button button-primary button-<%- provider.name %> login-provider register-<%- provider.name %>" data-provider-url="<%- provider.registerUrl %>"> <button type="button" class="button button-primary button-<%- provider.id %> login-provider register-<%- provider.id %>" data-provider-url="<%- provider.registerUrl %>">
<span class="icon fa <%- provider.iconClass %>" aria-hidden="true"></span> <span class="icon fa <%- provider.iconClass %>" aria-hidden="true"></span>
<%- provider.name %> <%- provider.name %>
</button> </button>
......
...@@ -19,10 +19,10 @@ from third_party_auth import pipeline ...@@ -19,10 +19,10 @@ from third_party_auth import pipeline
<i class="icon fa fa-unlink"></i><span class="copy">${_('Not Linked')}</span> <i class="icon fa fa-unlink"></i><span class="copy">${_('Not Linked')}</span>
% endif % endif
</div> </div>
<span class="provider">${state.provider.NAME}</span> <span class="provider">${state.provider.name}</span>
<span class="control"> <span class="control">
<form <form
action="${pipeline.get_disconnect_url(state.provider.NAME, state.association_id)}" action="${pipeline.get_disconnect_url(state.provider.provider_id, state.association_id)}"
method="post" method="post"
name="${state.get_unlink_form_name()}"> name="${state.get_unlink_form_name()}">
% if state.has_account: % if state.has_account:
...@@ -33,7 +33,7 @@ from third_party_auth import pipeline ...@@ -33,7 +33,7 @@ from third_party_auth import pipeline
${_("Unlink")} ${_("Unlink")}
</a> </a>
% else: % else:
<a href="${pipeline.get_login_url(state.provider.NAME, pipeline.AUTH_ENTRY_PROFILE)}"> <a href="${pipeline.get_login_url(state.provider.provider_id, pipeline.AUTH_ENTRY_PROFILE)}">
## Translators: clicking on this creates a link between a user's edX account and their account with an external authentication provider (like Google or LinkedIn). ## Translators: clicking on this creates a link between a user's edX account and their account with an external authentication provider (like Google or LinkedIn).
${_("Link")} ${_("Link")}
</a> </a>
......
...@@ -25,7 +25,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey ...@@ -25,7 +25,7 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
from django_comment_common import models from django_comment_common import models
from student.tests.factories import UserFactory from student.tests.factories import UserFactory
from third_party_auth.tests.testutil import simulate_running_pipeline from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPartyAuthTestMixin
from third_party_auth.tests.utils import ( from third_party_auth.tests.utils import (
ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle
) )
...@@ -800,7 +800,7 @@ class PasswordResetViewTest(ApiTestCase): ...@@ -800,7 +800,7 @@ class PasswordResetViewTest(ApiTestCase):
@ddt.ddt @ddt.ddt
@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
class RegistrationViewTest(ApiTestCase): class RegistrationViewTest(ThirdPartyAuthTestMixin, ApiTestCase):
"""Tests for the registration end-points of the User API. """ """Tests for the registration end-points of the User API. """
maxDiff = None maxDiff = None
...@@ -907,6 +907,7 @@ class RegistrationViewTest(ApiTestCase): ...@@ -907,6 +907,7 @@ class RegistrationViewTest(ApiTestCase):
def test_register_form_third_party_auth_running(self): def test_register_form_third_party_auth_running(self):
no_extra_fields_setting = {} no_extra_fields_setting = {}
self.configure_google_provider(enabled=True)
with simulate_running_pipeline( with simulate_running_pipeline(
"openedx.core.djangoapps.user_api.views.third_party_auth.pipeline", "openedx.core.djangoapps.user_api.views.third_party_auth.pipeline",
"google-oauth2", email="bob@example.com", "google-oauth2", email="bob@example.com",
......
...@@ -32,7 +32,7 @@ git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c ...@@ -32,7 +32,7 @@ git+https://github.com/hmarr/django-debug-toolbar-mongo.git@b0686a76f1ce3532088c
-e git+https://github.com/jazkarta/ccx-keys.git@e6b03704b1bb97c1d2f31301ecb4e3a687c536ea#egg=ccx-keys -e git+https://github.com/jazkarta/ccx-keys.git@e6b03704b1bb97c1d2f31301ecb4e3a687c536ea#egg=ccx-keys
# For SAML Support (To be moved to PyPi installation in base.txt once our changes are merged): # For SAML Support (To be moved to PyPi installation in base.txt once our changes are merged):
-e git+https://github.com/open-craft/python-saml.git@9602b8133056d8c3caa7c3038761147df3d4b257#egg=python-saml -e git+https://github.com/open-craft/python-saml.git@9602b8133056d8c3caa7c3038761147df3d4b257#egg=python-saml
-e git+https://github.com/open-craft/python-social-auth.git@17def186d4bb7165f9c37037936997ef39ae2f29#egg=python-social-auth -e git+https://github.com/open-craft/python-social-auth.git@02ab628b8961b969021de87aeb23551da4e751b7#egg=python-social-auth
# Our libraries: # Our libraries:
-e git+https://github.com/edx/XBlock.git@74fdc5a361f48e5596acf3846ca3790a33a05253#egg=XBlock -e git+https://github.com/edx/XBlock.git@74fdc5a361f48e5596acf3846ca3790a33a05253#egg=XBlock
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment