Skip to content

Conversation

pawiecz
Copy link
Contributor

@pawiecz pawiecz commented Aug 7, 2023

This PR is an initial implementation of a feature requested as #298. It is still missing method in the kernelci.api base class, command for kci user as well as proper unit and e2e tests.

Fixes #298

@pawiecz pawiecz requested a review from JenySadadia August 7, 2023 13:30
@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 7, 2023

I'm definitely missing a key part to run e2e tests because mine is continuously getting skipped:

  1. Dependency donor is marked with @pytest.mark.dependency()
  2. Dependency donor passes successfully
  3. Dependency recipient uses the same naming as pytest (e2e_tests/test_user_creation.py::test_create_regular_user)

Yet my test is not executed at all

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 7, 2023

Patches were squashed and force-pushed because fixup! commit didn't pass DCO check.

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 7, 2023

I'm definitely missing a key part to run e2e tests because mine is continuously getting skipped:

(...)

Yet my test is not executed at all

I was missing scope definition, sorry for the noise 🦆

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 7, 2023

More fixups will probably come with the review, I'll postpone squashing them until the comment threads are resolved.

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 7, 2023

Here's a list of tests this (or next) PR should probably include to cover this feature better:

  • nonexistent user
  • wrong password for existing user
  • empty new password (handler should probably reject such requests from making any changes)

Please let me know which test cases I missed on the list.

Comment on lines +359 to +365
authenticated = await auth.authenticate_user(
username, current_password.password.get_secret_value())
if not authenticated:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
detail="Incorrect username or password",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks good atm. Just thinking if we can use dependency injection for the authentication here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add myself a TODO for this. Could that injection be also reused in the /token endpoint (that's where I first saw this flow)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we can also optimize /token with that.

@JenySadadia
Copy link
Collaborator

The test looks good to me.
Although we are not running pylint for e2e tests atm, it would be nice to fix some of the pylint errors for this PR if we can.
You can check the annotated inline errors here https://github.com/kernelci/kernelci-api/pull/320/files.

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 8, 2023

@JenySadadia Thanks for the pointer to the linter annotations! I reformatted new e2e tests file with a black command. I also dropped:

  • explicit dependency marker (pytest deps work fine without it)
  • intermediary var for hashed password I temporarily used for debugging

I'll squash all the fixups and force push this branch unless more changes are requested.

@gctucker
Copy link
Collaborator

gctucker commented Aug 8, 2023

I reformatted new e2e tests file with a black command

Yes, feel free to use it as it's a powerful tool. However I wouldn't recommend making it a requirement as it can cause a lot of churn, and some text editors and personal taste can induce small variants of style that are perfectly acceptable so I would rather leave the checks with just pycodestyle. Also I would hope the black output passes pycodestyle.

@pawiecz
Copy link
Contributor Author

pawiecz commented Aug 8, 2023

Also I would hope the black output passes pycodestyle.

I just confirmed that - pycodestyle did not raise any warnings/errors on the reformatted file.

pawiecz added 2 commits August 8, 2023 12:24
This patch adds initial implementation of endpoint POST `/password` to
allow users to update their own password. The user can update password
to any requested string, additional checks will follow.

Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
Add a test for changing regular user's password from API.
The test depends on regular user creation test i.e.
'test_create_regular_user.

Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
@gctucker gctucker added this pull request to the merge queue Aug 8, 2023
Merged via the queue into kernelci:main with commit 2d0e7a8 Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add feature to let users update their own password
3 participants