From 71afa54504edae3e2538bd9644d754a9496584a7 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Fri, 23 Jan 2026 19:23:18 +0300 Subject: [PATCH 01/13] add: sync samAccName and user`s/computer`s principal task_1186 --- app/entities.py | 3 - .../scripts/principal_block_user_sync.py | 2 +- app/extra/scripts/uac_sync.py | 2 +- app/ldap_protocol/auth/auth_manager.py | 2 +- app/ldap_protocol/ldap_requests/add.py | 31 ++-- app/ldap_protocol/ldap_requests/bind.py | 2 +- app/ldap_protocol/ldap_requests/delete.py | 2 +- app/ldap_protocol/ldap_requests/extended.py | 2 +- app/ldap_protocol/ldap_requests/modify.py | 135 ++++++++++++++++-- interface | 2 +- tests/test_api/test_main/conftest.py | 24 ++++ .../test_main/test_router/test_add.py | 36 +++++ .../test_main/test_router/test_modify.py | 92 +++++++++++- 13 files changed, 296 insertions(+), 39 deletions(-) diff --git a/app/entities.py b/app/entities.py index 535da02f6..9ee945fe6 100644 --- a/app/entities.py +++ b/app/entities.py @@ -372,9 +372,6 @@ class User: "homedirectory": "homeDirectory", } - def get_upn_prefix(self) -> str: - return self.user_principal_name.split("@")[0] - def is_expired(self) -> bool: if self.account_exp is None: return False diff --git a/app/extra/scripts/principal_block_user_sync.py b/app/extra/scripts/principal_block_user_sync.py index 858a65a7d..d4c483728 100644 --- a/app/extra/scripts/principal_block_user_sync.py +++ b/app/extra/scripts/principal_block_user_sync.py @@ -32,7 +32,7 @@ async def principal_block_sync( if "@" in user.user_principal_name: principal_postfix = user.user_principal_name.split("@")[1].upper() - principal_name = f"{user.get_upn_prefix()}@{principal_postfix}" + principal_name = f"{user.sam_account_name}@{principal_postfix}" else: continue diff --git a/app/extra/scripts/uac_sync.py b/app/extra/scripts/uac_sync.py index f0623e1b1..8f9ce4f46 100644 --- a/app/extra/scripts/uac_sync.py +++ b/app/extra/scripts/uac_sync.py @@ -71,7 +71,7 @@ async def disable_accounts( ) # fmt: skip async for user in users: - await kadmin.lock_principal(user.get_upn_prefix()) + await kadmin.lock_principal(user.sam_account_name) await add_lock_and_expire_attributes( session, diff --git a/app/ldap_protocol/auth/auth_manager.py b/app/ldap_protocol/auth/auth_manager.py index 61c336f56..d2e9073fd 100644 --- a/app/ldap_protocol/auth/auth_manager.py +++ b/app/ldap_protocol/auth/auth_manager.py @@ -232,7 +232,7 @@ async def _update_password( if include_krb: await self._kadmin.create_or_update_principal_pw( - user.get_upn_prefix(), + user.sam_account_name, new_password, ) diff --git a/app/ldap_protocol/ldap_requests/add.py b/app/ldap_protocol/ldap_requests/add.py index 75be3f6fc..a16c6b182 100644 --- a/app/ldap_protocol/ldap_requests/add.py +++ b/app/ldap_protocol/ldap_requests/add.py @@ -249,11 +249,7 @@ async def handle( # noqa: C901 # in the attributes if ( attr_name in Directory.ro_fields - or attr_name - in ( - "userpassword", - "unicodepwd", - ) + or attr_name in ("userpassword", "unicodepwd") or attr_name == new_dir.rdname ): continue @@ -342,11 +338,11 @@ async def handle( # noqa: C901 ), ) - for uattr, value in { - "loginShell": "/bin/bash", - "uidNumber": str(create_integer_hash(user.sam_account_name)), - "homeDirectory": f"/home/{user.sam_account_name}", - }.items(): + for uattr, value in ( + ("loginShell", "/bin/bash"), + ("uidNumber", str(create_integer_hash(user.sam_account_name))), + ("homeDirectory", f"/home/{user.sam_account_name}"), + ): if uattr in user_attributes: value = user_attributes[uattr] del user_attributes[uattr] @@ -421,6 +417,15 @@ async def handle( # noqa: C901 ), ) + if is_computer: + attributes.append( + Attribute( + name="sAMAccountName", + value=f"{new_dir.name}", + directory_id=new_dir.id, + ), + ) + if not ctx.attribute_value_validator.is_directory_attributes_valid( entity_type.name if entity_type else "", attributes, @@ -461,16 +466,14 @@ async def handle( # noqa: C901 KRBAPIDeletePrincipalError, KRBAPIPrincipalNotFoundError, ): - await ctx.kadmin.del_principal( - user.get_upn_prefix(), - ) + await ctx.kadmin.del_principal(user.sam_account_name) pw = ( self.password.get_secret_value() if self.password else None ) - await ctx.kadmin.add_principal(user.get_upn_prefix(), pw) + await ctx.kadmin.add_principal(user.sam_account_name, pw) elif is_computer: await ctx.kadmin.add_principal( diff --git a/app/ldap_protocol/ldap_requests/bind.py b/app/ldap_protocol/ldap_requests/bind.py index 445b2f25c..ad764649e 100644 --- a/app/ldap_protocol/ldap_requests/bind.py +++ b/app/ldap_protocol/ldap_requests/bind.py @@ -209,7 +209,7 @@ async def handle( KRBAPIConnectionError, ): await ctx.kadmin.add_principal( - user.get_upn_prefix(), + user.sam_account_name, self.authentication_choice.password.get_secret_value(), 0.1, ) diff --git a/app/ldap_protocol/ldap_requests/delete.py b/app/ldap_protocol/ldap_requests/delete.py index e2b127331..3bf89343b 100644 --- a/app/ldap_protocol/ldap_requests/delete.py +++ b/app/ldap_protocol/ldap_requests/delete.py @@ -154,7 +154,7 @@ async def handle( # noqa: C901 await ctx.session_storage.clear_user_sessions( directory.user.id, ) - await ctx.kadmin.del_principal(directory.user.get_upn_prefix()) + await ctx.kadmin.del_principal(directory.user.sam_account_name) if await is_computer(directory.id, ctx.session): await ctx.kadmin.del_principal(directory.host_principal) diff --git a/app/ldap_protocol/ldap_requests/extended.py b/app/ldap_protocol/ldap_requests/extended.py index c3967889e..1f8cca946 100644 --- a/app/ldap_protocol/ldap_requests/extended.py +++ b/app/ldap_protocol/ldap_requests/extended.py @@ -248,7 +248,7 @@ async def handle( ): try: await ctx.kadmin.create_or_update_principal_pw( - user.get_upn_prefix(), + user.sam_account_name, new_password, ) except ( diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index 676550e3e..83c4749a3 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -8,7 +8,7 @@ from typing import AsyncGenerator, ClassVar from loguru import logger -from sqlalchemy import Select, and_, delete, or_, select, update +from sqlalchemy import Select, and_, delete, func, or_, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import joinedload, selectinload @@ -37,11 +37,16 @@ from ldap_protocol.policies.password import PasswordPolicyUseCases from ldap_protocol.session_storage import SessionStorage from ldap_protocol.utils.cte import check_root_group_membership_intersection -from ldap_protocol.utils.helpers import ft_to_dt, validate_entry +from ldap_protocol.utils.helpers import ( + ft_to_dt, + is_dn_in_base_directory, + validate_entry, +) from ldap_protocol.utils.queries import ( add_lock_and_expire_attributes, clear_group_membership, extend_group_membership, + get_base_directories, get_directories, get_directory_by_rid, get_filter_from_path, @@ -99,6 +104,7 @@ class ModifyRequest(BaseRequest): object: str changes: list[Changes] + _old_vals: dict[str, str | None] = {} @classmethod def from_data(cls, data: list[ASN1Row]) -> "ModifyRequest": @@ -222,7 +228,7 @@ async def handle( await ctx.session.rollback() yield ModifyResponse( result_code=LDAPCodes.UNDEFINED_ATTRIBUTE_TYPE, - message="Invalid attribute value(s)", + errorMessage="Invalid attribute value(s)", ) return @@ -612,6 +618,18 @@ async def _validate_object_class_modification( if is_object_class_in_replaced or is_object_class_in_deleted: raise ModifyForbiddenError("ObjectClass can't be deleted.") + def _need_to_cache_old_value( + self, + change: Changes, + directory: Directory, + ) -> bool: + return bool( + directory.entity_type + and directory.entity_type.name == EntityTypeNames.COMPUTER + and change.get_name() == "samaccountname" + and not self._old_vals.get(change.get_name()), + ) + async def _delete( self, change: Changes, @@ -656,11 +674,26 @@ async def _delete( attrs.append( and_( - qa(Attribute.name) == change.modification.type, + func.lower(qa(Attribute.name)) + == change.modification.type.lower(), condition, ), ) + if self._need_to_cache_old_value(change, directory): + self._old_vals[change.get_name()] = ( + ( + await session.execute( + select(Attribute).filter_by( + directory=directory, + name=change.modification.type, + ), + ) + ) + .scalar_one() + .value + ) + if attrs: del_query = ( delete(Attribute) @@ -800,7 +833,7 @@ async def _add( # noqa: C901 attrs = [] name = change.get_name() - if name in {"memberof", "member", "primarygroupid"}: + if name in ("memberof", "member", "primarygroupid"): await self._add_group_attrs(change, directory, session) return @@ -812,9 +845,7 @@ async def _add( # noqa: C901 continue elif ( - bool( - uac_val & UserAccountControlFlag.ACCOUNTDISABLE, - ) + bool(uac_val & UserAccountControlFlag.ACCOUNTDISABLE) and directory.user ): if directory.path_dn == current_user.dn: @@ -823,7 +854,7 @@ async def _add( # noqa: C901 ) await kadmin.lock_principal( - directory.user.get_upn_prefix(), + directory.user.sam_account_name, ) await add_lock_and_expire_attributes( @@ -837,9 +868,7 @@ async def _add( # noqa: C901 ) elif ( - not bool( - uac_val & UserAccountControlFlag.ACCOUNTDISABLE, - ) + not bool(uac_val & UserAccountControlFlag.ACCOUNTDISABLE) and directory.user ): await unlock_principal( @@ -860,7 +889,7 @@ async def _add( # noqa: C901 if name == "pwdlastset" and value == "0" and directory.user: await kadmin.force_princ_pw_change( - directory.user.get_upn_prefix(), + directory.user.sam_account_name, ) if name == directory.rdname: @@ -883,11 +912,89 @@ async def _add( # noqa: C901 else: new_value = value # type: ignore + base_dir = None + for base_directory in await get_base_directories( + session, + ): + if is_dn_in_base_directory( + base_directory, + directory.path_dn, + ): + base_dir = base_directory + break + else: + raise ModifyForbiddenError( + "Base directory for computer not found.", + ) + + if ( + name == "userprincipalname" + and directory.entity_type + and directory.entity_type.name == EntityTypeNames.USER + and directory.user + ): + new_samaccountname = str(new_value).split("@")[0] + + await kadmin.rename_princ( + directory.user.sam_account_name, + str(new_samaccountname), + ) + + await session.execute( + update(User) + .filter_by(directory=directory) + .values(samaccountname=new_samaccountname), + ) + + if ( + name == "samaccountname" and directory.entity_type + ): # TODO это поле может быть и у компа, но находится в блоке с юзером + if directory.entity_type.name == EntityTypeNames.COMPUTER: + samaccountname_old_val = self._old_vals.get( + change.get_name(), + ) + + await kadmin.rename_princ( + f"host/{samaccountname_old_val}", + f"host/{str(new_value)}", + ) + await kadmin.rename_princ( + f"host/{samaccountname_old_val}.{base_dir.name}", + f"host/{str(new_value)}.{base_dir.name}", + ) + attrs.append( + Attribute( + name=change.modification.type, + value=new_value if isinstance(new_value, str) else None, # noqa: E501 + bvalue=new_value if isinstance(new_value, bytes) else None, # noqa: E501 + directory_id=directory.id, + ), + ) # fmt: skip + + elif ( + directory.entity_type.name == EntityTypeNames.USER + and directory.user + ): + await kadmin.rename_princ( + directory.user.sam_account_name, + str(new_value), + ) + + new_userprincipalname = ( + f"{str(new_value)}@{base_dir.name}" + ) + await session.execute( + update(User) + .filter_by(directory=directory) + .values(userprincipalname=new_userprincipalname), + ) + await session.execute( update(User) .filter_by(directory=directory) .values({name: new_value}), ) + elif name in ("userpassword", "unicodepwd") and directory.user: if not settings.USE_CORE_TLS: raise PermissionError("TLS required") @@ -918,7 +1025,7 @@ async def _add( # noqa: C901 directory.user, ) await kadmin.create_or_update_principal_pw( - directory.user.get_upn_prefix(), + directory.user.sam_account_name, value, ) diff --git a/interface b/interface index f31962020..111da1662 160000 --- a/interface +++ b/interface @@ -1 +1 @@ -Subproject commit f31962020a6689e6a4c61fb3349db5b5c7895f92 +Subproject commit 111da16625188ec6090c2811b773a8b62858b5f9 diff --git a/tests/test_api/test_main/conftest.py b/tests/test_api/test_main/conftest.py index 8f1b58dea..5ad39a9c2 100644 --- a/tests/test_api/test_main/conftest.py +++ b/tests/test_api/test_main/conftest.py @@ -138,6 +138,30 @@ async def adding_test_user( assert auth.cookies.get("id") +@pytest_asyncio.fixture(scope="function") +async def adding_test_computer( + http_client: AsyncClient, +) -> None: + """Test api correct (name) add.""" + response = await http_client.post( + "/entry/add", + json={ + "entry": "cn=mycomputer,dc=md,dc=test", + "password": None, + "attributes": [ + {"type": "name", "vals": ["mycomputer name"]}, + {"type": "cn", "vals": ["mycomputer"]}, + {"type": "objectClass", "vals": ["computer", "top"]}, + ], + }, + ) + + data = response.json() + + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.SUCCESS + + @pytest_asyncio.fixture(scope="function") async def add_dns_settings( session: AsyncSession, diff --git a/tests/test_api/test_main/test_router/test_add.py b/tests/test_api/test_main/test_router/test_add.py index 3050bedec..bb2f38e6c 100644 --- a/tests/test_api/test_main/test_router/test_add.py +++ b/tests/test_api/test_main/test_router/test_add.py @@ -71,6 +71,35 @@ async def test_api_add_incorrect_computer_name( assert data.get("resultCode") == LDAPCodes.UNDEFINED_ATTRIBUTE_TYPE +@pytest.mark.asyncio +@pytest.mark.usefixtures("session") +async def test_api_add_correct_computer( + http_client: AsyncClient, +) -> None: + """Test api correct (name) add.""" + response = await http_client.post( + "/entry/add", + json={ + "entry": "cn=mycomputer,dc=md,dc=test", + "password": None, + "attributes": [ + {"type": "name", "vals": ["mycomputer name"]}, + {"type": "cn", "vals": ["mycomputer"]}, + {"type": "objectClass", "vals": ["computer", "top"]}, + { + "type": "memberOf", + "vals": ["cn=domain admins,cn=groups,dc=md,dc=test"], + }, + ], + }, + ) + + data = response.json() + + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.SUCCESS + + @pytest.mark.asyncio @pytest.mark.usefixtures("session") async def test_api_add_incorrect_user_samaccount_with_dot( @@ -171,6 +200,13 @@ async def test_api_add_computer(http_client: AsyncClient) -> None: else: raise Exception("Computer without userAccountControl") + for attr in data["search_result"][0]["partial_attributes"]: + if attr["type"] == "sAMAccountName": + assert attr["vals"][0] + break + else: + raise Exception("Computer without sAMAccountName") + @pytest.mark.asyncio @pytest.mark.usefixtures("session") diff --git a/tests/test_api/test_main/test_router/test_modify.py b/tests/test_api/test_main/test_router/test_modify.py index 3e46e879d..451340615 100644 --- a/tests/test_api/test_main/test_router/test_modify.py +++ b/tests/test_api/test_main/test_router/test_modify.py @@ -8,6 +8,7 @@ from httpx import AsyncClient from sqlalchemy.ext.asyncio import AsyncSession +from ldap_protocol.kerberos.base import AbstractKadmin from ldap_protocol.ldap_codes import LDAPCodes from ldap_protocol.ldap_requests.modify import Operation @@ -16,7 +17,10 @@ @pytest.mark.usefixtures("adding_test_user") @pytest.mark.usefixtures("setup_session") @pytest.mark.usefixtures("session") -async def test_api_correct_modify(http_client: AsyncClient) -> None: +async def test_api_correct_modify_user( + http_client: AsyncClient, + kadmin: AbstractKadmin, +) -> None: """Test API for modify object attribute.""" entry_dn = "cn=test,dc=md,dc=test" new_value = "133632677730000000" @@ -32,6 +36,13 @@ async def test_api_correct_modify(http_client: AsyncClient) -> None: "vals": [new_value], }, }, + { + "operation": Operation.REPLACE, + "modification": { + "type": "sAMAccountName", + "vals": ["NEW user name"], + }, + }, ], }, ) @@ -40,6 +51,7 @@ async def test_api_correct_modify(http_client: AsyncClient) -> None: assert isinstance(data, dict) assert data.get("resultCode") == LDAPCodes.SUCCESS + assert kadmin.rename_princ.call_args.args == ("new_user", "NEW user name") # type: ignore response = await http_client.post( "entry/search", @@ -64,6 +76,84 @@ async def test_api_correct_modify(http_client: AsyncClient) -> None: for attr in data["search_result"][0]["partial_attributes"]: if attr["type"] == "accountExpires": assert attr["vals"][0] == new_value + break + else: + raise Exception("User without accountExpires") + + for attr in data["search_result"][0]["partial_attributes"]: + if attr["type"] == "sAMAccountName": + assert attr["vals"][0] == "NEW user name" + break + else: + raise Exception("User without sAMAccountName") + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("adding_test_computer") +@pytest.mark.usefixtures("setup_session") +@pytest.mark.usefixtures("session") +async def test_api_correct_modify_computer( + http_client: AsyncClient, + kadmin: AbstractKadmin, +) -> None: + """Test API for modify computer sAMAccountName.""" + entry_dn = "cn=mycomputer,dc=md,dc=test" + response = await http_client.patch( + "/entry/update", + json={ + "object": entry_dn, + "changes": [ + { + "operation": Operation.REPLACE, + "modification": { + "type": "sAMAccountName", + "vals": ["maincomputer"], + }, + }, + ], + }, + ) + + data = response.json() + + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.SUCCESS + assert kadmin.rename_princ.call_count == 2 # type: ignore + assert kadmin.rename_princ.call_args_list[0].args == ( # type: ignore + "host/mycomputer", + "host/maincomputer", + ) + assert kadmin.rename_princ.call_args_list[1].args == ( # type: ignore + "host/mycomputer.md.test", + "host/maincomputer.md.test", + ) + + response = await http_client.post( + "entry/search", + json={ + "base_object": entry_dn, + "scope": 0, + "deref_aliases": 0, + "size_limit": 1000, + "time_limit": 10, + "types_only": True, + "filter": "(objectClass=*)", + "attributes": [], + "page_number": 1, + }, + ) + + data = response.json() + + assert data["resultCode"] == LDAPCodes.SUCCESS + assert data["search_result"][0]["object_name"] == entry_dn + + for attr in data["search_result"][0]["partial_attributes"]: + if attr["type"] == "sAMAccountName": + assert attr["vals"][0] == "maincomputer" + break + else: + raise Exception("Computer without sAMAccountName") @pytest.mark.asyncio From 3d60579367217139c7fcc9bf4a23be043a4da128 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Mon, 26 Jan 2026 12:49:13 +0300 Subject: [PATCH 02/13] refactor: modify: split user and computer b_logic task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 143 ++++++++++------------ 1 file changed, 68 insertions(+), 75 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index 83c4749a3..f3d3b7353 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -681,18 +681,14 @@ async def _delete( ) if self._need_to_cache_old_value(change, directory): - self._old_vals[change.get_name()] = ( - ( - await session.execute( - select(Attribute).filter_by( - directory=directory, - name=change.modification.type, - ), - ) - ) - .scalar_one() - .value - ) + result = await session.execute( + select(Attribute) + .filter_by( + directory=directory, + name=change.modification.type, + ), + ) # fmt: skip + self._old_vals[change.get_name()] = result.scalar_one().value if attrs: del_query = ( @@ -837,6 +833,21 @@ async def _add( # noqa: C901 await self._add_group_attrs(change, directory, session) return + base_dir = None + for base_directory in await get_base_directories( + session, + ): + if is_dn_in_base_directory( + base_directory, + directory.path_dn, + ): + base_dir = base_directory + break + else: + raise ModifyForbiddenError( + "Base directory for computer not found.", + ) + for value in change.modification.vals: if name == "useraccountcontrol": uac_val = int(value) @@ -906,38 +917,23 @@ async def _add( # noqa: C901 .values({name: value}), ) - elif name in User.search_fields: + elif ( + name in User.search_fields + and directory.entity_type + and directory.entity_type.name == EntityTypeNames.USER + and directory.user + ): if name == "accountexpires": new_value = ft_to_dt(int(value)) if value != "0" else None else: new_value = value # type: ignore - base_dir = None - for base_directory in await get_base_directories( - session, - ): - if is_dn_in_base_directory( - base_directory, - directory.path_dn, - ): - base_dir = base_directory - break - else: - raise ModifyForbiddenError( - "Base directory for computer not found.", - ) - - if ( - name == "userprincipalname" - and directory.entity_type - and directory.entity_type.name == EntityTypeNames.USER - and directory.user - ): + if name == "userprincipalname": new_samaccountname = str(new_value).split("@")[0] await kadmin.rename_princ( directory.user.sam_account_name, - str(new_samaccountname), + new_samaccountname, ) await session.execute( @@ -946,48 +942,19 @@ async def _add( # noqa: C901 .values(samaccountname=new_samaccountname), ) - if ( - name == "samaccountname" and directory.entity_type - ): # TODO это поле может быть и у компа, но находится в блоке с юзером - if directory.entity_type.name == EntityTypeNames.COMPUTER: - samaccountname_old_val = self._old_vals.get( - change.get_name(), - ) + elif name == "samaccountname": + new_userprincipalname = f"{str(new_value)}@{base_dir.name}" - await kadmin.rename_princ( - f"host/{samaccountname_old_val}", - f"host/{str(new_value)}", - ) - await kadmin.rename_princ( - f"host/{samaccountname_old_val}.{base_dir.name}", - f"host/{str(new_value)}.{base_dir.name}", - ) - attrs.append( - Attribute( - name=change.modification.type, - value=new_value if isinstance(new_value, str) else None, # noqa: E501 - bvalue=new_value if isinstance(new_value, bytes) else None, # noqa: E501 - directory_id=directory.id, - ), - ) # fmt: skip - - elif ( - directory.entity_type.name == EntityTypeNames.USER - and directory.user - ): - await kadmin.rename_princ( - directory.user.sam_account_name, - str(new_value), - ) + await kadmin.rename_princ( + directory.user.sam_account_name, + str(new_value), + ) - new_userprincipalname = ( - f"{str(new_value)}@{base_dir.name}" - ) - await session.execute( - update(User) - .filter_by(directory=directory) - .values(userprincipalname=new_userprincipalname), - ) + await session.execute( + update(User) + .filter_by(directory=directory) + .values(userprincipalname=new_userprincipalname), + ) await session.execute( update(User) @@ -995,6 +962,32 @@ async def _add( # noqa: C901 .values({name: new_value}), ) + elif ( + name == "samaccountname" + and directory.entity_type + and directory.entity_type.name == EntityTypeNames.COMPUTER + ): + samaccountname_old_val = self._old_vals.get( + change.get_name(), + ) + + await kadmin.rename_princ( + f"host/{samaccountname_old_val}", + f"host/{str(value)}", + ) + await kadmin.rename_princ( + f"host/{samaccountname_old_val}.{base_dir.name}", + f"host/{str(value)}.{base_dir.name}", + ) + attrs.append( + Attribute( + name=change.modification.type, + value=value if isinstance(value, str) else None, + bvalue=value if isinstance(value, bytes) else None, + directory_id=directory.id, + ), + ) # fmt: skip + elif name in ("userpassword", "unicodepwd") and directory.user: if not settings.USE_CORE_TLS: raise PermissionError("TLS required") From 8f76819233cde5baa169913a474aef1e80c9fc2a Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:16:45 +0300 Subject: [PATCH 03/13] refactor: pr comments task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 38 +++--- docker-compose.test.yml | 2 +- interface | 2 +- .../test_main/test_router/test_add.py | 29 ----- .../test_main/test_router/test_modify.py | 123 ++++++++++++++++-- 5 files changed, 130 insertions(+), 64 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index f3d3b7353..d03710979 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -8,7 +8,7 @@ from typing import AsyncGenerator, ClassVar from loguru import logger -from sqlalchemy import Select, and_, delete, func, or_, select, update +from sqlalchemy import Select, and_, delete, or_, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import joinedload, selectinload @@ -104,6 +104,10 @@ class ModifyRequest(BaseRequest): object: str changes: list[Changes] + + # NOTE: If the old value was changed (for example, in _delete) + # in one method, then you need to have access to the old value + # from other methods (for example, from _add) _old_vals: dict[str, str | None] = {} @classmethod @@ -228,7 +232,7 @@ async def handle( await ctx.session.rollback() yield ModifyResponse( result_code=LDAPCodes.UNDEFINED_ATTRIBUTE_TYPE, - errorMessage="Invalid attribute value(s)", + error_message="Invalid attribute value(s)", ) return @@ -618,7 +622,7 @@ async def _validate_object_class_modification( if is_object_class_in_replaced or is_object_class_in_deleted: raise ModifyForbiddenError("ObjectClass can't be deleted.") - def _need_to_cache_old_value( + def _need_to_cache_samaccountname_old_value( self, change: Changes, directory: Directory, @@ -626,8 +630,8 @@ def _need_to_cache_old_value( return bool( directory.entity_type and directory.entity_type.name == EntityTypeNames.COMPUTER - and change.get_name() == "samaccountname" - and not self._old_vals.get(change.get_name()), + and change.modification.type == "sAMAccountName" + and not self._old_vals.get(change.modification.type), ) async def _delete( @@ -674,21 +678,15 @@ async def _delete( attrs.append( and_( - func.lower(qa(Attribute.name)) - == change.modification.type.lower(), + qa(Attribute.name) == change.modification.type, condition, ), ) - if self._need_to_cache_old_value(change, directory): - result = await session.execute( - select(Attribute) - .filter_by( - directory=directory, - name=change.modification.type, - ), - ) # fmt: skip - self._old_vals[change.get_name()] = result.scalar_one().value + if self._need_to_cache_samaccountname_old_value(change, directory): + vals = directory.attributes_dict.get(change.modification.type) + if vals: + self._old_vals[change.modification.type] = vals[0] if attrs: del_query = ( @@ -834,9 +832,7 @@ async def _add( # noqa: C901 return base_dir = None - for base_directory in await get_base_directories( - session, - ): + for base_directory in await get_base_directories(session): if is_dn_in_base_directory( base_directory, directory.path_dn, @@ -967,9 +963,7 @@ async def _add( # noqa: C901 and directory.entity_type and directory.entity_type.name == EntityTypeNames.COMPUTER ): - samaccountname_old_val = self._old_vals.get( - change.get_name(), - ) + samaccountname_old_val = self._old_vals.get(change.modification.type) # noqa: E501 # fmt: skip await kadmin.rename_princ( f"host/{samaccountname_old_val}", diff --git a/docker-compose.test.yml b/docker-compose.test.yml index 96076b657..d25d515e3 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -20,7 +20,7 @@ services: POSTGRES_HOST: postgres # PYTHONTRACEMALLOC: 1 PYTHONDONTWRITEBYTECODE: 1 - command: sh -c "python -B -m pytest -n auto -x -W ignore::DeprecationWarning -W ignore::coverage.exceptions.CoverageWarning -vv" + command: sh -c "python -B -m pytest -n auto -x tests/test_api/test_main/test_router/test_modify.py -W ignore::DeprecationWarning -W ignore::coverage.exceptions.CoverageWarning -vv" tty: true postgres: diff --git a/interface b/interface index 111da1662..e1ca5656a 160000 --- a/interface +++ b/interface @@ -1 +1 @@ -Subproject commit 111da16625188ec6090c2811b773a8b62858b5f9 +Subproject commit e1ca5656aeabc20a1862aeaf11ded72feaa97403 diff --git a/tests/test_api/test_main/test_router/test_add.py b/tests/test_api/test_main/test_router/test_add.py index bb2f38e6c..7abe564bf 100644 --- a/tests/test_api/test_main/test_router/test_add.py +++ b/tests/test_api/test_main/test_router/test_add.py @@ -71,35 +71,6 @@ async def test_api_add_incorrect_computer_name( assert data.get("resultCode") == LDAPCodes.UNDEFINED_ATTRIBUTE_TYPE -@pytest.mark.asyncio -@pytest.mark.usefixtures("session") -async def test_api_add_correct_computer( - http_client: AsyncClient, -) -> None: - """Test api correct (name) add.""" - response = await http_client.post( - "/entry/add", - json={ - "entry": "cn=mycomputer,dc=md,dc=test", - "password": None, - "attributes": [ - {"type": "name", "vals": ["mycomputer name"]}, - {"type": "cn", "vals": ["mycomputer"]}, - {"type": "objectClass", "vals": ["computer", "top"]}, - { - "type": "memberOf", - "vals": ["cn=domain admins,cn=groups,dc=md,dc=test"], - }, - ], - }, - ) - - data = response.json() - - assert isinstance(data, dict) - assert data.get("resultCode") == LDAPCodes.SUCCESS - - @pytest.mark.asyncio @pytest.mark.usefixtures("session") async def test_api_add_incorrect_user_samaccount_with_dot( diff --git a/tests/test_api/test_main/test_router/test_modify.py b/tests/test_api/test_main/test_router/test_modify.py index 451340615..b8d959823 100644 --- a/tests/test_api/test_main/test_router/test_modify.py +++ b/tests/test_api/test_main/test_router/test_modify.py @@ -17,13 +17,13 @@ @pytest.mark.usefixtures("adding_test_user") @pytest.mark.usefixtures("setup_session") @pytest.mark.usefixtures("session") -async def test_api_correct_modify_user( +async def test_api_correct_modify_user_accountexpires( http_client: AsyncClient, - kadmin: AbstractKadmin, ) -> None: """Test API for modify object attribute.""" entry_dn = "cn=test,dc=md,dc=test" new_value = "133632677730000000" + response = await http_client.patch( "/entry/update", json={ @@ -36,6 +36,57 @@ async def test_api_correct_modify_user( "vals": [new_value], }, }, + ], + }, + ) + + data = response.json() + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.SUCCESS + + response = await http_client.post( + "entry/search", + json={ + "base_object": entry_dn, + "scope": 0, + "deref_aliases": 0, + "size_limit": 1000, + "time_limit": 10, + "types_only": True, + "filter": "(objectClass=*)", + "attributes": [], + "page_number": 1, + }, + ) + + data = response.json() + assert data["resultCode"] == LDAPCodes.SUCCESS + assert data["search_result"][0]["object_name"] == entry_dn + + for attr in data["search_result"][0]["partial_attributes"]: + if attr["type"] == "accountExpires": + assert attr["vals"][0] == new_value + break + else: + raise Exception("User without accountExpires") + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("adding_test_user") +@pytest.mark.usefixtures("setup_session") +@pytest.mark.usefixtures("session") +async def test_api_correct_modify_user_samaccountname( + http_client: AsyncClient, + kadmin: AbstractKadmin, +) -> None: + """Test API for modify object attribute.""" + entry_dn = "cn=test,dc=md,dc=test" + + response = await http_client.patch( + "/entry/update", + json={ + "object": entry_dn, + "changes": [ { "operation": Operation.REPLACE, "modification": { @@ -48,7 +99,6 @@ async def test_api_correct_modify_user( ) data = response.json() - assert isinstance(data, dict) assert data.get("resultCode") == LDAPCodes.SUCCESS assert kadmin.rename_princ.call_args.args == ("new_user", "NEW user name") # type: ignore @@ -69,30 +119,81 @@ async def test_api_correct_modify_user( ) data = response.json() - assert data["resultCode"] == LDAPCodes.SUCCESS assert data["search_result"][0]["object_name"] == entry_dn for attr in data["search_result"][0]["partial_attributes"]: - if attr["type"] == "accountExpires": - assert attr["vals"][0] == new_value + if attr["type"] == "sAMAccountName": + assert attr["vals"][0] == "NEW user name" break else: - raise Exception("User without accountExpires") + raise Exception("User without sAMAccountName") + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("adding_test_user") +@pytest.mark.usefixtures("setup_session") +@pytest.mark.usefixtures("session") +async def test_api_correct_modify_user_userprincipalname( + http_client: AsyncClient, + kadmin: AbstractKadmin, +) -> None: + """Test API for modify object attribute.""" + entry_dn = "cn=test,dc=md,dc=test" + + response = await http_client.patch( + "/entry/update", + json={ + "object": entry_dn, + "changes": [ + { + "operation": Operation.REPLACE, + "modification": { + "type": "userPrincipalName", + "vals": ["newbiguser@md.test"], + }, + }, + ], + }, + ) + + data = response.json() + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.SUCCESS + assert kadmin.rename_princ.call_args.args == ("new_user", "newbiguser") # type: ignore + + response = await http_client.post( + "entry/search", + json={ + "base_object": entry_dn, + "scope": 0, + "deref_aliases": 0, + "size_limit": 1000, + "time_limit": 10, + "types_only": True, + "filter": "(objectClass=*)", + "attributes": [], + "page_number": 1, + }, + ) + + data = response.json() + assert data["resultCode"] == LDAPCodes.SUCCESS + assert data["search_result"][0]["object_name"] == entry_dn for attr in data["search_result"][0]["partial_attributes"]: - if attr["type"] == "sAMAccountName": - assert attr["vals"][0] == "NEW user name" + if attr["type"] == "userPrincipalName": + assert attr["vals"][0] == "newbiguser@md.test" break else: - raise Exception("User without sAMAccountName") + raise Exception("User without userPrincipalName") @pytest.mark.asyncio @pytest.mark.usefixtures("adding_test_computer") @pytest.mark.usefixtures("setup_session") @pytest.mark.usefixtures("session") -async def test_api_correct_modify_computer( +async def test_api_correct_modify_computer_samaccountname( http_client: AsyncClient, kadmin: AbstractKadmin, ) -> None: From 7ab025fd812e73370759c40f0aca62a47d541eb3 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:18:45 +0300 Subject: [PATCH 04/13] fix: ModifyRequest: get base dir task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 34 +++++++++++++++-------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index d03710979..c28cd9a0b 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -831,18 +831,7 @@ async def _add( # noqa: C901 await self._add_group_attrs(change, directory, session) return - base_dir = None - for base_directory in await get_base_directories(session): - if is_dn_in_base_directory( - base_directory, - directory.path_dn, - ): - base_dir = base_directory - break - else: - raise ModifyForbiddenError( - "Base directory for computer not found.", - ) + base_dir = await self._new_method(directory, session) for value in change.modification.vals: if name == "useraccountcontrol": @@ -1029,3 +1018,24 @@ async def _add( # noqa: C901 ) session.add_all(attrs) + + async def _new_method( + self, + directory: Directory, + session: AsyncSession, + ) -> Directory: + base_dir = None + + for base_directory in await get_base_directories(session): + if is_dn_in_base_directory( + base_directory, + directory.path_dn, + ): + base_dir = base_directory + break + else: + raise ModifyForbiddenError( + "Base directory for computer not found.", + ) + + return base_dir From 9a320049478f927188f2dd3e3d8c44e6d869ff7f Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:19:12 +0300 Subject: [PATCH 05/13] tests: fix --- docker-compose.test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.test.yml b/docker-compose.test.yml index d25d515e3..96076b657 100644 --- a/docker-compose.test.yml +++ b/docker-compose.test.yml @@ -20,7 +20,7 @@ services: POSTGRES_HOST: postgres # PYTHONTRACEMALLOC: 1 PYTHONDONTWRITEBYTECODE: 1 - command: sh -c "python -B -m pytest -n auto -x tests/test_api/test_main/test_router/test_modify.py -W ignore::DeprecationWarning -W ignore::coverage.exceptions.CoverageWarning -vv" + command: sh -c "python -B -m pytest -n auto -x -W ignore::DeprecationWarning -W ignore::coverage.exceptions.CoverageWarning -vv" tty: true postgres: From 53025b5c628c440795e62c00e5c1e765781dad99 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:32:34 +0300 Subject: [PATCH 06/13] add: ModifyRequest: KRBAPIRenamePrincipalError into exc pack task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index c28cd9a0b..51cd6225a 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -25,6 +25,7 @@ KRBAPIForcePasswordChangeError, KRBAPILockPrincipalError, KRBAPIPrincipalNotFoundError, + KRBAPIRenamePrincipalError, ) from ldap_protocol.ldap_codes import LDAPCodes from ldap_protocol.ldap_responses import ModifyResponse, PartialAttribute @@ -76,6 +77,7 @@ class ModifyForbiddenError(Exception): PermissionError, ModifyForbiddenError, KRBAPIPrincipalNotFoundError, + KRBAPIRenamePrincipalError, KRBAPILockPrincipalError, KRBAPIForcePasswordChangeError, ) From dcc6af39148fa065ba80bf8e32720babd9ceab56 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:44:16 +0300 Subject: [PATCH 07/13] fix: attributes delete query condition task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index 51cd6225a..5754ab46b 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -8,7 +8,7 @@ from typing import AsyncGenerator, ClassVar from loguru import logger -from sqlalchemy import Select, and_, delete, or_, select, update +from sqlalchemy import Select, and_, delete, func, or_, select, update from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import joinedload, selectinload @@ -680,10 +680,10 @@ async def _delete( attrs.append( and_( - qa(Attribute.name) == change.modification.type, + func.lower(qa(Attribute.name)) == change.modification.type.lower(), # noqa: E501 condition, ), - ) + ) # fmt: skip if self._need_to_cache_samaccountname_old_value(change, directory): vals = directory.attributes_dict.get(change.modification.type) From 07bdcad554c7448273019c8251521083d8a33401 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:49:49 +0300 Subject: [PATCH 08/13] fix: rename task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index 5754ab46b..f3bb31a39 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -833,7 +833,7 @@ async def _add( # noqa: C901 await self._add_group_attrs(change, directory, session) return - base_dir = await self._new_method(directory, session) + base_dir = await self._get_base_dir(directory, session) for value in change.modification.vals: if name == "useraccountcontrol": @@ -1021,7 +1021,7 @@ async def _add( # noqa: C901 session.add_all(attrs) - async def _new_method( + async def _get_base_dir( self, directory: Directory, session: AsyncSession, From 183dc81b991c4e6675bb300df55bf51da977daae Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 12:53:49 +0300 Subject: [PATCH 09/13] tests: improved task_1186 --- tests/test_api/test_main/test_router/test_add.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api/test_main/test_router/test_add.py b/tests/test_api/test_main/test_router/test_add.py index 7abe564bf..323775256 100644 --- a/tests/test_api/test_main/test_router/test_add.py +++ b/tests/test_api/test_main/test_router/test_add.py @@ -173,7 +173,7 @@ async def test_api_add_computer(http_client: AsyncClient) -> None: for attr in data["search_result"][0]["partial_attributes"]: if attr["type"] == "sAMAccountName": - assert attr["vals"][0] + assert attr["vals"][0] == "PC" break else: raise Exception("Computer without sAMAccountName") From 8b4492c2a49a21de3bf119abdde127b36e8ec79a Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 13:02:08 +0300 Subject: [PATCH 10/13] refactor: method `get_name()` to property `l_name` task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 58 +++++++++++++---------- app/ldap_protocol/objects.py | 5 +- app/ldap_protocol/roles/access_manager.py | 9 ++-- 3 files changed, 39 insertions(+), 33 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index f3bb31a39..f391ada97 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -196,7 +196,7 @@ async def handle( entity_type_id=directory.entity_type_id, ) - names = {change.get_name() for change in self.changes} + names = {change.l_type for change in self.changes} password_change_requested = self._is_password_change_requested(names) self_modify = directory.id == ctx.ldap_session.user.directory_id @@ -224,7 +224,7 @@ async def handle( return for change in self.changes: - if change.modification.type.lower() in Directory.ro_fields: + if change.l_type in Directory.ro_fields: continue if not ctx.attribute_value_validator.is_partial_attribute_valid( # noqa: E501 @@ -645,9 +645,8 @@ async def _delete( name_only: bool = False, ) -> None: attrs = [] - name = change.modification.type.lower() - if name == "memberof": + if change.l_type == "memberof": await self._delete_memberof( change=change, directory=directory, @@ -656,7 +655,7 @@ async def _delete( ) return - if name == "member": + if change.l_type == "member": await self._delete_member( change=change, directory=directory, @@ -665,14 +664,16 @@ async def _delete( ) return - if name == "objectclass": + if change.l_type == "objectclass": await self._validate_object_class_modification(change, directory) if name_only or not change.modification.vals: attrs.append(qa(Attribute.name) == change.modification.type) else: for value in change.modification.vals: - if name not in (Directory.search_fields | User.search_fields): + if change.l_type not in ( + Directory.search_fields | User.search_fields + ): if isinstance(value, str): condition = qa(Attribute.value) == value elif isinstance(value, bytes): @@ -680,7 +681,7 @@ async def _delete( attrs.append( and_( - func.lower(qa(Attribute.name)) == change.modification.type.lower(), # noqa: E501 + func.lower(qa(Attribute.name)) == change.l_type, condition, ), ) # fmt: skip @@ -802,16 +803,15 @@ async def _add_group_attrs( directory: Directory, session: AsyncSession, ) -> None: - name = change.get_name() - if name == "primarygroupid": + if change.l_type == "primarygroupid": await self._add_primary_group_attribute( change, directory, session, ) - elif name == "memberof": + elif change.l_type == "memberof": await self._add_memberof(change, directory, session) - elif name == "member": + elif change.l_type == "member": await self._add_member(change, directory, session) async def _add( # noqa: C901 @@ -827,16 +827,15 @@ async def _add( # noqa: C901 password_utils: PasswordUtils, ) -> None: attrs = [] - name = change.get_name() - if name in ("memberof", "member", "primarygroupid"): + if change.l_type in ("memberof", "member", "primarygroupid"): await self._add_group_attrs(change, directory, session) return base_dir = await self._get_base_dir(directory, session) for value in change.modification.vals: - if name == "useraccountcontrol": + if change.l_type == "useraccountcontrol": uac_val = int(value) if not UserAccountControlFlag.is_value_valid(uac_val): @@ -885,37 +884,41 @@ async def _add( # noqa: C901 ), ) # fmt: skip - if name == "pwdlastset" and value == "0" and directory.user: + if ( + change.l_type == "pwdlastset" + and value == "0" + and directory.user + ): await kadmin.force_princ_pw_change( directory.user.sam_account_name, ) - if name == directory.rdname: + if change.l_type == directory.rdname: await session.execute( update(Directory) .filter(directory_table.c.id == directory.id) .values(name=value), ) - if name in Directory.search_fields: + if change.l_type in Directory.search_fields: await session.execute( update(Directory) .filter(directory_table.c.id == directory.id) - .values({name: value}), + .values({change.l_type: value}), ) elif ( - name in User.search_fields + change.l_type in User.search_fields and directory.entity_type and directory.entity_type.name == EntityTypeNames.USER and directory.user ): - if name == "accountexpires": + if change.l_type == "accountexpires": new_value = ft_to_dt(int(value)) if value != "0" else None else: new_value = value # type: ignore - if name == "userprincipalname": + if change.l_type == "userprincipalname": new_samaccountname = str(new_value).split("@")[0] await kadmin.rename_princ( @@ -929,7 +932,7 @@ async def _add( # noqa: C901 .values(samaccountname=new_samaccountname), ) - elif name == "samaccountname": + elif change.l_type == "samaccountname": new_userprincipalname = f"{str(new_value)}@{base_dir.name}" await kadmin.rename_princ( @@ -946,11 +949,11 @@ async def _add( # noqa: C901 await session.execute( update(User) .filter_by(directory=directory) - .values({name: new_value}), + .values({change.l_type: new_value}), ) elif ( - name == "samaccountname" + change.l_type == "samaccountname" and directory.entity_type and directory.entity_type.name == EntityTypeNames.COMPUTER ): @@ -973,7 +976,10 @@ async def _add( # noqa: C901 ), ) # fmt: skip - elif name in ("userpassword", "unicodepwd") and directory.user: + elif ( + change.l_type in ("userpassword", "unicodepwd") + and directory.user + ): if not settings.USE_CORE_TLS: raise PermissionError("TLS required") diff --git a/app/ldap_protocol/objects.py b/app/ldap_protocol/objects.py index 75effb3f0..1114aa7c8 100644 --- a/app/ldap_protocol/objects.py +++ b/app/ldap_protocol/objects.py @@ -82,8 +82,9 @@ class Changes(BaseModel): operation: Operation modification: PartialAttribute - def get_name(self) -> str: - """Get mod name.""" + @property + def l_type(self) -> str: + """Get modification type (it's attribute name) in lower case.""" return self.modification.type.lower() diff --git a/app/ldap_protocol/roles/access_manager.py b/app/ldap_protocol/roles/access_manager.py index 9e7a964f4..e651e5d62 100644 --- a/app/ldap_protocol/roles/access_manager.py +++ b/app/ldap_protocol/roles/access_manager.py @@ -123,17 +123,16 @@ def check_modify_access( return False for change in changes: - attr_name = change.get_name() if change.operation == Operation.DELETE: if not cls._check_modify_access( - attr_name, + change.l_type, filtered_aces, AceType.DELETE, ): return False elif change.operation == Operation.ADD: if not cls._check_modify_access( - attr_name, + change.l_type, filtered_aces, AceType.WRITE, ): @@ -141,12 +140,12 @@ def check_modify_access( else: if not ( cls._check_modify_access( - attr_name, + change.l_type, filtered_aces, AceType.WRITE, ) and cls._check_modify_access( - attr_name, + change.l_type, filtered_aces, AceType.DELETE, ) From 2040f6e0391085994b0471ffb24386c2a2baf6ba Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 13:05:48 +0300 Subject: [PATCH 11/13] fix: Changes l_name -> cached_property task_1186 --- app/ldap_protocol/objects.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/ldap_protocol/objects.py b/app/ldap_protocol/objects.py index 1114aa7c8..d69301c0e 100644 --- a/app/ldap_protocol/objects.py +++ b/app/ldap_protocol/objects.py @@ -5,6 +5,7 @@ """ from enum import IntEnum, IntFlag, StrEnum, unique +from functools import cached_property from typing import Annotated import annotated_types @@ -82,7 +83,7 @@ class Changes(BaseModel): operation: Operation modification: PartialAttribute - @property + @cached_property def l_type(self) -> str: """Get modification type (it's attribute name) in lower case.""" return self.modification.type.lower() From 11f45e1c9e3799e40913121a343919f91edc791a Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 16:02:23 +0300 Subject: [PATCH 12/13] refactor: ModifyRequest: user/computer samAccName/UPN task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 113 +++++++++++++----- .../test_main/test_router/test_modify.py | 33 ++++- 2 files changed, 114 insertions(+), 32 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index f391ada97..f5f8faaa9 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -919,31 +919,22 @@ async def _add( # noqa: C901 new_value = value # type: ignore if change.l_type == "userprincipalname": - new_samaccountname = str(new_value).split("@")[0] - - await kadmin.rename_princ( - directory.user.sam_account_name, - new_samaccountname, - ) - - await session.execute( - update(User) - .filter_by(directory=directory) - .values(samaccountname=new_samaccountname), + await self._modify_user_userprincipalname( + directory, + directory.user, + session, + kadmin, + new_value, ) elif change.l_type == "samaccountname": - new_userprincipalname = f"{str(new_value)}@{base_dir.name}" - - await kadmin.rename_princ( - directory.user.sam_account_name, - str(new_value), - ) - - await session.execute( - update(User) - .filter_by(directory=directory) - .values(userprincipalname=new_userprincipalname), + await self._modify_user_samaccountname( + directory, + directory.user, + session, + kadmin, + base_dir, + new_value, ) await session.execute( @@ -957,15 +948,11 @@ async def _add( # noqa: C901 and directory.entity_type and directory.entity_type.name == EntityTypeNames.COMPUTER ): - samaccountname_old_val = self._old_vals.get(change.modification.type) # noqa: E501 # fmt: skip - - await kadmin.rename_princ( - f"host/{samaccountname_old_val}", - f"host/{str(value)}", - ) - await kadmin.rename_princ( - f"host/{samaccountname_old_val}.{base_dir.name}", - f"host/{str(value)}.{base_dir.name}", + await self._modify_computer_samaccountname( + change, + kadmin, + base_dir, + value, ) attrs.append( Attribute( @@ -1027,6 +1014,70 @@ async def _add( # noqa: C901 session.add_all(attrs) + async def _modify_computer_samaccountname( + self, + change: Changes, + kadmin: AbstractKadmin, + base_dir: Directory, + value: bytes | str, + ) -> None: + samaccountname_old_val = self._old_vals.get(change.modification.type) + + if not samaccountname_old_val: + raise ModifyForbiddenError("Old sAMAccountName value not found.") + + if samaccountname_old_val != str(value): + await kadmin.rename_princ( + f"host/{samaccountname_old_val}", + f"host/{str(value)}", + ) + await kadmin.rename_princ( + f"host/{samaccountname_old_val}.{base_dir.name}", + f"host/{str(value)}.{base_dir.name}", + ) + + async def _modify_user_samaccountname( + self, + directory: Directory, + user: User, + session: AsyncSession, + kadmin: AbstractKadmin, + base_dir: Directory, + new_value: datetime | bytes | str | None, + ) -> None: + new_userprincipalname = f"{str(new_value)}@{base_dir.name}" + + if user.sam_account_name != str(new_value): + await kadmin.rename_princ(user.sam_account_name, str(new_value)) + + await session.execute( + update(User) + .filter_by(directory=directory) + .values(userprincipalname=new_userprincipalname), + ) + + async def _modify_user_userprincipalname( + self, + directory: Directory, + user: User, + session: AsyncSession, + kadmin: AbstractKadmin, + new_value: datetime | bytes | str | None, + ) -> None: + new_samaccountname = str(new_value).split("@")[0] + + if user.user_principal_name != str(new_value): + await kadmin.rename_princ( + user.sam_account_name, + new_samaccountname, + ) + + await session.execute( + update(User) + .filter_by(directory=directory) + .values(samaccountname=new_samaccountname), + ) + async def _get_base_dir( self, directory: Directory, diff --git a/tests/test_api/test_main/test_router/test_modify.py b/tests/test_api/test_main/test_router/test_modify.py index b8d959823..c1aa72535 100644 --- a/tests/test_api/test_main/test_router/test_modify.py +++ b/tests/test_api/test_main/test_router/test_modify.py @@ -193,7 +193,7 @@ async def test_api_correct_modify_user_userprincipalname( @pytest.mark.usefixtures("adding_test_computer") @pytest.mark.usefixtures("setup_session") @pytest.mark.usefixtures("session") -async def test_api_correct_modify_computer_samaccountname( +async def test_api_correct_modify_computer_samaccountname_replace( http_client: AsyncClient, kadmin: AbstractKadmin, ) -> None: @@ -257,6 +257,37 @@ async def test_api_correct_modify_computer_samaccountname( raise Exception("Computer without sAMAccountName") +@pytest.mark.asyncio +@pytest.mark.usefixtures("adding_test_computer") +@pytest.mark.usefixtures("setup_session") +@pytest.mark.usefixtures("session") +async def test_api_incorrect_modify_computer_samaccountname_add( + http_client: AsyncClient, +) -> None: + """Test API for modify computer sAMAccountName.""" + entry_dn = "cn=mycomputer,dc=md,dc=test" + response = await http_client.patch( + "/entry/update", + json={ + "object": entry_dn, + "changes": [ + { + "operation": Operation.ADD, + "modification": { + "type": "sAMAccountName", + "vals": ["maincomputer"], + }, + }, + ], + }, + ) + + data = response.json() + + assert isinstance(data, dict) + assert data.get("resultCode") == LDAPCodes.OPERATIONS_ERROR + + @pytest.mark.asyncio @pytest.mark.usefixtures("setup_session") @pytest.mark.usefixtures("session") From febb0c719c0ae824645a8b2b73af29d8cb08f306 Mon Sep 17 00:00:00 2001 From: Milov Dmitriy Date: Tue, 27 Jan 2026 18:05:57 +0300 Subject: [PATCH 13/13] refactor: Modify task_1186 --- app/ldap_protocol/ldap_requests/modify.py | 101 +++++++--------------- 1 file changed, 29 insertions(+), 72 deletions(-) diff --git a/app/ldap_protocol/ldap_requests/modify.py b/app/ldap_protocol/ldap_requests/modify.py index f5f8faaa9..7ab3333fb 100644 --- a/app/ldap_protocol/ldap_requests/modify.py +++ b/app/ldap_protocol/ldap_requests/modify.py @@ -918,31 +918,29 @@ async def _add( # noqa: C901 else: new_value = value # type: ignore - if change.l_type == "userprincipalname": - await self._modify_user_userprincipalname( - directory, - directory.user, - session, - kadmin, - new_value, - ) + if change.l_type in ("userprincipalname", "samaccountname"): + if change.l_type == "userprincipalname": + new_user_principal_name = str(new_value) + new_sam_account_name = new_user_principal_name.split("@")[0] # noqa: E501 # fmt: skip + elif change.l_type == "samaccountname": + new_sam_account_name = str(new_value) + new_user_principal_name = f"{new_sam_account_name}@{base_dir.name}" # noqa: E501 # fmt: skip + + if directory.user.sam_account_name != new_sam_account_name: + await kadmin.rename_princ( + directory.user.sam_account_name, + new_sam_account_name, + ) - elif change.l_type == "samaccountname": - await self._modify_user_samaccountname( - directory, - directory.user, - session, - kadmin, - base_dir, - new_value, + directory.user.user_principal_name = new_user_principal_name # noqa: E501 # fmt: skip + directory.user.sam_account_name = new_sam_account_name + else: + await session.execute( + update(User) + .filter_by(directory=directory) + .values({change.l_type: new_value}), ) - await session.execute( - update(User) - .filter_by(directory=directory) - .values({change.l_type: new_value}), - ) - elif ( change.l_type == "samaccountname" and directory.entity_type @@ -1019,63 +1017,22 @@ async def _modify_computer_samaccountname( change: Changes, kadmin: AbstractKadmin, base_dir: Directory, - value: bytes | str, + new_sam_account_name: bytes | str, ) -> None: - samaccountname_old_val = self._old_vals.get(change.modification.type) + old_sam_account_name = self._old_vals.get(change.modification.type) + new_sam_account_name = str(new_sam_account_name) - if not samaccountname_old_val: + if not old_sam_account_name: raise ModifyForbiddenError("Old sAMAccountName value not found.") - if samaccountname_old_val != str(value): - await kadmin.rename_princ( - f"host/{samaccountname_old_val}", - f"host/{str(value)}", - ) + if old_sam_account_name != new_sam_account_name: await kadmin.rename_princ( - f"host/{samaccountname_old_val}.{base_dir.name}", - f"host/{str(value)}.{base_dir.name}", - ) - - async def _modify_user_samaccountname( - self, - directory: Directory, - user: User, - session: AsyncSession, - kadmin: AbstractKadmin, - base_dir: Directory, - new_value: datetime | bytes | str | None, - ) -> None: - new_userprincipalname = f"{str(new_value)}@{base_dir.name}" - - if user.sam_account_name != str(new_value): - await kadmin.rename_princ(user.sam_account_name, str(new_value)) - - await session.execute( - update(User) - .filter_by(directory=directory) - .values(userprincipalname=new_userprincipalname), + f"host/{old_sam_account_name}", + f"host/{new_sam_account_name}", ) - - async def _modify_user_userprincipalname( - self, - directory: Directory, - user: User, - session: AsyncSession, - kadmin: AbstractKadmin, - new_value: datetime | bytes | str | None, - ) -> None: - new_samaccountname = str(new_value).split("@")[0] - - if user.user_principal_name != str(new_value): await kadmin.rename_princ( - user.sam_account_name, - new_samaccountname, - ) - - await session.execute( - update(User) - .filter_by(directory=directory) - .values(samaccountname=new_samaccountname), + f"host/{old_sam_account_name}.{base_dir.name}", + f"host/{new_sam_account_name}.{base_dir.name}", ) async def _get_base_dir(