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.

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

[FEATURE] remove random password generation when creating a user and password is not provided #4993

Merged
merged 4 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
5 changes: 1 addition & 4 deletions argilla-sdk/docs/how_to_guides/user.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@ current_user = client.me

To create a new user in Argilla, you can define it in the `User` class and then call the `create` method. This method is inherited from the `Resource` base class and operates without modifications.

!!! info Password generation
If you don't provide a password, a random one will be generated for you. Ensure you store this password securely by accessing it through `user.password` immediately after user creation. For security reasons, the password will not be retrievable later.

```python
import argilla_sdk as rg

Expand Down Expand Up @@ -198,4 +195,4 @@ client = rg.Argilla(api_url="<api_url>", api_key="<api_key>")
user_to_delete = client.users('my_username')

deleted_user = user_to_delete.delete()
```
```
20 changes: 4 additions & 16 deletions argilla-sdk/src/argilla_sdk/users/_resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@

from argilla_sdk import Workspace
from argilla_sdk._api import UsersAPI
from argilla_sdk._models import UserModel, Role
from argilla_sdk._models import Role, UserModel
from argilla_sdk._resource import Resource
from argilla_sdk.client import Argilla


class User(Resource):
"""Class for interacting with Argilla users in the Argilla server. User profiles \
are used to manage access to the Argilla server and track responses to records.

Attributes:
username (str): The username of the user.
first_name (str): The first name of the user.
Expand Down Expand Up @@ -56,7 +56,7 @@ def __init__(
first_name (str): The first name of the user
last_name (str): The last name of the user
role (str): The role of the user, either 'annotator', admin, or 'owner'
password (str): The password of the user. If not provided, a random password will be generated
password (str): The password of the user
client (Argilla): The client used to interact with Argilla

Returns:
Expand All @@ -69,7 +69,7 @@ def __init__(
if _model is None:
_model = UserModel(
username=username,
password=password or self._generate_random_password(),
password=password,
first_name=first_name or username,
last_name=last_name,
role=role or Role.annotator,
Expand Down Expand Up @@ -176,15 +176,3 @@ def role(self) -> Role:
@role.setter
def role(self, value: Role) -> None:
self._model.role = value

############################
# Private methods
############################

@staticmethod
def _generate_random_password(n: int = 12) -> str:
"""Generates a random password for the user"""
import random
import string

return "".join(random.choices(string.ascii_letters + string.digits, k=n))
9 changes: 7 additions & 2 deletions argilla-sdk/tests/integration/test_manage_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
# limitations under the License.

import pytest

from argilla_sdk import User, Argilla
from argilla_sdk import Argilla, User
from argilla_sdk._exceptions import UnprocessableEntityError


@pytest.fixture(scope="session", autouse=True)
Expand All @@ -35,6 +35,11 @@ def test_create_user(self, client: Argilla):
assert user.id is not None
assert client.users(username=user.username).id == user.id

def test_create_user_without_password(self, client: Argilla):
user = User(username="test_user")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test on retrieving an existing user via client.users("existing_user")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test with your suggestion. Please can you take a look to it?

with pytest.raises(expected_exception=UnprocessableEntityError):
client.users.add(user)

def test_delete_user(self, client: Argilla):
user = User(username="test_delete_user", password="test_password")
client.users.add(user)
Expand Down
4 changes: 3 additions & 1 deletion argilla-sdk/tests/unit/test_resources/test_users.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def test_serialize(self):
)

assert user.serialize()["username"] == "test-user"
assert user.serialize()["password"] == None

def test_json_serialize(self):
mock_uuid = uuid.uuid4()
Expand Down Expand Up @@ -283,6 +284,7 @@ def test_create_user(self, httpx_mock: HTTPXMock):
"first_name": "Test",
"last_name": "User",
"role": "admin",
"password": "test-password",
"inserted_at": datetime.utcnow().isoformat(),
"updated_at": datetime.utcnow().isoformat(),
}
Expand All @@ -295,4 +297,4 @@ def test_create_user(self, httpx_mock: HTTPXMock):
user = client.api.users.create(user_create)
assert user.id == user_id
assert user.username == "test-user"
assert user.password is None
assert user.password == "test-password"
Loading