-
Notifications
You must be signed in to change notification settings - Fork 479
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
Check old password during password change and add missing CLI commands #2587
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
…y-password-change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, nice feature!
Do we have dashboard changes ready? If not I would pause merging this, so it is not in the upcoming release, until Dashboard has this. @Cahllagerfeld
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! The changes look good.
I just added a small comment for a minor QoL improvement (feel free to ignore).
I had the same question as @avishniakov. I am guessing the changes will go in once the dashboard is ready.
hide_input=True, | ||
) | ||
if password is None: | ||
password = click.prompt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In many cases where I had to reset my password, I had to enter the new password twice. It is a nice mechanism to fight against the typos, perhaps we can also add it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, great idea, I will add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@avishniakov yes, the dashboard changes have already been merged: zenml-io/zenml-dashboard#556 |
E2E template updates in |
LLM Finetuning template updates in |
NLP template updates in |
Describe changes
As an extra security measure, this PR always requires that the current password value be supplied during a password change.
Additional related changes:
zenml user change-password
CLI command for changing the password for the current userzenml user change-password
,zenml user create
andzenml user update
zenml user update
zenml user deactivate
CLI command to be used to reset other user accounts through the CLI (by admins only).IMPORTANT: this change invalidates the current ZenML Dashboard password change support. A dashboard update is required to pass the current password to the call made to the API (handled with zenml-io/zenml-dashboard#556)
Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes