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/Identity] Reset Password API #6309

Merged

Conversation

RyanL1997
Copy link
Contributor

@RyanL1997 RyanL1997 commented Feb 13, 2023

Signed-off-by: Ryan Liang jiallian@amazon.com

Description

Internal users are able to reset password by using this API.

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

This looks good overall. Can you break down the tests into smaller units?

public static final ResetPasswordAction INSTANCE = new ResetPasswordAction();

// TODO : revisit this action type
public static final String NAME = "cluster:admin/user/resetpassword";
Copy link
Member

Choose a reason for hiding this comment

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

Since this API is used by all users, it may need a different namespace than cluster:admin/user/* which are a group of actions intended for the cluster admin. When it comes to authorizing this request, all users of the cluster should be able to call and use this API without being granted permission for it.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

public class ResetPasswordRequest extends ActionRequest implements ToXContentObject {

private String username;
private String oldPassword;
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't require the old password, if a user forgot there password how would they get a new password issued?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's leave this as a prototype design for now. (I just wanna transfer our conversation on github :)) I'm not gonna resolve this comment, so that I can use this as a reminder for myself, in case if we wanna change it in the future.

@RyanL1997
Copy link
Contributor Author

RyanL1997 commented Feb 20, 2023

We shouldn't require the old password, if a user forgot their password how would they get a new password issued?

Hi @shanilpa and @jimishs, may I ask your suggestions on how this reset password flow should look like for non-admin user? This PR is about the resetting password API of identity. The use case for the current design is that a non-admin user using this API to reset their password by providing their current password and new password.

For the case that a non-admin user forgot their current password, the above design required them to contact their admin, so by doing that, the admin user can use another internal user API for updating user profile to update their password into a new one.

What do you think about this flow? It's would be nice to have you guys' inputs on this one. :)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
Signed-off-by: Ryan Liang <jiallian@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Ryan Liang <jiallian@amazon.com>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@peternied
Copy link
Member

The identity test workflow is passing, merging!

@peternied peternied merged commit c56b06b into opensearch-project:feature/identity Feb 23, 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.

3 participants