events: add reason to the MODEL_UPDATED event for password hash updates - #18068
events: add reason to the MODEL_UPDATED event for password hash updates#18068danidee10 wants to merge 13 commits into
Conversation
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-integrations ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18068 +/- ##
==========================================
- Coverage 93.23% 93.18% -0.06%
==========================================
Files 968 969 +1
Lines 53589 53648 +59
==========================================
+ Hits 49965 49993 +28
- Misses 3624 3655 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| export COMPOSE_PROJECT_NAME="authentik-test-${AUTHENTIK_TAG}" | ||
|
|
||
| if [[ -v BUILD ]]; then | ||
| if [ -n "${BUILD:-}" ]; then |
There was a problem hiding this comment.
When trying to test with docker. I discovered that this syntax wasn't POSIX compliant and didn't work on macos (with bash 3.2)
There was a problem hiding this comment.
I can confirm the same with zsh 5.9 (arm64-apple-darwin25.0)
rissson
left a comment
There was a problem hiding this comment.
@danidee10 fancy seeing you here! Thank you for the contribution!
| if isinstance(instance, User) and instance.tracker.has_changed("password"): | ||
| context["reason"] = PASSWORD_HASH_UPGRADE_REASON |
There was a problem hiding this comment.
How do we know this is not just the user changing their password?
There was a problem hiding this comment.
We don't know as there's no way to distinguish why the hash changed here
But I thought it might be acceptable since there's already an existing PASSWORD_SET event that tracks password changes. Thus, anyone looking at the event log can figure out why the hash changed.
I had a different approach here which shouldn't log the event (when the user changes their password) as it doesn't depend on the User model's post_save signal.
I guess it depends on how you answer these questions:
Should we track all password hash changes regardless of how they happened ? or
Should we only track password hash changes because Django changed the iterations of the hasher ?
|
ideas from chatting with @rissson
|
That was my Initial approach by overriding
It has more moving parts (We have to apply the context manager to all existing auth backends and future auth backends)
I will go with the |
|
@BeryJu I went with Option 1 as stated earlier. I replaced |
|
We wanted to look into overwriting the check_password function from django to detect when it's doing a hash upgrade and there using |
Signed-off-by: Osaetin Daniel <osaetindaniel@gmail.com>
I wasn't aware of the |
|
Thanks for being so responsive, even if we have been anything but. The changes look alright to me, but I'll let the team take another pass to determine if any change is still needed. |
Checklist
ak test authentik/)make lint-fix)Initially, I started with a more complex Implementation which detected the password hash change in all cases but it felt a bit hacky as I had to use
Queryset.updateto avoid emitting a signal.I switched to a simpler approach that uses
model_utils.FieldTracker. It only detects changes when the audit middleware runs and register the signal handlers i.e It will not detect changes that happen outside the request/response cycle.The rationale behind this is that anyone who changes the
iterations/roundsof the hashers is probably an advanced user that knows that the hashes will be updated the next time django runscheck_passwordand they shouldn't need an explicit event logcloses #17840