Added a UI for confirming OAuth Access. #11464

justinabrahms · 2016-02-08T19:09:21Z

What are the relevant tickets?

Fixes mitocw#176
No JIRA ticket that I know of, but this continues the work mentioned in https://groups.google.com/forum/#!searchin/edx-code/justin$20abrahms/edx-code/BS0oowwARlM/2SHqJmtuBwAJ

What's this PR do?

Adds a UI for OAuth access, which allows users to delegate permissions to 3rd party services.

Where should the reviewer start?

A familiarity with django-oauth2-provider is essential, as this PR is just some custom templating around that library's workings. I think @clintonb or @jimabramson are probably good candidates to review this from the oauth side.

How should this be manually tested?

Create an oauth client.
Visit the URL localhost:8000/oauth2/authorize?client_id=test-id&response_type=code&scope=email

The client_id is the one you made. Scopes are space separated. Valid scopes are: default, openid, profile, email, course_staff, course_instructor, and permissions.

Clicking cancel will take you to your redirect url with access_denied in the URL. Clicking accept will send you to the redirect url with the authorization code to continue the oauth flow.

Any background context you want to provide?

This is relevant for interacting with edX as a third party application, which MIT will need for upcoming feature work. Specifically, solving the "who is this user on edx?" question is difficult and error prone without this capability.

This affects LMS only (afaik), but will be an authorization avenue for all edX users.

The template was copied from django-oauth2-provider repository and edited to integrate with main_django.html and provide some styling hooks.

Screenshots (if appropriate)

(the box is an artifact of screen capture, not in the actual page)

What gif best describes this PR or how it makes you feel?

openedx-webhooks · 2016-02-08T19:10:37Z

Thanks for the pull request, @justinabrahms! I've created OSPR-1131 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

nedbat · 2016-02-08T20:42:11Z

@justinabrahms thank you for the gif clarifying your emotional state! If others were as good at finding gifs, I might add that section to the usual PR cover letter template :)

Are there any tests around this kind of UI? It seems like the kind of fragile corner that might break.

@clintonb @jimabramson If you wouldn't mind...

nedbat · 2016-02-08T20:42:39Z

lms/templates/provider/authorize.html

+<div class="authorization-confirmation">
+  {% if not error %}
+  {% blocktrans with application_name=client.name %}
+  <p><strong>{{ application_name }}</strong> would like to access your data with the following permissions:</p>


It'd be better to leave the <p> and </p> tags outside the blocktrans.

nedbat · 2016-02-08T20:43:13Z

@edx/doc this seems like it will need docs.

clintonb · 2016-02-08T20:59:56Z

lms/templates/provider/authorize.html

+          {% endfor %}
+        </select>
+      </div>
+      <input type="submit" class="btn login large danger" value="Cancel" />


Should this be a submit button? Is there anyway we can simply redirect without the POST submission?

Based on the underlying library.. yes. This should be a submit button. We could do this all client-side, if that's what you mean, but that would be a bigger departure from the underlying library than I feel comfortable with. See here:

https://github.com/edx/django-oauth2-provider/blob/edx/provider/views.py#L397

Got it. Thanks.

clintonb · 2016-02-08T21:03:55Z

This looks great, but we need tests, ideally at the acceptance level to better facilitate the eventual replacement of django-oauth2-provider with django-oauth-toolkit.

justinabrahms · 2016-02-09T16:10:02Z

@clintonb This is my first bokchoy test. It's unclear how I populate data into the database. It seems like I can't start from a working state, and must, instead, built a working state from scratch.

I think this means creating superusers, clicking around in the admin to setup an oauth app, then actually performing the tests.

Can you confirm that this is expected? How do I create the initial super user to access the admin?

