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

Add delete user identity server endpoint and delete profile and forms api endpoint #442

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

irinel-nistor
Copy link
Contributor

@irinel-nistor irinel-nistor commented May 27, 2020

What does it fix?

Closes #418

  1. Added API endpoint that deletes the user profile and its forms, as well as its family members profiles and forms
  2. Added IdentiyServer endpoint that:
    • deletes the IdentiyServer user
    • generates access token and calls the delete profile api endpoint (1)

How has it been tested?

I manually tested the changes, by creating test data and removing it using the endpoints

*** Should I add a new client in IdentiyServer configuration, that would be the only one to access the DeleteProfile endpoint from the api, using a new scope?

@vercel
Copy link

vercel bot commented May 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/code4romania/stam-acasa/kple4xeia
✅ Preview: https://stam-acasa-git-fork-irinel-nistor-deleteuserendpoint.code4romania.vercel.app

[HttpPost]
public async Task<IActionResult> DeleteAccountAsync([FromBody] DeleteAccountModel model)
{
var user = await _userManager.FindByNameAsync(model.Username);
Copy link
Member

Choose a reason for hiding this comment

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

we should not provide info to anyone if a user with specified username exists or not

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I agree with this one.
Can you please make the change @irinel-nistor ?

/// </summary>
/// <returns></returns>
[HttpDelete]
public async Task<IActionResult> DeleteProfile()
Copy link
Member

Choose a reason for hiding this comment

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

this endpoint should be called only by IdentityServer . If someone with valid token will call this endpoint he will delete from just from User table and will not delete from AspNetUsers

Copy link
Member

Choose a reason for hiding this comment

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

in order to implement this we could add a claim named admin or idsrv and add a policy for this endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good points, Thank you!

idormenco
idormenco previously approved these changes May 31, 2020
@RaduCStefanescu
Copy link
Contributor

Also, we will need the update on frontend.
Would you be able to take care of that @irinel-nistor ?

@RaduCStefanescu
Copy link
Contributor

The frontend view for this issue will be handled in this issue:
#419

if (sub == null)
return new UnauthorizedResult();

await _userService.DeleteUserAndDependentData(sub);
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we get the user email from the current session instead of requiring the UI to provide the email ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I think this is a very fair point.

@surdu surdu mentioned this pull request Jun 28, 2020
@RaduCStefanescu
Copy link
Contributor

@irinel-nistor can you please update the PR to take the email from the session? The UI will only provide the password in this situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete account feature
4 participants