feat(api): delete the authenticated user #20410

dhruvmanila · 2022-07-19T17:05:33Z

Delete the authenticated user.

Endpoint: DELETE /user

resolves: #19439

routers/api/v1/user/user.go

6543 · 2022-07-21T18:31:56Z

thanks

zeripath · 2022-07-22T08:34:47Z

ARGH!

I am completely unconvinced that this should be an API call. At the very least this needs to be wrapped in some kind of ARE YOU SURE?! check likely through email confirmation.

Can you imagine the griefing potentials for self user deletion through API let alone purging?!

This API could be a serious problem.

wxiaoguang · 2022-07-22T10:10:12Z

I do not have a good feel about this API either. What's the real use case / real user story for such API?

dhruvmanila · 2022-07-25T16:48:46Z

Hey @zeripath @wxiaoguang, I get where you're coming from and the problems this API might create. As per the discussions in the linked issue (#19439), it seemed clear that for this API there's no point in having the "confirmation" part. That part should be taken care by the frontend.

As per my limited knowledge of gitea (this is my first PR), I think before this only an admin can delete a user but given this API, a frontend component could be added to delete yourself and the frontend part will take care of the confirmation part.

routers/api/v1/api.go

6543 · 2022-09-27T01:13:15Z

This API could be a serious problem.

agree but:

for acidential -> the api client is responsible for that
for malicious: feat(api): delete the authenticated user #20410 (comment), so on app with just a token can dell stuff (similar to how the token endpoint's are handled)

wxiaoguang · 2022-09-27T03:52:16Z

I have strong objection about making it as a /api/ API before there is a clear design, quote comment #19439 (comment)

So, if a user can only delete themself from the web UI, then function can be written in the web backend code.

If a user can delete themself by API, then I think there need more details: why they need to do so, what's the real use case?

Endpoint: DELETE /user

Check if it fails when user still has ownership of repositories and then verify the use of the purge parameter.

6543 · 2022-09-29T02:35:37Z

@dhruvmanila would be nice if you merge main, like so ☝️

as each force push do mess up reviewing

dhruvmanila · 2022-09-29T02:36:55Z

Oh sorry, will keep that in mind from next time 👍

hello-smile6 · 2022-09-29T02:39:26Z

Oh sorry, will keep that in mind from next time 👍

Is this ready?

6543 · 2022-09-29T02:44:18Z

did add an smal code-format to make it more redable

wxiaoguang

~~I still have the question why it should be an API, what's the real use case, why a user would call an API to delete themself.~~

outdated

wxiaoguang · 2022-09-29T04:40:16Z

Hmm, just saw the latest reqBasicOrRevProxyAuth change. It should be fine? I will dismiss my request change and hide out-dated comments.

Will the reqBasicOrRevProxyAuth enforce users to re-enter their password?

.

lunny · 2022-10-23T03:58:14Z

routers/api/v1/user/user.go

