Auth fixes#640
Conversation
varmar05
commented
Jun 17, 2026
varmar05
commented
- Session cookie gap fix: load_user now only returns active users; @auth_required checks is_active
- Bcrypt lazy rehash: configurable BCRYPT_LOG_ROUNDS, passwords rehashed on next login if rounds differ
- Account lockout: progressive tiers via LOCKOUT_POLICY env var, 423 response with AccountLockedError + added db migration: new failed_login_attempts and locked_until columns added
- `load_user` now returns None for inactive users, so their session cookies are rejected by Flask-Login - @auth_required adds `is_active` check - anonymize() now explicitly sets active=False for defence-in-depth
Existing passwords will be rehashed organically if needed.
Coverage Report for CI Build 27823802673Coverage increased (+0.006%) to 92.162%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
| from ..app import db | ||
| from ..sync.models import ProjectUser | ||
| from ..sync.utils import get_user_agent, get_ip, get_device_id, is_reserved_word | ||
| from .errors import AccountLockedError |
|
|
||
| def to_dict(self) -> Dict: | ||
| data = super().to_dict() | ||
| data["locked_until"] = self.locked_until.isoformat() |
There was a problem hiding this comment.
shall we make it timezone aware when it's used in client-facing public API?
we already use it when going through schemas
| try: | ||
| # bcrypt hash format: $2b$<rounds>$<salt+hash> | ||
| hash_rounds = int(self.passwd.split("$")[2]) | ||
| return hash_rounds != rounds |
There was a problem hiding this comment.
< would be probably safer, but not a big deal
MarcelGeo
left a comment
MarcelGeo
left a comment
There was a problem hiding this comment.
Do we need something to do in FE/clients/mobile - login attempts logic? @tomasMizera
@varmar05 We didn't implemented ip reputation in this mechanism -> There could be case when somebody can lock another account which he knows by email or accidentally similar emails can block each together. This could be probably handled by infra rate limiting - needs to discuss what values to adjust for current nginx config files in this repository - could be upgraded also nginx proxy.
Flask limiter wouldn't do the same job as database locking - probably issue with redis backend again.
| try: | ||
| user = authenticate(form.login.data, form.password.data) | ||
| except AccountLockedError as e: | ||
| abort(423, f"Account temporarily locked until {e.locked_until.isoformat()}") |
There was a problem hiding this comment.
If we want to return exact time (not sure If it's compatible with security guys) - give it to attribute in AccountLockedError new attribute. I think we can show something for user in clients - received count and reset-at time.
| if needs_commit: | ||
| db.session.commit() | ||
| return user | ||
| else: |
There was a problem hiding this comment.
If the user attempts failing after 5 times -> wait -> is_locked_out will remove locked_until. After 60 seconds another fail will lead to next 60s ... It's like not reseting properly.
| now = datetime.datetime.utcnow() | ||
| if self.locked_until <= now: | ||
| # lockout has expired — clear it so subsequent queries see a clean state | ||
| self.locked_until = None |
There was a problem hiding this comment.
Is commited somewhere or it's just related to needsCommit in authenticate
4 participants
