add user and ssh management #12

Merged
inex merged 21 commits from def_graphql into graphql 2022-08-01 13:40:41 +03:00
Collaborator
There is no content yet.
def added 1 commit 2022-07-21 00:37:26 +03:00
def requested review from inex 2022-07-21 00:38:39 +03:00
inex changed title from add user and ssh management functions to WIP: add user and ssh management functions 2022-07-22 11:20:16 +03:00
inex requested changes 2022-07-22 12:08:49 +03:00
@ -0,0 +11,4 @@
username: str
# userHomeFolderspace: UserHomeFolderUsage
sshKeys: typing.Optional[typing.List[str]] = None

Возможно будет лучше сделать тип списком, который по умолчанию пустой.

sshKeys: typing.List[str] = []
Возможно будет лучше сделать тип списком, который по умолчанию пустой. ```python sshKeys: typing.List[str] = [] ```
inex marked this conversation as resolved
@ -0,0 +7,4 @@
from selfprivacy_api.graphql import IsAuthenticated
from selfprivacy_api.graphql.common_types.user import User, UserMutationReturn
from selfprivacy_api.graphql.mutations.mutation_interface import (
MutationReturnInterface,

Неиспользованный импорт. Сверяйся с линтером перед коммитом.

Среда должна быть настроена на использование питона из nix-shell, чтобы линтер работал корректно.

Неиспользованный импорт. Сверяйся с линтером перед коммитом. Среда должна быть настроена на использование питона из nix-shell, чтобы линтер работал корректно.
inex marked this conversation as resolved
@ -0,0 +30,4 @@
"""Mutations ssh"""
@strawberry.mutation(permission_classes=[IsAuthenticated])
def create_ssh(self, settings: SshMutationsInput) -> UserMutationReturn:

settingsinput

`settings` → `input`
inex marked this conversation as resolved
@ -0,0 +33,4 @@
def create_ssh(self, settings: SshMutationsInput) -> UserMutationReturn:
"""Create a new ssh"""
with ReadUserData() as data:
if settings.username == "root":

Эта проверка была из-за legacy кода со стороны клиента.
С новой API это больше не нужно, эта функция может быть использована для добавления ключей рут пользователю.

Эта проверка была из-за legacy кода со стороны клиента. С новой API это больше не нужно, эта функция может быть использована для добавления ключей рут пользователю.
inex marked this conversation as resolved
@ -0,0 +46,4 @@
success=False,
message="Error: Invalid key type. Only ssh-ed25519 and ssh-rsa are supported",
code=400,
user=User("root", data["ssh"]["rootKeys"]),

Возврат этого ненастоящего пользователя не имеет смысла. Мог быть запрошен другой пользователь.

Возврат этого ненастоящего пользователя не имеет смысла. Мог быть запрошен другой пользователь.
inex marked this conversation as resolved
@ -0,0 +106,4 @@
)
@strawberry.mutation(permission_classes=[IsAuthenticated])
def delete_ssh(self, settings: SshMutationsInput) -> UserMutationReturn:

settingsinput

`settings` → `input`
inex marked this conversation as resolved
@ -0,0 +107,4 @@
@strawberry.mutation(permission_classes=[IsAuthenticated])
def delete_ssh(self, settings: SshMutationsInput) -> UserMutationReturn:
"""Delete ssh"""

"Delete ssh key from user" было бы яснее.

"Delete ssh key from user" было бы яснее.
inex marked this conversation as resolved
@ -0,0 +126,4 @@
success=True,
message="SSH key deleted",
code=200,
user=User("root", data["ssh"]["rootKeys"]),

data["ssh"]["rootKeys"] мог быть удалён несколькими строками ранее, в этом случае произошла бы ошибка.

Лучше убрать удаление массива с 124 строки, оно не имеет практического смысла.

`data["ssh"]["rootKeys"]` мог быть удалён несколькими строками ранее, в этом случае произошла бы ошибка. Лучше убрать удаление массива с 124 строки, оно не имеет практического смысла.
inex marked this conversation as resolved
@ -0,0 +145,4 @@
success=True,
message="SSH key deleted",
code=200,
user=User("root", data["ssh"]["rootKeys"]),

Здесь мы работаем с основным пользователем, а не рутом. Надо вернуть его, а не рута.

Здесь мы работаем с основным пользователем, а не рутом. Надо вернуть его, а не рута.
inex marked this conversation as resolved
@ -0,0 +26,4 @@
"""Mutations change user settings"""
@strawberry.mutation(permission_classes=[IsAuthenticated])
def create_user(self, settings: UserMutationsInput) -> UserMutationReturn:

settingsuser

`settings` → `user`
inex marked this conversation as resolved
@ -0,0 +28,4 @@
@strawberry.mutation(permission_classes=[IsAuthenticated])
def create_user(self, settings: UserMutationsInput) -> UserMutationReturn:
"""Create a new user"""
hashing_command = ["mkpasswd", "-m", "sha-512", settings.password]

Стоит вынести код для получения хэша в другое место, и вызывать эту функцию и отсюда, и из REST кода.

Папка utils может для этого подойти.

