Add Programs tab to Studio - #10701
Conversation
There was a problem hiding this comment.
I'm having trouble getting the "administrator" claim — provided by the "permissions" scope — into the ID tokens generated for use in the context of Studio. Prior to rebasing this PR onto Django 1.8, including this IDTokenHandler in OAUTH_OIDC_ID_TOKEN_HANDLERS was apparently enough to get the "administrator" claim. Without the "administrator" claim in the token, the Programs API won't return unpublished programs. I'd appreciate any help/suggestions.
There was a problem hiding this comment.
A little more on what I've tried here. I've taken a debugger down into the depths of edx/edx-oauth2-provider, looking specifically at how scopes are pulled from the handlers registered in OAUTH_OIDC_ID_TOKEN_HANDLERS (oauth2_provider/oidc/collect.py#L50).
This exercise reveals that the IDTokenHandler is registered and "visited" during scope collection, but that the "permissions" scope is missing from the set of candidate scopes.
There was a problem hiding this comment.
The 1.8 upgrade was a red herring.
ID tokens created as part of the OIDC flow used by our IDAs include the "administrator" claim by way of the EXTRA_SCOPES setting consumed by edx/auth-backends. Signing a staff user into the Programs service via OIDC forces the creation of a token with the "administrator" claim. The scopes requested by Programs appear to be retained by the user's access token. When Studio later requests an ID token for the user, the user's existing access token is used, the "permissions" scope is detected, and the "administrator" claim is included in the token, allowing unpublished programs to be listed:
I wouldn't have caught this without destroying my devstack, forcing me to sign into Programs again and create a new ID token. The tricky part remains finding a good way to specify the "permissions" scope for tokens requested in process (instead of by an IDA using OIDC).
There was a problem hiding this comment.
edx-oauth2-provider intentionally makes it difficult to customize scopes. Given the circumstances, it looks like creating these JWTs manually is justifiable. Doing so will result in limited code duplication, but it'll let us get the claims we need while staying away from most of the OAuth2 code, meaning it'll be easier to understand what's going on.
d66464c to
4d0e6af
Compare
There was a problem hiding this comment.
is there a reason to manipulate the ordering here? seems like it would be better to push that logic down into the server API call.
There was a problem hiding this comment.
I agree, I'll make that change.
753ebca to
67ce516
Compare
|
@jimabramson I'm rebasing frequently on this branch, so your comments on the JWT construction commit may be lost. I'm preserving them here. In reference to the
This has been addressed. In reference to the ordering of programs contained in the response from the Programs service:
This has been done. |
322062a to
cce1b1c
Compare
|
👍 once tests pass, and assuming @andy-armstrong is comfortable. |
cce1b1c to
4c32625
Compare
|
@cahrens @explorerleslie FYI here's the PR I mentioned at stand up. |
|
@rlucioni what's the timeline you're looking to merge this? And is there a particular part you want the TNL team to focus their review on? I'm trying to prioritize this against other work TNL is doing. cc @scottrish |
|
@rlucioni What is the timeframe that you need this reviewed by? Are you looking only for a TNL review of the Studio portion? In general, it is best to tag me as scrummaster so we can put the review in our sprint and decide who is available to review it (there are multiple people who can review Studio changes). |
|
@explorerleslie @cahrens I'm taking over this PR from @rlucioni (on holiday). We are looking to merge this ASAP. Apologies for the injection - as Renzo mentioned we are looking for feedback only on the Studio portion, as the changes there are something Andy and Renzo have had a number of prior conversations about. |
|
@jimabramson OK, @andy-armstrong can review the Studio portions once the PR is ready for final review (note the bok choy failures and need for rebase). |
There was a problem hiding this comment.
Not your fault, but this is an insane tangle of imports!
There was a problem hiding this comment.
This should probably be button too, although I see that there are several other faux buttons above it that use a.
|
@jimabramson @rlucioni Unless I'm mistaken, I don't see any tests for the new Studio program tab. I would like to see at least a couple of Bok Choy tests that verify that the tab only appears when the feature flag is on. Other than that, this looks great (although it is a shame that we ran out of time on the cleaner plugin model). |
|
@andy-armstrong thanks for the quick review. I will work on addressing feedback shortly. To your comment about the tests for the tab, the changes to cms/djangoapps/contentstore/tests/test_programs.py in this PR seem intended to cover that. |
|
@jimabramson Thanks. For some reason I was expecting the tests to be under a views directory. There needs to be a test that the JS logic works too (see the broken boolean condition), so it seems that a Bok Choy test might still be a good idea. |
|
@andy-armstrong I've added bok choy tests for this tab on studio home. Note that coverage for the creationrights js thing didn't already exist and i've modified it (in the first fixup commit) to work in a more neutral way. Sadly that piece can only be tested with bok choy, but that is a significant additional effort/change on top of what's already done here, and I think that bok choy would be overkill for this. A better solution IMO, and probably less effort, would be to refactor the template into js so it can be tested in jasmine, but I'm not planning to pile that onto this PR. (I can create a ticket for it, however.) |
|
note that bok choy test failures are for a different / unrelated test. i can reproduce locally, and not sure what's wrong, still investigating. |
|
👍 Thanks for the excellent new tests. This looks good to me once you rebase and fix the Bok Choy failure. |
9aa88c3 to
6ae6320
Compare
|
rebased, waiting for tests with crossed fingers. |
Extends the Programs ConfigurationModel, cleans up Programs-related utilities and corresponding tests, and corrects caching. Uses the Programs API to list programs within Studio. ECOM-2769.
6ae6320 to
70d5732
Compare
|
jenkins run bokchoy |
|
woohoo |
Add Programs tab to Studio
|
@jimabramson thank you very much for carrying this across the finish line, and @andy-armstrong for the review on short notice. |
5 participants
Extends the Programs ConfigurationModel, cleans up Programs-related utilities and corresponding tests, and corrects caching. Uses the Programs API to list programs within Studio. ECOM-2769.
When the Studio tab is enabled and no programs are available:
When the Studio tab is enabled and programs are available:
The programs tab is implemented in a way similar to the libraries tab. We weren't able to commit to making Studio's tabs pluggable, so I've removed the unused StudioTabPluginManager and related code.
@jimabramson, could you please review this? @andy-armstrong, could you please take a look at the parts of this which touch Studio? @AlasdairSwan, FYI.