Skip to content

Conversation

dulowski-marek
Copy link
Contributor

@dulowski-marek dulowski-marek commented Aug 1, 2021

Summary of change

This exposes an endpoint: PUT /recipe/user/credentials accepting the following body:

{
    "userId": string, // required
    "email": string, 
    "password": string
}

Related issues

Test Plan

tbd

Documentation changes

  • CDI changes

Checklist for important updates

  • Changelog has been updated
  • coreDriverInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In build.gradle
  • Had installed and ran the pre-commit hook
  • If there are new dependencies that have been added in build.gradle, please make sure to add them in implementationDependencies.json.
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

  • Swagger CDI spec
  • API tests
  • Logic tests

@dulowski-marek dulowski-marek self-assigned this Aug 1, 2021
@rishabhpoddar
Copy link
Contributor

rishabhpoddar commented Aug 2, 2021

  • Please start by making changes to the CDI spec.
  • This API is supposed to allow changing of a user's password or email by simply editing those fields in the db. No need to create any additional token etc.. So an APi like:
    /recipe/user PUT
    {
       userId: string,
       newPassword?: string,
       email?: string
    }
    
    "?" means they are optional. If the user has provided both those fields, both of them are changed. And the response can be:
    {
       status: "OK" | "EMAIL_ALREADY_EXISTS_ERROR" | "UNKNOWN_USER_ID_ERROR"
    }
    
    The password validator would need to run in the backend SDK inside the recipe function that is used to call this API.
  • Due to this large difference between what was expected and what is implemented, you should close this PR and start this from scratch.

@rishabhpoddar rishabhpoddar marked this pull request as draft August 2, 2021 05:54
@dulowski-marek
Copy link
Contributor Author

Why do you think such API would be better than 2 endpoints allowing to separately change user's credentials? How is that different from password reset?
How do you want to approach change password request validation, if you can change the email in the same request?

There are reasons why I chose to do two separate requests and token generation for password change. I don't know yours for the API design that you proposed at this moment, so please explain.

@rishabhpoddar
Copy link
Contributor

Why do you think such API would be better than 2 endpoints allowing to separately change user's credentials?

Because this is not a password reset flow.. it's simply one of the CRUD operations we provide to the backend against the objects of this recipe.

How do you want to approach change password request validation, if you can change the email in the same request?

What is password request validation?

There are reasons why I chose to do two separate requests and token generation for password change. I don't know yours for the API design that you proposed at this moment, so please explain.

My logic is simply to provide CRUD operation on the objects of this recipe. That's all. The actual password reset flow is different. Happy to discuss on a call.

@dulowski-marek dulowski-marek marked this pull request as ready for review August 9, 2021 08:59
@dulowski-marek
Copy link
Contributor Author

Then this PR provides simple CRUD without session removal logic.

@rishabhpoddar rishabhpoddar merged commit 8b145b6 into 3.5 Aug 21, 2021
@rishabhpoddar rishabhpoddar deleted the 275-update-users-password branch August 21, 2021 12:41
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.

2 participants