Стоит вынести код для получения хэша в другое место, и вызывать эту функцию и отсюда, и из REST кода. Папка utils может для этого подойти.
inex marked this conversation as resolved
@ -0,0 +43,4 @@
if is_username_forbidden(settings.username):
return UserMutationReturn(
success=False,
message="Error: Username is forbidden",

Error: излишне, мы и так знаем что случилась ошибка.

`Error: ` излишне, мы и так знаем что случилась ошибка.
inex marked this conversation as resolved
@ -0,0 +127,4 @@
return MutationReturnInterface(
success=True,
message="Nice frag - user deleted",

Эти сообщения будут отображаться в приложении, оставайся нейтральным.

Эти сообщения будут отображаться в приложении, оставайся нейтральным.
inex marked this conversation as resolved
@ -0,0 +132,4 @@
)
@strawberry.mutation(permission_classes=[IsAuthenticated])
def update_user(self, settings: UserMutationsInput) -> UserMutationReturn:

settingsuser

`settings` → `user`
inex marked this conversation as resolved
@ -0,0 +134,4 @@
@strawberry.mutation(permission_classes=[IsAuthenticated])
def update_user(self, settings: UserMutationsInput) -> UserMutationReturn:
"""Update user mutation"""
hashing_command = ["mkpasswd", "-m", "sha-512", settings.password]

Определённо надо вынести в отдельную функцию, см комментарий выше.

Код вызывающий subproccess потенциально опасен, и его должно быть как можно меньше.

Определённо надо вынести в отдельную функцию, см комментарий выше. Код вызывающий subproccess потенциально опасен, и его должно быть как можно меньше.
inex marked this conversation as resolved
@ -0,0 +8,4 @@
def get_users() -> typing.List[User]:
"""Get users"""

В список не попадает основной пользователь. Но ты мне напомнил что приложению нужно знать тип пользователя.

Давай определим enum UserType у которого могут быть следующие значения:

  • NORMAL для обычных пользователей
  • PRIMARY для основного пользователя, который указывается при создании сервера.
  • ROOT для рута.

Добавляем в тип User поле type с типом UserType.

Внёс эту правку также в документ в Figma.

В список не попадает основной пользователь. Но ты мне напомнил что приложению нужно знать тип пользователя. Давай определим enum `UserType` у которого могут быть следующие значения: - `NORMAL` для обычных пользователей - `PRIMARY` для основного пользователя, который указывается при создании сервера. - `ROOT` для рута. Добавляем в тип `User` поле `type` с типом `UserType`. Внёс эту правку также в документ в Figma.
inex marked this conversation as resolved
@ -0,0 +14,4 @@
for user in data["users"]:
user_list.append(User(**user))
user_list.append(User(data["username"], data["sshKeys"]))

Может случиться ошибка если поля data["sshKeys"] нет. Обрати внимание на проверки, что были в REST версии.

501c4ce43c/selfprivacy_api/resources/services/ssh.py (L204)

Может случиться ошибка если поля `data["sshKeys"]` нет. Обрати внимание на проверки, что были в REST версии. https://git.selfprivacy.org/SelfPrivacy/selfprivacy-rest-api/src/commit/501c4ce43c4e61b1daa0d541cb535ff7453cf29b/selfprivacy_api/resources/services/ssh.py#L204
inex marked this conversation as resolved
def added 1 commit 2022-07-22 13:34:06 +03:00
def added 1 commit 2022-07-22 13:36:22 +03:00
def added 1 commit 2022-07-23 12:40:05 +03:00
def added 1 commit 2022-07-23 12:47:03 +03:00
inex added 1 commit 2022-07-24 17:11:43 +03:00
def added 1 commit 2022-07-24 17:26:50 +03:00
def added 1 commit 2022-07-24 18:38:53 +03:00
def added 1 commit 2022-07-25 03:00:02 +03:00
def added 1 commit 2022-07-25 20:31:09 +03:00
def added 1 commit 2022-07-26 19:45:10 +03:00
def added 1 commit 2022-07-27 20:06:32 +03:00
def added 1 commit 2022-07-27 20:49:33 +03:00
def added 1 commit 2022-07-28 03:17:39 +03:00
def added 1 commit 2022-07-29 18:21:18 +03:00
inex added 1 commit 2022-07-30 18:39:21 +03:00
def added 2 commits 2022-07-31 00:09:59 +03:00
def added 1 commit 2022-07-31 00:43:59 +03:00
def added 1 commit 2022-07-31 01:16:52 +03:00
def requested review from inex 2022-07-31 01:17:35 +03:00
inex requested changes 2022-07-31 16:45:09 +03:00
@ -0,0 +5,4 @@
)
from enum import Enum
from selfprivacy_api.utils import ReadUserData, ensure_ssh_and_users_fields_exist

Беспорядочные импорты. Надо стремиться оформлять их так, что сначала идут системные, потом библиотечные, и потом уже локальные.

Беспорядочные импорты. Надо стремиться оформлять их так, что сначала идут системные, потом библиотечные, и потом уже локальные.
inex marked this conversation as resolved
@ -0,0 +17,4 @@
@strawberry.type
class User:
"""Users management"""

