From 3ab80ebef9ed53afb724265868318279d5a330cb Mon Sep 17 00:00:00 2001 From: Masaharu Hayashi Date: Sun, 27 Sep 2026 21:37:24 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(weko-accounts):=20=E3=83=AD=E3=82=B0?= =?UTF-8?q?=E3=82=A4=E3=83=B3=20API=20=E3=81=AE=E5=A4=B1=E6=95=97=E5=BF=9C?= =?UTF-8?q?=E7=AD=94=E3=82=92=E6=8F=83=E3=81=88=E3=80=81=E3=83=AC=E3=83=BC?= =?UTF-8?q?=E3=83=88=E5=88=B6=E9=99=90=E3=82=92=E6=9C=89=E5=8A=B9=E3=81=AB?= =?UTF-8?q?=E3=81=99=E3=82=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 認証情報が誤っている場合の応答を1種類にまとめる - 不正なリクエスト本文は 400 を返す - REST アプリでも共通の limiter を初期化する Co-Authored-By: Claude Opus 5.5 --- modules/weko-accounts/tests/test_rest.py | 66 +++++++++++++++++-- modules/weko-accounts/weko_accounts/errors.py | 14 ++++ modules/weko-accounts/weko_accounts/ext.py | 16 +++++ modules/weko-accounts/weko_accounts/rest.py | 25 ++++--- 4 files changed, 108 insertions(+), 13 deletions(-) diff --git a/modules/weko-accounts/tests/test_rest.py b/modules/weko-accounts/tests/test_rest.py index 124d40b7a5..de261b52ae 100644 --- a/modules/weko-accounts/tests/test_rest.py +++ b/modules/weko-accounts/tests/test_rest.py @@ -22,7 +22,9 @@ from flask import json -from weko_accounts.errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, UserNotFoundError, InvalidPasswordError, DisabledUserError +from weko_accounts.errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, \ + InvalidCredentialsError, InvalidLoginRequestError, DisabledUserError +from weko_accounts.utils import limiter # .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoLogin_post -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp @@ -57,8 +59,9 @@ def test_WekoLogin_post(app, client, users_login): content_type='application/json', ) res_data = json.loads(res.get_data()) - assert res.status_code == UserNotFoundError.code - assert res_data['message'] == UserNotFoundError.description + assert res.status_code == InvalidCredentialsError.code + assert res_data['message'] == InvalidCredentialsError.description + unknown_user_body = res.get_data() # Invalid password : 403 error req_json = { @@ -71,8 +74,10 @@ def test_WekoLogin_post(app, client, users_login): content_type='application/json', ) res_data = json.loads(res.get_data()) - assert res.status_code == InvalidPasswordError.code - assert res_data['message'] == InvalidPasswordError.description + assert res.status_code == InvalidCredentialsError.code + assert res_data['message'] == InvalidCredentialsError.description + # Unknown account and wrong password are indistinguishable + assert res.get_data() == unknown_user_body # Inactive user : 403 error req_json = { @@ -118,6 +123,57 @@ def test_WekoLogin_post(app, client, users_login): assert res_data['message'] == UserAllreadyLoggedInError.description +# .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoLogin_post_invalid_body -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp +def test_WekoLogin_post_invalid_body(app, client, users_login): + """Malformed request bodies are rejected with 400.""" + version = 'v1' + bodies = [ + None, + [], + {}, + {'email': users_login[5]['email']}, + {'password': 'dummy'}, + {'email': users_login[5]['email'], 'password': 1}, + {'email': ['a'], 'password': 'dummy'}, + {'email': '', 'password': ''}, + ] + for body in bodies: + res = client.post( + f'/{version}/login', + data=json.dumps(body), + content_type='application/json', + ) + res_data = json.loads(res.get_data()) + assert res.status_code == InvalidLoginRequestError.code + assert res_data['message'] == InvalidLoginRequestError.description + + # Body that is not JSON at all + res = client.post( + f'/{version}/login', + data='not json', + content_type='application/json', + ) + assert res.status_code == InvalidLoginRequestError.code + + +# .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoAccountsREST_limiter -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp +def test_WekoAccountsREST_limiter(instance_path): + """REST-only application also gets the rate limiter.""" + from flask import Flask + from weko_accounts import WekoAccountsREST + + app_ = Flask('testapi', instance_path=instance_path) + before = len(app_.before_request_funcs.get(None, [])) + ext = WekoAccountsREST(app_) + assert app_.extensions.get('limiter') is limiter + after = len(app_.before_request_funcs.get(None, [])) + assert after > before + + # Initializing again on the same app does not register the hook twice + ext.init_limiter(app_) + assert len(app_.before_request_funcs.get(None, [])) == after + + # .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoLogout_post -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp def test_WekoLogout_post(app, client, users_login): """Test WekoLogout.post method.""" diff --git a/modules/weko-accounts/weko_accounts/errors.py b/modules/weko-accounts/weko_accounts/errors.py index cdd4faee75..798321c0ed 100644 --- a/modules/weko-accounts/weko_accounts/errors.py +++ b/modules/weko-accounts/weko_accounts/errors.py @@ -55,6 +55,20 @@ class InvalidPasswordError(RESTException): description = 'Invalid password.' +class InvalidCredentialsError(RESTException): + """Email or password is incorrect.""" + + code = 403 + description = 'Invalid email or password.' + + +class InvalidLoginRequestError(RESTException): + """Login request body is malformed.""" + + code = 400 + description = 'Invalid request.' + + class DisabledUserError(RESTException): """Account is disabled.""" diff --git a/modules/weko-accounts/weko_accounts/ext.py b/modules/weko-accounts/weko_accounts/ext.py index ca051f966c..7ecda57e6e 100644 --- a/modules/weko-accounts/weko_accounts/ext.py +++ b/modules/weko-accounts/weko_accounts/ext.py @@ -137,6 +137,7 @@ def init_limiter(self, app): """ from .utils import limiter limiter.init_app(app) + app.extensions.setdefault('limiter', limiter) def init_login(self, app): """Initialize login context processor. @@ -171,8 +172,23 @@ def init_app(self, app): blueprint = create_blueprint(app, app.config['WEKO_ACCOUNTS_REST_ENDPOINTS']) app.register_blueprint(blueprint) app.extensions['weko_accounts_rest'] = self + self.init_limiter(app) self.init_unauthorized_handler(app) + def init_limiter(self, app): + """Initialize rate limiting for the REST application. + + The limiter is shared with :class:`WekoAccounts`; skip it when the + same application has already been initialized by that extension. + + :param app: An instance of :class:`flask.Flask`. + """ + from .utils import limiter + if app.extensions.get('limiter') is limiter: + return + limiter.init_app(app) + app.extensions.setdefault('limiter', limiter) + def init_unauthorized_handler(self, app): """Return 401 JSON instead of redirecting to the login screen. diff --git a/modules/weko-accounts/weko_accounts/rest.py b/modules/weko-accounts/weko_accounts/rest.py index 0fb97c1436..ff62def689 100644 --- a/modules/weko-accounts/weko_accounts/rest.py +++ b/modules/weko-accounts/weko_accounts/rest.py @@ -25,14 +25,15 @@ from flask import Blueprint, current_app, jsonify, request, make_response from flask_login import login_user, logout_user from flask_security import current_user -from flask_security.utils import verify_password +from flask_security.utils import hash_password, verify_password from invenio_accounts.models import User from invenio_db import db from invenio_rest import ContentNegotiatedMethodView from weko_logging.activity_logger import UserActivityLogger -from .errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, UserNotFoundError, InvalidPasswordError, DisabledUserError +from .errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, \ + InvalidCredentialsError, InvalidLoginRequestError, DisabledUserError from .utils import limiter @@ -113,9 +114,14 @@ def post(self, **kwargs): def post_v1(self, **kwargs): - data = request.get_json() - email = data['email'] - password = data['password'] + data = request.get_json(silent=True) + if not isinstance(data, dict): + raise InvalidLoginRequestError() + email = data.get('email') + password = data.get('password') + if not isinstance(email, str) or not isinstance(password, str) \ + or not email or not password: + raise InvalidLoginRequestError() # Check if user is already logged in if current_user.is_authenticated: @@ -123,11 +129,14 @@ def post_v1(self, **kwargs): # Get User user = User.query.filter_by(email=email).first() - if not user: - raise UserNotFoundError() + if not user or not user.password: + # Spend the same hashing cost as a real check so that the + # response does not depend on whether the account exists. + hash_password(password) + raise InvalidCredentialsError() # Verify password if not verify_password(password, user.password): - raise InvalidPasswordError() + raise InvalidCredentialsError() # Check if user is active if not user.active: From 59fa15d2fa0b9f2269badb64d925881cbcee0023 Mon Sep 17 00:00:00 2001 From: Masaharu Hayashi Date: Mon, 28 Sep 2026 22:34:36 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(weko-accounts):=20=E5=9B=9E=E6=95=B0?= =?UTF-8?q?=E5=88=B6=E9=99=90=E3=82=92=E3=83=AD=E3=82=B0=E3=82=A4=E3=83=B3?= =?UTF-8?q?=20API=20=E3=81=A0=E3=81=91=E3=81=AB=E3=81=8B=E3=81=91=E3=82=8B?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 直前のコミットは、UI 側と共有の limiter を /api アプリにも初期化したため、 既定の制限(WEKO_API_LIMIT_RATE_DEFAULT)が /api 配下の全経路にかかって いた。検索・統計・ファイル・IIIF の画像などは、同じ IP を共有する環境で 通常の閲覧でも制限に達しうる。 - 既定の制限を持たないログイン専用の Limiter を /api アプリにだけ初期化し、 ログイン API にだけ付ける。制限値は WEKO_API_LIMIT_RATE_DEFAULT を使う - 共有の limiter は従来どおり /api アプリには初期化しない - Flask-Limiter はビュー関数の名前で制限を照合するため、MethodView の メソッドではなく decorators 属性で as_view() の関数に付ける (メソッドに付けた従来の指定は名前が合わず効いていなかった) Co-Authored-By: Claude Opus 5.5 --- modules/weko-accounts/tests/test_rest.py | 45 ++++++++++++++++++-- modules/weko-accounts/weko_accounts/ext.py | 15 +++---- modules/weko-accounts/weko_accounts/rest.py | 7 ++- modules/weko-accounts/weko_accounts/utils.py | 18 ++++++++ 4 files changed, 70 insertions(+), 15 deletions(-) diff --git a/modules/weko-accounts/tests/test_rest.py b/modules/weko-accounts/tests/test_rest.py index de261b52ae..b54cd59028 100644 --- a/modules/weko-accounts/tests/test_rest.py +++ b/modules/weko-accounts/tests/test_rest.py @@ -24,7 +24,6 @@ from weko_accounts.errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, \ InvalidCredentialsError, InvalidLoginRequestError, DisabledUserError -from weko_accounts.utils import limiter # .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoLogin_post -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp @@ -158,21 +157,59 @@ def test_WekoLogin_post_invalid_body(app, client, users_login): # .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoAccountsREST_limiter -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp def test_WekoAccountsREST_limiter(instance_path): - """REST-only application also gets the rate limiter.""" + """Only the login API of the REST application is rate limited.""" + import os + from flask import Flask + from invenio_db import db as db_ from weko_accounts import WekoAccountsREST + from weko_accounts.utils import login_limiter app_ = Flask('testapi', instance_path=instance_path) + app_.config.update( + WEKO_API_LIMIT_RATE_DEFAULT=['2 per minute'], + # the teardown of the REST blueprint commits the db session. + # sqlite is not used, because invenio-db registers sqlite settings + # on every engine and breaks the other tests + SQLALCHEMY_DATABASE_URI=os.getenv( + 'SQLALCHEMY_DATABASE_URI', + 'postgresql+psycopg2://invenio:dbpass123@postgresql:5432/wekotest'), + SQLALCHEMY_TRACK_MODIFICATIONS=False, + ) + db_.init_app(app_) + # Flask-Limiter registers the limit every time as_view() applies the + # decorator, so the limits of the apps made by the other tests pile up. + # A real process makes the REST application only once. + login_limiter._dynamic_route_limits.clear() before = len(app_.before_request_funcs.get(None, [])) ext = WekoAccountsREST(app_) - assert app_.extensions.get('limiter') is limiter + + # the shared limiter, whose default limits apply to every endpoint, + # is not initialized on the REST application + assert app_.extensions.get('limiter') is login_limiter after = len(app_.before_request_funcs.get(None, [])) - assert after > before + assert after == before + 1 # Initializing again on the same app does not register the hook twice ext.init_limiter(app_) assert len(app_.before_request_funcs.get(None, [])) == after + @app_.route('/other') + def other(): + return 'ok' + + login_limiter.reset() + with app_.test_client() as c: + # the login API is limited by WEKO_API_LIMIT_RATE_DEFAULT + codes = [c.post('/v1/login', data='x', + content_type='application/json').status_code + for _ in range(3)] + assert codes[:2] == [InvalidLoginRequestError.code] * 2 + assert codes[2] == 429 + + # other endpoints are not limited + assert all(c.get('/other').status_code == 200 for _ in range(5)) + # .tox/c1/bin/pytest --cov=weko_accounts tests/test_rest.py::test_WekoLogout_post -vv -s --cov-branch --cov-report=term --basetemp=/code/modules/weko-accounts/.tox/c1/tmp def test_WekoLogout_post(app, client, users_login): diff --git a/modules/weko-accounts/weko_accounts/ext.py b/modules/weko-accounts/weko_accounts/ext.py index 7ecda57e6e..d20c79a1eb 100644 --- a/modules/weko-accounts/weko_accounts/ext.py +++ b/modules/weko-accounts/weko_accounts/ext.py @@ -137,7 +137,6 @@ def init_limiter(self, app): """ from .utils import limiter limiter.init_app(app) - app.extensions.setdefault('limiter', limiter) def init_login(self, app): """Initialize login context processor. @@ -176,18 +175,16 @@ def init_app(self, app): self.init_unauthorized_handler(app) def init_limiter(self, app): - """Initialize rate limiting for the REST application. + """Initialize rate limiting of the login API. - The limiter is shared with :class:`WekoAccounts`; skip it when the - same application has already been initialized by that extension. + Only the login API is limited. The shared limiter of + :class:`WekoAccounts` is not used here, because its default limits + would apply to every endpoint of the REST application. :param app: An instance of :class:`flask.Flask`. """ - from .utils import limiter - if app.extensions.get('limiter') is limiter: - return - limiter.init_app(app) - app.extensions.setdefault('limiter', limiter) + from .utils import login_limiter + login_limiter.init_app(app) def init_unauthorized_handler(self, app): """Return 401 JSON instead of redirecting to the login screen. diff --git a/modules/weko-accounts/weko_accounts/rest.py b/modules/weko-accounts/weko_accounts/rest.py index ff62def689..35e9a37719 100644 --- a/modules/weko-accounts/weko_accounts/rest.py +++ b/modules/weko-accounts/weko_accounts/rest.py @@ -34,7 +34,7 @@ from .errors import VersionNotFoundRESTError, UserAllreadyLoggedInError, \ InvalidCredentialsError, InvalidLoginRequestError, DisabledUserError -from .utils import limiter +from .utils import limiter, login_limit_value, login_limiter def create_blueprint(app, endpoints): @@ -92,11 +92,14 @@ class WekoLogin(ContentNegotiatedMethodView): view_name = '{0}_accounts' + # Flask-Limiter matches limits by the name of the view function, so the + # limit is applied to the function made by as_view(), not to post(). + decorators = [login_limiter.limit(login_limit_value)] + def __init__(self, *args, **kwargs): """Constructor.""" super(WekoLogin, self).__init__(*args, **kwargs) - @limiter.limit('') def post(self, **kwargs): """ Login as weko user. diff --git a/modules/weko-accounts/weko_accounts/utils.py b/modules/weko-accounts/weko_accounts/utils.py index eed4ea6017..6204d9111d 100644 --- a/modules/weko-accounts/weko_accounts/utils.py +++ b/modules/weko-accounts/weko_accounts/utils.py @@ -54,6 +54,24 @@ def api_view(): """ +login_limiter = Limiter( + app=None, + key_func=lambda: f"{request.endpoint}_{get_remote_addr()}", + default_limits=[], +) +"""Limiter only for the login API of the REST application. + +It has no default limits, so only the views decorated with it are limited. +The shared :data:`limiter` is not initialized on the REST application, +because its default limits would apply to every API endpoint. +""" + + +def login_limit_value(): + """Return the rate limit of the login API from the configuration.""" + return ';'.join(current_app.config.get( + 'WEKO_API_LIMIT_RATE_DEFAULT', WEKO_API_LIMIT_RATE_DEFAULT)) + def get_remote_addr(): """