GitHub PullRequestReviewCommentEvent at 2014-04-18 20:05:41
Repository: edx-platform
Actor: jzoldak
Payload: {"comment":{"url":"https://api.github.com/repos/edx/edx-platform/pulls/comments/11785057","id":11785057,"diff_hunk":"@@ -1,9 +1,248 @@\n-\"\"\"Auth pipeline definitions.\"\"\"\n+\"\"\"Auth pipeline definitions.\n \n+Auth pipelines handle the process of authenticating a user. They involve a\n+consumer system and a provider service. The general pattern is:\n+\n+    1. The consumer system exposes a URL endpoint that starts the process.\n+    2. When a user visits that URL, the client system redirects the user to a\n+       page served by the provider. The user authenticates with the provider.\n+       The provider handles authentication failure however it wants.\n+    3. On success, the provider POSTs to a URL endpoint on the consumer to\n+       invoke the pipeline. It sends back an arbitrary payload of data about\n+       the user.\n+    4. The pipeline begins, executing each function in its stack. The stack is\n+       defined on django's settings object's SOCIAL_AUTH_PIPELINE. This is done\n+       in settings._set_global_settings.\n+    5. Each pipeline function is variadic. Most pipeline functions are part of\n+       the pythons-social-auth library; our extensions are defined below. The\n+       pipeline is the same no matter what provider is used.\n+    6. Pipeline functions can return a dict to add arguments to the function\n+       invoked next. They can return None if this is not necessary.\n+    7. Pipeline functions may be decorated with @partial.partial. This pauses\n+       the pipeline and serializes its state onto the request's session. When\n+       this is done they may redirect to other edX handlers to execute edX\n+       account registration/sign in code.\n+    8. In that code, redirecting to get_complete_url() resumes the pipeline.\n+       This happens by hitting a handler exposed by the consumer system.\n+    9. In this way, execution moves between the provider, the pipeline, and\n+       arbitrary consumer system code.\n+\n+Gotcha alert!:\n+\n+Bear in mind that when pausing and resuming a pipeline function decorated with\n+@partial.partial, execution resumes by re-invoking the decorated function\n+instead of invoking the next function in the pipeline stack. For example, if\n+you have a pipeline of\n+\n+    A\n+    B\n+    C\n+\n+with an implementation of\n+\n+    @partial.partial\n+    def B(*args, **kwargs):\n+        [...]\n+\n+B will be invoked twice: once when initially proceeding through the pipeline\n+before it is paused, and once when other code finishes and the pipeline\n+resumes. Consequently, many decorated functions will first invoke a predicate\n+to determine if they are in their first or second execution (usually by\n+checking side-effects from the first run).\n+\n+This is surprising but important behavior, since it allows a single function in\n+the pipeline to consolidate all the operations needed to establish invariants\n+rather than spreading them across two functions in the pipeline.\n+\n+See http://psa.matiasaguirre.net/docs/pipeline.html for more docs.\n+\"\"\"\n+\n+import random\n+import string  # pylint: disable-msg=deprecated-module\n+\n+from django.contrib.auth.models import User\n+from django.core.urlresolvers import reverse\n+from django.shortcuts import redirect\n+from social.apps.django_app.default import models\n+from social.exceptions import AuthException\n from social.pipeline import partial\n \n+from . import provider\n+\n+\n+AUTH_ENTRY_KEY = 'auth_entry'\n+AUTH_ENTRY_LOGIN = 'login'\n+AUTH_ENTRY_REGISTER = 'register'\n+_AUTH_ENTRY_CHOICES = frozenset([AUTH_ENTRY_LOGIN, AUTH_ENTRY_REGISTER])\n+_DEFAULT_RANDOM_PASSWORD_LENGTH = 12\n+_PASSWORD_CHARSET = string.letters + string.digits\n+\n+\n+class AuthEntryError(AuthException):\n+    \"\"\"Raised when auth_entry is missing or invalid on URLs.\n+\n+    auth_entry tells us whether the auth flow was initiated to register a new\n+    user (in which case it has the value of AUTH_ENTRY_REGISTER) or log in an\n+    existing user (in which case it has the value of AUTH_ENTRY_LOGIN).\n+\n+    This is necessary because the edX code we hook into the pipeline to\n+    redirect to the existing auth flows needs to know what case we are in in\n+    order to format its output correctly (for example, the register code is\n+    invoked earlier than the login code, and it needs to know if the login flow\n+    was requested to dispatch correctly).\n+    \"\"\"\n+\n+\n+def get(request):\n+    \"\"\"Gets the running pipeline from the passed request.\"\"\"\n+    return request.session.get('partial_pipeline')\n+\n+\n+def get_authenticated_user(username, backend_name):\n+    \"\"\"Gets a saved user authenticated by a particular backend.\n+\n+    Between pipeline steps User objects are not saved. We need to reconstitute\n+    the user and set its .backend, which is ordinarily monkey-patched on by\n+    Django during authenticate(), so it will function like a user returned by\n+    authenticate().\n+\n+    Args:\n+        username: string. Username of user to get.\n+        backend_name: string. The name of the third-party auth backend from\n+            the running pipeline.\n+\n+    Returns:\n+        User if user is found and has a social auth from the passed\n+        backend_name.\n+\n+    Raises:\n+        User.DoesNotExist: if no user matching user is found, or the matching\n+        user has no social auth associated with the given backend.\n+        AssertionError: if the user is not authenticated.\n+    \"\"\"\n+    user = models.DjangoStorage.user.user_model().objects.get(username=username)\n+\n+    if not user:\n+        raise User.DoesNotExist\n+\n+    match = models.DjangoStorage.user.get_social_auth_for_user(user, provider=backend_name)\n+\n+    if not match:\n+        raise User.DoesNotExist\n+\n+    user.backend = provider.Registry.get_by_backend_name(backend_name).get_authentication_backend()\n+    return user\n+\n+\n+def _get_url(view_name, backend_name, auth_entry=None):\n+    \"\"\"Creates a URL to hook into social auth endpoints.\"\"\"\n+    kwargs = {'backend': backend_name}\n+    url = reverse(view_name, kwargs=kwargs)\n+\n+    if auth_entry:\n+        url += '?%s=%s' % (AUTH_ENTRY_KEY, auth_entry)\n+\n+    return url\n+\n+\n+def get_complete_url(backend_name):\n+    \"\"\"Gets URL for the endpoint that returns control to the auth pipeline.\n+\n+    Args:\n+        backend_name: string. Name of the python-social-auth backend from the\n+            currently-running pipeline.\n+\n+    Returns:\n+        String. URL that finishes the auth pipeline for a provider.\n+\n+    Raises:\n+        ValueError: if no provider is enabled with the given backend_name.\n+    \"\"\"\n+    enabled_provider = provider.Registry.get_by_backend_name(backend_name)\n+\n+    if not enabled_provider:\n+        raise ValueError('Provider with backend %s not enabled' % backend_name)\n+\n+    return _get_url('social:complete', backend_name)\n+\n+\n+def get_login_url(provider_name, auth_entry):\n+    \"\"\"Gets the login URL for the endpoint that kicks off auth with a provider.\n+\n+    Args:\n+        provider_name: string. The name of the provider.Provider that has been\n+            enabled.\n+        auth_entry: string. Query argument specifying the desired entry point\n+            for the auth pipeline. Used by the pipeline for later branching.\n+            Must be one of _AUTH_ENTRY_CHOICES.\n+\n+    Returns:\n+        String. URL that starts the auth pipeline for a provider.\n+\n+    Raises:\n+        ValueError: if no provider is enabled with the given provider_name.\n+    \"\"\"\n+    assert auth_entry in _AUTH_ENTRY_CHOICES\n+    enabled_provider = provider.Registry.get(provider_name)\n+\n+    if not enabled_provider:\n+        raise ValueError('Provider %s not enabled' % provider_name)\n+\n+    return _get_url('social:begin', enabled_provider.BACKEND_CLASS.name, auth_entry=auth_entry)\n+\n+\n+def make_random_password(length=None, choice_fn=random.SystemRandom().choice):\n+    \"\"\"Makes a random password.\n+\n+    When a user creates an account via a social provider, we need to create a\n+    placeholder password for them to satisfy the ORM's consistency and\n+    validation requirements. Users don't know (and hence cannot sign in with)\n+    this password; that's OK because they can always use the reset password\n+    flow to set it to a known value.\n+\n+    Args:\n+        choice_fn: function or method. Takes an iterable and returns a random\n+            element.\n+        length: int. Number of chars in the returned value. None to use default.\n+\n+    Returns:\n+        String. The resulting password.\n+    \"\"\"\n+    length = length if length is not None else _DEFAULT_RANDOM_PASSWORD_LENGTH\n+    return ''.join(choice_fn(_PASSWORD_CHARSET) for _ in xrange(length))\n+\n+\n+def running(request):\n+    \"\"\"Returns True iff request is running a third-party auth pipeline.\"\"\"\n+    return request.session.get('partial_pipeline') is not None  # Avoid False for {}.\n+\n+\n+# Pipeline functions.\n+# Signatures are set by python-social-auth; prepending 'unused_' causes\n+# TypeError on dispatch to the auth backend's authenticate().\n+# pylint: disable-msg=unused-argument\n+\n+\n+def parse_query_params(strategy, response, *args, **kwargs):\n+    \"\"\"Reads whitelisted query params, transforms them into pipeline args.\"\"\"\n+    auth_entry = strategy.session.get(AUTH_ENTRY_KEY)\n+\n+    if not (auth_entry and auth_entry in _AUTH_ENTRY_CHOICES):\n+        raise AuthEntryError(strategy.backend, 'auth_entry missing or invalid')\n+\n+    return {\n+        # Whether the auth pipeline entered from /login.\n+        'is_login': auth_entry == AUTH_ENTRY_LOGIN,\n+        # Whether the auth pipeline entered from /register.\n+        'is_register': auth_entry == AUTH_ENTRY_REGISTER,\n+    }\n+\n \n @partial.partial\n-def step(*args, **kwargs):\n-    \"\"\"Fake pipeline step; just throws loudly for now.\"\"\"\n-    raise NotImplementedError('%s, %s' % (args, kwargs))\n+def redirect_to_supplementary_form(strategy, details, response, uid, is_login=None, is_register=None, user=None, *args, **kwargs):\n+    \"\"\"Dispatches user to a create account form if they are new.\"\"\"\n+    if is_login and user is None:\n+        return redirect('/login', name='signin_user')","path":"common/djangoapps/third_party_auth/pipeline.py","position":249,"original_position":249,"commit_id":"452344cc1e121cc4f1c6dc373bcea9579b46d90a","original_commit_id":"452344cc1e121cc4f1c6dc373bcea9579b46d90a","user":{"login":"jzoldak","id":2338889,"avatar_url":"https://avatars.githubusercontent.com/u/2338889?","gravatar_id":"95184b9bc8ce30dea682715997220939","url":"https://api.github.com/users/jzoldak","html_url":"https://github.com/jzoldak","followers_url":"https://api.github.com/users/jzoldak/followers","following_url":"https://api.github.com/users/jzoldak/following{/other_user}","gists_url":"https://api.github.com/users/jzoldak/gists{/gist_id}","starred_url":"https://api.github.com/users/jzoldak/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jzoldak/subscriptions","organizations_url":"https://api.github.com/users/jzoldak/orgs","repos_url":"https://api.github.com/users/jzoldak/repos","events_url":"https://api.github.com/users/jzoldak/events{/privacy}","received_events_url":"https://api.github.com/users/jzoldak/received_events","type":"User","site_admin":false},"body":"I think this needs to be redirecting to a reverse of the login page, as I know that on the cms side the url is \"/signin\" not \"/login\" so it wouldn't work there. We expect this to be enable-able for both studio and lms right?","created_at":"2014-04-18T20:05:41Z","updated_at":"2014-04-18T20:05:41Z","html_url":"https://github.com/edx/edx-platform/pull/2994#discussion_r11785057","pull_request_url":"https://api.github.com/repos/edx/edx-platform/pulls/2994","_links":{"self":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/comments/11785057"},"html":{"href":"https://github.com/edx/edx-platform/pull/2994#discussion_r11785057"},"pull_request":{"href":"https://api.github.com/repos/edx/edx-platform/pulls/2994"}}}}