+		return
+	}
+
+	if err := user_service.DeleteUser(ctx, ctx.Doer, ctx.FormBool("purge")); err != nil {


We should ensure only site administrator could do it.

lunny · 2022-10-23T03:59:35Z

Please move the api under /api/v1/admin/users.

wxiaoguang · 2022-10-23T04:00:43Z

When this PR mixes with

Allow Token API calls be authorized using the reverse-proxy header #15119 (comment)

When the instance is running behind reverse-proxy auth, other users with access token (without password/otp auth) can delete the token's user.

denyskon · 2023-09-10T10:17:07Z

What is the state of this PR? Are you still working on it?

wxiaoguang · 2023-09-10T10:26:21Z

What is the state of this PR? Are you still working on it?

My opinion is:

I do not think Gitea has a good and safe auth system design. That's the root problem.
The account deletion should have extra confirmation, but some features made the situation more complex (eg: the reverse-proxy header auth).
The account deletion API doesn't seem bring enough benefit but it might cause security & design problem if some cases are missing.

I would suggest to clarify the details before continuing. eg: various edge cases, what's the correct approach for auth and account deletion.

That's just my opinion, if most maintainers think this is right, I won't block.

denyskon · 2023-09-10T10:51:29Z

I think I'd agree with you on this. If it brings security risks, I don't think this feature is that much of a benefit that we could ignore this issue. Besides the fact that I don't even really see a reason for such an API endpoint 😆

dhruvmanila · 2023-09-10T11:06:09Z

What is the state of this PR? Are you still working on it?

Hey, no I'm currently not working on this. But from all the discussion around this, I think it might be useful to take a step back and see if this is actually beneficial. It's been a while so I've lost context around this 😅

That said, I'm totally fine in closing the PR for the time being and someone can pick it up if we decide to go forward.

denyskon · 2023-09-10T11:21:16Z

Okay, so let's close this and move the discussion back to the issue

dhruvmanila marked this pull request as ready for review July 20, 2022 16:20

dhruvmanila changed the title ~~[WIP] feat(api): delete the authenticated user~~ feat(api): delete the authenticated user Jul 20, 2022

dhruvmanila force-pushed the feat/delete-user-api branch from 39dc73e to e86b42d Compare July 21, 2022 03:03

6543 reviewed Jul 21, 2022

View reviewed changes

routers/api/v1/user/user.go Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 21, 2022

6543 added this to the 1.18.0 milestone Jul 21, 2022

6543 added type/feature Completely new functionality. Can only be merged if feature freeze is not active. modifies/api This PR adds API routes or modifies them labels Jul 21, 2022

6543 approved these changes Jul 21, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 21, 2022

dhruvmanila force-pushed the feat/delete-user-api branch 2 times, most recently from 8295fbb to 86ea6a9 Compare July 22, 2022 04:11

This comment was marked as outdated.

Sign in to view

wxiaoguang mentioned this pull request Jul 28, 2022

[API] Delete Yourself #19439

Open

6543 approved these changes Sep 27, 2022

View reviewed changes

6543 requested changes Sep 27, 2022

View reviewed changes

routers/api/v1/api.go Outdated Show resolved Hide resolved

dhruvmanila added 7 commits September 28, 2022 20:43

feat(api): delete the authenticated user

1d950cf

Endpoint: DELETE /user

test: add test cases for delete user API

59304aa

fix: lint errors

418b2bc

docs(api): add query parameter info

0c51dc9

test: API purge user testing

42eb77c

Check if it fails when user still has ownership of repositories and then verify the use of the purge parameter.

refactor: use auth middleware for delete user api

8dabe12

refactor: fix test for the latest changes

24ecbd0

Merge branch 'main' into feat/delete-user-api

fe4e845

just use Combo

fce394f

6543 approved these changes Sep 29, 2022

View reviewed changes

wxiaoguang previously requested changes Sep 29, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

dhruvmanila and others added 3 commits October 2, 2022 14:50

Merge branch 'main' into feat/delete-user-api

d027fa5

Merge branch 'main' into feat/delete-user-api

196b96a

Merge branch 'main' into feat/delete-user-api

aa612ce

lunny reviewed Oct 23, 2022

View reviewed changes

lunny modified the milestones: 1.18.0, 1.19.0 Oct 26, 2022

lunny modified the milestones: 1.19.0, 1.20.0 Feb 3, 2023

wxiaoguang removed this from the 1.20.0 milestone May 27, 2023

denyskon added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Sep 10, 2023

denyskon closed this Sep 10, 2023

go-gitea locked as resolved and limited conversation to collaborators Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): delete the authenticated user #20410

feat(api): delete the authenticated user #20410

dhruvmanila commented Jul 19, 2022

6543 commented Jul 21, 2022

zeripath commented Jul 22, 2022

wxiaoguang commented Jul 22, 2022 •

edited

Loading

dhruvmanila commented Jul 25, 2022

This comment was marked as outdated.

6543 commented Sep 27, 2022

wxiaoguang commented Sep 27, 2022

6543 commented Sep 29, 2022

dhruvmanila commented Sep 29, 2022

hello-smile6 commented Sep 29, 2022

6543 commented Sep 29, 2022

wxiaoguang left a comment •

edited

Loading

This comment was marked as outdated.

wxiaoguang commented Sep 29, 2022 •

edited

Loading

lunny Oct 23, 2022

lunny commented Oct 23, 2022

wxiaoguang commented Oct 23, 2022

denyskon commented Sep 10, 2023

wxiaoguang commented Sep 10, 2023

denyskon commented Sep 10, 2023

dhruvmanila commented Sep 10, 2023

denyskon commented Sep 10, 2023

feat(api): delete the authenticated user #20410

feat(api): delete the authenticated user #20410

Conversation

dhruvmanila commented Jul 19, 2022

6543 commented Jul 21, 2022

zeripath commented Jul 22, 2022

wxiaoguang commented Jul 22, 2022 • edited Loading

dhruvmanila commented Jul 25, 2022

This comment was marked as outdated.

6543 commented Sep 27, 2022

wxiaoguang commented Sep 27, 2022

6543 commented Sep 29, 2022

dhruvmanila commented Sep 29, 2022

hello-smile6 commented Sep 29, 2022

6543 commented Sep 29, 2022

wxiaoguang left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

wxiaoguang commented Sep 29, 2022 • edited Loading

lunny Oct 23, 2022

Choose a reason for hiding this comment

lunny commented Oct 23, 2022

wxiaoguang commented Oct 23, 2022

denyskon commented Sep 10, 2023

wxiaoguang commented Sep 10, 2023

denyskon commented Sep 10, 2023

dhruvmanila commented Sep 10, 2023

denyskon commented Sep 10, 2023

wxiaoguang commented Jul 22, 2022 •

edited

Loading

wxiaoguang left a comment •

edited

Loading

wxiaoguang commented Sep 29, 2022 •

edited

Loading