Вводящий в заблуждение комментарий, подразумевающий что в этом классе есть функции для управления юзером.

Вводящий в заблуждение комментарий, подразумевающий что в этом классе есть функции для управления юзером.
inex marked this conversation as resolved
@ -0,0 +1,54 @@
#!/usr/bin/env python3
"""Users management module"""
# pylint: disable=too-few-public-methods

Ненужные пустые строки.

Ненужные пустые строки.
inex marked this conversation as resolved
@ -0,0 +29,4 @@
@strawberry.mutation(permission_classes=[IsAuthenticated])
def create_ssh(self, ssh_input: SshMutationInput) -> UserMutationReturn:
"""Create a new ssh"""

Некорректный комментарий и название функции.
В референсе было указано add_ssh_key.

Некорректный комментарий и название функции. В референсе было указано `add_ssh_key`.
inex marked this conversation as resolved
@ -0,0 +41,4 @@
)
@strawberry.mutation(permission_classes=[IsAuthenticated])
def delete_ssh(self, ssh_input: SshMutationInput) -> UserMutationReturn:

remove_ssh_key

`remove_ssh_key`
inex marked this conversation as resolved
@ -0,0 +5,4 @@
)
def create_ssh_key(username, ssh_key):

Как насчёт указывать тип аргументов и возвращаемые значения? Относится ко всем функциям в файле.

Как насчёт указывать тип аргументов и возвращаемые значения? Относится ко всем функциям в файле.
inex marked this conversation as resolved
@ -0,0 +8,4 @@
from selfprivacy_api.utils import hash_password
def create_user_util(username, password):

Суффикс util лишён смысла. Касается всех функций в файле.

Не хватает типов аргументов и возвращаемых значений

Суффикс `util` лишён смысла. Касается всех функций в файле. Не хватает типов аргументов и возвращаемых значений
inex marked this conversation as resolved
@ -162,0 +175,4 @@
return hashed_password
def ensure_ssh_and_users_fields_exist(data):

Этой функции тут не место, мне кажется. У тебя уже есть файл user_utils

Этой функции тут не место, мне кажется. У тебя уже есть файл user_utils
inex marked this conversation as resolved
@ -0,0 +46,4 @@
API_CREATE_SSH_MUTATION = """
mutation createSsh($sshInput: SshMutationInput!) {
createSsh(sshInput: $sshInput) {

Нужно будет поправить

Нужно будет поправить
inex marked this conversation as resolved
@ -0,0 +59,4 @@
"""
def test_graphql_add_ssh_unauthorized(client, some_users, mock_subprocess_popen):

test_graphql_add_ssh_key_unauthorized

Аналогично ко всему остальному.

test_graphql_add_ssh_**key**_unauthorized Аналогично ко всему остальному.
inex marked this conversation as resolved
@ -0,0 +178,4 @@
assert response.json["data"]["createSsh"]["success"] is False
def test_graphql_add_ssh_404user(authorized_client, some_users, mock_subprocess_popen):

Что такое 404user?
test_graphql_add_ssh_key_nonexistent_user

Что такое `404user`? `test_graphql_add_ssh_key_nonexistent_user`
inex marked this conversation as resolved
@ -0,0 +200,4 @@
API_DELETE_SSH_MUTATION = """
mutation deleteSsh($sshInput: SshMutationInput!) {

Удаляем ключ, а не SSH

Удаляем ключ, а не SSH
inex marked this conversation as resolved
@ -0,0 +214,4 @@
"""
def test_graphql_dell_ssh_unauthorized(client, some_users, mock_subprocess_popen):

Причём тут Dell?

Причём тут Dell?
inex marked this conversation as resolved
@ -0,0 +291,4 @@
assert response.json["data"]["users"]["getUser"] is None
API_CHANGE_USERS_MUTATION = """

Так меняем или создаём?

Так меняем или создаём?
inex marked this conversation as resolved
@ -0,0 +550,4 @@
assert response.json["data"]["createUser"]["user"] is None
API_DELETE_USERS_MUTATION = """

Удаляем одного, а не нескольких.

Удаляем одного, а не нескольких.
inex marked this conversation as resolved
@ -0,0 +646,4 @@
assert response.json["data"]["deleteUser"]["success"] is False
API_UPDATE_USERS_MUTATION = """

Меняем одного, а не нескольких

Меняем одного, а не нескольких
inex marked this conversation as resolved
def added 1 commit 2022-07-31 22:06:59 +03:00
def requested review from inex 2022-07-31 22:07:39 +03:00
def changed title from WIP: add user and ssh management functions to add user and ssh management functions 2022-07-31 22:08:48 +03:00
def changed title from add user and ssh management functions to add user and ssh management 2022-07-31 22:09:28 +03:00
def started working 2022-07-31 22:11:19 +03:00
def cancelled time tracking 2022-07-31 22:12:01 +03:00
inex merged commit 337cf29884 into graphql 2022-08-01 13:40:41 +03:00
inex deleted branch def_graphql 2022-08-01 13:40:41 +03:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: SelfPrivacy/selfprivacy-rest-api#12
There is no content yet.