Prevent startup permission initialization from re-granting legacy collection access#7992
Prevent startup permission initialization from re-granting legacy collection access#7992acwhite211 wants to merge 3 commits intov7_12_0_4from
Conversation
melton-jason
left a comment
melton-jason
left a comment
There was a problem hiding this comment.
Generally looks good! Nice work 💯
| @app.task(bind=True, max_retries=SCHEMA_DEFAULTS_MISSING_DISCIPLINE_MAX_RETRIES) | ||
| def apply_schema_defaults_task(self, discipline_id: int): | ||
| """Run schema localization defaults for one discipline in a background worker.""" | ||
| task_id = getattr(self.request, 'id', None) |
There was a problem hiding this comment.
(pretty optional)
Right now I see we're calling this function using the run method?
I see the Celery docs reccommend using apply to run tasks synchronously/locally:
https://docs.celeryq.dev/en/latest/reference/celery.app.task.html#celery.app.task.Task.apply
I'm not entirely sure what run does, but I'm assuming apply boils down to run. run generally states:
When called tasks apply the run() method. This method must be defined by all tasks (that is unless the call() method is overridden).
Run
The body of the task executed by workers.
https://docs.celeryq.dev/en/latest/reference/celery.app.task.html#celery.app.task.Task.run
Perhaps we can be more in-line with what Celery expects by instead using apply here. I'm not sure what the tradeoffs are for going with the current approach over apply, but it seems to run the function anyway, so not too big of a deal.
melton-jason
left a comment
melton-jason
left a comment
There was a problem hiding this comment.
Actually, I don't think we can/should disable the run_key_migration_functions by default in the v7.12.0.4 release, unfortunately.
There's a lot of deduplication functionality in the run_key_migration_functions in the release that resolve regressions.
We could make an argument for keeping run_key_migration_functions disabled by default, but that would require everyone who had updated to v7.12.0.0-v7.12.0.3 to intentionally re-enable the function and set the environment variable as part of their update process...
@grantfitzsimmons @acwhite211
What are your thoughts on this?
github-project-automation
bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
|
@melton-jason I say we keep it enabled for now and discuss for 7.12.1. |
|
I brought these changes except for the disabling of run_key_migration_functions into another branch/PR that I brought to |
3 participants
Summary
This change stops the startup
run_key_migration_functionspath from re-importing legacy SP6 collection access into SP7 permissions. Because the startup path re-ran the full initializer, users with legacy SP6 principal rows could regain/system/sp7/collectionaccess on startup, even if that SP7 access had been intentionally removed.Made running the key migrations function optional at startup with RUN_KEY_MIGRATION_ON_STARTUP=0 in the .env file. Also fixed the warning of null tasks from happening in the key migration logs.
Checklist
self-explanatory (or properly documented)