Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove legacy attributes from UserUpdate/Read schemas #1794

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Remove legacy attributes from UserUpdate/Read schemas #1794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f83fc13
removed slurm_users, slurm_account and cache_dir from User schemas
ychiucco Sep 23, 2024
18686c3
fix tests (one still broken)
ychiucco Sep 23, 2024
a403370
fix last test
ychiucco Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 1 addition & 62 deletions fractal_server/app/schemas/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,10 @@
from fastapi_users import schemas
from pydantic import BaseModel
from pydantic import Extra
from pydantic import Field
from pydantic import validator
from pydantic.types import StrictStr

from ._validators import val_absolute_path
from ._validators import val_unique_list
from ._validators import valstr
from fractal_server.string_tools import validate_cmd

__all__ = (
"UserRead",
Expand Down Expand Up @@ -41,16 +37,10 @@ class UserRead(schemas.BaseUser[int]):
Schema for `User` read from database.

Attributes:
slurm_user:
cache_dir:
username:
slurm_accounts:
"""

slurm_user: Optional[str]
cache_dir: Optional[str]
username: Optional[str]
slurm_accounts: list[str]
group_names: Optional[list[str]] = None
group_ids: Optional[list[int]] = None
oauth_accounts: list[OAuthAccountRead]
Expand All @@ -61,32 +51,14 @@ class UserUpdate(schemas.BaseUserUpdate):
Schema for `User` update.

Attributes:
slurm_user:
cache_dir:
username:
slurm_accounts:
"""

slurm_user: Optional[str]
cache_dir: Optional[str]
username: Optional[str]
slurm_accounts: Optional[list[StrictStr]]

# Validators
_slurm_user = validator("slurm_user", allow_reuse=True)(
valstr("slurm_user")
)
_username = validator("username", allow_reuse=True)(valstr("username"))

_slurm_accounts = validator("slurm_accounts", allow_reuse=True)(
val_unique_list("slurm_accounts")
)

@validator("cache_dir")
def cache_dir_validator(cls, value):
validate_cmd(value)
return val_absolute_path("cache_dir")(value)

@validator(
"is_active",
"is_verified",
Expand All @@ -106,21 +78,9 @@ class UserUpdateStrict(BaseModel, extra=Extra.forbid):
Schema for `User` self-editing.

Attributes:
cache_dir:
slurm_accounts:
"""

cache_dir: Optional[str]
slurm_accounts: Optional[list[StrictStr]]

_slurm_accounts = validator("slurm_accounts", allow_reuse=True)(
val_unique_list("slurm_accounts")
)

@validator("cache_dir")
def cache_dir_validator(cls, value):
validate_cmd(value)
return val_absolute_path("cache_dir")(value)
pass


class UserUpdateWithNewGroupIds(UserUpdate):
Expand All @@ -136,32 +96,11 @@ class UserCreate(schemas.BaseUserCreate):
Schema for `User` creation.

Attributes:
slurm_user:
cache_dir:
username:
slurm_accounts:
"""

slurm_user: Optional[str]
cache_dir: Optional[str]
username: Optional[str]
slurm_accounts: list[StrictStr] = Field(default_factory=list)

# Validators

@validator("slurm_accounts")
def slurm_accounts_validator(cls, value):
for i, element in enumerate(value):
value[i] = valstr(attribute=f"slurm_accounts[{i}]")(element)
val_unique_list("slurm_accounts")(value)
return value

_slurm_user = validator("slurm_user", allow_reuse=True)(
valstr("slurm_user")
)
_username = validator("username", allow_reuse=True)(valstr("username"))

@validator("cache_dir")
def cache_dir_validator(cls, value):
validate_cmd(value)
return val_absolute_path("cache_dir")(value)
110 changes: 14 additions & 96 deletions tests/no_version/test_auth_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,7 @@ async def test_register_user(registered_client, registered_superuser_client):
"""

EMAIL = "[email protected]"
payload_register = dict(
email=EMAIL, password="12345", slurm_accounts=["A", "B"]
)
payload_register = dict(email=EMAIL, password="12345")

# Non-superuser user: FORBIDDEN
res = await registered_client.post(
Expand All @@ -53,7 +51,6 @@ async def test_register_user(registered_client, registered_superuser_client):
)
assert res.status_code == 201
assert res.json()["email"] == EMAIL
assert res.json()["slurm_accounts"] == payload_register["slurm_accounts"]
assert res.json()["oauth_accounts"] == []


Expand Down Expand Up @@ -105,8 +102,6 @@ async def test_show_user(registered_client, registered_superuser_client):
assert res.status_code == 200
assert res.json()["oauth_accounts"] == []


async def test_patch_current_user_cache_dir(registered_client):
"""
Test several scenarios for updating `slurm_accounts` and `cache_dir`
for the current user.
Expand All @@ -121,50 +116,14 @@ async def test_patch_current_user_cache_dir(registered_client):
assert res.status_code == 200
assert res.json() == pre_patch_user

# Successful update
assert pre_patch_user["cache_dir"] is None
NEW_SLURM_ACCOUNTS = ["foo", "bar"]
assert pre_patch_user["slurm_accounts"] != NEW_SLURM_ACCOUNTS
res = await registered_client.patch(
f"{PREFIX}/current-user/",
json={"cache_dir": "/tmp", "slurm_accounts": NEW_SLURM_ACCOUNTS},
)
assert res.status_code == 200
assert res.json()["cache_dir"] == "/tmp"
assert res.json()["slurm_accounts"] == NEW_SLURM_ACCOUNTS

# slurm_accounts must be a list of StrictStr without repetitions
res = await registered_client.patch(
f"{PREFIX}/current-user/",
json={"slurm_accounts": ["a", "b", "c"]},
)
assert res.status_code == 200
assert res.json()["slurm_accounts"] == ["a", "b", "c"]

# Failed update due to empty string
res = await registered_client.patch(
f"{PREFIX}/current-user/", json={"cache_dir": ""}
)
assert res.status_code == 422

# Failed update due to null value
res = await registered_client.patch(
f"{PREFIX}/current-user/", json={"cache_dir": None}
)
assert res.status_code == 422

# Failed update due to non-absolute path
res = await registered_client.patch(
f"{PREFIX}/current-user/", json={"cache_dir": "not_abs"}
)
assert res.status_code == 422


async def test_patch_current_user_no_extra(registered_client):
"""
Test that the PATCH-current-user endpoint fails when extra attributes are
provided.
"""
res = await registered_client.patch(f"{PREFIX}/current-user/", json={})
assert res.status_code == 200
res = await registered_client.patch(
f"{PREFIX}/current-user/", json={"cache_dir": "/tmp", "foo": "bar"}
)
Expand Down Expand Up @@ -248,11 +207,7 @@ async def test_edit_users_as_superuser(registered_superuser_client):

res = await registered_superuser_client.post(
f"{PREFIX}/register/",
json=dict(
email="[email protected]",
password="12345",
slurm_accounts=["foo", "bar"],
),
json=dict(email="[email protected]", password="12345"),
)
assert res.status_code == 201
pre_patch_user = res.json()
Expand All @@ -264,23 +219,6 @@ async def test_edit_users_as_superuser(registered_superuser_client):
pre_patch_user["group_ids"] = []
debug(pre_patch_user)

update = dict(
email="[email protected]",
is_active=False,
is_superuser=True,
is_verified=True,
slurm_user="slurm_patch",
cache_dir="/patch",
username="user_patch",
slurm_accounts=["FOO", "BAR", "FOO"],
)
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{pre_patch_user['id']}/",
json=update,
)
# Fail because of repeated "FOO" in update.slurm_accounts
assert res.status_code == 422

# Fail because invalid password
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{pre_patch_user['id']}/",
Expand All @@ -290,9 +228,14 @@ async def test_edit_users_as_superuser(registered_superuser_client):
debug(res.json())
assert "The password is too short" in str(res.json()["detail"])

# succeed without the repetition
# remove one of the two "FOO" in update.slurm_accounts, so that
update["slurm_accounts"] = ["FOO", "BAR"]
# succeed
update = dict(
email="[email protected]",
is_active=False,
is_superuser=True,
is_verified=True,
username="user_patch",
)
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{pre_patch_user['id']}/",
json=update,
Expand Down Expand Up @@ -342,21 +285,6 @@ async def test_edit_users_as_superuser(registered_superuser_client):
)
assert res.status_code == 422