I need to create an oauth application. This either means writing it into the database (which I can't seem to do, given I don't have access to a django context) or doing it through the admin (for which I'll need an admin user I can access).

edit: found out how to run faster bokchoy tests

clintonb · 2016-02-09T17:10:59Z

@justinabrahms I can't recall writing tests for edx-platform; but, for ecommerce, we need to setup data before the tests run. I must defer to @benpatterson and @jzoldak regarding fixtures/data setup for edx-platform.

jzoldak · 2016-02-09T20:36:19Z

@justinabrahms here is some hopefully helpful information:

The django settings for the bok-choy servers are configured via lms/envs/bok_choy.* (py, auth.json, env.json)
Fixtures for the bok-choy server are loaded on its startup from common/test/db_fixtures
If your oauth server needs to be a stub, here are some pointers:
- Existing stubs in common/djangoapps/terrain/stubs
- Tests of the stubs themselved live in the tests folder under that
- The stub servers for bok-choy are started up by pavelib/utils/envs.py

Development/troubleshooting tips:

The bok-choy server output is directed to log files in test_root/log
There are a couple helpful options to the test_bokchoy paver command. See paver test_bokchoy --help
In particular you can start the server in one window with --serversonly and then iterate through test development in another shell window using --testsonly.

justinabrahms · 2016-02-09T21:12:10Z

This now has acceptance tests, as requested.

clintonb · 2016-02-09T22:22:47Z

common/test/acceptance/pages/lms/oauth2_confirmation.py

+"""
+Pages relevant for OAuth2 confirmation.
+"""
+from . import BASE_URL


Use absolute imports.

Done. No other acceptance test uses absolute imports for these things. If this is something you care about, you might consider filing a ticket for it.

clintonb · 2016-02-09T22:36:51Z

👍

jzoldak · 2016-02-10T13:23:51Z

common/test/acceptance/tests/lms/test_oauth2.py

+        """
+        self._auth()
+        self.oauth_page.scopes = ('email', 'does-not-exist')
+        assert self.oauth_page.visit().is_browser_on_page()


Bok choy's visit method has the assertions for being on the page built-in, so instead you should simply do self.oauth_page.visit()

justinabrahms · 2016-02-10T15:32:54Z

This is ready for another round of review. AFAIK, all comments have been addressed.

As I mentioned to Clinton, if you want absolute imports on acceptance tests, no other file does this. You might consider filing a ticket to clean it up in the future.

clintonb · 2016-02-10T17:15:25Z

Ticket filed to clean imports: https://openedx.atlassian.net/browse/TE-1194

justinabrahms · 2016-02-11T21:39:43Z

What's left to be done on this PR?

edx notes bokchoy tests are failing. As are lettuce tests. Looking at the error messages, it doesn't appear to have anything to do with my work. Would love to push this along. :)

benpatterson · 2016-02-11T22:01:37Z

Hey @justinabrahms I think those tests need to be addressed. Perhaps it is an easy fix, but I suspect something is breaking them (at least, for bok-choy). From what I recall, the Notes tests do rely on some oauth settings; I just don't know or remember what the dependency is, exactly.

I could potentially help you debug them early next week if you need a hand.

benpatterson · 2016-02-11T22:01:48Z

jenkins run lettuce

jzoldak · 2016-02-12T13:59:09Z

@justinabrahms

I am unfamiliar with edx Notes, but looking quickly at the bok-choy server log from a failed test run my first guess is that you'll need to add another oauth client to the oauth.json fixture with a client_id of edx-notes.

I haven't looked into it closely enough to understand why the tests worked before (when the ENABLE_OAUTH2_PROVIDER feature flag was not turned on.)

HTH.

Traceback (most recent call last):
[...]
  File "/home/jenkins/workspace/edx-platform-test-subset/lms/djangoapps/edxnotes/decorators.py", line 49, in get_html
    "token": get_edxnotes_id_token(self.runtime.get_real_user(self.runtime.anonymous_student_id)),
  File "/home/jenkins/workspace/edx-platform-test-subset/lms/djangoapps/edxnotes/helpers.py", line 54, in get_edxnotes_id_token
    return get_id_token(user, CLIENT_NAME)
  File "openedx/core/lib/token_utils.py", line 42, in get_id_token
    raise ImproperlyConfigured('OAuth2 Client with name [%s] does not exist' % client_name)
ImproperlyConfigured: OAuth2 Client with name [edx-notes] does not exist

justinabrahms · 2016-02-17T18:57:49Z

Tests now pass. Next steps?

clintonb · 2016-02-17T19:00:06Z

@justinabrahms, I will merge after you squash your commits and all tests pass. It looks like only the a11y test ran on your most recent commit.

justinabrahms · 2016-02-17T19:01:39Z

And here I was all excited. :-/

Thanks.

This will allow users to delegate permissions to a 3rd party service.

justinabrahms · 2016-02-17T19:58:56Z

All of the tests have passed. 🏈 The commits have been squashed.

Added a UI for confirming OAuth Access.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Feb 8, 2016

nedbat reviewed Feb 8, 2016
View reviewed changes

openedx-webhooks added awaiting prioritization and removed needs triage labels Feb 8, 2016

clintonb reviewed Feb 8, 2016
View reviewed changes

clintonb reviewed Feb 9, 2016
View reviewed changes

jzoldak reviewed Feb 10, 2016
View reviewed changes

justinabrahms force-pushed the oauth-confirmation-views branch from 61a466c to 4c8754d Compare February 17, 2016 19:03

Added a UI for confirming OAuth Access.

885785d

This will allow users to delegate permissions to a 3rd party service.

justinabrahms force-pushed the oauth-confirmation-views branch from 4c8754d to 885785d Compare February 17, 2016 19:04

clintonb added a commit that referenced this pull request Feb 17, 2016

Merge pull request #11464 from mitocw/oauth-confirmation-views

922269f

Added a UI for confirming OAuth Access.

clintonb merged commit 922269f into openedx:master Feb 17, 2016

justinabrahms deleted the oauth-confirmation-views branch February 17, 2016 20:07

pdpinch mentioned this pull request Apr 11, 2016

recreate OAuth template that Justin wrote for the old OAuth library mitocw/edx-platform#236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a UI for confirming OAuth Access. #11464

Added a UI for confirming OAuth Access. #11464

justinabrahms commented Feb 8, 2016

openedx-webhooks commented Feb 8, 2016

nedbat commented Feb 8, 2016

nedbat Feb 8, 2016

nedbat commented Feb 8, 2016

clintonb Feb 8, 2016

justinabrahms Feb 9, 2016

clintonb Feb 9, 2016

clintonb commented Feb 8, 2016

justinabrahms commented Feb 9, 2016

clintonb commented Feb 9, 2016

jzoldak commented Feb 9, 2016

justinabrahms commented Feb 9, 2016

clintonb Feb 9, 2016

justinabrahms Feb 10, 2016

clintonb commented Feb 9, 2016

jzoldak Feb 10, 2016

justinabrahms Feb 10, 2016

justinabrahms commented Feb 10, 2016

clintonb commented Feb 10, 2016

justinabrahms commented Feb 11, 2016

benpatterson commented Feb 11, 2016

benpatterson commented Feb 11, 2016

jzoldak commented Feb 12, 2016

justinabrahms commented Feb 17, 2016

clintonb commented Feb 17, 2016

justinabrahms commented Feb 17, 2016

justinabrahms commented Feb 17, 2016

Added a UI for confirming OAuth Access. #11464

Added a UI for confirming OAuth Access. #11464

Conversation

justinabrahms commented Feb 8, 2016

What are the relevant tickets?

What's this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

Screenshots (if appropriate)

What gif best describes this PR or how it makes you feel?

openedx-webhooks commented Feb 8, 2016

nedbat commented Feb 8, 2016

Choose a reason for hiding this comment

nedbat commented Feb 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintonb commented Feb 8, 2016

justinabrahms commented Feb 9, 2016

clintonb commented Feb 9, 2016

jzoldak commented Feb 9, 2016

justinabrahms commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintonb commented Feb 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinabrahms commented Feb 10, 2016

clintonb commented Feb 10, 2016

justinabrahms commented Feb 11, 2016

benpatterson commented Feb 11, 2016

benpatterson commented Feb 11, 2016

jzoldak commented Feb 12, 2016

justinabrahms commented Feb 17, 2016

clintonb commented Feb 17, 2016

justinabrahms commented Feb 17, 2016

justinabrahms commented Feb 17, 2016