From a6dfea0b0cd531314e6dc89486f4af1b667dcb59 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Mon, 21 Aug 2023 16:22:51 +0200 Subject: [PATCH 01/17] intermediate commit with renames --- .../action/actions/user/__init__.py | 4 +- .../user/{import_.py => import_account.py} | 0 ...{json_upload.py => json_upload_account.py} | 12 +- .../action/mixins/import_mixins.py | 146 +++++++++++++++--- ...{test_import.py => test_import_account.py} | 0 ..._upload.py => test_json_upload_account.py} | 32 ++-- 6 files changed, 145 insertions(+), 49 deletions(-) rename openslides_backend/action/actions/user/{import_.py => import_account.py} (100%) rename openslides_backend/action/actions/user/{json_upload.py => json_upload_account.py} (95%) rename tests/system/action/user/{test_import.py => test_import_account.py} (100%) rename tests/system/action/user/{test_json_upload.py => test_json_upload_account.py} (95%) diff --git a/openslides_backend/action/actions/user/__init__.py b/openslides_backend/action/actions/user/__init__.py index f5cf031af..4db77ce53 100644 --- a/openslides_backend/action/actions/user/__init__.py +++ b/openslides_backend/action/actions/user/__init__.py @@ -5,8 +5,8 @@ forget_password, forget_password_confirm, generate_new_password, - import_, - json_upload, + import_account, + json_upload_account, merge_together, reset_password_to_default, save_saml_account, diff --git a/openslides_backend/action/actions/user/import_.py b/openslides_backend/action/actions/user/import_account.py similarity index 100% rename from openslides_backend/action/actions/user/import_.py rename to openslides_backend/action/actions/user/import_account.py diff --git a/openslides_backend/action/actions/user/json_upload.py b/openslides_backend/action/actions/user/json_upload_account.py similarity index 95% rename from openslides_backend/action/actions/user/json_upload.py rename to openslides_backend/action/actions/user/json_upload_account.py index 6725b09d8..4de288bdc 100644 --- a/openslides_backend/action/actions/user/json_upload.py +++ b/openslides_backend/action/actions/user/json_upload_account.py @@ -12,8 +12,8 @@ from .user_mixin import DuplicateCheckMixin, UsernameMixin -@register_action("user.json_upload") -class UserJsonUpload(DuplicateCheckMixin, UsernameMixin, JsonUploadMixin): +@register_action("account.json_upload") +class AccountJsonUpload(DuplicateCheckMixin, UsernameMixin, JsonUploadMixin): """ Action to allow to upload a json. It is used as first step of an import. """ @@ -54,12 +54,12 @@ class UserJsonUpload(DuplicateCheckMixin, UsernameMixin, JsonUploadMixin): {"property": "last_name", "type": "string"}, {"property": "is_active", "type": "boolean"}, {"property": "is_physical_person", "type": "boolean"}, - {"property": "default_password", "type": "string"}, + {"property": "default_password", "type": "string","is_object": True}, {"property": "email", "type": "string"}, - {"property": "username", "type": "string"}, + {"property": "username", "type": "string", "is_object": True}, {"property": "gender", "type": "string"}, {"property": "pronoun", "type": "string"}, - {"property": "saml_id", "type": "string"}, + {"property": "saml_id", "type": "string", "is_object": True}, ] permission = OrganizationManagementLevel.CAN_MANAGE_USERS skip_archived_meeting_check = True @@ -135,7 +135,7 @@ def generate_entry( state = ImportState.ERROR messages.append("Cannot generate username.") elif self.check_name_and_email_for_duplicate( - *UserJsonUpload._names_and_email(entry), payload_index + *AccountJsonUpload._names_and_email(entry), payload_index ): state = ImportState.DONE if searchdata := self.get_search_data(payload_index): diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index e45e83dcb..4099fa000 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -1,27 +1,100 @@ from enum import Enum -from time import time +from time import mktime, strptime, time from typing import Any, Callable, Dict, List, Optional, TypedDict +from ...services.datastore.commands import GetManyRequest from ...shared.exceptions import ActionException +from ...shared.filters import FilterOperator, Or from ...shared.interfaces.event import Event, EventType +from ...shared.interfaces.services import DatastoreService from ...shared.interfaces.write_request import WriteRequest from ...shared.patterns import fqid_from_collection_and_id from ..action import Action from ..util.typing import ActionData, ActionResultElement +from .singular_action_mixin import SingularActionMixin -TRUE_VALUES = ("1", "true", "yes", "t") -FALSE_VALUES = ("0", "false", "no", "f") - +TRUE_VALUES = ("1", "true", "yes", "y", "t") +FALSE_VALUES = ("0", "false", "no", "n", "f") class ImportState(str, Enum): - ERROR = "error" - NEW = "new" + NONE = "none" WARNING = "warning" + NEW = "new" DONE = "done" GENERATED = "generated" + REMOVE = "remove" + ERROR = "error" + + +class ResultType(Enum): + """Used by Lookup to differ the possible results in check_duplicate.""" + FOUND_ID = 1 + FOUND_MORE_IDS = 2 + NOT_FOUND = 3 + + +class Lookup: + def __init__( + self, + datastore: DatastoreService, + collection: str, + names: List[str], + field: str = "name", + mapped_fields: List[str] = [], + ) -> None: + self.datastore = datastore + self.collection = collection + self.field = field + self.name_to_ids: Dict[str, List[Dict[str, Any]]] = {name: [] for name in names} + self.id_to_name: Dict[int, str] = {} + if "id" not in mapped_fields: + mapped_fields.append("id") + if field not in mapped_fields: + mapped_fields.append(field) + if names: + for entry in datastore.filter( + collection, + Or(*[FilterOperator(field, "=", name) for name in names]), + mapped_fields, + lock_result=False, + ).values(): + self.name_to_ids[entry[field]].append(entry) + self.id_to_name[entry["id"]] = entry[field] + + def check_duplicate(self, name: str) -> ResultType: + if not self.name_to_ids.get(name): + return ResultType.NOT_FOUND + elif len(self.name_to_ids[name]) > 1: + return ResultType.FOUND_MORE_IDS + else: + return ResultType.FOUND_ID + + + def get_id_by_name(self, name: str) -> Optional[int]: + if len(self.name_to_ids[name]) == 1: + return self.name_to_ids[name][0]["id"] + return None + def get_name_by_id(self, id_: int) -> Optional[str]: + if name := self.id_to_name.get(id_): + return name + return None -class ImportMixin(Action): + def read_missing_ids(self, ids: List[int]) -> None: + result = self.datastore.get_many( + [GetManyRequest(self.collection, ids, [self.field])], + lock_result=False, + use_changed_models=False, + ) + self.id_to_name.update( + { + key: value.get(self.field, "") + for key, value in result[self.collection].items() + } + ) + + +class ImportMixin(SingularActionMixin): """ Mixin for import actions. It works together with the json_upload. """ @@ -85,29 +158,27 @@ def on_success() -> None: return on_success -class HeaderEntry(TypedDict): - property: str - type: str - - class StatisticEntry(TypedDict): name: str value: int -class JsonUploadMixin(Action): - headers: List[HeaderEntry] +class JsonUploadMixin(SingularActionMixin): + headers: List[Dict[str, Any]] rows: List[Dict[str, Any]] statistics: List[StatisticEntry] - state: ImportState + import_state: ImportState def set_state(self, number_errors: int, number_warnings: int) -> None: + """ + To remove, but is used in some backend imports + """ if number_errors > 0: - self.state = ImportState.ERROR + self.import_state = ImportState.ERROR elif number_warnings > 0: - self.state = ImportState.WARNING + self.import_state = ImportState.WARNING else: - self.state = ImportState.DONE + self.import_state = ImportState.DONE def store_rows_in_the_action_worker(self, import_name: str) -> None: self.new_store_id = self.datastore.reserve_id(collection="action_worker") @@ -124,7 +195,7 @@ def store_rows_in_the_action_worker(self, import_name: str) -> None: "result": {"import": import_name, "rows": self.rows}, "created": time_created, "timestamp": time_created, - "state": self.state, + "state": self.import_state, }, ) ], @@ -147,21 +218,36 @@ def create_action_result_element( "headers": self.headers, "rows": self.rows, "statistics": self.statistics, - "state": self.state, + "state": self.import_state, } def validate_instance(self, instance: Dict[str, Any]) -> None: # filter extra, not needed fields before validate and parse some fields property_to_type = { - header["property"]: header["type"] for header in self.headers + header["property"]: ( + header["type"], + header.get("is_object"), + header.get("is_list", False), + ) + for header in self.headers } for entry in list(instance.get("data", [])): for field in dict(entry): if field not in property_to_type: del entry[field] else: - type_ = property_to_type[field] - if type_ == "integer": + type_, is_object, is_list = property_to_type[field] + if type_ == "string" and is_list: + try: + entry[field] = [ + item.strip() + for item in list(csv.reader([entry[field]]))[0] + ] + except Exception: + pass + elif type_ == "string": + continue + elif type_ == "integer": try: entry[field] = int(entry[field]) except ValueError: @@ -177,5 +263,15 @@ def validate_instance(self, instance: Dict[str, Any]) -> None: raise ActionException( f"Could not parse {entry[field]} expect boolean" ) - - super().validate_instance(instance) + elif type_ == "date": + try: + entry[field] = int( + mktime(strptime(entry[field], "%Y-%m-%d")) + ) + except Exception: + pass + else: + raise ActionException( + f"Unknown type in conversion: type:{type_} is_object:{str(is_object)} is_list:{str(is_list)}" + ) + super().validate_instance(instance) \ No newline at end of file diff --git a/tests/system/action/user/test_import.py b/tests/system/action/user/test_import_account.py similarity index 100% rename from tests/system/action/user/test_import.py rename to tests/system/action/user/test_import_account.py diff --git a/tests/system/action/user/test_json_upload.py b/tests/system/action/user/test_json_upload_account.py similarity index 95% rename from tests/system/action/user/test_json_upload.py rename to tests/system/action/user/test_json_upload_account.py index b677c0708..ea28ac6b1 100644 --- a/tests/system/action/user/test_json_upload.py +++ b/tests/system/action/user/test_json_upload_account.py @@ -9,7 +9,7 @@ class TopicJsonUpload(BaseActionTestCase): def test_json_upload(self) -> None: start_time = int(time()) response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -41,7 +41,7 @@ def test_json_upload(self) -> None: def test_json_upload_empty_data(self) -> None: response = self.request( - "user.json_upload", + "account.json_upload", {"data": []}, ) self.assert_status_code(response, 400) @@ -49,7 +49,7 @@ def test_json_upload_empty_data(self) -> None: def test_json_upload_parse_boolean_error(self) -> None: response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -65,7 +65,7 @@ def test_json_upload_parse_boolean_error(self) -> None: def test_json_upload_results(self) -> None: response = self.request( - "user.json_upload", + "account.json_upload", {"data": [{"username": "test", "default_password": "secret"}]}, ) self.assert_status_code(response, 200) @@ -102,12 +102,12 @@ def test_json_upload_results(self) -> None: {"property": "last_name", "type": "string"}, {"property": "is_active", "type": "boolean"}, {"property": "is_physical_person", "type": "boolean"}, - {"property": "default_password", "type": "string"}, + {"property": "default_password", "type": "string", "is_object": True}, {"property": "email", "type": "string"}, - {"property": "username", "type": "string"}, + {"property": "username", "type": "string", "is_object": True}, {"property": "gender", "type": "string"}, {"property": "pronoun", "type": "string"}, - {"property": "saml_id", "type": "string"}, + {"property": "saml_id", "type": "string", "is_object": True}, ], "rows": [ { @@ -139,7 +139,7 @@ def test_json_upload_duplicate_in_db(self) -> None: } ) response = self.request( - "user.json_upload", + "account.json_upload", {"data": [{"username": "test"}]}, ) self.assert_status_code(response, 200) @@ -172,7 +172,7 @@ def test_json_upload_multiple_duplicates(self) -> None: } ) response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -200,7 +200,7 @@ def test_json_upload_multiple_duplicates(self) -> None: def test_json_upload_duplicate_in_data(self) -> None: self.maxDiff = None response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ {"username": "test", "default_password": "secret"}, @@ -275,7 +275,7 @@ def test_json_upload_names_and_email_generate_username(self) -> None: ) response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -306,7 +306,7 @@ def test_json_upload_names_and_email_set_username(self) -> None: } ) response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -329,7 +329,7 @@ def test_json_upload_names_and_email_set_username(self) -> None: def test_json_upload_generate_default_password(self) -> None: response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -349,7 +349,7 @@ def test_json_upload_generate_default_password(self) -> None: def test_json_upload_saml_id(self) -> None: response = self.request( - "user.json_upload", + "account.json_upload", { "data": [ { @@ -377,13 +377,13 @@ def test_json_upload_saml_id(self) -> None: def test_json_upload_no_permission(self) -> None: self.base_permission_test( - {}, "user.json_upload", {"data": [{"username": "test"}]} + {}, "account.json_upload", {"data": [{"username": "test"}]} ) def test_json_upload_permission(self) -> None: self.base_permission_test( {}, - "user.json_upload", + "account.json_upload", {"data": [{"username": "test"}]}, OrganizationManagementLevel.CAN_MANAGE_USERS, ) From e245e4ce4a0c41b7ae82d248846b8ce797408683 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 29 Aug 2023 16:43:18 +0200 Subject: [PATCH 02/17] Base account import on Lookup class and fix some smaller problems in topic and account import --- .../action/actions/user/__init__.py | 4 +- .../{import_account.py => account_import.py} | 0 .../actions/user/account_json_upload.py | 306 ++++++++++++++++++ .../actions/user/json_upload_account.py | 219 ------------- .../action/mixins/import_mixins.py | 109 ++++--- ...port_account.py => test_account_import.py} | 0 ...account.py => test_account_json_upload.py} | 273 ++++++++++++++-- 7 files changed, 619 insertions(+), 292 deletions(-) rename openslides_backend/action/actions/user/{import_account.py => account_import.py} (100%) create mode 100644 openslides_backend/action/actions/user/account_json_upload.py delete mode 100644 openslides_backend/action/actions/user/json_upload_account.py rename tests/system/action/user/{test_import_account.py => test_account_import.py} (100%) rename tests/system/action/user/{test_json_upload_account.py => test_account_json_upload.py} (58%) diff --git a/openslides_backend/action/actions/user/__init__.py b/openslides_backend/action/actions/user/__init__.py index 4db77ce53..87fdee213 100644 --- a/openslides_backend/action/actions/user/__init__.py +++ b/openslides_backend/action/actions/user/__init__.py @@ -1,12 +1,12 @@ from . import ( # noqa + account_import, + account_json_upload, assign_meetings, create, delete, forget_password, forget_password_confirm, generate_new_password, - import_account, - json_upload_account, merge_together, reset_password_to_default, save_saml_account, diff --git a/openslides_backend/action/actions/user/import_account.py b/openslides_backend/action/actions/user/account_import.py similarity index 100% rename from openslides_backend/action/actions/user/import_account.py rename to openslides_backend/action/actions/user/account_import.py diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py new file mode 100644 index 000000000..91f7bce7a --- /dev/null +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -0,0 +1,306 @@ +from collections import defaultdict +from typing import Any, Dict, List, Optional, Tuple, Union + +from ....models.models import User +from ....permissions.management_levels import OrganizationManagementLevel +from ...mixins.import_mixins import ImportState, JsonUploadMixin, Lookup, ResultType +from ...util.crypto import get_random_password +from ...util.default_schema import DefaultSchema +from ...util.register import register_action +from .user_mixin import UsernameMixin + + +@register_action("account.json_upload") +class AccountJsonUpload(JsonUploadMixin, UsernameMixin): + """ + Action to allow to upload a json. It is used as first step of an import. + """ + + model = User() + schema = DefaultSchema(User()).get_default_schema( + additional_required_fields={ + "data": { + "type": "array", + "items": { + "type": "object", + "properties": { + **model.get_properties( + "title", + "first_name", + "last_name", + "is_active", + "is_physical_person", + "default_password", + "email", + "username", + "gender", + "pronoun", + "saml_id", + ), + }, + "required": [], + "additionalProperties": False, + }, + "minItems": 1, + "uniqueItems": False, + }, + } + ) + headers = [ + {"property": "title", "type": "string"}, + {"property": "first_name", "type": "string"}, + {"property": "last_name", "type": "string"}, + {"property": "is_active", "type": "boolean"}, + {"property": "is_physical_person", "type": "boolean"}, + {"property": "default_password", "type": "string", "is_object": True}, + {"property": "email", "type": "string"}, + {"property": "username", "type": "string", "is_object": True}, + {"property": "gender", "type": "string"}, + {"property": "pronoun", "type": "string"}, + {"property": "saml_id", "type": "string", "is_object": True}, + ] + permission = OrganizationManagementLevel.CAN_MANAGE_USERS + skip_archived_meeting_check = True + row_state: ImportState + username_lookup: Lookup + saml_id_lookup: Lookup + names_email_lookup: Lookup + all_saml_id_lookup: Lookup + + def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: + data = instance.pop("data") + data = self.add_payload_index_to_action_data(data) + self.setup_lookups(data) + + self.rows = [ + self.validate_entry(entry, payload_index) + for payload_index, entry in enumerate(data) + ] + + # generate statistics + itemCount = len(self.rows) + state_to_count = {state: 0 for state in ImportState} + for entry in self.rows: + state_to_count[entry["state"]] += 1 + entry["data"].pop("payload_index", None) + + self.statistics = [ + {"name": "total", "value": itemCount}, + {"name": "created", "value": state_to_count[ImportState.NEW]}, + {"name": "updated", "value": state_to_count[ImportState.DONE]}, + {"name": "error", "value": state_to_count[ImportState.ERROR]}, + {"name": "warning", "value": state_to_count[ImportState.WARNING]}, + ] + + self.set_state( + state_to_count[ImportState.ERROR], state_to_count[ImportState.WARNING] + ) + self.store_rows_in_the_action_worker("account") + return {} + + def validate_entry( + self, entry: Dict[str, Any], payload_index: int + ) -> Dict[str, Any]: + messages: List[str] = [] + id_: Optional[int] = None + if username := entry.get("username"): + check_result = self.username_lookup.check_duplicate(username) + id_ = self.username_lookup.get_x_by_name(username, "id") + if check_result == ResultType.FOUND_ID and id_ != 0: + self.row_state = ImportState.DONE + entry["id"] = id_ + entry["username"] = { + "value": username, + "info": ImportState.DONE, + "id": id_, + } + elif check_result == ResultType.NOT_FOUND or id_ == 0: + self.row_state = ImportState.NEW + entry["username"] = { + "value": username, + "info": ImportState.DONE, + } + elif check_result == ResultType.FOUND_MORE_IDS: + self.row_state = ImportState.ERROR + entry["username"] = { + "value": username, + "info": ImportState.ERROR, + } + messages.append("Found more users with the same username") + elif saml_id := entry.get("saml_id"): + check_result = self.saml_id_lookup.check_duplicate(saml_id) + id_ = self.saml_id_lookup.get_x_by_name(saml_id, "id") + username = self.saml_id_lookup.get_x_by_name(saml_id, "username") + if check_result == ResultType.FOUND_ID and id_ != 0: + self.row_state = ImportState.DONE + entry["id"] = id_ + entry["username"] = { + "value": username, + "info": ImportState.DONE, + "id": id_, + } + elif check_result == ResultType.NOT_FOUND or id_ == 0: + self.row_state = ImportState.NEW + elif check_result == ResultType.FOUND_MORE_IDS: + self.row_state = ImportState.ERROR + messages.append("Found more users with the same saml_id") + else: + if not (entry.get("first_name") or entry.get("last_name")): + self.row_state = ImportState.ERROR + messages.append("Cannot generate username.") + else: + names_and_email = self._names_and_email(entry) + check_result = self.names_email_lookup.check_duplicate(names_and_email) + id_ = self.names_email_lookup.get_x_by_name(names_and_email, "id") + username = self.names_email_lookup.get_x_by_name( + names_and_email, "username" + ) + if check_result == ResultType.FOUND_ID and id_ != 0: + self.row_state = ImportState.DONE + entry["id"] = id_ + entry["username"] = { + "value": username, + "info": ImportState.DONE, + "id": id_, + } + elif check_result == ResultType.NOT_FOUND or id_ == 0: + self.row_state = ImportState.NEW + elif check_result == ResultType.FOUND_MORE_IDS: + self.row_state = ImportState.ERROR + messages.append("Found more users with name and email") + + if id_ and len(self.all_id_mapping.get(id_, [])) > 1: + self.row_state = ImportState.ERROR + messages.append( + f"The account with id {id_} was found multiple times by different search criteria." + ) + + if not entry.get("username") and self.row_state == ImportState.NEW: + entry["username"] = { + "value": self.generate_username(entry), + "info": ImportState.GENERATED, + } + + self.handle_default_password(entry) + if saml_id := entry.get("saml_id"): + check_result = self.all_saml_id_lookup.check_duplicate(saml_id) + if id_ := entry.get("id"): + if check_result == ResultType.FOUND_ID: + idFound = self.all_saml_id_lookup.get_x_by_name(saml_id, "id") + if check_result == ResultType.NOT_FOUND or ( + check_result == ResultType.FOUND_ID and id_ == idFound + ): + entry["saml_id"] = { + "value": saml_id, + "info": ImportState.DONE, + } + else: + self.row_state = ImportState.ERROR + messages.append(f"saml_id {saml_id} must be unique.") + entry["saml_id"] = { + "value": saml_id, + "info": ImportState.ERROR, + } + else: + if check_result != ResultType.NOT_FOUND: + self.row_state = ImportState.ERROR + entry["saml_id"] = { + "value": saml_id, + "info": ImportState.ERROR, + } + messages.append(f"saml_id {saml_id} must be unique.") + else: + entry["saml_id"] = { + "value": saml_id, + "info": ImportState.NEW, + } + + if entry.get("password") or entry.get("default_password"): + messages.append("Removed password or default_password from entry.") + entry.pop("password", None) + entry.pop("default_password", None) + entry["can_change_own_password"] = False + return {"state": self.row_state, "messages": messages, "data": entry} + + def handle_default_password(self, entry: Dict[str, Any]) -> None: + if self.row_state == ImportState.NEW: + if "default_password" in entry: + value = entry["default_password"] + info = ImportState.DONE + else: + value = get_random_password() + info = ImportState.GENERATED + entry["default_password"] = {"value": value, "info": info} + elif self.row_state in (ImportState.DONE, ImportState.ERROR): + if "default_password" in entry: + entry["default_password"] = { + "value": entry["default_password"], + "info": ImportState.DONE, + } + + @staticmethod + def _names_and_email(entry: Dict[str, Any]) -> Tuple[str, ...]: + return ( + entry.get("first_name", ""), + entry.get("last_name", ""), + entry.get("email", ""), + ) + + def setup_lookups(self, data: List[Dict[str, Any]]) -> None: + self.username_lookup = Lookup( + self.datastore, + "user", + [ + (username, entry) + for entry in data + if (username := entry.get("username")) + ], + field="username", + ) + self.saml_id_lookup = Lookup( + self.datastore, + "user", + [ + (saml_id, entry) + for entry in data + if not entry.get("username") and (saml_id := entry.get("saml_id")) + ], + field="saml_id", + mapped_fields=["username"], + ) + self.names_email_lookup = Lookup( + self.datastore, + "user", + [ + (names_email, entry) + for entry in data + if not entry.get("username") + and not entry.get("saml_id") + and ( + names_email := ( + entry.get("first_name", ""), + entry.get("last_name", ""), + entry.get("email", ""), + ) + ) + ], + field=tuple(("first_name", "last_name", "email")), + mapped_fields=["username"], + ) + self.all_saml_id_lookup = Lookup( + self.datastore, + "user", + [(saml_id, entry) for entry in data if (saml_id := entry.get("saml_id"))], + field="saml_id", + mapped_fields=["username"], + ) + + self.all_id_mapping: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( + list + ) + for id, values in self.username_lookup.id_to_name.items(): + self.all_id_mapping[id].extend(values) + for id, values in self.saml_id_lookup.id_to_name.items(): + self.all_id_mapping[id].extend(values) + for id, values in self.names_email_lookup.id_to_name.items(): + self.all_id_mapping[id].extend(values) diff --git a/openslides_backend/action/actions/user/json_upload_account.py b/openslides_backend/action/actions/user/json_upload_account.py deleted file mode 100644 index 4de288bdc..000000000 --- a/openslides_backend/action/actions/user/json_upload_account.py +++ /dev/null @@ -1,219 +0,0 @@ -from typing import Any, Dict, Optional, Tuple - -import fastjsonschema - -from ....models.models import User -from ....permissions.management_levels import OrganizationManagementLevel -from ...mixins.import_mixins import ImportState, JsonUploadMixin -from ...util.crypto import get_random_password -from ...util.default_schema import DefaultSchema -from ...util.register import register_action -from .create import UserCreate -from .user_mixin import DuplicateCheckMixin, UsernameMixin - - -@register_action("account.json_upload") -class AccountJsonUpload(DuplicateCheckMixin, UsernameMixin, JsonUploadMixin): - """ - Action to allow to upload a json. It is used as first step of an import. - """ - - model = User() - schema = DefaultSchema(User()).get_default_schema( - additional_required_fields={ - "data": { - "type": "array", - "items": { - "type": "object", - "properties": { - **model.get_properties( - "title", - "first_name", - "last_name", - "is_active", - "is_physical_person", - "default_password", - "email", - "username", - "gender", - "pronoun", - "saml_id", - ), - }, - "required": [], - "additionalProperties": False, - }, - "minItems": 1, - "uniqueItems": False, - }, - } - ) - headers = [ - {"property": "title", "type": "string"}, - {"property": "first_name", "type": "string"}, - {"property": "last_name", "type": "string"}, - {"property": "is_active", "type": "boolean"}, - {"property": "is_physical_person", "type": "boolean"}, - {"property": "default_password", "type": "string","is_object": True}, - {"property": "email", "type": "string"}, - {"property": "username", "type": "string", "is_object": True}, - {"property": "gender", "type": "string"}, - {"property": "pronoun", "type": "string"}, - {"property": "saml_id", "type": "string", "is_object": True}, - ] - permission = OrganizationManagementLevel.CAN_MANAGE_USERS - skip_archived_meeting_check = True - - def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: - data = instance.pop("data") - - # validate and check for duplicates - self.init_duplicate_set( - [ - { - field: entry.get(field, "") - for field in ( - "username", - "saml_id", - "first_name", - "last_name", - "email", - ) - } - for entry in data - ] - ) - self.rows = [ - self.generate_entry(entry, payload_index) - for payload_index, entry in enumerate(data) - ] - - # generate statistics - itemCount = len(self.rows) - state_to_count = {state: 0 for state in ImportState} - for entry in self.rows: - state_to_count[entry["state"]] += 1 - - self.statistics = [ - {"name": "total", "value": itemCount}, - {"name": "created", "value": state_to_count[ImportState.NEW]}, - {"name": "updated", "value": state_to_count[ImportState.DONE]}, - {"name": "error", "value": state_to_count[ImportState.ERROR]}, - {"name": "warning", "value": state_to_count[ImportState.WARNING]}, - ] - - self.set_state( - state_to_count[ImportState.ERROR], state_to_count[ImportState.WARNING] - ) - self.store_rows_in_the_action_worker("account") - return {} - - def generate_entry( - self, entry: Dict[str, Any], payload_index: int - ) -> Dict[str, Any]: - state, messages = None, [] - try: - UserCreate.schema_validator(entry) - if entry.get("username"): - if self.check_username_for_duplicate(entry["username"], payload_index): - state, msg = self.handle_done_entry( - "username", entry, payload_index - ) - if msg: - messages.append(msg) - else: - state = self.handle_new_entry("username", entry) - elif entry.get("saml_id"): - if self.check_saml_id_for_duplicate(entry["saml_id"], payload_index): - state, msg = self.handle_done_entry("saml_id", entry, payload_index) - if msg: - messages.append(msg) - else: - state = self.handle_new_entry("saml_id", entry) - else: - if not (entry.get("first_name") or entry.get("last_name")): - state = ImportState.ERROR - messages.append("Cannot generate username.") - elif self.check_name_and_email_for_duplicate( - *AccountJsonUpload._names_and_email(entry), payload_index - ): - state = ImportState.DONE - if searchdata := self.get_search_data(payload_index): - entry["username"] = { - "value": searchdata["username"], - "info": ImportState.DONE, - "id": searchdata["id"], - } - else: - state = ImportState.ERROR - if usernames := self.has_multiple_search_data(payload_index): - messages.append( - "Found more than one user: " + ", ".join(usernames) - ) - else: - messages.append( - f"Duplicate in csv list index: {payload_index}" - ) - else: - state = ImportState.NEW - entry["username"] = { - "value": self.generate_username(entry), - "info": ImportState.GENERATED, - } - self.handle_default_password(entry, state) - if entry.get("saml_id", {}).get("value"): - if entry.get("password") or entry.get("default_password"): - messages.append("Remove password or default_password from entry.") - entry.pop("password", None) - entry.pop("default_password", None) - entry["can_change_own_password"] = False - except fastjsonschema.JsonSchemaException as exception: - state = ImportState.ERROR - messages.append(exception.message) - return {"state": state, "messages": messages, "data": entry} - - def handle_default_password(self, entry: Dict[str, Any], state: str) -> None: - if state == ImportState.NEW: - if "default_password" in entry: - value = entry["default_password"] - info = ImportState.DONE - else: - value = get_random_password() - info = ImportState.GENERATED - entry["default_password"] = {"value": value, "info": info} - elif state in (ImportState.DONE, ImportState.ERROR): - if "default_password" in entry: - entry["default_password"] = { - "value": entry["default_password"], - "info": ImportState.DONE, - } - - @staticmethod - def _names_and_email(entry: Dict[str, Any]) -> Tuple[str, str, str]: - return ( - entry.get("first_name", ""), - entry.get("last_name", ""), - entry.get("email", ""), - ) - - def handle_done_entry( - self, fieldname: str, entry: Dict[str, Any], payload_index: int - ) -> Tuple[ImportState, Optional[str]]: - state = ImportState.DONE - message = None - if searchdata := self.get_search_data(payload_index): - entry[fieldname] = { - "value": entry[fieldname], - "info": "done", - "id": searchdata["id"], - } - else: - entry[fieldname] = {"value": entry[fieldname], "info": "done"} - state = ImportState.ERROR - message = f"Duplicate in csv list index: {payload_index}" - return state, message - - def handle_new_entry(self, fieldname: str, entry: Dict[str, Any]) -> ImportState: - state = ImportState.NEW - entry[fieldname] = {"value": entry[fieldname], "info": "done"} - return state diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 4099fa000..ac46621da 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -1,21 +1,22 @@ +import csv +from collections import defaultdict from enum import Enum from time import mktime, strptime, time -from typing import Any, Callable, Dict, List, Optional, TypedDict +from typing import Any, Callable, Dict, List, Optional, Tuple, TypedDict, Union -from ...services.datastore.commands import GetManyRequest from ...shared.exceptions import ActionException -from ...shared.filters import FilterOperator, Or +from ...shared.filters import And, Filter, FilterOperator, Or from ...shared.interfaces.event import Event, EventType from ...shared.interfaces.services import DatastoreService from ...shared.interfaces.write_request import WriteRequest from ...shared.patterns import fqid_from_collection_and_id -from ..action import Action from ..util.typing import ActionData, ActionResultElement from .singular_action_mixin import SingularActionMixin TRUE_VALUES = ("1", "true", "yes", "y", "t") FALSE_VALUES = ("0", "false", "no", "n", "f") + class ImportState(str, Enum): NONE = "none" WARNING = "warning" @@ -38,61 +39,82 @@ def __init__( self, datastore: DatastoreService, collection: str, - names: List[str], - field: str = "name", - mapped_fields: List[str] = [], + name_entries: List[Tuple[Union[str, Tuple[str, ...]], Dict[str, Any]]], + field: Union[str, Tuple[str, ...]] = "name", + mapped_fields: Optional[List[str]] = None, ) -> None: + if mapped_fields is None: + mapped_fields = [] self.datastore = datastore self.collection = collection self.field = field - self.name_to_ids: Dict[str, List[Dict[str, Any]]] = {name: [] for name in names} - self.id_to_name: Dict[int, str] = {} + self.name_to_ids: Dict[Union[str, Tuple[str, ...]], List[Dict[str, Any]]] = { + name: [] for name, _ in name_entries + } + self.id_to_name: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( + list + ) + or_filters: List[Filter] = [] if "id" not in mapped_fields: mapped_fields.append("id") - if field not in mapped_fields: - mapped_fields.append(field) - if names: + if type(field) == str: + if field not in mapped_fields: + mapped_fields.append(field) + if name_entries: + or_filters = [ + FilterOperator(field, "=", name) for name, _ in name_entries + ] + else: + mapped_fields.extend((f for f in field if f not in mapped_fields)) + if name_entries: + or_filters = [ + And(*[FilterOperator(field[i], "=", name_tpl[i]) for i in range(3)]) + for name_tpl, _ in name_entries + ] + if or_filters: for entry in datastore.filter( collection, - Or(*[FilterOperator(field, "=", name) for name in names]), + Or(*or_filters), mapped_fields, lock_result=False, ).values(): - self.name_to_ids[entry[field]].append(entry) - self.id_to_name[entry["id"]] = entry[field] - - def check_duplicate(self, name: str) -> ResultType: - if not self.name_to_ids.get(name): - return ResultType.NOT_FOUND - elif len(self.name_to_ids[name]) > 1: + if type(field) == str: + self.name_to_ids[entry[field]].append(entry) + self.id_to_name[entry["id"]].append(entry[field]) + else: + key: Tuple[Any, ...] = tuple(entry.get(f, "") for f in field) + self.name_to_ids[key].append(entry) + self.id_to_name[entry["id"]].append(key) + + # Add action data items not found in database to lookup dict + for name, entry in name_entries: + if not (values := self.name_to_ids.get(name, [])): + values.append(entry) + else: + if not values[0].get("id"): + values.append(entry) + + def check_duplicate(self, name: Union[str, Tuple[str, ...]]) -> ResultType: + if len(values := self.name_to_ids.get(name, [])) == 1: + if values[0].get("id"): + return ResultType.FOUND_ID + else: + return ResultType.NOT_FOUND + elif len(values) > 1: return ResultType.FOUND_MORE_IDS - else: - return ResultType.FOUND_ID + raise ActionException("Logical Error in Lookup Class") - - def get_id_by_name(self, name: str) -> Optional[int]: - if len(self.name_to_ids[name]) == 1: - return self.name_to_ids[name][0]["id"] + def get_x_by_name(self, name: Union[str, Tuple[str, ...]], x: str) -> Optional[int]: + """Gets fieldname 'x' from value of name_to_ids-dict""" + if len(self.name_to_ids.get(name, [])) == 1: + return self.name_to_ids[name][0].get(x) return None - def get_name_by_id(self, id_: int) -> Optional[str]: + def get_name_by_id(self, id_: int) -> Optional[List[Union[str, Tuple[str, ...]]]]: if name := self.id_to_name.get(id_): return name return None - def read_missing_ids(self, ids: List[int]) -> None: - result = self.datastore.get_many( - [GetManyRequest(self.collection, ids, [self.field])], - lock_result=False, - use_changed_models=False, - ) - self.id_to_name.update( - { - key: value.get(self.field, "") - for key, value in result[self.collection].items() - } - ) - class ImportMixin(SingularActionMixin): """ @@ -221,6 +243,11 @@ def create_action_result_element( "state": self.import_state, } + def add_payload_index_to_action_data(self, action_data: ActionData) -> ActionData: + for payload_index, entry in enumerate(action_data): + entry["payload_index"] = payload_index + return action_data + def validate_instance(self, instance: Dict[str, Any]) -> None: # filter extra, not needed fields before validate and parse some fields property_to_type = { @@ -274,4 +301,4 @@ def validate_instance(self, instance: Dict[str, Any]) -> None: raise ActionException( f"Unknown type in conversion: type:{type_} is_object:{str(is_object)} is_list:{str(is_list)}" ) - super().validate_instance(instance) \ No newline at end of file + super().validate_instance(instance) diff --git a/tests/system/action/user/test_import_account.py b/tests/system/action/user/test_account_import.py similarity index 100% rename from tests/system/action/user/test_import_account.py rename to tests/system/action/user/test_account_import.py diff --git a/tests/system/action/user/test_json_upload_account.py b/tests/system/action/user/test_account_json_upload.py similarity index 58% rename from tests/system/action/user/test_json_upload_account.py rename to tests/system/action/user/test_account_json_upload.py index ea28ac6b1..a112494e7 100644 --- a/tests/system/action/user/test_json_upload_account.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -5,8 +5,8 @@ from tests.system.action.base import BaseActionTestCase -class TopicJsonUpload(BaseActionTestCase): - def test_json_upload(self) -> None: +class AccountJsonUpload(BaseActionTestCase): + def test_json_upload_simple(self) -> None: start_time = int(time()) response = self.request( "account.json_upload", @@ -135,23 +135,66 @@ def test_json_upload_results(self) -> None: def test_json_upload_duplicate_in_db(self) -> None: self.set_models( { - "user/3": {"username": "test"}, - } + "user/3": { + "username": "test", + "saml_id": "12345", + "first_name": "Max", + "last_name": "Mustermann", + "email": "max@mustermann.org", + }, + }, ) response = self.request( "account.json_upload", - {"data": [{"username": "test"}]}, + { + "data": [ + {"username": "test"}, + {"saml_id": "12345"}, + { + "first_name": "Max", + "last_name": "Mustermann", + "email": "max@mustermann.org", + }, + ] + }, ) self.assert_status_code(response, 200) result = response.json["results"][0][0] assert result["rows"] == [ { - "state": ImportState.DONE, - "messages": [], + "state": ImportState.ERROR, + "messages": [ + "The account with id 3 was found multiple times by different search criteria." + ], "data": { - "username": {"value": "test", "info": ImportState.DONE, "id": 3}, + "username": {"value": "test", "info": "done", "id": 3}, + "id": 3, }, - } + }, + { + "state": ImportState.ERROR, + "messages": [ + "The account with id 3 was found multiple times by different search criteria." + ], + "data": { + "saml_id": {"value": "12345", "info": "done"}, + "id": 3, + "username": {"value": "test", "info": "done", "id": 3}, + }, + }, + { + "state": ImportState.ERROR, + "messages": [ + "The account with id 3 was found multiple times by different search criteria." + ], + "data": { + "first_name": "Max", + "last_name": "Mustermann", + "email": "max@mustermann.org", + "id": 3, + "username": {"value": "test", "info": "done", "id": 3}, + }, + }, ] def test_json_upload_multiple_duplicates(self) -> None: @@ -188,7 +231,7 @@ def test_json_upload_multiple_duplicates(self) -> None: assert result["rows"] == [ { "state": ImportState.ERROR, - "messages": ["Found more than one user: test, test2"], + "messages": ["Found more users with name and email"], "data": { "first_name": "Max", "last_name": "Mustermann", @@ -197,7 +240,7 @@ def test_json_upload_multiple_duplicates(self) -> None: } ] - def test_json_upload_duplicate_in_data(self) -> None: + def test_json_upload_username_duplicate_in_data(self) -> None: self.maxDiff = None response = self.request( "account.json_upload", @@ -211,21 +254,29 @@ def test_json_upload_duplicate_in_data(self) -> None: ) self.assert_status_code(response, 200) result = response.json["results"][0][0] - assert result["rows"][2]["messages"] == ["Duplicate in csv list index: 2"] + assert result["rows"][0]["messages"] == [ + "Found more users with the same username" + ] + assert result["rows"][0]["state"] == ImportState.ERROR + assert result["rows"][2]["messages"] == [ + "Found more users with the same username" + ] assert result["rows"][2]["state"] == ImportState.ERROR self.assert_model_exists( "action_worker/1", { + "id": 1, + "state": ImportState.ERROR, "result": { "import": "account", "rows": [ { - "state": ImportState.NEW, - "messages": [], + "state": ImportState.ERROR, + "messages": ["Found more users with the same username"], "data": { "username": { "value": "test", - "info": ImportState.DONE, + "info": ImportState.ERROR, }, "default_password": { "value": "secret", @@ -246,11 +297,11 @@ def test_json_upload_duplicate_in_data(self) -> None: }, { "state": ImportState.ERROR, - "messages": ["Duplicate in csv list index: 2"], + "messages": ["Found more users with the same username"], "data": { "username": { "value": "test", - "info": ImportState.DONE, + "info": ImportState.ERROR, }, "default_password": { "value": "secret", @@ -259,7 +310,7 @@ def test_json_upload_duplicate_in_data(self) -> None: }, }, ], - } + }, }, ) @@ -287,12 +338,14 @@ def test_json_upload_names_and_email_generate_username(self) -> None: ) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] + assert entry["state"] == ImportState.NEW assert entry["data"]["first_name"] == "Max" assert entry["data"]["last_name"] == "Mustermann" assert entry["data"]["username"] == { "value": "MaxMustermann1", "info": ImportState.GENERATED, } + assert entry["data"]["default_password"]["info"] == ImportState.GENERATED def test_json_upload_names_and_email_set_username(self) -> None: self.set_models( @@ -321,11 +374,8 @@ def test_json_upload_names_and_email_set_username(self) -> None: entry = response.json["results"][0][0]["rows"][0] assert entry["data"]["first_name"] == "Max" assert entry["data"]["last_name"] == "Mustermann" - assert entry["data"]["username"] == { - "value": "test", - "info": ImportState.DONE, - "id": 34, - } + assert entry["data"]["id"] == 34 + assert entry["data"]["username"]["value"] == "test" def test_json_upload_generate_default_password(self) -> None: response = self.request( @@ -364,16 +414,179 @@ def test_json_upload_saml_id(self) -> None: worker = self.assert_model_exists("action_worker/1") assert worker["result"]["import"] == "account" assert worker["result"]["rows"][0]["messages"] == [ - "Remove password or default_password from entry." + "Removed password or default_password from entry." ] data = worker["result"]["rows"][0]["data"] - assert data.get("saml_id") == { - "value": "test", - "info": "done", + assert data == { + "saml_id": {"info": "new", "value": "test"}, + "username": {"info": "generated", "value": ""}, + "can_change_own_password": False, } - assert "password" not in data - assert "default_password" not in data - assert data.get("can_change_own_password") is False + + def test_json_upload_duplicated_one_saml_id(self) -> None: + self.set_models( + { + "user/3": { + "username": "test", + "saml_id": "12345", + "first_name": "Max", + "last_name": "Mustermann", + "email": "max@mustermann.org", + }, + }, + ) + + response = self.request( + "account.json_upload", + { + "data": [ + { + "username": "test2", + "saml_id": "12345", + "first_name": "John", + "default_password": "test3", + } + ], + }, + ) + self.assert_status_code(response, 200) + result = response.json["results"][0][0] + assert result["rows"] == [ + { + "state": ImportState.ERROR, + "messages": [ + "saml_id 12345 must be unique.", + "Removed password or default_password from entry.", + ], + "data": { + "username": {"value": "test2", "info": ImportState.DONE}, + "saml_id": {"value": "12345", "info": "error"}, + "first_name": "John", + "can_change_own_password": False, + }, + } + ] + assert result["statistics"] == [ + {"name": "total", "value": 1}, + {"name": "created", "value": 0}, + {"name": "updated", "value": 0}, + {"name": "error", "value": 1}, + {"name": "warning", "value": 0}, + ] + assert result["state"] == ImportState.ERROR + + def test_json_upload_duplicated_two_new_saml_ids(self) -> None: + response = self.request( + "account.json_upload", + { + "data": [ + { + "username": "test1", + "saml_id": "12345", + }, + { + "username": "test2", + "saml_id": "12345", + }, + ], + }, + ) + self.assert_status_code(response, 200) + result = response.json["results"][0][0] + assert result["rows"] == [ + { + "state": ImportState.ERROR, + "messages": [ + "saml_id 12345 must be unique.", + "Removed password or default_password from entry.", + ], + "data": { + "username": {"value": "test1", "info": ImportState.DONE}, + "saml_id": {"value": "12345", "info": ImportState.ERROR}, + "can_change_own_password": False, + }, + }, + { + "state": ImportState.ERROR, + "messages": [ + "saml_id 12345 must be unique.", + "Removed password or default_password from entry.", + ], + "data": { + "username": {"value": "test2", "info": ImportState.DONE}, + "saml_id": {"value": "12345", "info": ImportState.ERROR}, + "can_change_own_password": False, + }, + }, + ] + assert result["statistics"] == [ + {"name": "total", "value": 2}, + {"name": "created", "value": 0}, + {"name": "updated", "value": 0}, + {"name": "error", "value": 2}, + {"name": "warning", "value": 0}, + ] + assert result["state"] == ImportState.ERROR + + def test_json_upload_duplicated_two_found_saml_ids(self) -> None: + self.set_models( + { + "user/3": { + "username": "test1", + "saml_id": "1", + }, + "user/4": { + "username": "test2", + "saml_id": "2", + }, + }, + ) + + response = self.request( + "account.json_upload", + { + "data": [ + { + "username": "test1", + "saml_id": "12345", + }, + { + "username": "test2", + "saml_id": "12345", + }, + ], + }, + ) + self.assert_status_code(response, 200) + result = response.json["results"][0][0] + assert result["rows"] == [ + { + "state": ImportState.ERROR, + "messages": ["saml_id 12345 must be unique."], + "data": { + "username": {"value": "test1", "info": "done", "id": 3}, + "saml_id": {"value": "12345", "info": "error"}, + "id": 3, + }, + }, + { + "state": ImportState.ERROR, + "messages": ["saml_id 12345 must be unique."], + "data": { + "username": {"value": "test2", "info": "done", "id": 4}, + "saml_id": {"value": "12345", "info": "error"}, + "id": 4, + }, + }, + ] + assert result["statistics"] == [ + {"name": "total", "value": 2}, + {"name": "created", "value": 0}, + {"name": "updated", "value": 0}, + {"name": "error", "value": 2}, + {"name": "warning", "value": 0}, + ] + assert result["state"] == ImportState.ERROR def test_json_upload_no_permission(self) -> None: self.base_permission_test( From e372a856787e770cbef7c7d179b2a9c460cac5a9 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 29 Aug 2023 16:44:30 +0200 Subject: [PATCH 03/17] format --- openslides_backend/action/mixins/import_mixins.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index ac46621da..3661414f5 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -29,6 +29,7 @@ class ImportState(str, Enum): class ResultType(Enum): """Used by Lookup to differ the possible results in check_duplicate.""" + FOUND_ID = 1 FOUND_MORE_IDS = 2 NOT_FOUND = 3 From 85c55f455b05c912cef1d099a229b265811f4163 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 29 Aug 2023 17:23:32 +0200 Subject: [PATCH 04/17] set username from saml_id, if no first-/lastname is present, but the saml_id --- .../action/actions/user/account_json_upload.py | 2 +- tests/system/action/user/test_account_json_upload.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index 91f7bce7a..9f6cb7678 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -177,7 +177,7 @@ def validate_entry( if not entry.get("username") and self.row_state == ImportState.NEW: entry["username"] = { - "value": self.generate_username(entry), + "value": self.generate_username(entry) or entry.get("saml_id"), "info": ImportState.GENERATED, } diff --git a/tests/system/action/user/test_account_json_upload.py b/tests/system/action/user/test_account_json_upload.py index a112494e7..d0ddce2cd 100644 --- a/tests/system/action/user/test_account_json_upload.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -403,7 +403,7 @@ def test_json_upload_saml_id(self) -> None: { "data": [ { - "saml_id": "test", + "saml_id": "test_saml_id", "password": "test2", "default_password": "test3", } @@ -418,8 +418,8 @@ def test_json_upload_saml_id(self) -> None: ] data = worker["result"]["rows"][0]["data"] assert data == { - "saml_id": {"info": "new", "value": "test"}, - "username": {"info": "generated", "value": ""}, + "saml_id": {"info": "new", "value": "test_saml_id"}, + "username": {"info": "generated", "value": "test_saml_id"}, "can_change_own_password": False, } From d4a1104306008d0b058f73f7bbafe5c3325315ce Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Wed, 30 Aug 2023 10:53:36 +0200 Subject: [PATCH 05/17] Rename from user.import to account.import --- .../action/actions/user/account_import.py | 4 +-- .../system/action/user/test_account_import.py | 36 +++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/openslides_backend/action/actions/user/account_import.py b/openslides_backend/action/actions/user/account_import.py index 3310254af..4a8419ffb 100644 --- a/openslides_backend/action/actions/user/account_import.py +++ b/openslides_backend/action/actions/user/account_import.py @@ -11,8 +11,8 @@ from .user_mixin import DuplicateCheckMixin -@register_action("user.import") -class UserImport(DuplicateCheckMixin, ImportMixin): +@register_action("account.import") +class AccountImport(DuplicateCheckMixin, ImportMixin): """ Action to import a result from the action_worker. """ diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index 24b700de1..89667da94 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -66,7 +66,7 @@ def setUp(self) -> None: ) def test_import_username_and_create(self) -> None: - response = self.request("user.import", {"id": 2, "import": True}) + response = self.request("account.import", {"id": 2, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( "user/2", @@ -75,13 +75,13 @@ def test_import_username_and_create(self) -> None: self.assert_model_not_exists("action_worker/2") def test_import_abort(self) -> None: - response = self.request("user.import", {"id": 2, "import": False}) + response = self.request("account.import", {"id": 2, "import": False}) self.assert_status_code(response, 200) self.assert_model_not_exists("action_worker/2") self.assert_model_not_exists("user/2") def test_import_wrong_action_worker(self) -> None: - response = self.request("user.import", {"id": 5, "import": True}) + response = self.request("account.import", {"id": 5, "import": True}) self.assert_status_code(response, 400) assert ( "Wrong id doesn't point on account import data." in response.json["message"] @@ -114,13 +114,13 @@ def test_import_username_and_update(self) -> None: }, } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) self.assert_model_not_exists("user/2") self.assert_model_exists("user/1", {"first_name": "Testy"}) def test_import_names_and_email_and_create(self) -> None: - response = self.request("user.import", {"id": 3, "import": True}) + response = self.request("account.import", {"id": 3, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( "user/2", @@ -159,7 +159,7 @@ def test_import_with_saml_id(self) -> None: {"saml_id": {"value": "testsaml", "info": ImportState.NEW}}, ) ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( "user/2", @@ -182,7 +182,7 @@ def test_import_saml_id_error_new_and_saml_id_exists(self) -> None: ), } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -205,7 +205,7 @@ def test_import_done_with_saml_id(self) -> None: ), } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( "user/2", @@ -230,7 +230,7 @@ def test_import_done_error_missing_user(self) -> None: ), } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -262,7 +262,7 @@ def test_import_error_at_state_new(self) -> None: ), } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -270,7 +270,7 @@ def test_import_error_at_state_new(self) -> None: "Error: Want to create user, but missing username in import data." ] - response = self.request("user.import", {"id": 7, "import": True}) + response = self.request("account.import", {"id": 7, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -288,7 +288,7 @@ def test_import_error_state_done_missing_username(self) -> None: }, ) ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -307,7 +307,7 @@ def test_import_error_state_done_missing_user_in_db(self) -> None: }, ) ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -345,7 +345,7 @@ def test_import_error_state_done_search_data_error(self) -> None: } } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][1] assert entry["state"] == ImportState.ERROR @@ -371,7 +371,7 @@ def test_import_error_state_done_not_matching_ids(self) -> None: ), } ) - response = self.request("user.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -380,7 +380,7 @@ def test_import_error_state_done_not_matching_ids(self) -> None: ] def test_import_error_state(self) -> None: - response = self.request("user.import", {"id": 4, "import": True}) + response = self.request("account.import", {"id": 4, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] assert entry["state"] == ImportState.ERROR @@ -388,12 +388,12 @@ def test_import_error_state(self) -> None: self.assert_model_exists("action_worker/4") def test_import_no_permission(self) -> None: - self.base_permission_test({}, "user.import", {"id": 2, "import": True}) + self.base_permission_test({}, "account.import", {"id": 2, "import": True}) def test_import_permission(self) -> None: self.base_permission_test( {}, - "user.import", + "account.import", {"id": 2, "import": True}, OrganizationManagementLevel.CAN_MANAGE_USERS, ) From 84abcd16b7817e4a486d0c50f01f00ad6fb9441a Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Thu, 31 Aug 2023 11:30:47 +0200 Subject: [PATCH 06/17] json_upload ready for client test, import with state errors --- global/meta/models.yml | 3 ++ .../actions/user/account_json_upload.py | 19 ++++++------ .../action/mixins/import_mixins.py | 15 ++++++++- openslides_backend/models/models.py | 5 +-- .../system/action/user/test_account_import.py | 10 ++++-- .../action/user/test_account_json_upload.py | 31 ++++++++++--------- 6 files changed, 53 insertions(+), 30 deletions(-) diff --git a/global/meta/models.yml b/global/meta/models.yml index 9fbc516a7..dff5c48c3 100644 --- a/global/meta/models.yml +++ b/global/meta/models.yml @@ -3825,6 +3825,9 @@ action_worker: - running - end - aborted + - warning + - error + - done restriction_mode: A created: type: timestamp diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index 9f6cb7678..f87d36c19 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -80,9 +80,10 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: # generate statistics itemCount = len(self.rows) state_to_count = {state: 0 for state in ImportState} - for entry in self.rows: - state_to_count[entry["state"]] += 1 - entry["data"].pop("payload_index", None) + for row in self.rows: + state_to_count[row["state"]] += 1 + state_to_count[ImportState.WARNING] += self.count_warnings_in_payload(row.get("data", {}).values()) + row["data"].pop("payload_index", None) self.statistics = [ {"name": "total", "value": itemCount}, @@ -181,7 +182,6 @@ def validate_entry( "info": ImportState.GENERATED, } - self.handle_default_password(entry) if saml_id := entry.get("saml_id"): check_result = self.all_saml_id_lookup.check_duplicate(saml_id) if id_ := entry.get("id"): @@ -214,12 +214,11 @@ def validate_entry( "value": saml_id, "info": ImportState.NEW, } - - if entry.get("password") or entry.get("default_password"): - messages.append("Removed password or default_password from entry.") - entry.pop("password", None) - entry.pop("default_password", None) - entry["can_change_own_password"] = False + if entry["saml_id"]["info"] == ImportState.DONE or entry.get("default_password"): + entry["default_password"] = {"value": "", "info": ImportState.WARNING} + messages.append("Will remove password and default_password and forbid changing your OpenSlides password.") + else: + self.handle_default_password(entry) return {"state": self.row_state, "messages": messages, "data": entry} def handle_default_password(self, entry: Dict[str, Any]) -> None: diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 3661414f5..f356841e1 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -139,8 +139,10 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: raise ActionException( f"Wrong id doesn't point on {self.import_name} import data." ) + if worker.get("state") not in ImportState.__members__.values(): + raise ActionException("Error in import: Missing valid state in stored worker.") if worker.get("state") == ImportState.ERROR: - raise ActionException("Error in import.") + raise ActionException("Error in import. Data will not be imported.") self.result = worker["result"] return instance @@ -249,6 +251,17 @@ def add_payload_index_to_action_data(self, action_data: ActionData) -> ActionDat entry["payload_index"] = payload_index return action_data + @staticmethod + def count_warnings_in_payload(data: Union[str, List[Dict[str, Any]]]) -> int: + count = 0 + for col in data: + if type(col) == dict: + if col.get("info") == ImportState.WARNING: + count += 1 + elif type(col) == list: + count += JsonUploadMixin.count_warnings_in_payload(col) + return count + def validate_instance(self, instance: Dict[str, Any]) -> None: # filter extra, not needed fields before validate and parse some fields property_to_type = { diff --git a/openslides_backend/models/models.py b/openslides_backend/models/models.py index 92a6dc1e9..72ab97fbb 100644 --- a/openslides_backend/models/models.py +++ b/openslides_backend/models/models.py @@ -4,7 +4,7 @@ from .base import Model from .mixins import AgendaItemModelMixin, MeetingModelMixin, PollModelMixin -MODELS_YML_CHECKSUM = "295720b86599b69e9340af0427f4f2bc" +MODELS_YML_CHECKSUM = "583c60c2e2ead61990169ab080bc9173" class Organization(Model): @@ -2104,7 +2104,8 @@ class ActionWorker(Model): id = fields.IntegerField() name = fields.CharField(required=True) state = fields.CharField( - required=True, constraints={"enum": ["running", "end", "aborted"]} + required=True, + constraints={"enum": ["running", "end", "aborted", "warning", "error", "done"]}, ) created = fields.TimestampField(required=True) timestamp = fields.TimestampField(required=True) diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index 89667da94..c93ea64ce 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -5,12 +5,13 @@ from tests.system.action.base import BaseActionTestCase -class UserJsonImport(BaseActionTestCase): +class AccountJsonImport(BaseActionTestCase): def setUp(self) -> None: super().setUp() self.set_models( { "action_worker/2": { + "state": ImportState.DONE, "result": { "import": "account", "rows": [ @@ -29,6 +30,7 @@ def setUp(self) -> None: }, }, "action_worker/3": { + "state": ImportState.DONE, "result": { "import": "account", "rows": [ @@ -50,6 +52,7 @@ def setUp(self) -> None: }, }, "action_worker/4": { + "state": ImportState.ERROR, "result": { "import": "account", "rows": [ @@ -68,10 +71,12 @@ def setUp(self) -> None: def test_import_username_and_create(self) -> None: response = self.request("account.import", {"id": 2, "import": True}) self.assert_status_code(response, 200) - self.assert_model_exists( + user2 = self.assert_model_exists( "user/2", {"username": "test", "first_name": "Testy"}, ) + assert user2.get("default_password") + assert user2.get("password") self.assert_model_not_exists("action_worker/2") def test_import_abort(self) -> None: @@ -94,6 +99,7 @@ def test_import_username_and_update(self) -> None: "username": "test", }, "action_worker/6": { + "state": ImportState.DONE, "result": { "import": "account", "rows": [ diff --git a/tests/system/action/user/test_account_json_upload.py b/tests/system/action/user/test_account_json_upload.py index d0ddce2cd..d108717b2 100644 --- a/tests/system/action/user/test_account_json_upload.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -174,12 +174,13 @@ def test_json_upload_duplicate_in_db(self) -> None: { "state": ImportState.ERROR, "messages": [ - "The account with id 3 was found multiple times by different search criteria." + "The account with id 3 was found multiple times by different search criteria.", "Will remove password and default_password and forbid changing your OpenSlides password." ], "data": { "saml_id": {"value": "12345", "info": "done"}, "id": 3, "username": {"value": "test", "info": "done", "id": 3}, + "default_password": {"info": "warning", "value": ""}, }, }, { @@ -414,13 +415,13 @@ def test_json_upload_saml_id(self) -> None: worker = self.assert_model_exists("action_worker/1") assert worker["result"]["import"] == "account" assert worker["result"]["rows"][0]["messages"] == [ - "Removed password or default_password from entry." + "Will remove password and default_password and forbid changing your OpenSlides password." ] data = worker["result"]["rows"][0]["data"] assert data == { "saml_id": {"info": "new", "value": "test_saml_id"}, "username": {"info": "generated", "value": "test_saml_id"}, - "can_change_own_password": False, + "default_password": {"info": "warning", "value": ""}, } def test_json_upload_duplicated_one_saml_id(self) -> None: @@ -432,6 +433,9 @@ def test_json_upload_duplicated_one_saml_id(self) -> None: "first_name": "Max", "last_name": "Mustermann", "email": "max@mustermann.org", + "default_password": "test1", + "password": "secret", + "can_change_own_password": True, }, }, ) @@ -444,7 +448,7 @@ def test_json_upload_duplicated_one_saml_id(self) -> None: "username": "test2", "saml_id": "12345", "first_name": "John", - "default_password": "test3", + "default_password": "test2", } ], }, @@ -456,13 +460,13 @@ def test_json_upload_duplicated_one_saml_id(self) -> None: "state": ImportState.ERROR, "messages": [ "saml_id 12345 must be unique.", - "Removed password or default_password from entry.", + "Will remove password and default_password and forbid changing your OpenSlides password.", ], "data": { "username": {"value": "test2", "info": ImportState.DONE}, "saml_id": {"value": "12345", "info": "error"}, "first_name": "John", - "can_change_own_password": False, + "default_password": {"value": "", "info": ImportState.WARNING}, }, } ] @@ -471,7 +475,7 @@ def test_json_upload_duplicated_one_saml_id(self) -> None: {"name": "created", "value": 0}, {"name": "updated", "value": 0}, {"name": "error", "value": 1}, - {"name": "warning", "value": 0}, + {"name": "warning", "value": 1}, ] assert result["state"] == ImportState.ERROR @@ -487,6 +491,7 @@ def test_json_upload_duplicated_two_new_saml_ids(self) -> None: { "username": "test2", "saml_id": "12345", + "default_password": "def_password", }, ], }, @@ -496,26 +501,22 @@ def test_json_upload_duplicated_two_new_saml_ids(self) -> None: assert result["rows"] == [ { "state": ImportState.ERROR, - "messages": [ - "saml_id 12345 must be unique.", - "Removed password or default_password from entry.", - ], + "messages": ["saml_id 12345 must be unique."], "data": { "username": {"value": "test1", "info": ImportState.DONE}, "saml_id": {"value": "12345", "info": ImportState.ERROR}, - "can_change_own_password": False, }, }, { "state": ImportState.ERROR, "messages": [ "saml_id 12345 must be unique.", - "Removed password or default_password from entry.", + "Will remove password and default_password and forbid changing your OpenSlides password.", ], "data": { "username": {"value": "test2", "info": ImportState.DONE}, "saml_id": {"value": "12345", "info": ImportState.ERROR}, - "can_change_own_password": False, + "default_password": {"info": ImportState.WARNING, "value": ""}, }, }, ] @@ -524,7 +525,7 @@ def test_json_upload_duplicated_two_new_saml_ids(self) -> None: {"name": "created", "value": 0}, {"name": "updated", "value": 0}, {"name": "error", "value": 2}, - {"name": "warning", "value": 0}, + {"name": "warning", "value": 1}, ] assert result["state"] == ImportState.ERROR From 29e413d2048c42c4e3f12308425ff9c55f7260b5 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Fri, 1 Sep 2023 11:51:24 +0200 Subject: [PATCH 07/17] Fix some user.create bugs concerning saml_id --- .../action/actions/user/create.py | 11 ++--- .../action/actions/user/user_mixin.py | 47 +++++++++--------- tests/system/action/user/test_create.py | 48 ++++++++++++++++++- 3 files changed, 76 insertions(+), 30 deletions(-) diff --git a/openslides_backend/action/actions/user/create.py b/openslides_backend/action/actions/user/create.py index d26fc1b2e..8a3358a27 100644 --- a/openslides_backend/action/actions/user/create.py +++ b/openslides_backend/action/actions/user/create.py @@ -64,7 +64,7 @@ class UserCreate( def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: self.meeting_id: Optional[int] = instance.get("meeting_id") - instance = super().update_instance(instance) + if instance.get("is_active"): self.check_limit_of_user(1) saml_id = instance.get("saml_id") @@ -72,7 +72,10 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: if saml_id: instance["username"] = saml_id else: + if not (instance.get("first_name") or instance.get("last_name")): + raise ActionException("Need username or first_name or last_name") instance["username"] = self.generate_username(instance) + instance = super().update_instance(instance) if saml_id: instance["can_change_own_password"] = False if instance.get("can_change_own_password") or instance.get( @@ -85,12 +88,6 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: if not instance.get("default_password"): instance["default_password"] = get_random_password() instance = self.set_password(instance) - if not ( - instance.get("username") - or instance.get("first_name") - or instance.get("last_name") - ): - raise ActionException("Need username or first_name or last_name") instance["organization_id"] = ONE_ORGANIZATION_ID check_gender_helper(self.datastore, instance) return instance diff --git a/openslides_backend/action/actions/user/user_mixin.py b/openslides_backend/action/actions/user/user_mixin.py index 40419ab3f..a7e225e42 100644 --- a/openslides_backend/action/actions/user/user_mixin.py +++ b/openslides_backend/action/actions/user/user_mixin.py @@ -5,7 +5,7 @@ from openslides_backend.shared.typing import HistoryInformation from openslides_backend.shared.util import ONE_ORGANIZATION_FQID -from ....action.action import Action +from ....action.action import Action, original_instances from ....action.mixins.archived_meeting_check_mixin import CheckForArchivedMeetingMixin from ....presenter.search_users import SearchUsers from ....services.datastore.interface import DatastoreService @@ -13,6 +13,7 @@ from ....shared.filters import FilterOperator from ....shared.patterns import FullQualifiedId, fqid_from_collection_and_id from ....shared.schema import decimal_schema, id_list_schema, optional_id_schema +from ...util.typing import ActionData from ..meeting_user.set_data import MeetingUserSetData @@ -83,31 +84,33 @@ class UserMixin(CheckForArchivedMeetingMixin): "group_ids": id_list_schema, } - def validate_instance(self, instance: Dict[str, Any]) -> None: - super().validate_instance(instance) - if "meeting_id" not in instance and any( - key in self.transfer_field_list for key in instance.keys() - ): - raise ActionException( - "Missing meeting_id in instance, because meeting related fields used" - ) + @original_instances + def get_updated_instances(self, action_data: ActionData) -> ActionData: + for instance in action_data: + for field in ("username", "first_name", "last_name", "email", "saml_id"): + self.strip_field(field, instance) + return super().get_updated_instances(action_data) def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: instance = super().update_instance(instance) - for field in ("username", "first_name", "last_name", "email"): - self.strip_field(field, instance) - if "username" in instance: - if not instance["username"]: - raise ActionException("This username is forbidden.") - result = self.datastore.filter( - "user", - FilterOperator("username", "=", instance["username"]), - ["id"], - ) - if result and instance["id"] not in result.keys(): - raise ActionException( - f"A user with the username {instance['username']} already exists." + + def check_existence(what:str) -> None: + if what in instance: + if not instance[what]: + raise ActionException(f"This {what} is forbidden.") + result = self.datastore.filter( + "user", + FilterOperator(what, "=", instance[what]), + ["id"], ) + if result and instance["id"] not in result.keys(): + raise ActionException( + f"A user with the {what} {instance[what]} already exists." + ) + + check_existence("username") + check_existence("saml_id") + self.check_meeting_and_users( instance, fqid_from_collection_and_id("user", instance["id"]) ) diff --git a/tests/system/action/user/test_create.py b/tests/system/action/user/test_create.py index 76b731f4f..411331a70 100644 --- a/tests/system/action/user/test_create.py +++ b/tests/system/action/user/test_create.py @@ -448,6 +448,52 @@ def test_create_saml_id_and_empty_values(self) -> None: }, ) + def test_create_saml_id_but_duplicate_error(self) -> None: + self.set_models({ + "user/2": { + "username": "x", + "saml_id": "123saml" + } + }) + response = self.request( + "user.create", + { + "saml_id": "123saml", + "default_password": "", + "can_change_own_password": False, + }, + ) + self.assert_status_code(response, 400) + self.assertIn("A user with the saml_id 123saml already exists.", response.json["message"]) + + def test_create_saml_id_but_duplicate_error(self) -> None: + self.set_models({ + "user/2": { + "username": "123saml" + } + }) + response = self.request( + "user.create", + { + "saml_id": "123saml", + "default_password": "", + "can_change_own_password": False, + }, + ) + self.assert_status_code(response, 400) + self.assertIn("A user with the username 123saml already exists.", response.json["message"]) + + def test_create_empty_saml_id_and_empty_values(self) -> None: + response = self.request( + "user.create", + { + "saml_id": " ", + "username": "x" + }, + ) + self.assert_status_code(response, 400) + self.assertIn("This saml_id is forbidden.", response.json["message"]) + def test_create_permission_nothing(self) -> None: self.permission_setup() response = self.request( @@ -1150,7 +1196,7 @@ def test_create_forbidden_username(self) -> None: }, ) self.assert_status_code(response, 400) - assert "This username is forbidden." in response.json["message"] + assert "Need username or first_name or last_name" in response.json["message"] def test_create_gender(self) -> None: self.set_models({"organization/1": {"genders": ["male", "female"]}}) From 70ce3aa18fc6a480b5ca46b83bf2adea0216ba0f Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Fri, 1 Sep 2023 19:04:36 +0200 Subject: [PATCH 08/17] fixed bugs in account.json_upload --- .../actions/user/account_json_upload.py | 74 +++++++++++---- .../action/actions/user/user_mixin.py | 10 +- .../action/mixins/import_mixins.py | 69 ++++++++------ .../system/action/user/test_account_import.py | 56 +++++++++-- .../action/user/test_account_json_upload.py | 92 +++++++++++++++++-- tests/system/action/user/test_create.py | 30 +++--- 6 files changed, 248 insertions(+), 83 deletions(-) diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index f87d36c19..6480952d8 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -1,5 +1,5 @@ from collections import defaultdict -from typing import Any, Dict, List, Optional, Tuple, Union +from typing import Any, Dict, List, Optional, Tuple, Union, cast from ....models.models import User from ....permissions.management_levels import OrganizationManagementLevel @@ -71,6 +71,7 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: data = instance.pop("data") data = self.add_payload_index_to_action_data(data) self.setup_lookups(data) + self.create_usernames(data) self.rows = [ self.validate_entry(entry, payload_index) @@ -82,7 +83,9 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: state_to_count = {state: 0 for state in ImportState} for row in self.rows: state_to_count[row["state"]] += 1 - state_to_count[ImportState.WARNING] += self.count_warnings_in_payload(row.get("data", {}).values()) + state_to_count[ImportState.WARNING] += self.count_warnings_in_payload( + row.get("data", {}).values() + ) row["data"].pop("payload_index", None) self.statistics = [ @@ -104,9 +107,13 @@ def validate_entry( ) -> Dict[str, Any]: messages: List[str] = [] id_: Optional[int] = None - if username := entry.get("username"): + old_saml_id: Optional[str] = None + if (username := entry.get("username")) and type(username) == str: check_result = self.username_lookup.check_duplicate(username) - id_ = self.username_lookup.get_x_by_name(username, "id") + id_ = cast(int, self.username_lookup.get_x_by_name(username, "id")) + old_saml_id = cast( + str, self.all_saml_id_lookup.get_x_by_name(username, "saml_id") + ) if check_result == ResultType.FOUND_ID and id_ != 0: self.row_state = ImportState.DONE entry["id"] = id_ @@ -130,9 +137,9 @@ def validate_entry( messages.append("Found more users with the same username") elif saml_id := entry.get("saml_id"): check_result = self.saml_id_lookup.check_duplicate(saml_id) - id_ = self.saml_id_lookup.get_x_by_name(saml_id, "id") - username = self.saml_id_lookup.get_x_by_name(saml_id, "username") + id_ = cast(int, self.saml_id_lookup.get_x_by_name(saml_id, "id")) if check_result == ResultType.FOUND_ID and id_ != 0: + username = self.saml_id_lookup.get_x_by_name(saml_id, "username") self.row_state = ImportState.DONE entry["id"] = id_ entry["username"] = { @@ -152,11 +159,13 @@ def validate_entry( else: names_and_email = self._names_and_email(entry) check_result = self.names_email_lookup.check_duplicate(names_and_email) - id_ = self.names_email_lookup.get_x_by_name(names_and_email, "id") - username = self.names_email_lookup.get_x_by_name( - names_and_email, "username" + id_ = cast( + int, self.names_email_lookup.get_x_by_name(names_and_email, "id") ) if check_result == ResultType.FOUND_ID and id_ != 0: + username = self.names_email_lookup.get_x_by_name( + names_and_email, "username" + ) self.row_state = ImportState.DONE entry["id"] = id_ entry["username"] = { @@ -176,12 +185,6 @@ def validate_entry( f"The account with id {id_} was found multiple times by different search criteria." ) - if not entry.get("username") and self.row_state == ImportState.NEW: - entry["username"] = { - "value": self.generate_username(entry) or entry.get("saml_id"), - "info": ImportState.GENERATED, - } - if saml_id := entry.get("saml_id"): check_result = self.all_saml_id_lookup.check_duplicate(saml_id) if id_ := entry.get("id"): @@ -192,7 +195,9 @@ def validate_entry( ): entry["saml_id"] = { "value": saml_id, - "info": ImportState.DONE, + "info": ImportState.DONE + if saml_id == old_saml_id + else ImportState.NEW, } else: self.row_state = ImportState.ERROR @@ -214,9 +219,13 @@ def validate_entry( "value": saml_id, "info": ImportState.NEW, } - if entry["saml_id"]["info"] == ImportState.DONE or entry.get("default_password"): + if entry["saml_id"]["info"] == ImportState.NEW or entry.get( + "default_password" + ): entry["default_password"] = {"value": "", "info": ImportState.WARNING} - messages.append("Will remove password and default_password and forbid changing your OpenSlides password.") + messages.append( + "Will remove password and default_password and forbid changing your OpenSlides password." + ) else: self.handle_default_password(entry) return {"state": self.row_state, "messages": messages, "data": entry} @@ -245,6 +254,32 @@ def _names_and_email(entry: Dict[str, Any]) -> Tuple[str, ...]: entry.get("email", ""), ) + def create_usernames(self, data: List[Dict[str, Any]]) -> List[Dict[str, Any]]: + usernames: List[str] = [] + fix_usernames: List[str] = [] + payload_indices: List[int] = [] + + for entry in data: + if "username" not in entry.keys(): + if saml_id := entry.get("saml_id"): + username = saml_id + else: + username = entry.get("first_name", "") + entry.get("last_name", "") + usernames.append(username) + payload_indices.append(entry["payload_index"]) + else: + fix_usernames.append(entry["username"]) + + usernames = self.generate_usernames(usernames, fix_usernames) + + for index, username in zip(payload_indices, usernames): + data[index]["username"] = { + "value": username, + "info": ImportState.GENERATED, + } + self.username_lookup.add_item(data[index]) + return data + def setup_lookups(self, data: List[Dict[str, Any]]) -> None: self.username_lookup = Lookup( self.datastore, @@ -255,6 +290,7 @@ def setup_lookups(self, data: List[Dict[str, Any]]) -> None: if (username := entry.get("username")) ], field="username", + mapped_fields=["saml_id"], ) self.saml_id_lookup = Lookup( self.datastore, @@ -291,7 +327,7 @@ def setup_lookups(self, data: List[Dict[str, Any]]) -> None: "user", [(saml_id, entry) for entry in data if (saml_id := entry.get("saml_id"))], field="saml_id", - mapped_fields=["username"], + mapped_fields=["username", "saml_id"], ) self.all_id_mapping: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( diff --git a/openslides_backend/action/actions/user/user_mixin.py b/openslides_backend/action/actions/user/user_mixin.py index a7e225e42..f59a71e2f 100644 --- a/openslides_backend/action/actions/user/user_mixin.py +++ b/openslides_backend/action/actions/user/user_mixin.py @@ -18,16 +18,20 @@ class UsernameMixin(Action): - def generate_usernames(self, usernames: List[str]) -> List[str]: + def generate_usernames( + self, usernames: List[str], fix_usernames: Optional[List[str]] = None + ) -> List[str]: """ Generate unique usernames in parallel to a given list of usernames """ + if fix_usernames is None: + fix_usernames = [] used_usernames: List[str] = [] for username in usernames: template_username = username count = 0 while True: - if username in used_usernames: + if username in used_usernames or username in fix_usernames: count += 1 username = template_username + str(count) continue @@ -94,7 +98,7 @@ def get_updated_instances(self, action_data: ActionData) -> ActionData: def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: instance = super().update_instance(instance) - def check_existence(what:str) -> None: + def check_existence(what: str) -> None: if what in instance: if not instance[what]: raise ActionException(f"This {what} is forbidden.") diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index f356841e1..34c75ca1f 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -2,7 +2,7 @@ from collections import defaultdict from enum import Enum from time import mktime, strptime, time -from typing import Any, Callable, Dict, List, Optional, Tuple, TypedDict, Union +from typing import Any, Callable, Dict, List, Optional, Tuple, TypedDict, Union, cast from ...shared.exceptions import ActionException from ...shared.filters import And, Filter, FilterOperator, Or @@ -49,9 +49,11 @@ def __init__( self.datastore = datastore self.collection = collection self.field = field - self.name_to_ids: Dict[Union[str, Tuple[str, ...]], List[Dict[str, Any]]] = { - name: [] for name, _ in name_entries - } + self.name_to_ids: Dict[ + Union[str, Tuple[str, ...]], List[Dict[str, Any]] + ] = defaultdict(list) + for name, _ in name_entries: + self.name_to_ids[name] = [] self.id_to_name: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( list ) @@ -79,17 +81,11 @@ def __init__( mapped_fields, lock_result=False, ).values(): - if type(field) == str: - self.name_to_ids[entry[field]].append(entry) - self.id_to_name[entry["id"]].append(entry[field]) - else: - key: Tuple[Any, ...] = tuple(entry.get(f, "") for f in field) - self.name_to_ids[key].append(entry) - self.id_to_name[entry["id"]].append(key) + self.add_item(entry) # Add action data items not found in database to lookup dict for name, entry in name_entries: - if not (values := self.name_to_ids.get(name, [])): + if not (values := cast(list, self.name_to_ids[name])): values.append(entry) else: if not values[0].get("id"): @@ -105,7 +101,9 @@ def check_duplicate(self, name: Union[str, Tuple[str, ...]]) -> ResultType: return ResultType.FOUND_MORE_IDS raise ActionException("Logical Error in Lookup Class") - def get_x_by_name(self, name: Union[str, Tuple[str, ...]], x: str) -> Optional[int]: + def get_x_by_name( + self, name: Union[str, Tuple[str, ...]], x: str + ) -> Optional[Union[int, str]]: """Gets fieldname 'x' from value of name_to_ids-dict""" if len(self.name_to_ids.get(name, [])) == 1: return self.name_to_ids[name][0].get(x) @@ -116,8 +114,34 @@ def get_name_by_id(self, id_: int) -> Optional[List[Union[str, Tuple[str, ...]]] return name return None + def add_item(self, entry: Dict[str, Any]) -> None: + if type(self.field) == str: + if type(key := entry[self.field]) == dict: + key = key["value"] + self.name_to_ids[key].append(entry) + if entry.get("id"): + self.id_to_name[entry["id"]].append(entry[self.field]) + else: + key = tuple(entry.get(f, "") for f in self.field) + self.name_to_ids[key].append(entry) + if entry.get("id"): + self.id_to_name[entry["id"]].append(key) + + +class BaseImportJsonUpload(SingularActionMixin): + @staticmethod + def count_warnings_in_payload(data: Union[List[Union[Dict[str, str], List[Any]]], Dict[str, Any]]) -> int: + count = 0 + for col in data: + if type(col) == dict: + if col.get("info") == ImportState.WARNING: + count += 1 + elif type(col) == list: + count += BaseImportJsonUpload.count_warnings_in_payload(col) + return count -class ImportMixin(SingularActionMixin): + +class ImportMixin(BaseImportJsonUpload): """ Mixin for import actions. It works together with the json_upload. """ @@ -140,7 +164,9 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: f"Wrong id doesn't point on {self.import_name} import data." ) if worker.get("state") not in ImportState.__members__.values(): - raise ActionException("Error in import: Missing valid state in stored worker.") + raise ActionException( + "Error in import: Missing valid state in stored worker." + ) if worker.get("state") == ImportState.ERROR: raise ActionException("Error in import. Data will not be imported.") self.result = worker["result"] @@ -188,7 +214,7 @@ class StatisticEntry(TypedDict): value: int -class JsonUploadMixin(SingularActionMixin): +class JsonUploadMixin(BaseImportJsonUpload): headers: List[Dict[str, Any]] rows: List[Dict[str, Any]] statistics: List[StatisticEntry] @@ -251,17 +277,6 @@ def add_payload_index_to_action_data(self, action_data: ActionData) -> ActionDat entry["payload_index"] = payload_index return action_data - @staticmethod - def count_warnings_in_payload(data: Union[str, List[Dict[str, Any]]]) -> int: - count = 0 - for col in data: - if type(col) == dict: - if col.get("info") == ImportState.WARNING: - count += 1 - elif type(col) == list: - count += JsonUploadMixin.count_warnings_in_payload(col) - return count - def validate_instance(self, instance: Dict[str, Any]) -> None: # filter extra, not needed fields before validate and parse some fields property_to_type = { diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index c93ea64ce..1b8f0385c 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -1,6 +1,6 @@ from typing import Any, Dict -from openslides_backend.action.mixins.import_mixins import ImportState +from openslides_backend.action.mixins.import_mixins import ImportMixin, ImportState from openslides_backend.permissions.management_levels import OrganizationManagementLevel from tests.system.action.base import BaseActionTestCase @@ -65,6 +65,32 @@ def setUp(self) -> None: }, }, "action_worker/5": {"result": None}, + "user/2": { + "username": "test", + "default_password": "secret", + "password": "secret", + }, + "action_worker/6": { + "state": ImportState.WARNING, + "result": { + "import": "account", + "rows": [ + { + "state": ImportState.DONE, + "messages": ["test"], + "data": { + "username": { + "value": "test", + "info": ImportState.DONE, + "id": 2, + }, + "saml_id": "12345", + "default_password": "test2", + }, + }, + ], + }, + }, } ) @@ -140,15 +166,25 @@ def test_import_names_and_email_and_create(self) -> None: ) def get_action_worker_data( - self, number: int, state: ImportState, data: Dict[str, Any] + self, number: int, row_state: ImportState, data: Dict[str, Any] ) -> Dict[str, Any]: + def get_import_state() -> ImportState: + """Precondition: There is only 1 row(_state)""" + if row_state == ImportState.ERROR: + return row_state + if ImportMixin.count_warnings_in_payload(data): + return ImportState.WARNING + else: + return ImportState.DONE + return { f"action_worker/{number}": { + "state": get_import_state(), "result": { "import": "account", "rows": [ { - "state": state, + "state": row_state, "messages": [], "data": data, }, @@ -385,13 +421,19 @@ def test_import_error_state_done_not_matching_ids(self) -> None: "Error: want to update, but found search data doesn't match." ] - def test_import_error_state(self) -> None: + def test_import_error_state_action_worker4(self) -> None: response = self.request("account.import", {"id": 4, "import": True}) + self.assert_status_code(response, 400) + assert response.json["message"] == "Error in import. Data will not be imported." + self.assert_model_exists("action_worker/4") + + def test_import_warning_state_action_worker6(self) -> None: + response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] - assert entry["state"] == ImportState.ERROR - assert entry["messages"] == ["test", "Error in import."] - self.assert_model_exists("action_worker/4") + assert entry["state"] == ImportState.WARNING + assert entry["messages"] == ["test"] + self.assert_model_exists("action_worker/6") def test_import_no_permission(self) -> None: self.base_permission_test({}, "account.import", {"id": 2, "import": True}) diff --git a/tests/system/action/user/test_account_json_upload.py b/tests/system/action/user/test_account_json_upload.py index d108717b2..faf6653bf 100644 --- a/tests/system/action/user/test_account_json_upload.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -2,11 +2,12 @@ from openslides_backend.action.mixins.import_mixins import ImportState from openslides_backend.permissions.management_levels import OrganizationManagementLevel +from openslides_backend.shared.patterns import fqid_from_collection_and_id from tests.system.action.base import BaseActionTestCase class AccountJsonUpload(BaseActionTestCase): - def test_json_upload_simple(self) -> None: + def test_json_upload_simple(self) -> int: start_time = int(time()) response = self.request( "account.json_upload", @@ -34,11 +35,17 @@ def test_json_upload_simple(self) -> None: "is_physical_person": False, }, } - worker = self.assert_model_exists("action_worker/1") + action_worker_id = response.json["results"][0][0].get("id") + action_worker_fqid = fqid_from_collection_and_id( + "action_worker", action_worker_id + ) + worker = self.assert_model_exists(action_worker_fqid) assert worker["result"]["import"] == "account" assert start_time <= worker["created"] <= end_time assert start_time <= worker["timestamp"] <= end_time + return action_worker_id + def test_json_upload_empty_data(self) -> None: response = self.request( "account.json_upload", @@ -174,10 +181,11 @@ def test_json_upload_duplicate_in_db(self) -> None: { "state": ImportState.ERROR, "messages": [ - "The account with id 3 was found multiple times by different search criteria.", "Will remove password and default_password and forbid changing your OpenSlides password." + "The account with id 3 was found multiple times by different search criteria.", + "Will remove password and default_password and forbid changing your OpenSlides password.", ], "data": { - "saml_id": {"value": "12345", "info": "done"}, + "saml_id": {"value": "12345", "info": "new"}, "id": 3, "username": {"value": "test", "info": "done", "id": 3}, "default_password": {"info": "warning", "value": ""}, @@ -237,6 +245,7 @@ def test_json_upload_multiple_duplicates(self) -> None: "first_name": "Max", "last_name": "Mustermann", "email": "max@mustermann.org", + "username": {"value": "MaxMustermann", "info": "generated"}, }, } ] @@ -398,29 +407,96 @@ def test_json_upload_generate_default_password(self) -> None: == ImportState.GENERATED ) - def test_json_upload_saml_id(self) -> None: + def test_json_upload_saml_id_new(self) -> None: + self.set_models( + { + "user/34": { + "first_name": "Max", + "last_name": "Mustermann", + "email": "test@ntvtn.de", + "username": "test_saml_id", + } + } + ) + + response = self.request( + "account.json_upload", + { + "data": [ + { + "saml_id": "test_saml_id", + "default_password": "test2", + }, + { + "username": "test_saml_id1", + }, + { + "first_name": "test_sa", + "last_name": "ml_id2", + }, + ], + }, + ) + self.assert_status_code(response, 200) + worker = self.assert_model_exists("action_worker/1") + assert worker["result"]["import"] == "account" + assert worker["result"]["rows"][0]["messages"] == [ + "Will remove password and default_password and forbid changing your OpenSlides password." + ] + assert worker["result"]["rows"][0]["state"] == ImportState.NEW + data0 = worker["result"]["rows"][0]["data"] + assert data0 == { + "saml_id": {"info": "new", "value": "test_saml_id"}, + "username": {"info": "generated", "value": "test_saml_id2"}, + "default_password": {"info": "warning", "value": ""}, + } + assert worker["result"]["rows"][1]["data"]["username"] == { + "info": "done", + "value": "test_saml_id1", + } + assert worker["result"]["rows"][2]["data"]["username"] == { + "info": "generated", + "value": "test_saml_id21", + } + assert worker["result"]["rows"][2]["data"]["last_name"] == "ml_id2" + assert worker["result"]["rows"][2]["data"]["first_name"] == "test_sa" + + def test_json_upload_saml_id_done(self) -> None: + self.set_models( + { + "user/2": { + "username": "test", + "password": "secret", + "default_password": "secret", + "can_change_own_password": True, + } + } + ) response = self.request( "account.json_upload", { "data": [ { + "username": "test", "saml_id": "test_saml_id", - "password": "test2", - "default_password": "test3", + "default_password": "secret2", } ], }, ) self.assert_status_code(response, 200) worker = self.assert_model_exists("action_worker/1") + assert worker["state"] == ImportState.WARNING assert worker["result"]["import"] == "account" + assert worker["result"]["rows"][0]["state"] == ImportState.DONE assert worker["result"]["rows"][0]["messages"] == [ "Will remove password and default_password and forbid changing your OpenSlides password." ] data = worker["result"]["rows"][0]["data"] assert data == { + "id": 2, "saml_id": {"info": "new", "value": "test_saml_id"}, - "username": {"info": "generated", "value": "test_saml_id"}, + "username": {"info": "done", "value": "test", "id": 2}, "default_password": {"info": "warning", "value": ""}, } diff --git a/tests/system/action/user/test_create.py b/tests/system/action/user/test_create.py index 411331a70..667db8267 100644 --- a/tests/system/action/user/test_create.py +++ b/tests/system/action/user/test_create.py @@ -448,13 +448,8 @@ def test_create_saml_id_and_empty_values(self) -> None: }, ) - def test_create_saml_id_but_duplicate_error(self) -> None: - self.set_models({ - "user/2": { - "username": "x", - "saml_id": "123saml" - } - }) + def test_create_saml_id_but_duplicate_error1(self) -> None: + self.set_models({"user/2": {"username": "x", "saml_id": "123saml"}}) response = self.request( "user.create", { @@ -464,14 +459,12 @@ def test_create_saml_id_but_duplicate_error(self) -> None: }, ) self.assert_status_code(response, 400) - self.assertIn("A user with the saml_id 123saml already exists.", response.json["message"]) + self.assertIn( + "A user with the saml_id 123saml already exists.", response.json["message"] + ) - def test_create_saml_id_but_duplicate_error(self) -> None: - self.set_models({ - "user/2": { - "username": "123saml" - } - }) + def test_create_saml_id_but_duplicate_error2(self) -> None: + self.set_models({"user/2": {"username": "123saml"}}) response = self.request( "user.create", { @@ -481,15 +474,14 @@ def test_create_saml_id_but_duplicate_error(self) -> None: }, ) self.assert_status_code(response, 400) - self.assertIn("A user with the username 123saml already exists.", response.json["message"]) + self.assertIn( + "A user with the username 123saml already exists.", response.json["message"] + ) def test_create_empty_saml_id_and_empty_values(self) -> None: response = self.request( "user.create", - { - "saml_id": " ", - "username": "x" - }, + {"saml_id": " ", "username": "x"}, ) self.assert_status_code(response, 400) self.assertIn("This saml_id is forbidden.", response.json["message"]) From dbd0f8a2a5688e99e72fcbca8df8d779cde13a9c Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Mon, 4 Sep 2023 14:57:03 +0200 Subject: [PATCH 09/17] fix some account import errors --- openslides_backend/action/actions/user/account_import.py | 3 ++- tests/system/action/user/test_account_import.py | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/openslides_backend/action/actions/user/account_import.py b/openslides_backend/action/actions/user/account_import.py index 4a8419ffb..9645f8209 100644 --- a/openslides_backend/action/actions/user/account_import.py +++ b/openslides_backend/action/actions/user/account_import.py @@ -43,7 +43,8 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: if field in entry["data"]: if field == "username" and "id" in entry["data"][field]: entry["data"]["id"] = entry["data"][field]["id"] - entry["data"][field] = entry["data"][field]["value"] + if type(dvalue := entry["data"][field]) == dict: + entry["data"][field] = dvalue["value"] search_data_list = [ { diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index 1b8f0385c..241a655d2 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -196,15 +196,15 @@ def get_import_state() -> ImportState: def test_import_with_saml_id(self) -> None: self.set_models( self.get_action_worker_data( - 6, + 7, ImportState.NEW, {"saml_id": {"value": "testsaml", "info": ImportState.NEW}}, ) ) - response = self.request("account.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 7, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( - "user/2", + "user/3", { "username": "testsaml", "saml_id": "testsaml", From 28ba44fbd83ef658edc4a47bf6b394c852ccfaf8 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Mon, 4 Sep 2023 16:03:19 +0200 Subject: [PATCH 10/17] test fix --- tests/system/action/user/test_account_import.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index 241a655d2..d6c04221f 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -84,8 +84,15 @@ def setUp(self) -> None: "info": ImportState.DONE, "id": 2, }, - "saml_id": "12345", - "default_password": "test2", + "saml_id": { + "value": "12345", + "info": ImportState.DONE, + "id": 2, + }, + "default_password": { + "value": "test2", + "info": ImportState.WARNING, + }, }, }, ], @@ -431,9 +438,9 @@ def test_import_warning_state_action_worker6(self) -> None: response = self.request("account.import", {"id": 6, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][0] - assert entry["state"] == ImportState.WARNING + assert entry["state"] == ImportState.DONE assert entry["messages"] == ["test"] - self.assert_model_exists("action_worker/6") + self.assert_model_not_exists("action_worker/6") def test_import_no_permission(self) -> None: self.base_permission_test({}, "account.import", {"id": 2, "import": True}) From 0ed68b0d9d490c6d63b48903fc21647fce37cb5b Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 5 Sep 2023 15:44:11 +0200 Subject: [PATCH 11/17] Pre ready for review, still with some errors in test --- .../action/actions/user/account_import.py | 6 +- .../actions/user/account_json_upload.py | 6 +- .../action/actions/user/update.py | 4 + .../system/action/user/test_account_import.py | 41 ++++ .../action/user/test_account_json_upload.py | 223 ++++++++++-------- 5 files changed, 180 insertions(+), 100 deletions(-) diff --git a/openslides_backend/action/actions/user/account_import.py b/openslides_backend/action/actions/user/account_import.py index 9645f8209..0e438aaaf 100644 --- a/openslides_backend/action/actions/user/account_import.py +++ b/openslides_backend/action/actions/user/account_import.py @@ -132,10 +132,8 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: "Error: want to update, but found search data doesn't match." ) else: - for field in ("username", "saml_id"): - if field in entry["data"]: - del entry["data"][field] - + if "username" in entry["data"]: + del entry["data"]["username"] update_action_payload.append(entry["data"]) else: self.error = True diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index 6480952d8..f3d233f7e 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -112,7 +112,7 @@ def validate_entry( check_result = self.username_lookup.check_duplicate(username) id_ = cast(int, self.username_lookup.get_x_by_name(username, "id")) old_saml_id = cast( - str, self.all_saml_id_lookup.get_x_by_name(username, "saml_id") + str, self.username_lookup.get_x_by_name(username, "saml_id") ) if check_result == ResultType.FOUND_ID and id_ != 0: self.row_state = ImportState.DONE @@ -196,8 +196,8 @@ def validate_entry( entry["saml_id"] = { "value": saml_id, "info": ImportState.DONE - if saml_id == old_saml_id - else ImportState.NEW, + if old_saml_id + else ImportState.NEW, # only if newly set } else: self.row_state = ImportState.ERROR diff --git a/openslides_backend/action/actions/user/update.py b/openslides_backend/action/actions/user/update.py index 9eb315cf0..a2304861e 100644 --- a/openslides_backend/action/actions/user/update.py +++ b/openslides_backend/action/actions/user/update.py @@ -75,6 +75,10 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: raise ActionException( f"user {user['saml_id']} is a Single Sign On user and may not set the local default_passwort or the right to change it locally." ) + if instance.get("saml_id") and not user.get("saml_id"): + instance["can_change_own_password"] = False + instance["default_password"] = "" + instance["password"] = "" if ( instance["id"] == self.user_id diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index d6c04221f..503424339 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -3,6 +3,7 @@ from openslides_backend.action.mixins.import_mixins import ImportMixin, ImportState from openslides_backend.permissions.management_levels import OrganizationManagementLevel from tests.system.action.base import BaseActionTestCase +from .test_account_json_upload import AccountJsonUploadForUseInImport class AccountJsonImport(BaseActionTestCase): @@ -452,3 +453,43 @@ def test_import_permission(self) -> None: {"id": 2, "import": True}, OrganizationManagementLevel.CAN_MANAGE_USERS, ) + + +class AccountJsonImportWithIncludedJsonUpload(AccountJsonUploadForUseInImport): + def test_upload_import_with_generated_usernames_okay(self) -> None: + self.json_upload_saml_id_new() + response_import = self.request("account.import", {"id": 1, "import": True}) + self.assert_status_code(response_import, 200) + self.assert_model_exists("user/35", {"username": "test_saml_id2", "saml_id": "test_saml_id", "default_password": "", "can_change_own_password": False, "default_vote_weight": "1.000000"}) + self.assert_model_exists("user/36", {"username": "test_saml_id1", "saml_id": None, "can_change_own_password": True, "default_vote_weight": "1.000000"}) + self.assert_model_exists("user/37", {"username": "test_saml_id21", "saml_id": None, "can_change_own_password": True, "default_vote_weight": "1.000000"}) + self.assert_model_not_exists("action_worker/1") + + def test_upload_import_with_generated_usernames_error_username(self) -> None: + self.json_upload_saml_id_new() + self.set_models({ + "user/33": {"username": "test_saml_id21"} + }) + response = self.request("account.import", {"id": 1, "import": True}) + self.assert_status_code(response, 200) + assert response.json["results"][0][0]["rows"][2]["state"] == ImportState.ERROR + assert response.json["results"][0][0]["rows"][2]["messages"] == ["Error: want to create a new user, but username already exists."] + assert response.json["results"][0][0]["rows"][2]["data"]["username"] == 'test_saml_id21' + self.assert_model_not_exists("user/35") + self.assert_model_not_exists("user/36") + self.assert_model_not_exists("user/37") + self.assert_model_exists("action_worker/1") + + def test_json_upload_set_saml_id_in_existing_account(self) -> None: + self.json_upload_set_saml_id_in_existing_account() + response_import = self.request("account.import", {"id": 1, "import": True}) + self.assert_status_code(response_import, 200) + self.assert_model_exists("user/2", {"username": "test", "saml_id": "test_saml_id", "default_password": "", "can_change_own_password": False, "password": "", "default_vote_weight": "2.300000"}) + self.assert_model_not_exists("action_worker/1") + + def test_json_upload_update_saml_id_in_existing_account(self) -> None: + self.json_upload_update_saml_id_in_existing_account() + response_import = self.request("account.import", {"id": 1, "import": True}) + self.assert_status_code(response_import, 200) + self.assert_model_exists("user/2", {"username": "test", "saml_id": "new_one", "default_vote_weight": "2.300000"}) + self.assert_model_not_exists("action_worker/1") diff --git a/tests/system/action/user/test_account_json_upload.py b/tests/system/action/user/test_account_json_upload.py index faf6653bf..1b8ef410d 100644 --- a/tests/system/action/user/test_account_json_upload.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -407,99 +407,6 @@ def test_json_upload_generate_default_password(self) -> None: == ImportState.GENERATED ) - def test_json_upload_saml_id_new(self) -> None: - self.set_models( - { - "user/34": { - "first_name": "Max", - "last_name": "Mustermann", - "email": "test@ntvtn.de", - "username": "test_saml_id", - } - } - ) - - response = self.request( - "account.json_upload", - { - "data": [ - { - "saml_id": "test_saml_id", - "default_password": "test2", - }, - { - "username": "test_saml_id1", - }, - { - "first_name": "test_sa", - "last_name": "ml_id2", - }, - ], - }, - ) - self.assert_status_code(response, 200) - worker = self.assert_model_exists("action_worker/1") - assert worker["result"]["import"] == "account" - assert worker["result"]["rows"][0]["messages"] == [ - "Will remove password and default_password and forbid changing your OpenSlides password." - ] - assert worker["result"]["rows"][0]["state"] == ImportState.NEW - data0 = worker["result"]["rows"][0]["data"] - assert data0 == { - "saml_id": {"info": "new", "value": "test_saml_id"}, - "username": {"info": "generated", "value": "test_saml_id2"}, - "default_password": {"info": "warning", "value": ""}, - } - assert worker["result"]["rows"][1]["data"]["username"] == { - "info": "done", - "value": "test_saml_id1", - } - assert worker["result"]["rows"][2]["data"]["username"] == { - "info": "generated", - "value": "test_saml_id21", - } - assert worker["result"]["rows"][2]["data"]["last_name"] == "ml_id2" - assert worker["result"]["rows"][2]["data"]["first_name"] == "test_sa" - - def test_json_upload_saml_id_done(self) -> None: - self.set_models( - { - "user/2": { - "username": "test", - "password": "secret", - "default_password": "secret", - "can_change_own_password": True, - } - } - ) - response = self.request( - "account.json_upload", - { - "data": [ - { - "username": "test", - "saml_id": "test_saml_id", - "default_password": "secret2", - } - ], - }, - ) - self.assert_status_code(response, 200) - worker = self.assert_model_exists("action_worker/1") - assert worker["state"] == ImportState.WARNING - assert worker["result"]["import"] == "account" - assert worker["result"]["rows"][0]["state"] == ImportState.DONE - assert worker["result"]["rows"][0]["messages"] == [ - "Will remove password and default_password and forbid changing your OpenSlides password." - ] - data = worker["result"]["rows"][0]["data"] - assert data == { - "id": 2, - "saml_id": {"info": "new", "value": "test_saml_id"}, - "username": {"info": "done", "value": "test", "id": 2}, - "default_password": {"info": "warning", "value": ""}, - } - def test_json_upload_duplicated_one_saml_id(self) -> None: self.set_models( { @@ -677,3 +584,133 @@ def test_json_upload_permission(self) -> None: {"data": [{"username": "test"}]}, OrganizationManagementLevel.CAN_MANAGE_USERS, ) + + +class AccountJsonUploadForUseInImport(BaseActionTestCase): + def json_upload_saml_id_new(self) -> None: + self.set_models( + { + "user/34": { + "first_name": "Max", + "last_name": "Mustermann", + "email": "test@ntvtn.de", + "username": "test_saml_id", + } + } + ) + + response = self.request( + "account.json_upload", + { + "data": [ + { + "saml_id": "test_saml_id", + "default_password": "test2", + }, + { + "username": "test_saml_id1", + }, + { + "first_name": "test_sa", + "last_name": "ml_id2", + }, + ], + }, + ) + self.assert_status_code(response, 200) + worker = self.assert_model_exists("action_worker/1") + assert worker["result"]["import"] == "account" + assert worker["result"]["rows"][0]["messages"] == [ + "Will remove password and default_password and forbid changing your OpenSlides password." + ] + assert worker["result"]["rows"][0]["state"] == ImportState.NEW + data0 = worker["result"]["rows"][0]["data"] + assert data0 == { + "saml_id": {"info": "new", "value": "test_saml_id"}, + "username": {"info": "generated", "value": "test_saml_id2"}, + "default_password": {"info": "warning", "value": ""}, + } + assert worker["result"]["rows"][1]["data"]["username"] == { + "info": "done", + "value": "test_saml_id1", + } + assert worker["result"]["rows"][2]["data"]["username"] == { + "info": "generated", + "value": "test_saml_id21", + } + assert worker["result"]["rows"][2]["data"]["last_name"] == "ml_id2" + assert worker["result"]["rows"][2]["data"]["first_name"] == "test_sa" + + def json_upload_set_saml_id_in_existing_account(self) -> None: + self.set_models( + { + "user/2": { + "username": "test", + "password": "secret", + "default_password": "secret", + "can_change_own_password": True, + "default_vote_weight": "2.300000", + } + } + ) + response = self.request( + "account.json_upload", + { + "data": [ + { + "username": "test", + "saml_id": "test_saml_id", + "default_password": "secret2", + } + ], + }, + ) + self.assert_status_code(response, 200) + worker = self.assert_model_exists("action_worker/1") + assert worker["state"] == ImportState.WARNING + assert worker["result"]["import"] == "account" + assert worker["result"]["rows"][0]["state"] == ImportState.DONE + assert worker["result"]["rows"][0]["messages"] == [ + "Will remove password and default_password and forbid changing your OpenSlides password." + ] + data = worker["result"]["rows"][0]["data"] + assert data == { + "id": 2, + "saml_id": {"info": "new", "value": "test_saml_id"}, + "username": {"info": "done", "value": "test", "id": 2}, + "default_password": {"info": "warning", "value": ""}, + } + + def json_upload_update_saml_id_in_existing_account(self) -> None: + self.set_models( + { + "user/2": { + "username": "test", + "default_vote_weight": "2.300000", + "saml_id": "old_one", + } + } + ) + response = self.request( + "account.json_upload", + { + "data": [ + { + "username": "test", + "saml_id": "new_one", + } + ], + }, + ) + self.assert_status_code(response, 200) + worker = self.assert_model_exists("action_worker/1") + assert worker["state"] == ImportState.DONE + assert worker["result"]["import"] == "account" + assert worker["result"]["rows"][0]["state"] == ImportState.DONE + assert worker["result"]["rows"][0]["messages"] == [] + data = worker["result"]["rows"][0]["data"] + assert data == { + "id": 2, + "saml_id": {"info": "done", "value": "new_one"}, + "username": {"info": "done", "value": "test", "id": 2}, + } From e6ea1c0ad3b3049ff8aef0b7dcd87c0e2bb690f7 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 5 Sep 2023 17:23:50 +0200 Subject: [PATCH 12/17] fix acoount_import tests --- .../system/action/user/test_account_import.py | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index 503424339..d4965cd61 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -21,7 +21,7 @@ def setUp(self) -> None: "messages": [], "data": { "username": { - "value": "test", + "value": "jonny", "info": ImportState.DONE, }, "first_name": "Testy", @@ -105,19 +105,19 @@ def setUp(self) -> None: def test_import_username_and_create(self) -> None: response = self.request("account.import", {"id": 2, "import": True}) self.assert_status_code(response, 200) - user2 = self.assert_model_exists( - "user/2", - {"username": "test", "first_name": "Testy"}, + user = self.assert_model_exists( + "user/3", + {"username": "jonny", "first_name": "Testy"}, ) - assert user2.get("default_password") - assert user2.get("password") + assert user.get("default_password") + assert user.get("password") self.assert_model_not_exists("action_worker/2") def test_import_abort(self) -> None: response = self.request("account.import", {"id": 2, "import": False}) self.assert_status_code(response, 200) self.assert_model_not_exists("action_worker/2") - self.assert_model_not_exists("user/2") + self.assert_model_not_exists("user/3") def test_import_wrong_action_worker(self) -> None: response = self.request("account.import", {"id": 5, "import": True}) @@ -130,9 +130,9 @@ def test_import_username_and_update(self) -> None: self.set_models( { "user/1": { - "username": "test", + "username": "user1", }, - "action_worker/6": { + "action_worker/7": { "state": ImportState.DONE, "result": { "import": "account", @@ -142,7 +142,7 @@ def test_import_username_and_update(self) -> None: "messages": [], "data": { "username": { - "value": "test", + "value": "user1", "info": ImportState.DONE, "id": 1, }, @@ -154,16 +154,15 @@ def test_import_username_and_update(self) -> None: }, } ) - response = self.request("account.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 7, "import": True}) self.assert_status_code(response, 200) - self.assert_model_not_exists("user/2") self.assert_model_exists("user/1", {"first_name": "Testy"}) def test_import_names_and_email_and_create(self) -> None: response = self.request("account.import", {"id": 3, "import": True}) self.assert_status_code(response, 200) self.assert_model_exists( - "user/2", + "user/3", { "username": "TestyTester", "first_name": "Testy", @@ -353,7 +352,7 @@ def test_import_error_state_done_missing_user_in_db(self) -> None: ImportState.DONE, { "first_name": "Testy", - "username": {"value": "test", "info": ImportState.DONE}, + "username": {"value": "fred", "info": ImportState.DONE}, }, ) ) @@ -366,7 +365,8 @@ def test_import_error_state_done_missing_user_in_db(self) -> None: def test_import_error_state_done_search_data_error(self) -> None: self.set_models( { - "action_worker/6": { + "action_worker/7": { + "state": ImportState.DONE, "result": { "import": "account", "rows": [ @@ -375,7 +375,7 @@ def test_import_error_state_done_search_data_error(self) -> None: "messages": [], "data": { "username": { - "value": "test", + "value": "durban", "info": ImportState.DONE, } }, @@ -385,7 +385,7 @@ def test_import_error_state_done_search_data_error(self) -> None: "messages": [], "data": { "username": { - "value": "test", + "value": "durban", "info": ImportState.DONE, } }, @@ -395,7 +395,7 @@ def test_import_error_state_done_search_data_error(self) -> None: } } ) - response = self.request("account.import", {"id": 6, "import": True}) + response = self.request("account.import", {"id": 7, "import": True}) self.assert_status_code(response, 200) entry = response.json["results"][0][0]["rows"][1] assert entry["state"] == ImportState.ERROR @@ -406,14 +406,14 @@ def test_import_error_state_done_search_data_error(self) -> None: def test_import_error_state_done_not_matching_ids(self) -> None: self.set_models( { - "user/8": {"username": "test"}, + "user/8": {"username": "user8"}, **self.get_action_worker_data( 6, ImportState.DONE, { "first_name": "Testy", "username": { - "value": "test", + "value": "user8", "info": ImportState.DONE, "id": 5, }, From b8ed6875271060afee62f171e92cd17714b7bfb5 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Wed, 6 Sep 2023 09:10:33 +0200 Subject: [PATCH 13/17] fixed all errors --- .../action/mixins/import_mixins.py | 4 +- tests/system/action/topic/test_import.py | 14 ++-- .../system/action/user/test_account_import.py | 68 ++++++++++++++++--- 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 34c75ca1f..097c69580 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -130,7 +130,9 @@ def add_item(self, entry: Dict[str, Any]) -> None: class BaseImportJsonUpload(SingularActionMixin): @staticmethod - def count_warnings_in_payload(data: Union[List[Union[Dict[str, str], List[Any]]], Dict[str, Any]]) -> int: + def count_warnings_in_payload( + data: Union[List[Union[Dict[str, str], List[Any]]], Dict[str, Any]] + ) -> int: count = 0 for col in data: if type(col) == dict: diff --git a/tests/system/action/topic/test_import.py b/tests/system/action/topic/test_import.py index ef09d5982..3d1a1a2cc 100644 --- a/tests/system/action/topic/test_import.py +++ b/tests/system/action/topic/test_import.py @@ -9,6 +9,7 @@ def setUp(self) -> None: { "meeting/22": {"name": "test", "is_active_in_organization_id": 1}, "action_worker/2": { + "state": ImportState.DONE, "result": { "import": "topic", "rows": [ @@ -17,13 +18,14 @@ def setUp(self) -> None: "messages": [], "data": {"title": "test", "meeting_id": 22}, }, - { - "state": ImportState.ERROR, - "messages": ["test"], - "data": {"title": "broken", "meeting_id": 22}, - }, + # This one leads to an import state ImportState.ERROR + # { + # "state": ImportState.ERROR, + # "messages": ["test"], + # "data": {"title": "broken", "meeting_id": 22}, + # }, ], - } + }, }, } ) diff --git a/tests/system/action/user/test_account_import.py b/tests/system/action/user/test_account_import.py index d4965cd61..3dd577fdd 100644 --- a/tests/system/action/user/test_account_import.py +++ b/tests/system/action/user/test_account_import.py @@ -3,7 +3,8 @@ from openslides_backend.action.mixins.import_mixins import ImportMixin, ImportState from openslides_backend.permissions.management_levels import OrganizationManagementLevel from tests.system.action.base import BaseActionTestCase -from .test_account_json_upload import AccountJsonUploadForUseInImport + +from .test_account_json_upload import AccountJsonUploadForUseInImport class AccountJsonImport(BaseActionTestCase): @@ -460,21 +461,49 @@ def test_upload_import_with_generated_usernames_okay(self) -> None: self.json_upload_saml_id_new() response_import = self.request("account.import", {"id": 1, "import": True}) self.assert_status_code(response_import, 200) - self.assert_model_exists("user/35", {"username": "test_saml_id2", "saml_id": "test_saml_id", "default_password": "", "can_change_own_password": False, "default_vote_weight": "1.000000"}) - self.assert_model_exists("user/36", {"username": "test_saml_id1", "saml_id": None, "can_change_own_password": True, "default_vote_weight": "1.000000"}) - self.assert_model_exists("user/37", {"username": "test_saml_id21", "saml_id": None, "can_change_own_password": True, "default_vote_weight": "1.000000"}) + self.assert_model_exists( + "user/35", + { + "username": "test_saml_id2", + "saml_id": "test_saml_id", + "default_password": "", + "can_change_own_password": False, + "default_vote_weight": "1.000000", + }, + ) + self.assert_model_exists( + "user/36", + { + "username": "test_saml_id1", + "saml_id": None, + "can_change_own_password": True, + "default_vote_weight": "1.000000", + }, + ) + self.assert_model_exists( + "user/37", + { + "username": "test_saml_id21", + "saml_id": None, + "can_change_own_password": True, + "default_vote_weight": "1.000000", + }, + ) self.assert_model_not_exists("action_worker/1") def test_upload_import_with_generated_usernames_error_username(self) -> None: self.json_upload_saml_id_new() - self.set_models({ - "user/33": {"username": "test_saml_id21"} - }) + self.set_models({"user/33": {"username": "test_saml_id21"}}) response = self.request("account.import", {"id": 1, "import": True}) self.assert_status_code(response, 200) assert response.json["results"][0][0]["rows"][2]["state"] == ImportState.ERROR - assert response.json["results"][0][0]["rows"][2]["messages"] == ["Error: want to create a new user, but username already exists."] - assert response.json["results"][0][0]["rows"][2]["data"]["username"] == 'test_saml_id21' + assert response.json["results"][0][0]["rows"][2]["messages"] == [ + "Error: want to create a new user, but username already exists." + ] + assert ( + response.json["results"][0][0]["rows"][2]["data"]["username"] + == "test_saml_id21" + ) self.assert_model_not_exists("user/35") self.assert_model_not_exists("user/36") self.assert_model_not_exists("user/37") @@ -484,12 +513,29 @@ def test_json_upload_set_saml_id_in_existing_account(self) -> None: self.json_upload_set_saml_id_in_existing_account() response_import = self.request("account.import", {"id": 1, "import": True}) self.assert_status_code(response_import, 200) - self.assert_model_exists("user/2", {"username": "test", "saml_id": "test_saml_id", "default_password": "", "can_change_own_password": False, "password": "", "default_vote_weight": "2.300000"}) + self.assert_model_exists( + "user/2", + { + "username": "test", + "saml_id": "test_saml_id", + "default_password": "", + "can_change_own_password": False, + "password": "", + "default_vote_weight": "2.300000", + }, + ) self.assert_model_not_exists("action_worker/1") def test_json_upload_update_saml_id_in_existing_account(self) -> None: self.json_upload_update_saml_id_in_existing_account() response_import = self.request("account.import", {"id": 1, "import": True}) self.assert_status_code(response_import, 200) - self.assert_model_exists("user/2", {"username": "test", "saml_id": "new_one", "default_vote_weight": "2.300000"}) + self.assert_model_exists( + "user/2", + { + "username": "test", + "saml_id": "new_one", + "default_vote_weight": "2.300000", + }, + ) self.assert_model_not_exists("action_worker/1") From 9216821b563b19a02929091c60373479719d645a Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Wed, 6 Sep 2023 15:27:21 +0200 Subject: [PATCH 14/17] fix code review remarks --- .../actions/user/account_json_upload.py | 29 +++++++++++-------- .../action/mixins/import_mixins.py | 29 ++++++++++--------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index f3d233f7e..6dc02dcc0 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -1,9 +1,15 @@ from collections import defaultdict -from typing import Any, Dict, List, Optional, Tuple, Union, cast +from typing import Any, Dict, List, Optional, Tuple, cast from ....models.models import User from ....permissions.management_levels import OrganizationManagementLevel -from ...mixins.import_mixins import ImportState, JsonUploadMixin, Lookup, ResultType +from ...mixins.import_mixins import ( + ImportState, + JsonUploadMixin, + Lookup, + ResultType, + SearchFieldType, +) from ...util.crypto import get_random_password from ...util.default_schema import DefaultSchema from ...util.register import register_action @@ -110,9 +116,9 @@ def validate_entry( old_saml_id: Optional[str] = None if (username := entry.get("username")) and type(username) == str: check_result = self.username_lookup.check_duplicate(username) - id_ = cast(int, self.username_lookup.get_x_by_name(username, "id")) + id_ = cast(int, self.username_lookup.get_field_by_name(username, "id")) old_saml_id = cast( - str, self.username_lookup.get_x_by_name(username, "saml_id") + str, self.username_lookup.get_field_by_name(username, "saml_id") ) if check_result == ResultType.FOUND_ID and id_ != 0: self.row_state = ImportState.DONE @@ -137,9 +143,9 @@ def validate_entry( messages.append("Found more users with the same username") elif saml_id := entry.get("saml_id"): check_result = self.saml_id_lookup.check_duplicate(saml_id) - id_ = cast(int, self.saml_id_lookup.get_x_by_name(saml_id, "id")) + id_ = cast(int, self.saml_id_lookup.get_field_by_name(saml_id, "id")) if check_result == ResultType.FOUND_ID and id_ != 0: - username = self.saml_id_lookup.get_x_by_name(saml_id, "username") + username = self.saml_id_lookup.get_field_by_name(saml_id, "username") self.row_state = ImportState.DONE entry["id"] = id_ entry["username"] = { @@ -160,10 +166,11 @@ def validate_entry( names_and_email = self._names_and_email(entry) check_result = self.names_email_lookup.check_duplicate(names_and_email) id_ = cast( - int, self.names_email_lookup.get_x_by_name(names_and_email, "id") + int, + self.names_email_lookup.get_field_by_name(names_and_email, "id"), ) if check_result == ResultType.FOUND_ID and id_ != 0: - username = self.names_email_lookup.get_x_by_name( + username = self.names_email_lookup.get_field_by_name( names_and_email, "username" ) self.row_state = ImportState.DONE @@ -189,7 +196,7 @@ def validate_entry( check_result = self.all_saml_id_lookup.check_duplicate(saml_id) if id_ := entry.get("id"): if check_result == ResultType.FOUND_ID: - idFound = self.all_saml_id_lookup.get_x_by_name(saml_id, "id") + idFound = self.all_saml_id_lookup.get_field_by_name(saml_id, "id") if check_result == ResultType.NOT_FOUND or ( check_result == ResultType.FOUND_ID and id_ == idFound ): @@ -330,9 +337,7 @@ def setup_lookups(self, data: List[Dict[str, Any]]) -> None: mapped_fields=["username", "saml_id"], ) - self.all_id_mapping: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( - list - ) + self.all_id_mapping: Dict[int, List[SearchFieldType]] = defaultdict(list) for id, values in self.username_lookup.id_to_name.items(): self.all_id_mapping[id].extend(values) for id, values in self.saml_id_lookup.id_to_name.items(): diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 097c69580..846169852 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -16,6 +16,9 @@ TRUE_VALUES = ("1", "true", "yes", "y", "t") FALSE_VALUES = ("0", "false", "no", "n", "f") +# A searchfield can be a string or a tuple of strings +SearchFieldType = Union[str, Tuple[str, ...]] + class ImportState(str, Enum): NONE = "none" @@ -40,8 +43,8 @@ def __init__( self, datastore: DatastoreService, collection: str, - name_entries: List[Tuple[Union[str, Tuple[str, ...]], Dict[str, Any]]], - field: Union[str, Tuple[str, ...]] = "name", + name_entries: List[Tuple[SearchFieldType, Dict[str, Any]]], + field: SearchFieldType = "name", mapped_fields: Optional[List[str]] = None, ) -> None: if mapped_fields is None: @@ -49,14 +52,12 @@ def __init__( self.datastore = datastore self.collection = collection self.field = field - self.name_to_ids: Dict[ - Union[str, Tuple[str, ...]], List[Dict[str, Any]] - ] = defaultdict(list) - for name, _ in name_entries: - self.name_to_ids[name] = [] - self.id_to_name: Dict[int, List[Union[str, Tuple[str, ...]]]] = defaultdict( + self.name_to_ids: Dict[SearchFieldType, List[Dict[str, Any]]] = defaultdict( list ) + for name, _ in name_entries: + self.name_to_ids[name] = [] + self.id_to_name: Dict[int, List[SearchFieldType]] = defaultdict(list) or_filters: List[Filter] = [] if "id" not in mapped_fields: mapped_fields.append("id") @@ -91,7 +92,7 @@ def __init__( if not values[0].get("id"): values.append(entry) - def check_duplicate(self, name: Union[str, Tuple[str, ...]]) -> ResultType: + def check_duplicate(self, name: SearchFieldType) -> ResultType: if len(values := self.name_to_ids.get(name, [])) == 1: if values[0].get("id"): return ResultType.FOUND_ID @@ -101,15 +102,15 @@ def check_duplicate(self, name: Union[str, Tuple[str, ...]]) -> ResultType: return ResultType.FOUND_MORE_IDS raise ActionException("Logical Error in Lookup Class") - def get_x_by_name( - self, name: Union[str, Tuple[str, ...]], x: str + def get_field_by_name( + self, name: SearchFieldType, fieldname: str ) -> Optional[Union[int, str]]: - """Gets fieldname 'x' from value of name_to_ids-dict""" + """Gets 'fieldname' from value of name_to_ids-dict""" if len(self.name_to_ids.get(name, [])) == 1: - return self.name_to_ids[name][0].get(x) + return self.name_to_ids[name][0].get(fieldname) return None - def get_name_by_id(self, id_: int) -> Optional[List[Union[str, Tuple[str, ...]]]]: + def get_name_by_id(self, id_: int) -> Optional[List[SearchFieldType]]: if name := self.id_to_name.get(id_): return name return None From 982d165b32ea447946acbd0ca0c6c3bb1244638b Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Wed, 6 Sep 2023 15:53:19 +0200 Subject: [PATCH 15/17] add validate_instance --- openslides_backend/action/actions/user/user_mixin.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/openslides_backend/action/actions/user/user_mixin.py b/openslides_backend/action/actions/user/user_mixin.py index f59a71e2f..938245dfc 100644 --- a/openslides_backend/action/actions/user/user_mixin.py +++ b/openslides_backend/action/actions/user/user_mixin.py @@ -88,6 +88,15 @@ class UserMixin(CheckForArchivedMeetingMixin): "group_ids": id_list_schema, } + def validate_instance(self, instance: Dict[str, Any]) -> None: + super().validate_instance(instance) + if "meeting_id" not in instance and any( + key in self.transfer_field_list for key in instance.keys() + ): + raise ActionException( + "Missing meeting_id in instance, because meeting related fields used" + ) + @original_instances def get_updated_instances(self, action_data: ActionData) -> ActionData: for instance in action_data: From b50923384b2ff0641ee42c818d2bbf6b7fca284e Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Thu, 7 Sep 2023 18:30:56 +0200 Subject: [PATCH 16/17] add 3 fields default_number, default_structure_level and default_vote_weight --- .../action/actions/user/account_json_upload.py | 6 ++++++ openslides_backend/action/mixins/import_mixins.py | 2 +- tests/system/action/user/test_account_json_upload.py | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/openslides_backend/action/actions/user/account_json_upload.py b/openslides_backend/action/actions/user/account_json_upload.py index 6dc02dcc0..be43af621 100644 --- a/openslides_backend/action/actions/user/account_json_upload.py +++ b/openslides_backend/action/actions/user/account_json_upload.py @@ -41,6 +41,9 @@ class AccountJsonUpload(JsonUploadMixin, UsernameMixin): "username", "gender", "pronoun", + "default_number", + "default_structure_level", + "default_vote_weight", "saml_id", ), }, @@ -63,6 +66,9 @@ class AccountJsonUpload(JsonUploadMixin, UsernameMixin): {"property": "username", "type": "string", "is_object": True}, {"property": "gender", "type": "string"}, {"property": "pronoun", "type": "string"}, + {"property": "default_number", "type": "string"}, + {"property": "default_structure_level", "type": "string"}, + {"property": "default_vote_weight", "type": "decimal"}, {"property": "saml_id", "type": "string", "is_object": True}, ] permission = OrganizationManagementLevel.CAN_MANAGE_USERS diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 846169852..ad412fd21 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -304,7 +304,7 @@ def validate_instance(self, instance: Dict[str, Any]) -> None: ] except Exception: pass - elif type_ == "string": + elif type_ in ("string", "decimal"): continue elif type_ == "integer": try: diff --git a/tests/system/action/user/test_account_json_upload.py b/tests/system/action/user/test_account_json_upload.py index 1b8ef410d..a94a5587a 100644 --- a/tests/system/action/user/test_account_json_upload.py +++ b/tests/system/action/user/test_account_json_upload.py @@ -18,6 +18,9 @@ def test_json_upload_simple(self) -> int: "default_password": "secret", "is_active": "1", "is_physical_person": "F", + "default_number": "strange number", + "default_structure_level": "CEO", + "default_vote_weight": "1.000000", "wrong": 15, } ], @@ -33,6 +36,9 @@ def test_json_upload_simple(self) -> int: "default_password": {"value": "secret", "info": ImportState.DONE}, "is_active": True, "is_physical_person": False, + "default_number": "strange number", + "default_structure_level": "CEO", + "default_vote_weight": "1.000000", }, } action_worker_id = response.json["results"][0][0].get("id") @@ -114,6 +120,9 @@ def test_json_upload_results(self) -> None: {"property": "username", "type": "string", "is_object": True}, {"property": "gender", "type": "string"}, {"property": "pronoun", "type": "string"}, + {"property": "default_number", "type": "string"}, + {"property": "default_structure_level", "type": "string"}, + {"property": "default_vote_weight", "type": "decimal"}, {"property": "saml_id", "type": "string", "is_object": True}, ], "rows": [ From 845b3473418dd5130c562bfa82fe70f2375e6bb8 Mon Sep 17 00:00:00 2001 From: Ralf Peschke Date: Tue, 12 Sep 2023 16:46:10 +0200 Subject: [PATCH 17/17] fix review remark list(ImportState) --- openslides_backend/action/mixins/import_mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index ad412fd21..69add1088 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -166,7 +166,7 @@ def update_instance(self, instance: Dict[str, Any]) -> Dict[str, Any]: raise ActionException( f"Wrong id doesn't point on {self.import_name} import data." ) - if worker.get("state") not in ImportState.__members__.values(): + if worker.get("state") not in list(ImportState): raise ActionException( "Error in import: Missing valid state in stored worker." )