-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update django-oauth-toolkit #2710 #2727
Update django-oauth-toolkit #2710 #2727
Conversation
Remove pinning for django-oauth-toolkit and remove explicit declaration of oauthlib as it is a dependency of django-oauth-toolkit. Re-address prior work-around for older oauth2_provider migration file silently failing to apply, and holding up all subsequent oauth2_provider migrations, as this migration file, and a few subsequent ones, have now been squashed upstream. "oauth2_provider" is part of django-oauth-toolkit. # Includes: - Added logging for before & after django-oauth-toolkit migration. - Adopt dynamic client_secret for internal Oauth app. As from Django Oauth Toolkit 2.x onwards, Oauth app client_secret is hashed within Django's database, dictating that we can no longer source this secret from the db for our internal cli client app token requests. Move to establishing a dynamic Oathapp client_secret, established in settings.py, and reset by rockstor-bootstrap.service, i.e. on each service restart/reboot. - Adding a requests timeouts to client token requests. - Arbitrary fsting application. - Update disk, pool, share, snap state every 20s not every minute. - Abandon rockstor-bootstrap.service start (boostrap scritp) after 10, not 15 attempts.
A caveat for this PR is that we have moved, temporarily, from install persistent OAUTH_INTERNAL_APP client secret to boot persistent only (bootstrap script established). This is insufficient for our needs, but has been split out into the following (linked) issue as this relates to a wider requirement we have in this area. "Adopt dedicated secrets management library #2728" |
On a fresh RPM build (Host OS 15.5) we are still seeing a migration issue here regarding the upstream squashes and the removal of our prior work-around:
The DB state on this rpm build is a from-scratch scenario give we are failing on the fresh-install phase (i.e. no .initrock)! |
We seem to be failing in the clean-start scenario with the following newer packages migration:
The current suspicion is that the newly squash contents of 0001_initial, now containing may of the older migrations of the prior much older package, has ended up with our clean-start db setup skipping some of the earlier migrations. However this does not tally with the clean-start basis here !! So we likely have a db initialisation 'mistake' that we have thus-far avoided or worked around. I.e. an earlier squash of our own has inadvertently included an external packages migrations: See:
Indicates that what is expected to be a clean default db, from oauth2_providers perspective, does in fact already show the 0001_initial' migration to be in place. I suspect we have this embedded in one of our db setup systems and need to remove it so that we can leave these external app migrations to the external app. @FroggyFlox I know you are working on: I'll move this back to a Draft PR for now and look to weeding out our likely erroneously included oath2_provider migration that we must now have carried for quite some time!! But that is not our responsibility, but that of the Django oauth tookit - hence our current blocker here I suspect. |
That does seem like a very good suspicion, indeed, given our db restore using |
TestingIt is now possible, assuming a re-base on latest testing (in this case done by our backend buildbot test setup) to do an RPM build (Host OS 15.5) of this PR. And for that rpm to install a-fresh on the build host; and have all the resulting main rockstor services start as expected. This service start failed prior to the linked, and now merged, PR #2733. TODO:Assess if we are able to update from significant prior released RPM versions. |
Log of rpm's rockstor-pre.service's initrock script indicating that our new db setup via Django migrations only, in #2733, has successfully accomplished the oath2_provider migrations:
And that our existing (in a fresh install redundant) follow-up is of no effect - bar inefficiency to be address in a dedicated issue once we have this PR settled in, and that we are clear on the ramification regarding recent db setup/migration changes. |
We also have confirmation re the raised caveat in the following comment above (#2727 (comment)) re duplication: In System -> Access Keys
Our retrieval (from the Django default db) now only shows the hashed secret. See also: |
Summary of testing:RPMs built on host indicated host as version 5.0.5-2727, PR contents rebased on latest testing branch. Leap 15.5 base amd64
Leap 15.4 base amd64
|
@FroggyFlox & @Hooverdan96 Noting again the regression introduced here (see #2727 (comment)) I'll go ahead and merge, as we are holding up other important updates and improvements with this recent db stuff. I.e. @FroggyFlox's services stuff (no more sql.in dump file to worry about now) & the Django update Milestone. |
Remove pinning for django-oauth-toolkit and remove explicit declaration of oauthlib as it is a dependency of django-oauth-toolkit.
Re-address prior work-around for older oauth2_provider migration file silently failing to apply, and holding up all subsequent oauth2_provider migrations, as this migration file, and a few subsequent ones, have now been squashed upstream. "oauth2_provider" is part of django-oauth-toolkit.
Includes:
Fixes #2710
See issue text, and this pull requests prior draft #2726 for development context.