Improve order secret handling #4139
Conversation
- use hmac.compare_digest for all secret comparisons - use salted_hmac with sha256 instead of plain sha1 for hashed secrets - move secret handling into helper functions
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4139 +/- ##
========================================
Coverage 78.09% 78.10%
========================================
Files 429 430 +1
Lines 60312 60503 +191
========================================
+ Hits 47098 47253 +155
- Misses 13214 13250 +36
|
src/pretix/base/models/orders.py
Outdated
| @staticmethod | ||
| def get_with_secret_check(code, received_secret, tag, secret_length=64, qs=None): |
There was a problem hiding this comment.
Django style question: Would this better be a method on a custom QuerySet class so I can chain it with .filter() or .annotate() etc?
There was a problem hiding this comment.
Chaining as in event.orders.get_with_secret_check(...).annotate(...)? I think it wouldn't be possible to implement it that way, as we need the Order object in here for the secret check?
The other way round, would work I think, which would just change the API slightly from Order.get_with_secret_check(..., qs=event.orders.annotate(...)) to event.orders.annotate(...).get_with_secret_check(...) which seems nicer though, yes. I'll have a look into implementing that.
There was a problem hiding this comment.
Yeah, I meant the second mart and you're right it's mostly cosmetic
There was a problem hiding this comment.
could you confirm that this is in fact an okay way to specify the Manager for orders:
https://github.com/pretix/pretix/pull/4139/files#diff-a88712c33526e582606fb2e095cea07e0972a201338a955b91cf2dfc675fa3bbR318
in other places it's done in a more verbose way with a separate manager (e.g. ItemQuerySetManager), I think my way is equivalent with less duplicated code but I'm not sure if there is some subtlety I'm missing?
There was a problem hiding this comment.
as_manager() is certainly fine, and if it works with scopes, fine with me
after this is merged, our payment provider plugins can be refactored to use tagged_secret / get_with_secret_check instead of hashlib.sha1