-
Notifications
You must be signed in to change notification settings - Fork 69
Auth fixes #640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Auth fixes #640
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ | |
| ) | ||
| from .bearer import encode_token | ||
| from .models import User, LoginHistory | ||
| from .errors import AccountLockedError | ||
| from .schemas import UserSchema, UserSearchSchema, UserProfileSchema, UserInfoSchema | ||
| from .forms import ( | ||
| LoginForm, | ||
|
|
@@ -137,7 +138,10 @@ def login_public(): # noqa: E501 | |
| """ | ||
| form = ApiLoginForm() | ||
| if form.validate(): | ||
| user = authenticate(form.login.data, form.password.data) | ||
| try: | ||
| user = authenticate(form.login.data, form.password.data) | ||
| except AccountLockedError as e: | ||
| return e.response(423) | ||
| if user and user.active: | ||
| expire = datetime.now(pytz.utc) + timedelta( | ||
| seconds=current_app.config["BEARER_TOKEN_EXPIRATION"] | ||
|
|
@@ -221,7 +225,10 @@ def search_users(): # pylint: disable=W0613,W0612 | |
| def login(): # pylint: disable=W0613,W0612 | ||
| form = LoginForm() | ||
| if form.validate(): | ||
| user = authenticate(form.login.data, form.password.data) | ||
| try: | ||
| user = authenticate(form.login.data, form.password.data) | ||
| except AccountLockedError as e: | ||
| return e.response(423) | ||
| if user and user.active: | ||
| login_user(user) | ||
| if not os.path.isfile(current_app.config["MAINTENANCE_FILE"]): | ||
|
|
@@ -238,7 +245,10 @@ def admin_login(): # pylint: disable=W0613,W0612 | |
| if not form.validate(): | ||
| return jsonify(form.errors), 400 | ||
|
|
||
| user = authenticate(form.login.data, form.password.data) | ||
| try: | ||
| user = authenticate(form.login.data, form.password.data) | ||
| except AccountLockedError as e: | ||
| abort(423, f"Account temporarily locked until {e.locked_until.isoformat()}") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 user: | ||
| if user.active and user.is_admin: | ||
| login_user(user) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| # Copyright (C) Lutra Consulting Limited | ||
| # | ||
| # SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-MerginMaps-Commercial | ||
|
|
||
| import datetime | ||
| from typing import Dict | ||
|
|
||
| from ..app import ResponseError | ||
|
|
||
|
|
||
| class AccountLockedError(Exception, ResponseError): | ||
| code = "AccountLocked" | ||
| detail = "Account temporarily locked due to too many failed login attempts" | ||
|
|
||
| def __init__(self, locked_until: datetime.datetime): | ||
| self.locked_until = locked_until | ||
|
|
||
| def to_dict(self) -> Dict: | ||
| data = super().to_dict() | ||
| data["locked_until"] = self.locked_until.isoformat() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we make it timezone aware when it's used in client-facing public API? |
||
| return data | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,10 +13,20 @@ | |
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not used? |
||
|
|
||
| MAX_USERNAME_LENGTH = 50 | ||
|
|
||
|
|
||
| def _parse_lockout_policy(policy_str: str) -> list: | ||
| """Parse "5:300,10:3600" into [(5, 300), (10, 3600)] sorted ascending by threshold.""" | ||
| result = [] | ||
| for part in policy_str.split(","): | ||
| threshold, seconds = part.strip().split(":") | ||
| result.append((int(threshold), int(seconds))) | ||
| return sorted(result, key=lambda x: x[0]) | ||
|
|
||
|
|
||
| class User(db.Model): | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| username = db.Column(db.String(80), info={"label": "Username"}) | ||
|
|
@@ -33,6 +43,10 @@ class User(db.Model): | |
| default=datetime.datetime.utcnow, | ||
| ) | ||
| last_signed_in = db.Column(db.DateTime(), nullable=True) | ||
| failed_login_attempts = db.Column( | ||
| db.Integer, default=0, nullable=False, server_default="0" | ||
| ) | ||
| locked_until = db.Column(db.DateTime(), nullable=True) | ||
| receive_notifications = db.Column( | ||
| db.Boolean, default=True, nullable=False, index=True | ||
| ) | ||
|
|
@@ -64,12 +78,57 @@ def check_password(self, password): | |
| def assign_password(self, password): | ||
| if isinstance(password, str): | ||
| password = password.encode("utf-8") | ||
| rounds = current_app.config.get("BCRYPT_LOG_ROUNDS", 12) | ||
| self.passwd = ( | ||
| bcrypt.hashpw(password, bcrypt.gensalt()).decode("utf-8") | ||
| bcrypt.hashpw(password, bcrypt.gensalt(rounds)).decode("utf-8") | ||
| if password | ||
| else None | ||
| ) | ||
|
|
||
| def needs_rehash(self): | ||
| """Return True if the stored hash was generated with a different cost factor than configured.""" | ||
| if self.passwd is None: | ||
| return False | ||
| rounds = current_app.config.get("BCRYPT_LOG_ROUNDS", 12) | ||
| try: | ||
| # bcrypt hash format: $2b$<rounds>$<salt+hash> | ||
| hash_rounds = int(self.passwd.split("$")[2]) | ||
| return hash_rounds != rounds | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| except (IndexError, ValueError): | ||
| return False | ||
|
|
||
| def is_locked_out(self) -> bool: | ||
| """Return True if the account is currently under a temporary lockout.""" | ||
| if self.locked_until is None: | ||
| return False | ||
| 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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is commited somewhere or it's just related to needsCommit in authenticate |
||
| return False | ||
| return True | ||
|
|
||
| def record_failed_login(self) -> None: | ||
| """Increment the failed-login counter and apply a lockout if a threshold is crossed.""" | ||
| self.failed_login_attempts = (self.failed_login_attempts or 0) + 1 | ||
| policy = _parse_lockout_policy( | ||
| current_app.config.get("LOCKOUT_POLICY", "5:300,10:3600") | ||
| ) | ||
| # find the highest applicable tier | ||
| duration = None | ||
| for threshold, seconds in policy: | ||
| if self.failed_login_attempts >= threshold: | ||
| duration = seconds | ||
| if duration is not None: | ||
| self.locked_until = datetime.datetime.utcnow() + datetime.timedelta( | ||
| seconds=duration | ||
| ) | ||
|
|
||
| def reset_lockout(self) -> None: | ||
| """Clear lockout state after a successful login.""" | ||
| self.failed_login_attempts = 0 | ||
| self.locked_until = None | ||
|
|
||
| @property | ||
| def is_authenticated(self): | ||
| """For Flask-Login""" | ||
|
|
@@ -185,6 +244,7 @@ def anonymize(self): | |
| """Anonymize user object in database - remove personal information""" | ||
| ts = round(datetime.datetime.utcnow().timestamp() * 1000) | ||
| del_str = f"deleted_{ts}" | ||
| self.active = False | ||
| self.username = del_str | ||
| self.email = None | ||
| self.passwd = None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.