From 1cf73201da0ab6ff2ea3a6b31db0ad820cff97f2 Mon Sep 17 00:00:00 2001 From: Reimar Bauer Date: Thu, 8 Oct 2026 08:58:11 +0200 Subject: [PATCH] Fix the wire quirks of the MSColab schemas, saving, invitations and archived Group operations --- docs/mscolab.rst | 3 + mslib/mscolab/api/schemas.py | 199 ++++++++++-------- mslib/mscolab/app/__init__.py | 3 + mslib/mscolab/auth.py | 13 +- mslib/mscolab/blueprints/auth/__init__.py | 4 +- mslib/mscolab/blueprints/chat/__init__.py | 11 +- .../mscolab/blueprints/operation/__init__.py | 37 ++-- mslib/mscolab/file_manager.py | 54 ++++- mslib/mscolab/sockets_manager.py | 6 +- mslib/msui/mscolab.py | 110 +++++----- mslib/msui/mscolab_admin_window.py | 28 ++- mslib/msui/mscolab_chat.py | 24 +-- mslib/msui/mscolab_version_history.py | 16 +- mslib/msui/multiple_flightpath_dockwidget.py | 10 +- mslib/msui/socket_control.py | 22 +- tests/_test_api/test_schemas.py | 124 +++++++---- tests/_test_mscolab/test_api_contract.py | 66 +++++- tests/_test_mscolab/test_file_manager.py | 56 ++++- tests/_test_mscolab/test_server.py | 25 ++- tests/_test_mscolab/test_sockets_manager.py | 12 ++ tests/_test_msui/test_mscolab.py | 72 +++++-- tests/_test_msui/test_mscolab_admin_window.py | 18 ++ tests/_test_msui/test_mscolab_chat.py | 26 ++- tests/_test_msui/test_mscolab_operation.py | 2 +- tests/_test_msui/test_socket_control.py | 52 +++++ 25 files changed, 690 insertions(+), 303 deletions(-) create mode 100644 tests/_test_msui/test_socket_control.py diff --git a/docs/mscolab.rst b/docs/mscolab.rst index 10198b4b9..7314bc59f 100644 --- a/docs/mscolab.rst +++ b/docs/mscolab.rst @@ -147,6 +147,9 @@ msui gets a login token from the server when the user logs in. It is valid for 1 mscolab_settings.py sets another lifetime in seconds. "Log out everywhere" in the user menu of msui makes all earlier tokens of the user invalid: every msui of the user has to log in again. A password reset does the same. +msui sends the token of GET requests in the header :code:`X-MSColab-Token`, so it doesn't end up in access logs; a +reverse proxy in front of the server has to pass this header on, which nginx does by default. + After a device got lost, reset the password: msui stores the password in the keyring of the device, so "Log out everywhere" alone doesn't keep the device out, it can log in again. The profile dialog of msui has a "Reset Password" button, and the login dialog shows a link after a wrong password; both open the page of the server where you request diff --git a/mslib/mscolab/api/schemas.py b/mslib/mscolab/api/schemas.py index 7785ce2e7..107e8346a 100644 --- a/mslib/mscolab/api/schemas.py +++ b/mslib/mscolab/api/schemas.py @@ -14,8 +14,8 @@ Only migrated routes have a schema here -- see endpoints.py for which ones. This is stdlib dataclasses only (Python <3.12 pin, no new runtime dependency). The type hints are documentation for readers and IDEs - only: nothing checks them at runtime, and ``op_id: object`` marks - fields the server does not convert to int. tests/_test_api/ only checks + only: nothing checks them at runtime; integer ids are converted with + int_field, which raises InvalidRequest. tests/_test_api/ only checks each schema against itself; tests/_test_mscolab/test_api_contract.py checks schemas against the real Flask routes. @@ -39,9 +39,9 @@ """ import json +import re from dataclasses import dataclass, field from typing import List, Optional, Union -from urllib.parse import urlencode # mslib.mscolab.auth.verify_user (the @verify_user decorator wrapping every # authenticated route) returns this bare, non-JSON string -- instead of @@ -52,6 +52,48 @@ # always actually done on the wire. AUTH_FAILED_TEXT = "False" +# msui sends the login token of a GET request in this header, its parameters in +# the query string: a GET request has no body a proxy has to forward, and a +# token in the URL would end up in access logs. Servers up to 11.x read the +# token only from the query string or the body, msui sends it there too until +# their support is dropped. +TOKEN_HEADER = "X-MSColab-Token" + + +class InvalidRequest(ValueError): + """A request field the server can't use, e.g. a missing or non-integer op_id. + + The server answers it with HTTP 400 and the message. + """ + + +# the ids are INTEGER columns, 32 bit on PostgreSQL +_INT_FIELD_RANGE = range(-2 ** 31, 2 ** 31) +_INT_FIELD_PATTERN = re.compile(r"-?[0-9]{1,10}") + + +def int_field(value, name): + """Returns value of the request field name as int, raises InvalidRequest unless it is one. + + Only plain decimal digits are accepted: int() would also take e.g. "1_0" + as 10, " 1 " or other Unicode digits. + """ + if isinstance(value, str) and _INT_FIELD_PATTERN.fullmatch(value): + value = int(value) + if isinstance(value, int) and not isinstance(value, bool) and value in _INT_FIELD_RANGE: + return value + raise InvalidRequest(f"{name} must be an integer") + + +def _int_list_field(value, name): + try: + values = json.loads(value) + except (TypeError, ValueError): + values = None + if not isinstance(values, list): + raise InvalidRequest(f"{name} must be a JSON list of integers") + return [int_field(item, name) for item in values] + def _record_dict(record): # Server routes pass the managers' plain dicts through unchanged, so list @@ -120,7 +162,7 @@ class OperationInfo: path: str description: Optional[str] category: str - active: bool = True + active: bool def to_dict(self): return { @@ -140,10 +182,8 @@ def from_dict(cls, data): path=data["path"], description=data["description"], category=data["category"], - # mslib.msui.mscolab.add_operations_to_ui has always defaulted this - # to True for compatibility with servers older than 8.x that don't - # send "active" at all -- preserved here rather than in the client. - active=data.get("active", True), + # every supported server (8.3.3 and later) sends it + active=data["active"], ) @@ -153,15 +193,14 @@ class GetOperationsRequest: skip_archived: bool = False - def to_form_data(self): + def to_params(self): return {"skip_archived": str(self.skip_archived)} @classmethod def from_args_and_form(cls, args, form): """Mirrors the route's ``request.args.get(x, request.form.get(x, default))`` - fallback chain -- request_get() puts its payload in the request body, - so request.args is normally empty, but the handler has always checked - both. + fallback chain -- msui sends the parameters of a GET request in the + query string, msui up to 11.x sent them in the body. """ return cls(skip_archived=args.get("skip_archived", form.get("skip_archived", "False")) == "True") @@ -193,15 +232,12 @@ class GetOperationByIdRequest: op_id: int - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id} @classmethod def from_args_and_form(cls, args, form): - # int(None) raises TypeError here, same as the route's previous bare - # request.args/request.form reads -- a missing op_id was never - # validated any more gracefully than that. - return cls(op_id=int(args.get("op_id", form.get("op_id", None)))) + return cls(op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id")) @dataclass @@ -238,9 +274,9 @@ def to_form_data(self): @classmethod def from_form(cls, form): - # int(...) on a missing op_id defaults to 0 -- matches the route's - # previous request.form.get('op_id', 0). - return cls(op_id=int(form.get("op_id", 0))) + # a missing op_id defaults to 0 -- matches the route's previous + # request.form.get('op_id', 0). + return cls(op_id=int_field(form.get("op_id", 0), "op_id")) @dataclass @@ -285,10 +321,10 @@ def to_form_data(self): @classmethod def from_form(cls, form): - # int(None) raises TypeError on a missing op_id, and form["attribute"]/ - # form["value"] raise KeyError -- matches the route's previous bare - # request.form reads exactly. - return cls(op_id=int(form.get("op_id", None)), attribute=form["attribute"], value=form["value"]) + # form["attribute"]/form["value"] raise KeyError -- matches the route's + # previous bare request.form reads exactly. + return cls(op_id=int_field(form.get("op_id", None), "op_id"), attribute=form["attribute"], + value=form["value"]) @dataclass @@ -307,21 +343,16 @@ def from_text(cls, text): @dataclass class GetCreatorOfOperationRequest: - """GET endpoints.GET_CREATOR_OF_OPERATION. + """GET endpoints.GET_CREATOR_OF_OPERATION.""" - Unlike the other op_id-taking routes, this one never casts op_id to int - before querying the DB (SQLite tolerates the string; preserved as-is - rather than "fixed" here). - """ - - op_id: object + op_id: int - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id} @classmethod def from_args_and_form(cls, args, form): - return cls(op_id=args.get("op_id", form.get("op_id", None))) + return cls(op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id")) @dataclass @@ -330,8 +361,9 @@ class GetCreatorOfOperationResponse: HTTP 200 + {"success": True, "username": ...} on success, HTTP 403 + {"success": False, "message": ...} if the caller isn't a member of the - operation. A rejected token is answered by @verify_user with HTTP 200 and - bare "False" (see AUTH_FAILED_TEXT), for which from_text() returns None. + operation; HTTP 400 for a missing or non-integer op_id. A rejected token + is answered by @verify_user with HTTP 200 and bare "False" (see + AUTH_FAILED_TEXT), for which from_text() returns None. mslib.msui.mscolab.view_description never parses either failure: request_get() raises MSColabConnectionError on any non-200 status and MSColabSessionExpiredError on AUTH_FAILED_TEXT. @@ -369,7 +401,8 @@ def to_form_data(self): @classmethod def from_form(cls, form): - return cls(op_id=int(form.get("op_id")), user_ids=json.loads(form.get("selected_userids", "[]"))) + return cls(op_id=int_field(form.get("op_id"), "op_id"), + user_ids=_int_list_field(form.get("selected_userids", "[]"), "selected_userids")) @dataclass @@ -464,29 +497,17 @@ def from_text(cls, text): @dataclass -class UploadProfileImageRequest: - """POST endpoints.UPLOAD_PROFILE_IMAGE (multipart, plus an "image" file - field the client attaches separately -- not part of this dataclass). +class FetchProfileImageRequest: + """GET endpoints.FETCH_PROFILE_IMAGE. - user_id is sent by the client but ignored server-side (the route uses - g.user.id from the auth token instead) -- preserved as-is here, since - it's still part of the actual wire payload today. + FileManager.get_user_profile_image converts user_id and op_id to int and + refuses values it can't convert. """ - user_id: int - - def to_form_data(self): - return {"user_id": self.user_id} - - -@dataclass -class FetchProfileImageRequest: - """GET endpoints.FETCH_PROFILE_IMAGE.""" - user_id: int op_id: Optional[int] = None - def to_form_data(self): + def to_params(self): data = {"user_id": self.user_id} if self.op_id is not None: data["op_id"] = self.op_id @@ -494,7 +515,8 @@ def to_form_data(self): @classmethod def from_args_and_form(cls, args, form): - return cls(user_id=form["user_id"], op_id=args.get("op_id", form.get("op_id", None))) + return cls(user_id=args.get("user_id", form.get("user_id", None)), + op_id=args.get("op_id", form.get("op_id", None))) @dataclass @@ -559,12 +581,12 @@ class GetOperationUsersRequest: op_id: int - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id} @classmethod def from_args_and_form(cls, args, form): - return cls(op_id=int(args.get("op_id", form.get("op_id", None)))) + return cls(op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id")) @dataclass @@ -616,8 +638,8 @@ def to_form_data(self): @classmethod def from_form(cls, form): return cls( - op_id=int(form.get("op_id")), - user_ids=json.loads(form.get("selected_userids", "[]")), + op_id=int_field(form.get("op_id"), "op_id"), + user_ids=_int_list_field(form.get("selected_userids", "[]"), "selected_userids"), access_level=form.get("selected_access_level"), ) @@ -657,8 +679,8 @@ def to_form_data(self): @classmethod def from_form(cls, form): return cls( - current_op_id=int(form.get("current_op_id")), - import_op_id=int(form.get("import_op_id")), + current_op_id=int_field(form.get("current_op_id"), "current_op_id"), + import_op_id=int_field(form.get("import_op_id"), "import_op_id"), ) @@ -714,12 +736,12 @@ class GetAuthorizedUsersRequest: op_id: int - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id} @classmethod def from_args_and_form(cls, args, form): - return cls(op_id=int(args.get("op_id", form.get("op_id", None)))) + return cls(op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id")) @dataclass @@ -748,12 +770,12 @@ class GetActiveUsersRequest: op_id: int - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id} @classmethod def from_args_and_form(cls, args, form): - return cls(op_id=int(args.get("op_id", form.get("op_id", None)))) + return cls(op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id")) @dataclass @@ -810,25 +832,20 @@ def from_dict(cls, data): class GetAllChangesRequest: """GET endpoints.GET_ALL_CHANGES. - named_version is sent as a real URL query parameter (?named_version=...); - op_id (and token) go in the request body via requests' data= -- two - separate wire channels, preserved as-is rather than merged into one, - since that's the actual client behavior today. + msui up to 11.x sent op_id in the body and named_version in the query + string, which is why the server reads named_version only from there. """ op_id: int named_version: bool = False - def to_form_data(self): - return {"op_id": self.op_id} - - def to_query_string(self): - return urlencode({"named_version": self.named_version}) + def to_params(self): + return {"op_id": self.op_id, "named_version": str(self.named_version)} @classmethod def from_args_and_form(cls, args, form): return cls( - op_id=int(args.get("op_id", form.get("op_id", None))), + op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id"), named_version=args.get("named_version") == "True", ) @@ -861,12 +878,12 @@ class GetChangeContentRequest: ch_id: int - def to_form_data(self): + def to_params(self): return {"ch_id": self.ch_id} @classmethod def from_args_and_form(cls, args, form): - return cls(ch_id=int(args.get("ch_id", form.get("ch_id", 0)))) + return cls(ch_id=int_field(args.get("ch_id", form.get("ch_id", 0)), "ch_id")) @dataclass @@ -918,8 +935,8 @@ def to_form_data(self): @classmethod def from_form(cls, form): return cls( - op_id=int(form.get("op_id", 0)), - ch_id=int(form.get("ch_id", 0)), + op_id=int_field(form.get("op_id", 0), "op_id"), + ch_id=int_field(form.get("ch_id", 0), "ch_id"), version_name=form.get("version_name", None), ) @@ -953,7 +970,7 @@ def to_form_data(self): @classmethod def from_form(cls, form): - return cls(ch_id=int(form.get("ch_id", -1))) + return cls(ch_id=int_field(form.get("ch_id", -1), "ch_id")) @dataclass @@ -1014,6 +1031,8 @@ def from_text(cls, text): data = json.loads(text) except json.decoder.JSONDecodeError: data = {} + if not isinstance(data, dict): + data = {} return cls( message=data.get("message", ""), use_saml2=data.get("use_saml2", False), @@ -1192,22 +1211,18 @@ def from_dict(cls, data): @dataclass class GetMessagesRequest: - """GET endpoints.MESSAGES. + """GET endpoints.MESSAGES.""" - op_id is never cast to int server-side (same as - GetCreatorOfOperationRequest.op_id) -- preserved, not "fixed". - """ - - op_id: object + op_id: int timestamp: str = "1970-01-01T00:00:00+00:00" - def to_form_data(self): + def to_params(self): return {"op_id": self.op_id, "timestamp": self.timestamp} @classmethod def from_args_and_form(cls, args, form): return cls( - op_id=args.get("op_id", form.get("op_id", None)), + op_id=int_field(args.get("op_id", form.get("op_id", None)), "op_id"), timestamp=args.get("timestamp", form.get("timestamp", "1970-01-01T00:00:00+00:00")), ) @@ -1243,11 +1258,11 @@ class MessageAttachmentRequest: """POST endpoints.MESSAGE_ATTACHMENT (multipart, plus a "file" field the client attaches separately -- not part of this dataclass). - op_id is never cast to int server-side (same pattern as - GetCreatorOfOperationRequest.op_id) -- preserved, not "fixed". + op_id is also the name of the upload folder of the attachment, the + route serving attachments only accepts it as digits. """ - op_id: object + op_id: int message_type: Optional[int] def to_form_data(self): @@ -1257,8 +1272,8 @@ def to_form_data(self): def from_form(cls, form): # a missing message_type must not fail before the route's membership check message_type = form.get("message_type") - return cls(op_id=form.get("op_id", None), - message_type=int(message_type) if message_type is not None else None) + return cls(op_id=int_field(form.get("op_id", None), "op_id"), + message_type=int_field(message_type, "message_type") if message_type is not None else None) @dataclass diff --git a/mslib/mscolab/app/__init__.py b/mslib/mscolab/app/__init__.py index c0f380729..42c3f4b18 100644 --- a/mslib/mscolab/app/__init__.py +++ b/mslib/mscolab/app/__init__.py @@ -41,6 +41,7 @@ from flask_sqlalchemy import SQLAlchemy +from mslib.mscolab.api.schemas import InvalidRequest from mslib.mscolab.conf import mscolab_settings from mslib.mscolab import migrations from mslib.utils import prefix_route, release_info @@ -374,6 +375,8 @@ def create_app(config_object=mscolab_settings): check_secrets(app) check_basic_auth_setting(config_object, "mscolab_settings") app.register_error_handler(PasswordCheckBusy, lambda error: (str(error), 503, {"Retry-After": "10"})) + # e.g. a missing or non-integer op_id + app.register_error_handler(InvalidRequest, lambda error: (str(error), 400)) # uploads are limited to MAX_UPLOAD_SIZE, other requests to MAX_CONTENT_LENGTH app.before_request(limit_request_size) app.register_error_handler(413, error413) diff --git a/mslib/mscolab/auth.py b/mslib/mscolab/auth.py index 1aa752078..0de0ab761 100644 --- a/mslib/mscolab/auth.py +++ b/mslib/mscolab/auth.py @@ -37,6 +37,7 @@ from flask import current_app, request, abort, g, url_for from itsdangerous import URLSafeTimedSerializer, BadSignature +from mslib.mscolab.api.schemas import TOKEN_HEADER from mslib.mscolab.conf import setup_saml2_backend from mslib.mscolab.models import User @@ -92,12 +93,22 @@ def register_user(email, password, username, fullname): return {"success": result} +def request_token(): + """ + Returns the login token of the request, False if there is none + + msui sends it in the header TOKEN_HEADER with GET requests, in the body with others; older clients sent it in + the query string or the body. + """ + return request.headers.get(TOKEN_HEADER) or request.args.get('token', request.form.get('token', False)) + + def _request_user(): """ Returns the user authenticated by the token of the request, None if there is none or it is not confirmed """ try: - user = User.verify_auth_token(request.args.get('token', request.form.get('token', False))) + user = User.verify_auth_token(request_token()) except TypeError: logging.debug("no token in request form") abort(404) diff --git a/mslib/mscolab/blueprints/auth/__init__.py b/mslib/mscolab/blueprints/auth/__init__.py index ab1c47afa..7b6bbb396 100644 --- a/mslib/mscolab/blueprints/auth/__init__.py +++ b/mslib/mscolab/blueprints/auth/__init__.py @@ -38,7 +38,7 @@ from mslib.mscolab.auth import check_login, register_user, generate_confirmation_token, create_or_update_idp_user, \ get_idp_entity_id, confirm_token, generate_password_reset_token, confirm_password_reset_token, public_url_for, \ EMAIL_CONFIRMATION, IDP_LOGIN -from mslib.mscolab.auth import optional_auth +from mslib.mscolab.auth import optional_auth, request_token from mslib.mscolab.conf import setup_saml2_backend from mslib.mscolab.forms import ResetPasswordForm, ResetRequestForm from mslib.mscolab.models import User @@ -96,7 +96,7 @@ def get_auth_token(): @AUTH_BP.route('/test_authorized') def authorized(): - token = request.args.get('token', request.form.get('token')) + token = request_token() user = User.verify_auth_token(token) if user is not None: if current_app.config['MAIL_ENABLED']: diff --git a/mslib/mscolab/blueprints/chat/__init__.py b/mslib/mscolab/blueprints/chat/__init__.py index 8a3c813d9..6734bed40 100644 --- a/mslib/mscolab/blueprints/chat/__init__.py +++ b/mslib/mscolab/blueprints/chat/__init__.py @@ -42,6 +42,8 @@ GetMessagesResponse, MessageAttachmentRequest, MessageAttachmentResponse, + InvalidRequest, + int_field, ) CHAT_BP = Blueprint('chat', __name__) @@ -128,9 +130,12 @@ def uploads(name=None, filename=None): abort(404) if filename is None: abort(404) - # name is the id of the operation the attachment was sent to, only its members may fetch it, - # isdigit alone accepts digits like "²" which int() refuses - if not (name.isascii() and name.isdigit()) or not current_app.extensions['fm'].is_member(g.user.id, int(name)): + # name is the id of the operation the attachment was sent to, only its members may fetch it + try: + op_id = int_field(name, "name") + except InvalidRequest: + abort(404) + if not current_app.extensions['fm'].is_member(g.user.id, op_id): abort(404) # attachments are uploaded by any member, e.g. HTML or SVG files, a browser must not render them # as a page of the MSColab server diff --git a/mslib/mscolab/blueprints/operation/__init__.py b/mslib/mscolab/blueprints/operation/__init__.py index d4c18d5e7..083db423d 100644 --- a/mslib/mscolab/blueprints/operation/__init__.py +++ b/mslib/mscolab/blueprints/operation/__init__.py @@ -30,7 +30,7 @@ from flask import Blueprint, request, g, jsonify, current_app -from mslib.mscolab.auth import verify_user +from mslib.mscolab.auth import request_token, verify_user from mslib.mscolab.api import endpoints from mslib.mscolab.api.schemas import ( BulkPermissionsRequest, @@ -65,6 +65,7 @@ UndoChangesResponse, UpdateOperationRequest, UpdateOperationResponse, + int_field, ) OPERATION_BP = Blueprint('operation', __name__) @@ -84,7 +85,7 @@ def create_operation(): sockio = current_app.extensions['sockio'] # the members of the group operation of the category got permissions too sockio.sm.sync_rooms() - token = request.args.get('token', request.form.get('token', False)) + token = request_token() json_config = {"token": token} sockio.sm.update_operation_list(json_config) return response.to_text() @@ -144,11 +145,7 @@ def set_version_name(): @verify_user def authorized_users(): fm = current_app.extensions['fm'] - try: - req = GetAuthorizedUsersRequest.from_args_and_form(request.args, request.form) - except (TypeError, ValueError): - # a missing or non-numeric op_id - return "False", 400 + req = GetAuthorizedUsersRequest.from_args_and_form(request.args, request.form) if not fm.is_member(g.user.id, req.op_id): return "False", 403 return GetAuthorizedUsersResponse(users=fm.get_authorized_users(req.op_id)).to_text() @@ -159,11 +156,7 @@ def authorized_users(): def active_users(): fm = current_app.extensions['fm'] sockio = current_app.extensions['sockio'] - try: - req = GetActiveUsersRequest.from_args_and_form(request.args, request.form) - except (TypeError, ValueError): - # a missing or non-numeric op_id - return "False", 400 + req = GetActiveUsersRequest.from_args_and_form(request.args, request.form) if not fm.is_member(g.user.id, req.op_id): return "False", 403 # an operation nobody has selected yet has no entry @@ -205,10 +198,12 @@ def update_operation(): success = fm.update_operation(req.op_id, req.attribute, req.value, user) if success is True: sockio = current_app.extensions['sockio'] - if req.attribute == "path" and req.value.endswith(current_app.config['GROUP_POSTFIX']): - # the permissions of the group operation were imported into all operations of its category + if req.attribute == "path" and req.value.endswith(current_app.config['GROUP_POSTFIX']) or \ + req.attribute == "active": + # the permissions of the group operation were imported into all operations of its category, also + # when it was unarchived sockio.sm.sync_rooms() - token = request.args.get('token', request.form.get('token', False)) + token = request_token() json_config = {"token": token} sockio.sm.update_operation_list(json_config) return UpdateOperationResponse(success=success).to_text() @@ -218,9 +213,9 @@ def update_operation(): @verify_user def get_operation_details(): fm = current_app.extensions['fm'] - op_id = request.args.get('op_id', request.form.get('op_id', None)) + op_id = int_field(request.args.get('op_id', request.form.get('op_id', None)), "op_id") user = g.user - result = fm.get_operation_details(int(op_id), user) + result = fm.get_operation_details(op_id, user) if result is False: return "False" return json.dumps(result) @@ -229,15 +224,15 @@ def get_operation_details(): @OPERATION_BP.route('/set_last_used', methods=["POST"]) @verify_user def set_last_used(): - op_id = request.form.get('op_id', None) + op_id = int_field(request.form.get('op_id', None), "op_id") user = g.user - days_ago = int(request.form.get('days', 0)) + days_ago = int_field(request.form.get('days', 0), "days") if days_ago > 99999: days_ago = 99999 elif days_ago < -99999: days_ago = -99999 fm = current_app.extensions['fm'] - fm.update_operation(int(op_id), 'last_used', + fm.update_operation(op_id, 'last_used', datetime.datetime.now(tz=datetime.timezone.utc) - datetime.timedelta(days=days_ago), user) return jsonify({"success": True}), 200 @@ -380,7 +375,7 @@ def import_permissions(): # invalidate waypoint table, title of windows sockio.sm.emit_revoke_permission(u_id, req.current_op_id) - token = request.args.get('token', request.form.get('token', False)) + token = request_token() json_config = {"token": token} sockio.sm.update_operation_list(json_config) diff --git a/mslib/mscolab/file_manager.py b/mslib/mscolab/file_manager.py index 5fbbe292c..71ab70d18 100644 --- a/mslib/mscolab/file_manager.py +++ b/mslib/mscolab/file_manager.py @@ -111,8 +111,9 @@ def create_operation(self, path, description, user, last_used=None, content=None # here we can import the permissions from Group file if not path.endswith(current_app.config['GROUP_POSTFIX']): import_op = Operation.query.filter_by(path=f"{category}{current_app.config['GROUP_POSTFIX']}").first() - # only for the creator of the Group operation, admin can be granted by others without consent - if import_op is not None and self.is_creator(user.id, import_op.id): + # only for the creator of the Group operation, admin can be granted by others without consent; + # an archived Group operation is not used + if import_op is not None and import_op.active and self.is_creator(user.id, import_op.id): self.import_permissions(import_op.id, operation_id, user.id) operation_dir.mkdir(parents=True, exist_ok=True) @@ -436,6 +437,7 @@ def update_operation(self, op_id, attribute, value, user): elif attribute == "active": if isinstance(value, str): value = value.upper() == "TRUE" + unarchived = attribute == "active" and value and not operation.active setattr(operation, attribute, value) db.session.commit() if attribute == "path": @@ -443,8 +445,28 @@ def update_operation(self, op_id, attribute, value, user): # the user manages for ops in self._group_member_operations(operation, user): self.import_permissions(op_id, ops.id, user.id) + elif unarchived: + self._import_unarchived_group_operation(operation) return True + def _import_unarchived_group_operation(self, operation): + """ + An archived Group operation is not used for the members of its category. Once unarchived, its members + are imported into the operations of its category its creator created, as for a new operation. + """ + if not self._is_group_operation(operation): + return + creator = Permission.query.filter_by(op_id=operation.id, access_level="creator").first() + if creator is None: + return + category_operations = Operation.query \ + .join(Permission, Permission.op_id == Operation.id) \ + .filter(Operation.category == self._group_category(operation), Operation.id != operation.id, + Permission.u_id == creator.u_id, Permission.access_level == "creator").all() + for ops in category_operations: + if not self._is_group_operation(ops): + self.import_permissions(operation.id, ops.id, creator.u_id) + def delete_operation(self, op_id, user): """ op_id: operation id @@ -483,6 +505,9 @@ def save_file(self, op_id, content, user, version_name=None, comment=""): op_id: operation-id, content: content of the file to be saved # ToDo save change in schema + + Returns True if the flight track was saved, None if content is the stored flight track already, and False + if it was refused, e.g. for invalid waypoints or version name. """ if not verify_waypoint_data(content): return False @@ -528,7 +553,8 @@ def save_file(self, op_id, content, user, version_name=None, comment=""): db.session.add(change) db.session.commit() return True - return False + # msui sends some changes twice, nothing to save is no error + return None def get_file(self, op_id, user): """ @@ -699,16 +725,25 @@ def _group_member_operations(self, operation, user, managed_only=True): Changes to the members of the Group operation fan out only to these operations, so managing a Group operation never grants rights beyond those the user already has on the operations of that category. With managed_only=False all other operations of that category are returned, e.g. for a user leaving. + An archived Group operation is not used for the members of its category, nothing is returned for it. """ - postfix = current_app.config['GROUP_POSTFIX'] - if not postfix or operation is None or not operation.path.endswith(postfix): + if operation is None or not operation.active or not self._is_group_operation(operation): return [] - category = operation.path[:-len(postfix)] - query = Operation.query.filter(Operation.category == category, Operation.id != operation.id) + query = Operation.query.filter(Operation.category == self._group_category(operation), + Operation.id != operation.id) if managed_only: query = query.join(Permission, Permission.op_id == Operation.id) \ .filter(Permission.u_id == user.id, Permission.access_level.in_(("admin", "creator"))) - return [ops for ops in query if not ops.path.endswith(postfix)] + return [ops for ops in query if not self._is_group_operation(ops)] + + @staticmethod + def _is_group_operation(operation): + postfix = current_app.config['GROUP_POSTFIX'] + return bool(postfix) and operation.path.endswith(postfix) + + @staticmethod + def _group_category(operation): + return operation.path[:-len(current_app.config['GROUP_POSTFIX'])] def add_bulk_permission(self, op_id, user, new_u_ids, access_level): """ @@ -818,6 +853,9 @@ def import_permissions(self, import_op_id, current_op_id, u_id): perm = Permission.query.filter_by(u_id=u_id, op_id=import_op_id).first() if perm is None: return False, None, "Not a member of this operation" + # e.g. an archived Group operation is not used for the members of its category + if not db.session.get(Operation, import_op_id).active: + return False, None, "The operation is archived, its permissions can't be imported" existing_perms = Permission.query \ .filter(Permission.op_id == current_op_id) \ diff --git a/mslib/mscolab/sockets_manager.py b/mslib/mscolab/sockets_manager.py index 575a4b27d..eff136706 100644 --- a/mslib/mscolab/sockets_manager.py +++ b/mslib/mscolab/sockets_manager.py @@ -388,14 +388,16 @@ def handle_file_save(self, json_req): # e.g. the operation was archived while the user was editing it self._refuse_file_save(op_id, "You can't change this operation (any more), e.g. because it " "was archived or your access level is viewer.") - elif self.fm.save_file(int(op_id), content, user, version_name=version_name, comment=comment): + return + saved = self.fm.save_file(int(op_id), content, user, version_name=version_name, comment=comment) + if saved: # send service message message_ = f"[service message] **{user.username}** saved changes. {messageText}" new_message = self.cm.add_message(user, message_, str(op_id), message_type=MessageType.SYSTEM_MESSAGE) self.emit_chat_message(new_message) # emit file-changed event to trigger reload of flight track self._emit_to_operation(SocketEvents.FILE_CHANGED, op_id, json.dumps({"op_id": op_id, "u_id": user.id})) - else: + elif saved is False: self._refuse_file_save(op_id, "The server could not save the flight track, e.g. because its " "waypoints or its version name are invalid.") else: diff --git a/mslib/msui/mscolab.py b/mslib/msui/mscolab.py index 4e6d8cdaf..f54f464a4 100644 --- a/mslib/msui/mscolab.py +++ b/mslib/msui/mscolab.py @@ -73,7 +73,6 @@ LoginResponse, UpdateOperationRequest, UpdateOperationResponse, - UploadProfileImageRequest, ) from PyQt5 import QtCore, QtGui, QtWidgets @@ -202,6 +201,8 @@ def __init__(self, parent=None, local_operations_data=None): self.operations = None # store active_flight_path here as object self.waypoints_model = None + # model, operation and content of the last flight track sent to the server + self.last_sent_flight_track = None # Store active operation's file path self.local_ftml_file = None # Store active_operation_description @@ -257,7 +258,7 @@ def _activate_first_local_flighttrack(self): def view_description(self, _=None): req = GetCreatorOfOperationRequest(op_id=self.active_op_id) try: - response = self.conn.request_get(endpoints.GET_CREATOR_OF_OPERATION, req.to_form_data()) + response = self.conn.request_get(endpoints.GET_CREATOR_OF_OPERATION, req.to_params()) except MSColabSessionExpiredError as ex: show_popup(self.ui, "Error", str(ex)) self.logout() @@ -387,7 +388,7 @@ def set_profile_pixmap(self, img_data): def fetch_profile_image(self, refresh=False): req = FetchProfileImageRequest(user_id=str(self.user["id"])) try: - response = self.conn.request_get(endpoints.FETCH_PROFILE_IMAGE, req.to_form_data()) + response = self.conn.request_get(endpoints.FETCH_PROFILE_IMAGE, req.to_params()) except MSColabConnectionError: self.fetch_gravatar(refresh) else: @@ -528,11 +529,10 @@ def upload_image(self, _=None): # Prepare the file data for upload try: img_byte_arr.seek(0) # Reset buffer position - req = UploadProfileImageRequest(user_id=str(self.user["id"])) + # the image is stored for the user of the token response = self.conn.request_post( endpoints.UPLOAD_PROFILE_IMAGE, - req.to_form_data(), - {'image': (os.path.basename(file_name), img_byte_arr, mime_type)}) + files={'image': (os.path.basename(file_name), img_byte_arr, mime_type)}) # Check response status if response.status_code == 200: @@ -723,7 +723,7 @@ def add_operation(self, path, description, category, f_content): self.ui, "Creation successful", "Your operation was created successfully.", ) - op_id = self.get_recent_op_id() + op_id = self.get_op_id(path) self.new_op_id = op_id self.conn.handle_new_operation(op_id) self.signal_operation_added.emit(op_id, path) @@ -731,22 +731,27 @@ def add_operation(self, path, description, category, f_content): self.error_dialog = QtWidgets.QErrorMessage() self.error_dialog.showMessage('The path already exists') - def get_recent_op_id(self): + def _operations(self): """ - get most recent operation's op_id + Returns the operations of the user, in no particular order """ - logging.debug('get_recent_op_id') - skip_archived = config_loader(dataset="MSCOLAB_skip_archived_operations") - req = GetOperationsRequest(skip_archived=skip_archived) - response = self.conn.request_get(endpoints.OPERATIONS, req.to_form_data()) + response = self.conn.request_get(endpoints.OPERATIONS) parsed = GetOperationsResponse.from_text(response.text) if parsed is None: raise MSColabConnectionError("Session expired, new login required") - op_id = None - if parsed.operations: - op_id = parsed.operations[-1].op_id - logging.debug("recent op_id %s", op_id) - return op_id + return parsed.operations + + def get_op_id(self, path): + """ + Returns the op_id of the operation path of the user, None if the user has none of this path + """ + return next((operation.op_id for operation in self._operations() if operation.path == path), None) + + def get_operation(self, op_id): + """ + Returns the operation op_id of the user, None if the user is no member of it + """ + return next((operation for operation in self._operations() if operation.op_id == op_id), None) def operation_options_handler(self): if self.sender() == self.ui.actionChat: @@ -1152,20 +1157,6 @@ def save_wp_mscolab(self, comment=None): self.merge_dialog.close() self.merge_dialog = None - def get_recent_operation(self): - """ - get most recent operation - """ - logging.debug('get_recent_operation') - response = self.conn.request_get(endpoints.OPERATIONS) - parsed = GetOperationsResponse.from_text(response.text) - if parsed is None: - raise MSColabConnectionError("Session expired, new login required") - recent_operation = None - if parsed.operations: - recent_operation = parsed.operations[-1] - return recent_operation - @QtCore.pyqtSlot() def reload_operation_list(self): if self.mscolab_server_url is not None: @@ -1209,22 +1200,39 @@ def render_new_permission(self, op_id, u_id): parsed = GetUserResponse.from_text(response.text) if parsed is not None: if parsed.user.id == u_id: - operation = self.get_recent_operation() - operation_desc = f'{operation.path} - {operation.access_level}' - widgetItem = QtWidgets.QListWidgetItem(operation_desc, parent=self.ui.listOperationsMSC) - widgetItem.op_id = operation.op_id - widgetItem.operation_category = operation.category - widgetItem.operation_path = operation.path - widgetItem.access_level = operation.access_level - widgetItem.active_operation_description = operation.description - self.ui.listOperationsMSC.addItem(widgetItem) - self.signal_render_new_permission.emit(operation.op_id, operation.path) + self._add_operation_to_list(op_id) if self.chat_window is not None: self.chat_window.load_users() else: show_popup(self.ui, "Error", "Session expired, new login required") self.logout() + def _add_operation_to_list(self, op_id): + """ + Adds the operation op_id the user got a permission for to the list of operations, sorted by path like + add_operations_to_ui does + """ + operation = self.get_operation(op_id) + if operation is None or (not operation.active and config_loader(dataset="MSCOLAB_skip_archived_operations")): + return + operations_list = self.ui.listOperationsMSC if operation.active else \ + self.operation_archive_browser.listArchivedOperations + items = [operations_list.item(row) for row in range(operations_list.count())] + if any(item.op_id == op_id for item in items): + return + widgetItem = QtWidgets.QListWidgetItem(f'{operation.path} - {operation.access_level}') + widgetItem.op_id = operation.op_id + widgetItem.operation_category = operation.category + widgetItem.operation_path = operation.path + widgetItem.access_level = operation.access_level + widgetItem.active_operation_description = operation.description + widgetItem.active = operation.active + row = next((row for row, item in enumerate(items) if item.operation_path.lower() > operation.path.lower()), + len(items)) + operations_list.insertItem(row, widgetItem) + if operation.active: + self.signal_render_new_permission.emit(operation.op_id, operation.path) + @QtCore.pyqtSlot(int, int, str) def handle_update_permission(self, op_id, u_id, access_level): """ @@ -1367,7 +1375,7 @@ def add_operations_to_ui(self): logging.debug('add_operations_to_ui') skip_archived = config_loader(dataset="MSCOLAB_skip_archived_operations") req = GetOperationsRequest(skip_archived=skip_archived) - response = self.conn.request_get(endpoints.OPERATIONS, req.to_form_data()) + response = self.conn.request_get(endpoints.OPERATIONS, req.to_params()) parsed = GetOperationsResponse.from_text(response.text) if parsed is None: raise MSColabConnectionError("Session expired, new login required") @@ -1389,12 +1397,7 @@ def add_operations_to_ui(self): widgetItem.operation_path = operation["path"] widgetItem.access_level = operation["access_level"] widgetItem.active_operation_description = operation["description"] - try: - # compatibility to 7.x - # a newer server can distinguish older operations and move those into inactive state - widgetItem.active = operation["active"] - except KeyError: - widgetItem.active = True + widgetItem.active = operation["active"] if widgetItem.active: self.ui.listOperationsMSC.addItem(widgetItem) if widgetItem.op_id == self.active_op_id: @@ -1582,7 +1585,7 @@ def request_wps_from_server(self, op_id=None): if op_id is None: op_id = self.active_op_id req = GetOperationByIdRequest(op_id=op_id) - response = self.conn.request_get(endpoints.GET_OPERATION_BY_ID, req.to_form_data()) + response = self.conn.request_get(endpoints.GET_OPERATION_BY_ID, req.to_params()) parsed = GetOperationByIdResponse.from_text(response.text) if parsed is not None: return parsed.content @@ -1620,6 +1623,13 @@ def handle_waypoints_changed(self, _1=None, _2=None, _3=None, version_name=None) self.waypoints_model.save_to_ftml(self.local_ftml_file) else: xml_content = self.waypoints_model.get_xml_content() + # one change emits dataChanged twice, e.g. from setData and from update_distances; a reloaded flight + # track is a new model, so after it the same content is sent again + sent = (self.waypoints_model, self.active_op_id, xml_content) + if version_name is None and self.last_sent_flight_track is not None and \ + self.last_sent_flight_track[0] is sent[0] and self.last_sent_flight_track[1:] == sent[1:]: + return + self.last_sent_flight_track = sent self.conn.save_file(self.active_op_id, xml_content, comment=None, version_name=version_name) # Reset the last change message to make sure that it is used only once self.lastChangeMessage = "" diff --git a/mslib/msui/mscolab_admin_window.py b/mslib/msui/mscolab_admin_window.py index 3993daec6..b9b1b97db 100644 --- a/mslib/msui/mscolab_admin_window.py +++ b/mslib/msui/mscolab_admin_window.py @@ -30,6 +30,7 @@ from PyQt5 import QtCore, QtWidgets from mslib.msui.qt5 import ui_mscolab_admin_window as ui from mslib.utils.qt import show_popup +from mslib.msui.socket_control import mscolab_get from mslib.utils.config import config_loader from mslib.mscolab.api import endpoints from mslib.mscolab.api.schemas import ( @@ -95,6 +96,8 @@ def __init__(self, token, op_id, user, operation_name, operations, conn, parent= # Setting handlers for connection manager self.conn.signal_operation_permissions_updated.connect(self.handle_permissions_updated) + # e.g. an operation was archived, which is no longer offered for import + self.conn.signal_operation_list_updated.connect(self.load_import_operations) self.filterCategoryCb.currentIndexChanged.connect(self.operation_category_handler) self.set_label_text() self.load_import_operations() @@ -134,7 +137,9 @@ def populate_table(self, table, users): def populate_import_permission_cb(self): self.importPermissionsCB.clear() for operation in self.operations: - if operation['op_id'] != self.op_id: + # like the list of operations, archived ones are left out, e.g. an archived Group operation + # is not used for memberships + if operation['op_id'] != self.op_id and operation['active']: self.importPermissionsCB.addItem(operation['path'], operation['op_id']) def get_selected_userids(self, table, users): @@ -185,9 +190,7 @@ def apply_permission_filter(self, permission_filter): def set_label_text(self): req = GetCreatorOfOperationRequest(op_id=self.op_id) url = urljoin(self.mscolab_server_url, endpoints.GET_CREATOR_OF_OPERATION) - r = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + r = mscolab_get(url, self.token, req.to_params()) # chosen by users, must not be interpreted as markup for label in (self.operationNameLabel, self.creatorNameLabel, self.usernameLabel): label.setTextFormat(QtCore.Qt.PlainText) @@ -200,21 +203,18 @@ def set_label_text(self): def load_import_operations(self): req = GetOperationsRequest() url = urljoin(self.mscolab_server_url, endpoints.OPERATIONS) - r = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + r = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationsResponse.from_text(r.text) if parsed is not None: - self.operations = [op.to_dict() for op in parsed.operations] - self.populate_import_permission_cb() + # the category filter starts from these + self.initial_operations = [op.to_dict() for op in parsed.operations] + self.operation_category_handler() def load_users_without_permission(self): self.addUsers = [] req = GetOperationUsersRequest(op_id=self.op_id) url = urljoin(self.mscolab_server_url, endpoints.USERS_WITHOUT_PERMISSION) - res = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + res = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationUsersResponse.from_text(res.text) if parsed is not None: if parsed.success: @@ -232,9 +232,7 @@ def load_users_with_permission(self): self.modifyUsers = [] req = GetOperationUsersRequest(op_id=self.op_id) url = urljoin(self.mscolab_server_url, endpoints.USERS_WITH_PERMISSION) - res = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + res = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationUsersResponse.from_text(res.text) if parsed is not None: if parsed.success: diff --git a/mslib/msui/mscolab_chat.py b/mslib/msui/mscolab_chat.py index 281803bb9..37235989a 100644 --- a/mslib/msui/mscolab_chat.py +++ b/mslib/msui/mscolab_chat.py @@ -40,7 +40,7 @@ from mslib.mscolab.api.attachments import DEFAULT_ATTACHMENT_EXTENSIONS, IMAGE_EXTENSIONS, file_extension from mslib.mscolab.api.message_type import MessageType -from mslib.msui.socket_control import response_message +from mslib.msui.socket_control import mscolab_get, response_message from PyQt5 import QtCore, QtGui, QtWidgets from mslib.utils.qt import get_open_filename, get_save_filename, plain_text_message_box, show_popup from mslib.msui.qt5 import ui_mscolab_operation_window as ui @@ -463,12 +463,8 @@ def load_users(self): active_users_url = urljoin(self.mscolab_server_url, endpoints.ACTIVE_USERS) # Fetch both authorized and active users - users_response = requests.get( - users_url, data={**users_req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) - active_response = requests.get( - active_users_url, data={**active_req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + users_response = mscolab_get(users_url, self.token, users_req.to_params()) + active_response = mscolab_get(active_users_url, self.token, active_req.to_params()) users_parsed = GetAuthorizedUsersResponse.from_text(users_response.text) active_parsed = GetActiveUsersResponse.from_text(active_response.text) @@ -481,11 +477,10 @@ def load_users(self): item = QtWidgets.QListWidgetItem(display_text, parent=self.collaboratorsList) # Pixmap for icon i.e. profile image - image_req = FetchProfileImageRequest(user_id=str(user.id)) + # the server shows the images of other users only to members of an operation of both + image_req = FetchProfileImageRequest(user_id=user.id, op_id=self.op_id) url = urljoin(self.mscolab_server_url, endpoints.FETCH_PROFILE_IMAGE) - response = requests.get( - url, data={**image_req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + response = mscolab_get(url, self.token, image_req.to_params()) pixmap = QtGui.QPixmap() if response.status_code == 200: # pixmap = QtGui.QPixmap() @@ -528,9 +523,7 @@ def load_all_messages(self): # returns an array of messages url = urljoin(self.mscolab_server_url, endpoints.MESSAGES) - res = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + res = mscolab_get(url, self.token, req.to_params()) parsed = GetMessagesResponse.from_text(res.text) if parsed is not None: # render_new_message()/MessageItem also consume socket.io chat events @@ -691,8 +684,7 @@ def fetch_attachment(self): if url is None: return None try: - response = requests.get(url, data={"token": self.chat_window.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + response = mscolab_get(url, self.chat_window.token) except requests.exceptions.RequestException as ex: logging.warning("could not load attachment %s: %s", url, ex) return None diff --git a/mslib/msui/mscolab_version_history.py b/mslib/msui/mscolab_version_history.py index 46b4016f5..e3f7bd344 100644 --- a/mslib/msui/mscolab_version_history.py +++ b/mslib/msui/mscolab_version_history.py @@ -34,6 +34,7 @@ from mslib.msui.flighttrack import WaypointsTableModel from mslib.msui.qt5 import ui_mscolab_version_history as ui from mslib.utils.qt import show_popup +from mslib.msui.socket_control import mscolab_get from mslib.utils.config import config_loader from mslib.utils.time import utc_to_local_datetime from mslib.mscolab.api import endpoints @@ -122,9 +123,7 @@ def toggle_version_buttons(self, state): def load_current_waypoints(self): req = GetOperationByIdRequest(op_id=self.op_id) url = urljoin(self.mscolab_server_url, endpoints.GET_OPERATION_BY_ID) - res = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + res = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationByIdResponse.from_text(res.text) if parsed is not None: waypoint_model = WaypointsTableModel(name="Current Waypoints", xml_content=parsed.content) @@ -141,11 +140,8 @@ def load_all_changes(self): if self.versionFilterCB.currentIndex() == 1: named_version_only = True req = GetAllChangesRequest(op_id=self.op_id, named_version=named_version_only) - url_path = f'{endpoints.GET_ALL_CHANGES}?{req.to_query_string()}' - url = urljoin(self.mscolab_server_url, url_path) - r = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + url = urljoin(self.mscolab_server_url, endpoints.GET_ALL_CHANGES) + r = mscolab_get(url, self.token, req.to_params()) parsed = GetAllChangesResponse.from_text(r.text) if parsed is not None: self.changes.clear() @@ -180,9 +176,7 @@ def preview_change(self, current_item, previous_item): current_item.setFont(font) req = GetChangeContentRequest(ch_id=current_item.id) url = urljoin(self.mscolab_server_url, endpoints.GET_CHANGE_CONTENT) - res = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + res = mscolab_get(url, self.token, req.to_params()) parsed = GetChangeContentResponse.from_text(res.text) if parsed is not None: waypoint_model = WaypointsTableModel(xml_content=parsed.content) diff --git a/mslib/msui/multiple_flightpath_dockwidget.py b/mslib/msui/multiple_flightpath_dockwidget.py index 30e74a301..dc8886d51 100644 --- a/mslib/msui/multiple_flightpath_dockwidget.py +++ b/mslib/msui/multiple_flightpath_dockwidget.py @@ -25,12 +25,12 @@ limitations under the License. """ import random -import requests from PyQt5 import QtWidgets, QtGui, QtCore from mslib.msui.qt5 import ui_multiple_flightpath_dockwidget as ui from mslib.msui import flighttrack as ft import mslib.msui.msui_mainwindow as msui_mainwindow from mslib.utils.qt import Worker +from mslib.msui.socket_control import mscolab_get from mslib.utils.config import config_loader from urllib.parse import urljoin from mslib.utils.colordialog import CustomColorDialog @@ -803,9 +803,7 @@ def get_wps_from_server(self): skip_archived = config_loader(dataset="MSCOLAB_skip_archived_operations") req = GetOperationsRequest(skip_archived=skip_archived) url = urljoin(self.mscolab_server_url, endpoints.OPERATIONS) - r = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + r = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationsResponse.from_text(r.text) if parsed is not None: operations = parsed.operations @@ -817,9 +815,7 @@ def get_wps_from_server(self): def request_wps_from_server(self, op_id): req = GetOperationByIdRequest(op_id=op_id) url = urljoin(self.mscolab_server_url, endpoints.GET_OPERATION_BY_ID) - r = requests.get( - url, data={**req.to_form_data(), "token": self.token}, - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + r = mscolab_get(url, self.token, req.to_params()) parsed = GetOperationByIdResponse.from_text(r.text) if parsed is not None: return parsed.content diff --git a/mslib/msui/socket_control.py b/mslib/msui/socket_control.py index 4158c92db..baffe9736 100644 --- a/mslib/msui/socket_control.py +++ b/mslib/msui/socket_control.py @@ -33,7 +33,7 @@ from PyQt5 import QtCore from mslib.mscolab.api.events import SocketEvents -from mslib.mscolab.api.schemas import AUTH_FAILED_TEXT +from mslib.mscolab.api.schemas import AUTH_FAILED_TEXT, TOKEN_HEADER from mslib.msui.mscolab_exceptions import MSColabConnectionError, MSColabSessionExpiredError from mslib.utils.config import MSUIDefaultConfig as mss_default from mslib.utils.config import config_loader @@ -48,6 +48,19 @@ def response_message(response, default): return message if isinstance(message, str) and message else default +def mscolab_get(url, token, params=None): + """ + Sends a GET request with the login token to url of an MSColab server and returns the response + + params go into the query string and the token into the header TOKEN_HEADER. Servers up to 11.x read the token + only from the query string or the body, and user_id of fetch_profile_image only from the body, so until their + support is dropped params and token are sent in the body as well. + """ + return requests.get( + url, params=params, data=(params or {}) | {"token": token}, headers={TOKEN_HEADER: token}, + timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + + class ConnectionManager(QtCore.QObject): signal_reload = QtCore.pyqtSignal(int, name="reload_wps") @@ -243,11 +256,8 @@ def request_post(self, api, data=None, files=None): files=files, timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) return response - def request_get(self, api, data=None): - response = requests.get( - urljoin(self.mscolab_server_url, api), - data=((data if data is not None else {}) | {"token": self.token}), - timeout=tuple(config_loader(dataset="MSCOLAB_timeout"))) + def request_get(self, api, params=None): + response = mscolab_get(urljoin(self.mscolab_server_url, api), self.token, params) if response.status_code != 200: raise MSColabConnectionError # the server answers a rejected token with HTTP 200 and the bare body "False" instead of the JSON diff --git a/tests/_test_api/test_schemas.py b/tests/_test_api/test_schemas.py index 46b1896dd..9ad8f639e 100644 --- a/tests/_test_api/test_schemas.py +++ b/tests/_test_api/test_schemas.py @@ -71,6 +71,7 @@ IdpUserInfo, ImportPermissionsRequest, ImportPermissionsResponse, + InvalidRequest, LoginRequest, LoginResponse, MessageAttachmentRequest, @@ -86,8 +87,8 @@ UndoChangesResponse, UpdateOperationRequest, UpdateOperationResponse, - UploadProfileImageRequest, UserInfo, + int_field, ) @@ -144,17 +145,17 @@ def test_round_trip(self): op_id=1, access_level="creator", path="p", description="d", category="default", active=True) assert OperationInfo.from_dict(op.to_dict()) == op - def test_from_dict_defaults_active_for_pre_8x_servers(self): - # mslib.msui.mscolab.add_operations_to_ui has always tolerated servers - # that don't send "active" at all (mscolab < 8.x) by assuming True. + def test_from_dict_requires_active(self): + # every supported server (8.3.3 and later) sends it data = {"op_id": 1, "access_level": "creator", "path": "p", "description": None, "category": "default"} - assert OperationInfo.from_dict(data).active is True + with pytest.raises(KeyError): + OperationInfo.from_dict(data) class Test_GetOperationsRequest: def test_round_trip(self): req = GetOperationsRequest(skip_archived=True) - assert GetOperationsRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetOperationsRequest.from_args_and_form(req.to_params(), {}) == req def test_default(self): req = GetOperationsRequest() @@ -166,12 +167,18 @@ def test_args_take_precedence_over_form(self): parsed = GetOperationsRequest.from_args_and_form({"skip_archived": "True"}, {"skip_archived": "False"}) assert parsed.skip_archived is True + def test_form_of_older_clients(self): + # msui up to 11.x sent the parameters of GET requests in the body + assert GetOperationsRequest.from_args_and_form({}, {"skip_archived": "True"}).skip_archived is True + class Test_GetOperationsResponse: def test_round_trip(self): response = GetOperationsResponse(operations=[ - OperationInfo(op_id=1, access_level="creator", path="a", description=None, category="default"), - OperationInfo(op_id=2, access_level="collaborator", path="b", description="d", category="cat"), + OperationInfo(op_id=1, access_level="creator", path="a", description=None, category="default", + active=True), + OperationInfo(op_id=2, access_level="collaborator", path="b", description="d", category="cat", + active=False), ]) assert GetOperationsResponse.from_text(response.to_text()) == response @@ -180,7 +187,8 @@ def test_round_trip_empty(self): assert GetOperationsResponse.from_text(response.to_text()) == response def test_manager_dicts_serialize_like_typed_entries(self): - op = OperationInfo(op_id=1, access_level="creator", path="a", description=None, category="default") + op = OperationInfo(op_id=1, access_level="creator", path="a", description=None, category="default", + active=True) typed = GetOperationsResponse(operations=[op]) assert GetOperationsResponse(operations=[op.to_dict()]).to_text() == typed.to_text() @@ -191,11 +199,12 @@ def test_auth_failure_sentinel(self): class Test_GetOperationByIdRequest: def test_round_trip(self): req = GetOperationByIdRequest(op_id=42) - assert GetOperationByIdRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetOperationByIdRequest.from_args_and_form(req.to_params(), {}) == req - def test_missing_op_id_raises_like_the_bare_reads_did(self): - with pytest.raises(TypeError): - GetOperationByIdRequest.from_args_and_form({}, {}) + @pytest.mark.parametrize("args", [{}, {"op_id": "abc"}, {"op_id": "1.0"}, {"op_id": ""}]) + def test_missing_or_non_integer_op_id_is_invalid(self, args): + with pytest.raises(InvalidRequest, match="op_id must be an integer"): + GetOperationByIdRequest.from_args_and_form(args, {}) class Test_GetOperationByIdResponse: @@ -247,7 +256,7 @@ def test_from_form_requires_attribute_and_value(self): UpdateOperationRequest.from_form({"op_id": 1, "attribute": "category"}) def test_from_form_requires_op_id(self): - with pytest.raises(TypeError): + with pytest.raises(InvalidRequest): UpdateOperationRequest.from_form({"attribute": "category", "value": "mycat"}) @@ -265,12 +274,15 @@ def test_wire_format_is_unchanged(self): class Test_GetCreatorOfOperationRequest: def test_round_trip(self): req = GetCreatorOfOperationRequest(op_id=1) - assert GetCreatorOfOperationRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetCreatorOfOperationRequest.from_args_and_form(req.to_params(), {}) == req - def test_op_id_is_not_cast_to_int(self): - # Unlike the other op_id routes, this one passes op_id straight - # through to the DB query untouched -- preserved, not "fixed", here. - assert GetCreatorOfOperationRequest.from_args_and_form({}, {"op_id": "1"}).op_id == "1" + def test_op_id_is_converted_to_int(self): + assert GetCreatorOfOperationRequest.from_args_and_form({"op_id": "1"}, {}).op_id == 1 + + def test_non_integer_op_id_is_invalid(self): + # SQLite would match "1.0" to operation 1 + with pytest.raises(InvalidRequest): + GetCreatorOfOperationRequest.from_args_and_form({"op_id": "1.0"}, {}) class Test_GetCreatorOfOperationResponse: @@ -303,6 +315,11 @@ def test_missing_selected_userids_defaults_to_empty(self): assert DeleteBulkPermissionsRequest.from_form({"op_id": 1}) == DeleteBulkPermissionsRequest( op_id=1, user_ids=[]) + @pytest.mark.parametrize("selected_userids", ["abc", "5", '{"a": 1}', '["a"]', "[1.5]"]) + def test_selected_userids_must_be_a_list_of_integers(self, selected_userids): + with pytest.raises(InvalidRequest, match="selected_userids must be"): + DeleteBulkPermissionsRequest.from_form({"op_id": 1, "selected_userids": selected_userids}) + class Test_DeleteBulkPermissionsResponse: @pytest.mark.parametrize("success", [True, False]) @@ -348,21 +365,21 @@ def test_auth_failure_sentinel(self): assert LoginResponse.from_text(AUTH_FAILED_TEXT) is None -class Test_UploadProfileImageRequest: - def test_to_form_data(self): - assert UploadProfileImageRequest(user_id=1).to_form_data() == {"user_id": 1} - - class Test_FetchProfileImageRequest: def test_round_trip(self): req = FetchProfileImageRequest(user_id=1, op_id=2) - assert FetchProfileImageRequest.from_args_and_form({}, req.to_form_data()) == req + assert FetchProfileImageRequest.from_args_and_form(req.to_params(), {}) == req def test_op_id_defaults_to_none_and_is_omitted(self): req = FetchProfileImageRequest(user_id=1) - data = req.to_form_data() + data = req.to_params() assert "op_id" not in data - assert FetchProfileImageRequest.from_args_and_form({}, data) == req + assert FetchProfileImageRequest.from_args_and_form(data, {}) == req + + def test_user_id_in_the_body_of_older_clients(self): + # msui up to 11.x sent it in the body + req = FetchProfileImageRequest(user_id=1, op_id=2) + assert FetchProfileImageRequest.from_args_and_form({}, {"user_id": 1, "op_id": 2}) == req class Test_ProfileImageMessageResponse: @@ -393,7 +410,7 @@ def test_auth_failure_sentinel(self): class Test_GetOperationUsersRequest: def test_round_trip(self): req = GetOperationUsersRequest(op_id=1) - assert GetOperationUsersRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetOperationUsersRequest.from_args_and_form(req.to_params(), {}) == req class Test_GetOperationUsersResponse: @@ -454,7 +471,7 @@ def test_round_trip(self): class Test_GetAuthorizedUsersRequest: def test_round_trip(self): req = GetAuthorizedUsersRequest(op_id=1) - assert GetAuthorizedUsersRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetAuthorizedUsersRequest.from_args_and_form(req.to_params(), {}) == req class Test_GetAuthorizedUsersResponse: @@ -476,7 +493,7 @@ def test_auth_failure_sentinel(self): class Test_GetActiveUsersRequest: def test_round_trip(self): req = GetActiveUsersRequest(op_id=1) - assert GetActiveUsersRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetActiveUsersRequest.from_args_and_form(req.to_params(), {}) == req class Test_GetActiveUsersResponse: @@ -503,15 +520,15 @@ def test_round_trip_no_comment_or_version_name(self): class Test_GetAllChangesRequest: - def test_round_trip(self): - req = GetAllChangesRequest(op_id=1, named_version=True) - assert GetAllChangesRequest.from_args_and_form( - dict(pair.split("=") for pair in req.to_query_string().split("&")), req.to_form_data()) == req + @pytest.mark.parametrize("named_version", [True, False]) + def test_round_trip(self, named_version): + req = GetAllChangesRequest(op_id=1, named_version=named_version) + assert GetAllChangesRequest.from_args_and_form(req.to_params(), {}) == req - def test_query_string_is_separate_from_form_data(self): - req = GetAllChangesRequest(op_id=1, named_version=True) - assert "named_version" not in req.to_form_data() - assert "op_id" not in req.to_query_string() + def test_op_id_in_the_body_of_older_clients(self): + # msui up to 11.x sent op_id in the body and named_version in the query string + assert GetAllChangesRequest.from_args_and_form({"named_version": "True"}, {"op_id": "1"}) == \ + GetAllChangesRequest(op_id=1, named_version=True) class Test_GetAllChangesResponse: @@ -532,7 +549,7 @@ def test_round_trip_failure(self): class Test_GetChangeContentRequest: def test_round_trip(self): req = GetChangeContentRequest(ch_id=1) - assert GetChangeContentRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetChangeContentRequest.from_args_and_form(req.to_params(), {}) == req class Test_GetChangeContentResponse: @@ -597,8 +614,9 @@ def test_defaults_on_missing_keys(self): assert response.use_saml2 is False assert response.direct_login is True - def test_defaults_on_invalid_json(self): - response = StatusResponse.from_text("not json") + @pytest.mark.parametrize("text", ["not json", "[]", '"Mscolab server"', "null"]) + def test_defaults_on_invalid_json(self, text): + response = StatusResponse.from_text(text) assert response == StatusResponse(message="", use_saml2=False, direct_login=True) @pytest.mark.parametrize("mail_enabled", [True, False]) @@ -691,7 +709,7 @@ def test_round_trip_with_replies(self): class Test_GetMessagesRequest: def test_round_trip(self): req = GetMessagesRequest(op_id=1, timestamp="2026-01-01T00:00:00+00:00") - assert GetMessagesRequest.from_args_and_form({}, req.to_form_data()) == req + assert GetMessagesRequest.from_args_and_form(req.to_params(), {}) == req def test_default_timestamp(self): req = GetMessagesRequest(op_id=1) @@ -724,6 +742,15 @@ def test_round_trip(self): def test_missing_message_type_parses_to_none(self): assert MessageAttachmentRequest.from_form({"op_id": "1"}).message_type is None + def test_op_id_is_converted_to_int(self): + # it names the upload folder, which the attachment route only accepts as digits + assert MessageAttachmentRequest.from_form({"op_id": "1", "message_type": "2"}).op_id == 1 + + @pytest.mark.parametrize("form", [{}, {"op_id": "1.0"}, {"op_id": "1", "message_type": "x"}]) + def test_missing_or_non_integer_fields_are_invalid(self, form): + with pytest.raises(InvalidRequest): + MessageAttachmentRequest.from_form(form) + class Test_MessageAttachmentResponse: def test_round_trip_success(self): @@ -739,3 +766,16 @@ def test_to_dict_omits_unset_fields(self): def test_auth_failure_sentinel(self): assert MessageAttachmentResponse.from_text(AUTH_FAILED_TEXT) is None + + +class Test_int_field: + @pytest.mark.parametrize("value, expected", [("0", 0), ("42", 42), ("-1", -1), (7, 7), ("2147483647", 2 ** 31 - 1)]) + def test_integers(self, value, expected): + assert int_field(value, "op_id") == expected + + @pytest.mark.parametrize("value", [ + None, "", "abc", "1.0", "1_0", " 1", "1 ", "+1", "\u0661", "\uff11", "2147483648", "100000000000000000000", + 2 ** 31, True, 1.0]) + def test_no_integers(self, value): + with pytest.raises(InvalidRequest, match="op_id must be an integer"): + int_field(value, "op_id") diff --git a/tests/_test_mscolab/test_api_contract.py b/tests/_test_mscolab/test_api_contract.py index af83203af..62459cbad 100644 --- a/tests/_test_mscolab/test_api_contract.py +++ b/tests/_test_mscolab/test_api_contract.py @@ -36,9 +36,11 @@ GetCreatorOfOperationRequest, GetCreatorOfOperationResponse, LoginRequest, LoginResponse, MessageAttachmentResponse, + TOKEN_HEADER, ) from mslib.mscolab.auth import register_user from mslib.mscolab.seed import XML_CONTENT_INIT +from mslib.mscolab.utils import ATTACHMENTS_URL_PREFIX class Test_ApiContract: @@ -58,9 +60,10 @@ def setup(self, mscolab_app): def _post(self, endpoint, data): return self.client.post("/" + endpoint, data=data | {"token": self.token}) - def _get(self, endpoint, data): - # the client's request_get() sends its payload in the request body, not the query string - return self.client.get("/" + endpoint, data=data | {"token": self.token}) + def _get(self, endpoint, params): + # msui sends the parameters in the query string and the token in a header; no body, as through a proxy + # that drops the body of GET requests + return self.client.get("/" + endpoint, query_string=params, headers={TOKEN_HEADER: self.token}) def _create_operation(self, path="contract"): req = CreateOperationRequest(path=path, description="desc", content=XML_CONTENT_INIT) @@ -69,7 +72,7 @@ def _create_operation(self, path="contract"): return response def _op_id(self, path="contract"): - response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_form_data()) + response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_params()) return next(op.op_id for op in GetOperationsResponse.from_text(response.text).operations if op.path == path) @@ -80,7 +83,7 @@ def test_plain_text_response(self): def test_json_dumps_response(self): self._create_operation() - response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_form_data()) + response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_params()) assert response.status_code == 200 parsed = GetOperationsResponse.from_text(response.text) assert [op.path for op in parsed.operations] == ["contract"] @@ -94,7 +97,7 @@ def test_json_dumps_response(self): def test_jsonify_response(self): self._create_operation() req = GetCreatorOfOperationRequest(op_id=self._op_id()) - response = self._get(endpoints.GET_CREATOR_OF_OPERATION, req.to_form_data()) + response = self._get(endpoints.GET_CREATOR_OF_OPERATION, req.to_params()) assert response.status_code == 200 assert response.mimetype == "application/json" parsed = GetCreatorOfOperationResponse.from_text(response.text) @@ -103,7 +106,7 @@ def test_jsonify_response(self): def test_auth_failed_response(self): self.token = "not-a-valid-token" - response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_form_data()) + response = self._get(endpoints.OPERATIONS, GetOperationsRequest().to_params()) assert response.text == "False" assert GetOperationsResponse.from_text(response.text) is None @@ -127,6 +130,53 @@ def test_get_all_changes_non_member(self): "/" + endpoints.TOKEN, data=LoginRequest(email="outsider2@example.org", password="secret").to_form_data()) self.token = LoginResponse.from_text(response.text).token - response = self._get(endpoints.GET_ALL_CHANGES, GetAllChangesRequest(op_id=op_id).to_form_data()) + response = self._get(endpoints.GET_ALL_CHANGES, GetAllChangesRequest(op_id=op_id).to_params()) assert response.status_code == 200 assert GetAllChangesResponse.from_text(response.text) == GetAllChangesResponse(success=False, changes=[]) + + def test_get_of_older_clients(self): + # msui up to 11.x sent the token and the parameters of GET requests in the body + self._create_operation() + req = GetCreatorOfOperationRequest(op_id=self._op_id()) + response = self.client.get("/" + endpoints.GET_CREATOR_OF_OPERATION, + data={"op_id": req.op_id, "token": self.token}) + assert response.status_code == 200 + assert GetCreatorOfOperationResponse.from_text(response.text).username == "contract" + + def test_header_token_takes_precedence(self): + self._create_operation() + response = self.client.get("/" + endpoints.OPERATIONS, headers={TOKEN_HEADER: "not-a-valid-token"}, + data={"token": self.token}) + assert response.text == "False" + + @pytest.mark.parametrize("params", [{}, {"op_id": "abc"}, {"op_id": "1.0"}, {"op_id": "1_0"}, + {"op_id": "100000000000000000000"}]) + def test_missing_or_non_integer_op_id(self, params): + self._create_operation() + for endpoint in (endpoints.GET_CREATOR_OF_OPERATION, endpoints.GET_ALL_CHANGES, endpoints.MESSAGES, + endpoints.AUTHORIZED_USERS, endpoints.ACTIVE_USERS, "operation_details"): + response = self._get(endpoint, params) + assert response.status_code == 400, endpoint + assert response.text == "op_id must be an integer" + + @pytest.mark.parametrize("data", [{"op_id": "abc"}, {"op_id": "1", "days": "x"}]) + def test_set_last_used_non_integer_fields(self, data): + response = self._post("set_last_used", data) + assert response.status_code == 400 + + def test_delete_bulk_permissions_malformed_user_ids(self): + self._create_operation() + response = self._post(endpoints.DELETE_BULK_PERMISSIONS, {"op_id": self._op_id(), "selected_userids": "abc"}) + assert response.status_code == 400 + assert response.text == "selected_userids must be a JSON list of integers" + + @pytest.mark.parametrize("name", ["abc", "1_0", "100000000000000000000"]) + def test_attachment_of_non_integer_operation(self, name): + response = self.client.get(f"/{ATTACHMENTS_URL_PREFIX}/{name}/x.txt", headers={TOKEN_HEADER: self.token}) + assert response.status_code == 404 + + def test_message_attachment_non_integer_op_id(self): + # SQLite matched "1.0" to operation 1 and the file was stored in a folder "1.0" the attachment route refuses + self._create_operation() + response = self._post(endpoints.MESSAGE_ATTACHMENT, {"op_id": f"{self._op_id()}.0", "message_type": "2"}) + assert response.status_code == 400 diff --git a/tests/_test_mscolab/test_file_manager.py b/tests/_test_mscolab/test_file_manager.py index cba1058dd..8def56221 100644 --- a/tests/_test_mscolab/test_file_manager.py +++ b/tests/_test_mscolab/test_file_manager.py @@ -292,8 +292,8 @@ def test_get_authorized_users(self): def test_save_file(self): with self.app.test_client(): flight_path, operation = self._create_operation(flight_path="operation6", content=self.content1) - # nothing changed - assert self.fm.save_file(operation.id, self.content1, self.user) is False + # nothing changed, nothing to save + assert self.fm.save_file(operation.id, self.content1, self.user) is None assert self.fm.save_file(operation.id, self.content2, self.user) def test_upload_chat_attachment(self): @@ -768,6 +768,58 @@ def test_group_permissions_only_imported_for_group_creator(self): assert self.fm.is_admin(self.user.id, operation_admin.id) assert self.fm.is_creator(self.adminuser.id, operation_admin.id) + def test_archived_group_operation_is_not_used_for_memberships(self): + with self.app.test_client(): + _, operation_no_1 = self._create_operation(flight_path="flightno1", category="bergen") + assert self.fm.add_bulk_permission(operation_no_1.id, self.user, [self.vieweruser.id], "viewer") + _, operation_group = self._create_operation(flight_path="bergenGroup", category="bergen") + assert self.fm.update_operation(operation_group.id, "active", "False", self.user) + + # its members are changed, those of the operations of its category not + assert self.fm.add_bulk_permission(operation_group.id, self.user, + [self.collaboratoruser.id, self.vieweruser.id], "collaborator") == { + operation_group.id: [self.collaboratoruser.id, self.vieweruser.id]} + assert self.fm.is_member(self.collaboratoruser.id, operation_no_1.id) is False + assert self.fm.modify_bulk_permission(operation_group.id, self.user, [self.vieweruser.id], "admin") == { + operation_group.id: [self.vieweruser.id]} + assert self.fm.is_viewer(self.vieweruser.id, operation_no_1.id) + assert self.fm.delete_bulk_permission(operation_group.id, self.user, [self.vieweruser.id]) == { + operation_group.id: [self.vieweruser.id]} + assert self.fm.is_viewer(self.vieweruser.id, operation_no_1.id) + + # a new operation of the category doesn't get its members, they can't be cloned from it + _, operation_no_2 = self._create_operation(flight_path="flightno2", category="bergen") + assert self.fm.is_member(self.collaboratoruser.id, operation_no_2.id) is False + success, users, message = self.fm.import_permissions(operation_group.id, operation_no_2.id, self.user.id) + assert (success, users) == (False, None) + assert "archived" in message + assert self.fm.is_member(self.collaboratoruser.id, operation_no_2.id) is False + + def test_unarchived_group_operation_updates_the_operations_of_its_creator(self): + with self.app.test_client(): + _, operation_own = self._create_operation(flight_path="ownop", category="bergen") + _, operation_other = self._create_operation(flight_path="otherop", user=self.op2user, category="bergen") + assert self.fm.add_bulk_permission(operation_other.id, self.op2user, [self.user.id], "admin") + _, operation_oslo = self._create_operation(flight_path="osloop", category="oslo") + _, operation_group = self._create_operation(flight_path="bergenGroup", category="bergen") + assert self.fm.update_operation(operation_group.id, "active", "False", self.user) + assert self.fm.add_bulk_permission(operation_group.id, self.user, [self.vieweruser.id], "viewer") + assert self.fm.add_bulk_permission(operation_group.id, self.user, [self.adminuser.id], "admin") + assert self.fm.is_member(self.vieweruser.id, operation_own.id) is False + + # also when an admin of the Group operation unarchives it, the operations of its creator get updated + assert self.fm.update_operation(operation_group.id, "active", "True", self.adminuser) + assert self.fm.is_viewer(self.vieweruser.id, operation_own.id) + assert self.fm.is_admin(self.adminuser.id, operation_own.id) + assert self.fm.is_creator(self.user.id, operation_own.id) + # not those of other creators or of other categories + assert self.fm.is_member(self.vieweruser.id, operation_other.id) is False + assert self.fm.is_member(self.vieweruser.id, operation_oslo.id) is False + + # and it is used again for the memberships of the category + assert self.fm.add_bulk_permission(operation_group.id, self.user, [self.collaboratoruser.id], "viewer") + assert self.fm.is_viewer(self.collaboratoruser.id, operation_own.id) + def test_creator_permission_can_not_be_changed_by_admin(self): with self.app.test_client(): for flight_path in ("flightno1", "bergenGroup"): diff --git a/tests/_test_mscolab/test_server.py b/tests/_test_mscolab/test_server.py index 7da3dc3b7..66b5050dc 100644 --- a/tests/_test_mscolab/test_server.py +++ b/tests/_test_mscolab/test_server.py @@ -727,7 +727,7 @@ def test_operation_users_invalid_op_id(self, endpoint, op_id): data = {"token": token} if op_id is None else {"token": token, "op_id": op_id} response = test_client.get(endpoint, data=data) assert response.status_code == 400 - assert response.data == b"False" + assert response.data == b"op_id must be an integer" def test_operation_users_require_membership(self): # the members of an operation and who of them is online are shown only to its members @@ -1005,6 +1005,29 @@ def test_import_permissions(self): # creator is not listed assert data["success"] is True + @pytest.mark.parametrize("import_path", ["europeGroup", "import"]) + def test_import_permissions_from_archived_operation(self, import_path): + assert add_user(self.userdata[0], self.userdata[1], self.userdata[2], self.userdata[3]) + another_user = 'UV20@uv20', 'UV20', 'uv20', 'User20' + assert add_user(another_user[0], another_user[1], another_user[2], another_user[3]) + with self.app.test_client() as test_client: + import_operation, token = self._create_operation(test_client, self.userdata, path=import_path) + fm = FileManager(self.app.config["OPERATIONS_DATA"]) + fm.add_bulk_permission(import_operation.id, get_user(self.userdata[0]), [get_user(another_user[0]).id], + "viewer") + response = test_client.post('/update_operation', data={ + "token": token, "op_id": import_operation.id, "attribute": "active", "value": "False"}) + assert response.text == "True" + current_operation, token = self._create_operation(test_client, self.userdata, path="current") + response = test_client.post('/import_permissions', data={"token": token, + "import_op_id": import_operation.id, + "current_op_id": current_operation.id}) + assert response.status_code == 200 + data = json.loads(response.data.decode('utf-8')) + assert data["success"] is False + assert "archived" in data["message"] + assert fm.is_member(get_user(another_user[0]).id, current_operation.id) is False + def test_bulk_permissions_notify_group_member_operations(self): assert add_user(self.userdata[0], self.userdata[1], self.userdata[2], self.userdata[3]) another_user = 'UV20@uv20', 'UV20', 'uv20', 'User20' diff --git a/tests/_test_mscolab/test_sockets_manager.py b/tests/_test_mscolab/test_sockets_manager.py index 8ac4e11ba..eedd9f10d 100644 --- a/tests/_test_mscolab/test_sockets_manager.py +++ b/tests/_test_mscolab/test_sockets_manager.py @@ -829,6 +829,18 @@ def test_file_save_with_too_long_version_name_is_refused(self): with self.app.app_context(): assert Change.query.filter_by(op_id=self.operation.id).one().version_name == "x" * 255 + def test_file_save_without_changes_is_not_refused(self): + # msui sends a change twice, the second save finds the flight track saved already + sio = self._connect(self.token) + self._save(sio, self.token) + self._save(sio, self.token) + assert self._refusals(sio) == [] + with self.app.app_context(): + assert self.fm.get_file(self.operation.id, self.user) == XML_CONTENT1 + assert Change.query.filter_by(op_id=self.operation.id).count() == 1 + # one service message for the one change + assert Message.query.filter_by(op_id=self.operation.id).count() == 1 + def test_archived_operation_chat_and_flight_track_are_read_only(self): message_id = self._add_message(self.user, self.operation, "written before archiving") self._set_active(self.operation, False) diff --git a/tests/_test_msui/test_mscolab.py b/tests/_test_msui/test_mscolab.py index b16463463..4396e92e8 100644 --- a/tests/_test_msui/test_mscolab.py +++ b/tests/_test_msui/test_mscolab.py @@ -42,7 +42,7 @@ from mslib.mscolab.api import endpoints from mslib.mscolab.api.message_type import MessageType from mslib.mscolab.models import Permission, User -from mslib.msui.flighttrack import WaypointsTableModel +from mslib.msui.flighttrack import LON, WaypointsTableModel from PyQt5 import QtCore, QtGui, QtTest, QtWidgets from mslib.utils.config import MSUIDefaultConfig, read_config_file, config_loader, modify_config_file from tests.utils import create_msui_settings_file, ExceptionMock @@ -813,7 +813,7 @@ def test_handle_delete_operation(self, qtbot): assert os.path.isdir(os.path.join(MSCOLAB_DATA_DIR, operation_name)) self._activate_operation_at_index(0) - op_id = self.window.mscolab.get_recent_op_id() + op_id = self.window.mscolab.get_op_id(operation_name) assert op_id is not None assert self.window.listOperationsMSC.model().rowCount() == 1 with mock.patch("PyQt5.QtWidgets.QMessageBox.information", return_value=QtWidgets.QMessageBox.Ok) as m, \ @@ -824,7 +824,7 @@ def test_handle_delete_operation(self, qtbot): self.window, "Information", 'Active operation "flight7" is inaccessible!') ) assert self.window.mscolab.active_op_id is None - op_id = self.window.mscolab.get_recent_op_id() + op_id = self.window.mscolab.get_op_id(operation_name) assert op_id is None # check operation dir name removed assert os.path.isdir(os.path.join(MSCOLAB_DATA_DIR, operation_name)) is False @@ -841,7 +841,7 @@ def test_handle_leave_operation(self, mockmessage, qtbot): assert self.window.listOperationsMSC.model().rowCount() == 1 assert self.window.mscolab.active_op_id is None self._activate_operation_at_index(0) - op_id = self.window.mscolab.get_recent_op_id() + op_id = self.window.mscolab.active_op_id assert op_id is not None self.window.actionTopView.trigger() @@ -956,22 +956,23 @@ def test_any_special_category(self, qtbot): assert ["flight5678"] == operation_pathes @mock.patch("PyQt5.QtWidgets.QMessageBox.information", return_value=QtWidgets.QMessageBox.Ok) - def test_get_recent_op_id(self, mockbox, qtbot): + def test_get_op_id(self, mockbox, qtbot): self._connect_to_mscolab(qtbot) modify_config_file({"MSS_auth": {self.url: "anton@something.org"}}) self._create_user(qtbot, "anton", "anton@something.org", "something", "Test User") assert self.window.usernameLabel.text() == 'anton' assert self.window.connectBtn.isVisible() is False assert self.window.listOperationsMSC.model().rowCount() == 0 - self._create_operation(qtbot, "flight2", "Description flight2") - current_op_id = self.window.mscolab.get_recent_op_id() - self._create_operation(qtbot, "flight3", "Description flight3") - self._create_operation(qtbot, "flight4", "Description flight4") - # ToDo fix number after cleanup initial data - assert self.window.mscolab.get_recent_op_id() == current_op_id + 2 + for path in ("flight2", "flight3", "flight4"): + self._create_operation(qtbot, path, f"Description {path}") + listed = {self.window.listOperationsMSC.item(row).operation_path: self.window.listOperationsMSC.item(row).op_id + for row in range(self.window.listOperationsMSC.count())} + assert {path: self.window.mscolab.get_op_id(path) for path in listed} == listed + assert len(set(listed.values())) == 3 + assert self.window.mscolab.get_op_id("flight5") is None @mock.patch("PyQt5.QtWidgets.QMessageBox.information", return_value=QtWidgets.QMessageBox.Ok) - def test_get_recent_operation(self, mockbox, qtbot): + def test_get_operation(self, mockbox, qtbot): self._connect_to_mscolab(qtbot) modify_config_file({"MSS_auth": {self.url: "berta@something.org"}}) self._create_user(qtbot, "berta", "berta@something.org", "something", "Test User") @@ -980,9 +981,10 @@ def test_get_recent_operation(self, mockbox, qtbot): assert self.window.listOperationsMSC.model().rowCount() == 0 self._create_operation(qtbot, "flight1234", "Description flight1234") self._activate_operation_at_index(0) - operation = self.window.mscolab.get_recent_operation() + operation = self.window.mscolab.get_operation(self.window.mscolab.active_op_id) assert operation.path == "flight1234" assert operation.access_level == "creator" + assert self.window.mscolab.get_operation(self.window.mscolab.active_op_id + 1000) is None @mock.patch("PyQt5.QtWidgets.QMessageBox.information", return_value=QtWidgets.QMessageBox.Ok) def test_open_chat_window(self, mockbox, qtbot): @@ -1044,7 +1046,7 @@ def test_delete_operation_from_list(self, mockbox, qtbot): assert self.window.listOperationsMSC.model().rowCount() == 0 self._create_operation(qtbot, "flight3", "Description flight3") self._activate_operation_at_index(0) - op_id = self.window.mscolab.get_recent_op_id() + op_id = self.window.mscolab.get_op_id("flight3") self.window.mscolab.delete_operation_from_list(op_id) assert self.window.mscolab.active_op_id is None @@ -1151,6 +1153,48 @@ def test_file_save_refused_shows_why_and_reloads(self, qtbot): self.window.mscolab.conn.handle_file_save_refused(json.dumps({"op_id": op_id + 1000, "message": "x"})) popup.assert_called_once() + def test_new_permission_lists_the_operation_of_the_permission(self, qtbot): + # the operations of a user come in no particular order, "four" was created after "europe" + self._connect_to_mscolab(qtbot) + modify_config_file({"MSS_auth": {self.url: "something@something.org"}}) + self._create_user(qtbot, "something", "something@something.org", "something", "Test User") + self._create_operation(qtbot, "four", "Description four") + with self.app.app_context(): + assert add_user_to_operation(path=self.operation_name, access_level="collaborator", + emailid="something@something.org") + mscolab_ = self.window.mscolab + op_id = mscolab_.get_op_id(self.operation_name) + with mock.patch.object(mscolab_, "signal_render_new_permission") as signal: + mscolab_.render_new_permission(op_id, mscolab_.user["id"]) + # the event can come again, e.g. for a second socket of the user + mscolab_.render_new_permission(op_id, mscolab_.user["id"]) + operations = self.window.listOperationsMSC + listed = [operations.item(row).text() for row in range(operations.count())] + assert listed == ["europe - collaborator", "four - creator"] + assert self.window.listOperationsMSC.item(0).op_id == op_id + signal.emit.assert_called_once_with(op_id, self.operation_name) + + def test_waypoint_change_is_sent_once(self, qtbot): + self._connect_to_mscolab(qtbot) + modify_config_file({"MSS_auth": {self.url: "something@something.org"}}) + self._create_user(qtbot, "something", "something@something.org", "something", "Test User") + self._create_operation(qtbot, "flight1234", "Description flight1234") + self._activate_operation_at_index(0) + mscolab_ = self.window.mscolab + model = mscolab_.waypoints_model + with mock.patch.object(mscolab_.conn, "save_file") as save_file: + # setData emits dataChanged twice, from update_distances and for the changed cell + model.setData(model.index(0, LON), QtCore.QVariant(model.waypoint_data(0).lon + 1)) + save_file.assert_called_once() + assert save_file.call_args.args[:2] == (mscolab_.active_op_id, model.get_xml_content()) + # a reloaded flight track, e.g. after the server refused the change, is sent again when it changes + mscolab_.waypoints_model = WaypointsTableModel(xml_content=model.get_xml_content()) + mscolab_.handle_waypoints_changed() + assert save_file.call_count == 2 + # a version name is always sent + mscolab_.handle_waypoints_changed(version_name="imported") + assert save_file.call_count == 3 + def test_user_chosen_text_is_not_markup(self, qtbot): markup = 'bold' self._connect_to_mscolab(qtbot) diff --git a/tests/_test_msui/test_mscolab_admin_window.py b/tests/_test_msui/test_mscolab_admin_window.py index 402b94a2a..60f3a6779 100644 --- a/tests/_test_msui/test_mscolab_admin_window.py +++ b/tests/_test_msui/test_mscolab_admin_window.py @@ -33,6 +33,9 @@ from tests import constants from mslib.msui import mscolab from mslib.msui import msui +from mslib.mscolab.api import endpoints +from mslib.mscolab.api.schemas import UpdateOperationRequest +from mslib.mscolab.models import Operation from mslib.mscolab.seed import add_user, get_user, add_operation, add_user_to_operation from mslib.utils.config import modify_config_file @@ -189,6 +192,21 @@ def test_import_permissions(self): QtTest.QTest.mouseClick(self.admin_window.importPermissionsBtn, QtCore.Qt.LeftButton) assert self.admin_window.modifyUsersTable.rowCount() == 1 + def test_operation_archived_while_open_is_no_longer_offered_for_import(self, qtbot): + # e.g. an archived Group operation is not used for memberships + def offered(): + return [self.admin_window.importPermissionsCB.itemText(row) + for row in range(self.admin_window.importPermissionsCB.count())] + assert offered() == ["paris", "tokyo"] + with self.app.app_context(): + op_id = Operation.query.filter_by(path="tokyo").first().id + req = UpdateOperationRequest(op_id=op_id, attribute="active", value="False") + assert self.window.mscolab.conn.request_post(endpoints.UPDATE_OPERATION, req.to_form_data()).text == "True" + + def assert_(): + assert offered() == ["paris"] + qtbot.wait_until(assert_) + def _connect_to_mscolab(self, qtbot): self.connect_window = mscolab.MSColab_ConnectDialog(parent=self.window, mscolab=self.window.mscolab) self.window.mscolab.connect_window = self.connect_window diff --git a/tests/_test_msui/test_mscolab_chat.py b/tests/_test_msui/test_mscolab_chat.py index 75d3bade4..40fd781d0 100644 --- a/tests/_test_msui/test_mscolab_chat.py +++ b/tests/_test_msui/test_mscolab_chat.py @@ -31,8 +31,10 @@ from PyQt5 import QtCore, QtGui, QtWidgets from mslib.mscolab.api.message_type import MessageType +from mslib.mscolab.api import endpoints from mslib.msui.mscolab_chat import ( - ChatTextBrowser, DeregisterSyntax, MessageItem, MessageTextEdit, chat_link_target, open_chat_link, + ChatTextBrowser, DeregisterSyntax, MessageItem, MessageTextEdit, MSColabChatWindow, chat_link_target, + open_chat_link, ) @@ -207,3 +209,25 @@ def test_reply_link_of_document_is_opened_not_downloaded(qtbot, chat_window): # the document itself is still downloaded with the token item.messageBox.anchorClicked.emit(QtCore.QUrl(item.attachment_url())) download.assert_called_once() + + +def test_avatars_of_members_are_requested_for_the_operation(qtbot): + # the server shows the profile image of another user only to members of an operation of both + window = types.SimpleNamespace(op_id=5, token="token", mscolab_server_url="http://localhost:8083", + collaboratorsList=QtWidgets.QListWidget()) + qtbot.addWidget(window.collaboratorsList) + texts = { + endpoints.AUTHORIZED_USERS: '{"users": [{"username": "berta", "access_level": "collaborator", "id": 2}]}', + endpoints.ACTIVE_USERS: '{"active_users": []}', + } + + def get(url, token, params=None): + endpoint = url.rsplit("/", 1)[1] + return mock.Mock(status_code=200 if endpoint in texts else 404, text=texts.get(endpoint, "")) + + with mock.patch("mslib.msui.mscolab_chat.mscolab_get", side_effect=get) as mscolab_get: + MSColabChatWindow.load_users(window) + image_requests = [call for call in mscolab_get.call_args_list + if call.args[0].endswith(endpoints.FETCH_PROFILE_IMAGE)] + assert [call.args[2] for call in image_requests] == [{"user_id": 2, "op_id": 5}] + assert window.collaboratorsList.count() == 1 diff --git a/tests/_test_msui/test_mscolab_operation.py b/tests/_test_msui/test_mscolab_operation.py index 396cdfc2a..a554de458 100644 --- a/tests/_test_msui/test_mscolab_operation.py +++ b/tests/_test_msui/test_mscolab_operation.py @@ -312,7 +312,7 @@ def test_foreign_attachment_not_fetched_with_token(self, qtbot, tmp_path, messag "time": datetime.datetime.now(tz=datetime.timezone.utc).isoformat(), "op_id": self.chat_window.op_id} target = tmp_path / "x.png" - with mock.patch("mslib.msui.mscolab_chat.requests.get") as get, \ + with mock.patch("mslib.msui.mscolab_chat.mscolab_get") as get, \ mock.patch("mslib.msui.mscolab_chat.get_save_filename", return_value=str(target)), \ mock.patch("mslib.msui.mscolab_chat.show_popup") as popup: self.chat_window.handle_incoming_message(json.dumps(message)) diff --git a/tests/_test_msui/test_socket_control.py b/tests/_test_msui/test_socket_control.py new file mode 100644 index 000000000..f9015f70f --- /dev/null +++ b/tests/_test_msui/test_socket_control.py @@ -0,0 +1,52 @@ +# -*- coding: utf-8 -*- +""" + + tests._test_msui.test_socket_control + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + This module tests the requests msui sends to the MSColab server. + + This file is part of MSS. + + :copyright: Copyright 2026 Reimar Bauer + :license: APACHE-2.0, see LICENSE for details. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +""" +import mock +import requests + +from mslib.mscolab.api.schemas import TOKEN_HEADER +from mslib.msui.socket_control import mscolab_get + + +def _prepared_request(*args, **kwargs): + with mock.patch("mslib.msui.socket_control.requests.get") as get: + mscolab_get(*args, **kwargs) + (url,), sent = get.call_args + sent.pop("timeout") + return requests.Request("GET", url, **sent).prepare() + + +def test_mscolab_get_parameters_in_query_string_token_in_header(): + request = _prepared_request("http://localhost:8083/operations", "secret-token", {"skip_archived": "True"}) + assert request.url == "http://localhost:8083/operations?skip_archived=True" + assert request.headers[TOKEN_HEADER] == "secret-token" + assert "secret-token" not in request.url + + +def test_mscolab_get_body_for_servers_up_to_11(): + # they read the token, and user_id of fetch_profile_image, only from the query string or the body + request = _prepared_request("http://localhost:8083/fetch_profile_image", "secret-token", {"user_id": 1}) + assert request.url == "http://localhost:8083/fetch_profile_image?user_id=1" + assert request.body == "user_id=1&token=secret-token"