Add: sync user's and computer's names with principal name after ModifyRequest:`sAMAccountName` #911

milov-dmitriy · 2026-01-23T16:28:23Z

Исправлено переименование принципалов пользователя и компьютера.

Теперь: При создании компьютера: в таблице атрибуты создается запись sAMAccountName.
Из-за того что: При изменении имени принципала прошлые значения krbprincipalname (или как-то так) не хранятся в базе (в таблице Attributes) - и для обхода этого было сделано "кеширование" на уровне одного ModifyRequest по некоторым условиям.

Для юзера : синхронизированы изменения значений свойств sAMAccountName, UserPrincipalName и имени одного принципала.
Для компьютера : синхронизированы изменения значений свойств sAMAccountName и имени двух принципалов.

Задача: 1186

Copilot

Pull request overview

This PR synchronizes principal names with sAMAccountName changes for both user and computer entities in LDAP modify operations. When sAMAccountName is modified, the corresponding Kerberos principals are automatically renamed to maintain consistency.

Changes:

Added Kerberos principal renaming logic when sAMAccountName or userPrincipalName attributes are modified
Implemented caching of old sAMAccountName values during delete operations to enable subsequent rename operations
Added comprehensive test coverage for user and computer principal name synchronization

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
app/ldap_protocol/ldap_requests/modify.py	Core logic for synchronizing principal names with sAMAccountName/userPrincipalName modifications and caching old values
app/ldap_protocol/ldap_requests/add.py	Added sAMAccountName attribute creation for computers; refactored to use sam_account_name instead of get_upn_prefix()
app/ldap_protocol/ldap_requests/extended.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/ldap_protocol/ldap_requests/delete.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/ldap_protocol/ldap_requests/bind.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/ldap_protocol/auth/auth_manager.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/extra/scripts/uac_sync.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/extra/scripts/principal_block_user_sync.py	Updated to use sam_account_name instead of deprecated get_upn_prefix() method
app/entities.py	Removed deprecated get_upn_prefix() method from User class
tests/test_api/test_main/test_router/test_modify.py	Added comprehensive tests for user and computer principal name modification
tests/test_api/test_main/test_router/test_add.py	Added test for computer creation with sAMAccountName and assertion for sAMAccountName presence
tests/test_api/test_main/conftest.py	Added fixture for creating test computers
interface	Updated subproject commit reference

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

app/ldap_protocol/ldap_requests/modify.py

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

app/ldap_protocol/ldap_requests/modify.py

Naksen

Опиши пожалуйста подробнее:

Для чего необходимо это "кеширование" атрибута
Как именно в твоем представлении оно должно работать

Naksen · 2026-01-26T13:18:40Z

