Add support for signed tasks. #73
Conversation
Previously, this was re-validating the existing task, rather than considering the modifications
There was a problem hiding this comment.
Thanks so much for this feature! It's something we spoke of during the DEP process, but ended up working around with opt-in task functions. However, I do think it'd be a nice feature to add.
Along with my comments, it'd be great if you could add some tests for this new functionality!
| @@ -63,7 +63,7 @@ class DBTaskResult(GenericBase[P, T], models.Model): | |||
| max_length=max(len(value) for value in ResultStatus.values), | |||
| ) | |||
|
|
|||
| enqueued_at = models.DateTimeField(auto_now_add=True) | |||
There was a problem hiding this comment.
Question: What's the reason for this change?
There was a problem hiding this comment.
enqueued_at was getting set at insert rather than at model creation time. We would have to do an insert and an update to set the signature. Now whether or not enqueued_at should represented the time .enqueue was called or the time the record was inserted, I'm not sure about.
|
|
||
| def _task_to_db_task( | ||
| self, task: Task[P, T], args: P.args, kwargs: P.kwargs | ||
| ) -> "DBTaskResult": | ||
| from .models import DBTaskResult | ||
|
|
||
| return DBTaskResult( | ||
| db_task_result: DBTaskResult = DBTaskResult( |
There was a problem hiding this comment.
Nit: The explicit type shouldn't be necessary here
| db_task_result: DBTaskResult = DBTaskResult( | |
| db_task_result = DBTaskResult( |
There was a problem hiding this comment.
mypy yelled about it, it really should be able to infer that.
django_tasks/backends/database/management/commands/db_worker.py
Outdated
Show resolved
Hide resolved
| """ | ||
| Get canonical form | ||
| """ | ||
| return { |
There was a problem hiding this comment.
Suggestion: Because TaskResult is a dataclass, you can call asdict on it to get it as a dict natively, and then massage the types into something JSON-serializable.
There was a problem hiding this comment.
I moved .canonical inside the base task result, but for task I only wanted to include invariant values. I don't think status, started_at, or finished_at should form part of the signature.
|
Thanks, I'll get working on those. |
Remove supports_signed_task property from backend. Get SIGN_TASKS setting from backend options. Add salt field to DB task result model.
Update migrations for signed tasks in DB backend.
|
@RealOrangeOne Added tests and made updates based on your suggestions. |
|
CI is still failing. However, on reflection, I'm not sure the added complexity is worthwhile. Because task functions must be explicitly allowed, there's no ability to gain remote-code execution inside Django (at least not in ways which can be controlled by |
No description provided.