Skip to content

Conversation

pawiecz
Copy link
Collaborator

@pawiecz pawiecz commented Aug 8, 2023

This PR depends on changes from kernelci/kernelci-api#320.

Changes have been tested against local instance of API.

Two TODOs left in the command implementation might be blockers for merging this change.

Fixes kernelci/kernelci-api#298

pawiecz added 2 commits August 8, 2023 15:00
Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
Add command to change user's password.
Sample command is as below:
`./kci user change_password --username admin`

Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
@pawiecz pawiecz requested review from gctucker and JenySadadia August 8, 2023 13:14
@pawiecz
Copy link
Collaborator Author

pawiecz commented Aug 8, 2023

This PR adds features required for kernelci/kernelci-api#298 that were missing in kernelci/kernelci-api#320.

@gctucker
Copy link
Collaborator

gctucker commented Aug 8, 2023

Two TODOs left in the command implementation might be blockers for merging this change.

Yes, pylint won't like that. But that's fine, we can work on it to get them resolved.

pawiecz added 2 commits August 8, 2023 17:58
Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
Signed-off-by: Paweł Wieczorek <pawiecz@collabora.com>
@pawiecz
Copy link
Collaborator Author

pawiecz commented Aug 8, 2023

TODOs resolved. Patch retested manually (I have to look into test automation/gating in this repo), aligned with messages from passwd I have on my machine.

@pawiecz pawiecz marked this pull request as ready for review August 8, 2023 16:13
Copy link
Collaborator

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

I think it's missing the abstract method in the base API class, right? I only see the concrete method in the latest version.

Comment on lines +57 to +64
'/password',
{
'current_password': {'password': current},
'new_password': {'password': new},
},
{
'username': username,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I'm wondering if having /password/<username> would be a better design for the API endpoint. But that's just an implementation detail, wanted to share it here as a thought.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(or /<username>/password)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(or /<username>/password)

+1 on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the original idea was that the username wasn't required if the query was using an API token for authentication. But since it requires the old password we don't rely on a token so the user endpoint makes more sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(or /<username>/password)

+1 on this.

Do you mean a new API path or should it be a part of /user, i.e. /user/<username>/password?

I can adjust it to also accept API token instead of only the old password.

@gctucker
Copy link
Collaborator

gctucker commented Aug 9, 2023

Alright we can merge this now, and add the abstract method in the base class as a follow-up.

@gctucker gctucker added this pull request to the merge queue Aug 9, 2023
Merged via the queue into kernelci:main with commit 6c0363a Aug 9, 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