app/ldap_protocol/ldap_requests/modify.py

                        and_(
-                            qa(Attribute.name) == change.modification.type,
+                            func.lower(qa(Attribute.name))
+                            == change.modification.type.lower(),


С чем связано применение здесь .lower() ?

остатки от полета фантазии. отменил
но изначально задумывалось для кейса когда не все значения колонки name таблицы Attributes хранятся в нижнем регистре

не, это нужно. если нам приходит ModifyRequest: REPLACE: и сохранять старое значение не надо, то не для всех атрибутов это работает корректно, например это работало некорректно для атрибута krbprincipalname (в общий чат скинул пример)

app/ldap_protocol/ldap_requests/modify.py

Naksen · 2026-01-26T13:38:13Z

app/ldap_protocol/ldap_requests/modify.py

+                    await session.execute(
+                        update(User)
+                        .filter_by(directory=directory)
+                        .values(samaccountname=new_samaccountname),


а почему если приходит изменение на userprincipalname, то меняется только samaccountname ( и то же самое наоборот) ? и кажется, что можно объединить логику для этих атрибутов

всё дело в строках 959-963 на текущей ветке
при изменении свойства Х - свойство Х поменяет значение, но для этих двух атрибутов нужно менять не только этот атрибут но еще и атрибут, "связанный" с ним, т.е.:

изменяется samaccName - меняем userprincipalname и потом samaccName

изменяется userprincipalname - меняем samaccName и потом userprincipalname

по поводу объединения логик. сначала я так и сделал. но не надо. мне очень не понравилась такая реализация.
с разбиением на две разные ветки гораздо лучше выглядит
если хочешь можем вместе попробовать объединить логику, но мой предыдущий вариант мне не понравилась. все становится более "макаронным" чем есть

p.s.

для юзера дергаем rename принципала 1 раз

для компьютера дергаем rename принципала 2 раза

app/ldap_protocol/ldap_requests/modify.py

tests/test_api/test_main/test_router/test_add.py

Naksen · 2026-01-26T14:04:09Z

tests/test_api/test_main/test_router/test_modify.py

 @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(


этот тест представляет так скажем обычную модификацию атрибута пользователя. Логика обработки sAMAccountName отличается и более комплексная, на мой взгляд лучше вынести её проверку в отдельный тест, чем усложнять лишними проверками уже существующий. Предлагаю стремиться в новых тестах к тому, чтобы проверять только что-то одно.

согласен, лучше разбить, разбил

итого 3 теста вышло:

для юзера на изменение sAMAccountName

для юзера на изменение userPrincipalName

для компа на изменение sAMAccountName

milov-dmitriy · 2026-01-27T08:45:39Z

Опиши пожалуйста подробнее:

Для чего необходимо это "кеширование" атрибута

Как именно в твоем представлении оно должно работать

1.1. sAMAccountName у директории юзера хранится в таблице User, мы можем легко получить старое значение, поработать с ним, и заменить на новое значение
1.2. sAMAccountName у директории компьютер хранится в таблице attributes, при ModifyRequest с типом Replace сначала происходит _delete, затем _add, в методе _add нам нужно знать старое значение атрибута sAMAccountName, для этого я сделал сохранение старого значения в словарь

кеширование запоминает старое значение строки из таблицы Attributes для того, чтобы иметь возможность в других протектед методах получить его [старое значнеие]

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

app/ldap_protocol/ldap_requests/modify.py

tests/test_api/test_main/test_router/test_add.py

milov-dmitriy · 2026-01-27T10:00:19Z

p.s. я еще переделал get_name() с метода на проперти и имя поменял. стало получше

Naksen · 2026-01-27T11:33:53Z

app/ldap_protocol/ldap_requests/modify.py

+                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


а если для компьютера придет Modify с add на samaccountname здесь разве не вернется None ?

вообще это не корректный запрос в контексте работы с samaccountname
я подумаю

ну оно не должно упасть в этом случае. Можно зарейзить ModifyForbiddenError думаю

сделал. тест написал

Naksen · 2026-01-27T11:38:00Z

app/ldap_protocol/ldap_requests/modify.py

                else:
                    new_value = value  # type: ignore

+                if change.l_type == "userprincipalname":


Может обработку обновления userprincipalname и samaccountname вынести в отдельные методы ? Например:

_add_user_userprincipalname(self, ...)
_add_user_samaccountname(self, ...)
_add_computer_samaccountname(self, ...)

как сделано например с атрибутами группы:

сыглы, давай да

Naksen

А так, с l_type стало лучше на мой взгляд

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tests/test_api/test_main/test_router/test_modify.py

Naksen · 2026-01-27T14:18:56Z

app/ldap_protocol/ldap_requests/modify.py

+                elif change.l_type == "samaccountname":
+                    await self._modify_user_samaccountname(
+                        directory,
+                        directory.user,


А зачем передавать и directory и directory.user ? Вроде в directory.user лежит directory_id, то есть можно передать только user, проверь

отрефачил)

add: sync samAccName and users/computers principal task_1186

71afa54

milov-dmitriy self-assigned this Jan 23, 2026

milov-dmitriy added python Pull requests that update Python code ldap labels Jan 23, 2026

Copilot AI review requested due to automatic review settings January 23, 2026 16:28

Copilot AI reviewed Jan 23, 2026

View reviewed changes

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

refactor: modify: split user and computer b_logic task_1186

3d60579

milov-dmitriy requested a review from Copilot January 26, 2026 09:52

Copilot AI reviewed Jan 26, 2026

View reviewed changes

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

Naksen requested changes Jan 26, 2026

View reviewed changes

milov-dmitriy added 5 commits January 27, 2026 12:16

refactor: pr comments task_1186

8f76819

fix: ModifyRequest: get base dir task_1186

7ab025f

tests: fix

9a32004

add: ModifyRequest: KRBAPIRenamePrincipalError into exc pack task_1186

53025b5

fix: attributes delete query condition task_1186

dcc6af3

milov-dmitriy requested review from Naksen and Copilot January 27, 2026 09:44

Copilot AI reviewed Jan 27, 2026

View reviewed changes

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

app/ldap_protocol/ldap_requests/modify.py Outdated Show resolved Hide resolved

tests/test_api/test_main/test_router/test_add.py Outdated Show resolved Hide resolved

milov-dmitriy added 2 commits January 27, 2026 12:49

fix: rename task_1186

07bdcad

tests: improved task_1186

183dc81

milov-dmitriy added 2 commits January 27, 2026 13:02

refactor: method get_name() to property l_name task_1186

8b4492c

fix: Changes l_name -> cached_property task_1186

2040f6e

Naksen reviewed Jan 27, 2026

View reviewed changes

refactor: ModifyRequest: user/computer samAccName/UPN task_1186

11f45e1

milov-dmitriy requested review from Naksen and Copilot January 27, 2026 13:02

Copilot AI reviewed Jan 27, 2026

View reviewed changes

tests/test_api/test_main/test_router/test_modify.py Show resolved Hide resolved

tests/test_api/test_main/test_router/test_modify.py Show resolved Hide resolved

Naksen reviewed Jan 27, 2026

View reviewed changes

refactor: Modify task_1186

febb0c7

milov-dmitriy requested a review from Naksen January 27, 2026 15:10

Naksen approved these changes Jan 27, 2026

View reviewed changes

Add: sync user's and computer's names with principal name after ModifyRequest:sAMAccountName #911

Are you sure you want to change the base?

Add: sync user's and computer's names with principal name after ModifyRequest:sAMAccountName #911

Conversation

milov-dmitriy commented Jan 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Naksen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Naksen Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milov-dmitriy Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milov-dmitriy Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milov-dmitriy Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milov-dmitriy commented Jan 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milov-dmitriy commented Jan 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add: sync user's and computer's names with principal name after ModifyRequest:`sAMAccountName` #911

Add: sync user's and computer's names with principal name after ModifyRequest:`sAMAccountName` #911

milov-dmitriy commented Jan 23, 2026 •

edited

Loading

Naksen Jan 26, 2026 •

edited

Loading

milov-dmitriy Jan 27, 2026 •

edited

Loading

milov-dmitriy Jan 27, 2026 •

edited

Loading

milov-dmitriy Jan 27, 2026 •

edited

Loading