ARCH-135 Add new custom DOT Application model to support OAuth2 per-application scopes. by douglashall · Pull Request #18416 · openedx/openedx-platform

douglashall · 2018-06-20T13:49:12Z

ARCH-135

Add new custom DOT Application model to support OAuth2 per-application scopes.

This also introduces a model for persisting organization-based filters on
a per-application basis. See openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst
for additional details.

douglashall · 2018-06-20T15:34:37Z

@edx/devops Heads up! Migration coming through.

nasthagiri · 2018-06-20T16:12:33Z

Questions about this migration:

Can we pass reverse_code=RunPython.noop to be explicit that there's no rollback of this migration?

What happens if the migration is run, rolled back, and then re-run? Will migrate_application_data handle that?

What if migrate_application_data fails in the middle? Will re-running the migration coalesce any issues?

How will we prevent or detect that no new Application entries get created in between this PR and the one that changes settings.OAUTH2_PROVIDER_APPLICATION_MODEL? Should we create another small PR that changes the value of settings.OAUTH2_PROVIDER_APPLICATION_MODEL? Should we move this migration to a management command that runs again (just in case)?

#2 and #3 are all issues that have actually happened in production, so not just theoretical.

Done.
2+3. Switched to get_or_create instead of save.

We should accept that risk. We can do a manual check after we switch to the new application model to verify that the two application tables are in sync.

@feanil FYI on this migration strategy.

Adding:
5. The "id" field in the new and old Application models need to match exactly in order to support all previously created tokens/grants/etc. Those tables have foreign keys to the Application table via the "id" field.

nasthagiri · 2018-06-20T16:19:22Z

Where does this number come from? From the RFC?

nasthagiri · 2018-06-20T16:36:41Z

Should this be dependent on the setting - or always related to ScopedApplication since they seem to be sister models? Each way has its own trade-offs, of course. I suppose the current way is more extensible, right?

What if someone changes the setting? That won't result in a new migration. But that should be Ok, I believe.

Yes, I think this is good as is.

nasthagiri · 2018-06-20T16:41:59Z

Nit: These are unused in this PR at the moment.

nasthagiri · 2018-06-20T16:44:21Z

Where does this number come from? From the RFC?

I didn't see any specification of size limits in the RFC. This was copied over from the original Microsoft PR. Did you have a different number in mind?

@mettursathish Is there a reason your team chose this limit?

Bumping this from 10 to 25 as discussed on Slack#architecture.

no specific reason why we chose 10.

nasthagiri · 2018-06-20T17:32:38Z

Is this new dependency still needed?

Yes, I'm using this for the list of strings filed that stores the scopes on ScopedApplication.

feanil · 2018-06-20T20:00:16Z

Would it make sense to use update_or_create instead?

feanil · 2018-06-20T20:02:21Z

I think you may actually want to implement a proper reverse here because if we end up making changes to the new table, we will probably want to propagate them back to the old table so auth does not break. This is an unlikely case but I'd rather have it implemented correctly if we need it.

We won't need it. The new table will not be accessible from the app until we are certain we won't be rolling back.

nasthagiri · 2018-06-20T20:27:16Z

This also needs to update to 25 (to be consistent with max_length.

Good catch.

…n scopes. This also introduces a model for persisting organization-based filters on a per-application basis. See openedx/core/djangoapps/oauth_dispatch/docs/decisions/0007-include-organizations-in-tokens.rst for additional details.

edx-status-bot · 2018-06-20T20:58:09Z

Your PR has finished running tests. There were no failures.

edx-pipeline-bot · 2018-06-20T21:58:40Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, June 21, 2018.

edx-pipeline-bot · 2018-06-21T14:54:09Z

EdX Release Notice: This PR has been deployed to the production environment.

douglashall force-pushed the douglashall/oauth_scopes_part1 branch from bb12ba3 to bf3202b Compare June 20, 2018 13:53

douglashall changed the title ~~Add new custom DOT Application model to support OAuth2 per-application scopes.~~ ARCH-135 Add new custom DOT Application model to support OAuth2 per-application scopes. Jun 20, 2018

douglashall requested review from nasthagiri and robrap June 20, 2018 14:05

douglashall force-pushed the douglashall/oauth_scopes_part1 branch 2 times, most recently from 94a3008 to 8840b0e Compare June 20, 2018 15:34

douglashall force-pushed the douglashall/oauth_scopes_part1 branch from 8840b0e to f299de3 Compare June 20, 2018 16:42

nasthagiri reviewed Jun 20, 2018

View reviewed changes

douglashall force-pushed the douglashall/oauth_scopes_part1 branch from f299de3 to 0c228be Compare June 20, 2018 17:25

nasthagiri reviewed Jun 20, 2018

View reviewed changes

douglashall force-pushed the douglashall/oauth_scopes_part1 branch 3 times, most recently from 6118adc to cbfa5fd Compare June 20, 2018 18:26

nasthagiri mentioned this pull request Jun 20, 2018

WIP: Restricted OAuth Initial Design & Implementation #18117

Closed

douglashall force-pushed the douglashall/oauth_scopes_part1 branch 4 times, most recently from 61b3b43 to a4f9f67 Compare June 20, 2018 19:35

nasthagiri approved these changes Jun 20, 2018

View reviewed changes

feanil reviewed Jun 20, 2018

View reviewed changes

douglashall force-pushed the douglashall/oauth_scopes_part1 branch from a4f9f67 to bd3db55 Compare June 20, 2018 20:15

nasthagiri reviewed Jun 20, 2018

View reviewed changes

douglashall force-pushed the douglashall/oauth_scopes_part1 branch from bd3db55 to bab6e36 Compare June 20, 2018 20:32

nasthagiri mentioned this pull request Jun 20, 2018

Enable ScopedApplication #18426

Closed

douglashall merged commit cc5f62d into master Jun 20, 2018

edx-secure deleted the douglashall/oauth_scopes_part1 branch January 7, 2019 08:04

Uh oh!

Conversation

douglashall commented Jun 20, 2018 • edited by robrap Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

douglashall commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-status-bot commented Jun 20, 2018

Uh oh!

edx-pipeline-bot commented Jun 20, 2018

Uh oh!

edx-pipeline-bot commented Jun 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

douglashall commented Jun 20, 2018 •

edited by robrap

Loading