# SLURM_USER
# String attribute 'slurm_user' cannot be empty
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{user_id}/",
json={"slurm_user": " "},
)
assert res.status_code == 422
# String attribute 'slurm_user' cannot be None
assert res.status_code == 422
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{user_id}/",
json={"slurm_user": None},
)
assert res.status_code == 422

# USERNAME
# String attribute 'username' cannot be empty
res = await registered_superuser_client.patch(
Expand All @@ -371,13 +299,6 @@ async def test_edit_users_as_superuser(registered_superuser_client):
)
assert res.status_code == 422

# CACHE_DIR
# String attribute 'cache_dir' cannot be None
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{user_id}/", json={"cache_dir": None}
)
assert res.status_code == 422


async def test_add_superuser(registered_superuser_client):

Expand Down Expand Up @@ -505,7 +426,6 @@ async def test_edit_user_and_fail(registered_superuser_client):
json=dict(
email="[email protected]",
password="12345",
slurm_accounts=["foo", "bar"],
),
)
assert res.status_code == 201
Expand All @@ -515,7 +435,7 @@ async def test_edit_user_and_fail(registered_superuser_client):
res = await registered_superuser_client.patch(
f"{PREFIX}/users/{user_id}/",
json=dict(
slurm_user="new-slurm-user",
username="pippo",
new_group_ids=[],
),
)
Expand Down Expand Up @@ -609,9 +529,7 @@ async def test_oauth_accounts_list(

# test PATCH /auth/current-user/
async with MockCurrentUser(user_kwargs=dict(id=u2.id)):
res = await client.patch(
f"{PREFIX}/current-user/", json=dict(cache_dir="/foo/bar")
)
res = await client.patch(f"{PREFIX}/current-user/", json=dict())
assert len(res.json()["oauth_accounts"]) == 1


Expand Down
Loading
Loading