Fix user account deletion not working

[puni9869]

as title:
Fixes: #26210

[puni9869]

@techknowlogick Need your feedback on this approach.

[puni9869]

I think we should limit the use of ctx.ServerError("<namespace>", err)

[lunny]

No, I don't think it's right. We need to enumerate all possible errors.

[puni9869]

No, I don't think it's right. We need to enumerate all possible errors.

But it is limited to password check and do you want the whole user_model should be checked before proceeding to delete the account.

Could you suggest something in one or two liner the approach. It will be easy for me to understand.

[lunny]

No, I don't think it's right. We need to enumerate all possible errors.

But it is limited to password check and do you want the whole user_model should be checked before proceeding to delete the account.

Could you suggest something in one or two liner the approach. It will be easy for me to understand.

At least, to resolve #26210, you could catch paswrod is invalid error and display it in the UI and let other errors return 500.

Some errors are affected by wrong user's input, some are from system problems. We need to diff these two kinds of errors.

[puni9869]

ed to diff these two kinds of errors.

Ohh, I got it now. Sending the fix in a few

[puni9869]

passwd := ctx.FormString("password")
if len(passwd) == 0 || !ctx.Doer.ValidatePassword(passwd) {
    ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_password"), tplSettingsAccount, nil)
    return
}

[lunny]

see #26210 (comment)

[puni9869]

see #26210 (comment)

I will share couple samples of implementation in the followups. Apologies for this,

_, _, err := auth.UserSignIn(ctx.Doer.Name, ctx.FormString("password"))
if err != nil {
    if user_model.IsErrUserNotExist(err) {
	    loadAccountData(ctx)
	    ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_password"), tplSettingsAccount, nil)
    } else if errors.Is(err, err.(db.ErrUserPasswordNotSet)) {
	    ctx.RenderWithErr(ctx.Tr("form.password_not_set_on_user_account"), tplSettingsAccount, nil)
    } else if errors.Is(err, err.(db.ErrUserPasswordInvalid)) {
	    ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_password"), tplSettingsAccount, nil)
    } else if user_model.IsErrUserProhibitLogin(err) {
	    ctx.RenderWithErr(ctx.Tr("auth.prohibit_login"), tplSettingsAccount, nil)
    } else {
	    ctx.ServerError("UserSignIn", err)
    }
    return
}

this is the approach I was thinging about

[yp05327]

Is it possible to reuse handleSignInError?
And this is strange:

[puni9869]

Is it possible to reuse handleSignInError? And this is strange:

Yes, I have used it. If you see the commits but the error scope is large. I am getting panic in below snipped.

_, _, err := auth.UserSignIn(ctx.Doer.Name, ctx.FormString("password"))
if err != nil {
    if user_model.IsErrUserNotExist(err) {
	    loadAccountData(ctx)
	    ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_password"), tplSettingsAccount, nil)
    } else if errors.Is(err, err.(db.ErrUserPasswordNotSet)) {
	    ctx.RenderWithErr(ctx.Tr("form.password_not_set_on_user_account"), tplSettingsAccount, nil)
    } else if errors.Is(err, err.(db.ErrUserPasswordInvalid)) {
	    ctx.RenderWithErr(ctx.Tr("form.enterred_invalid_password"), tplSettingsAccount, nil)
    } else if user_model.IsErrUserProhibitLogin(err) {
	    ctx.RenderWithErr(ctx.Tr("auth.prohibit_login"), tplSettingsAccount, nil)
    } else {
	    ctx.ServerError("UserSignIn", err)
    }
    return
}

[techknowlogick]

Thanks for making this change. auth.UserSignIn below should return ErrUserPasswordInvalid if the password is invalid, so perhaps that could be checked in an if conditional instead (pseudocode err == ErrUserPasswordInvalid)?

[puni9869]

I will add this check too. But lil bit confused with the error stack that I have create in above comments are right way of doing it? Need a bit of help there.
The committed solution will just works as fine. But there are other errors thats may occurs that needs to be covered.

[wxiaoguang]

How does it work with external auth sources? eg: LDAP, the passwd field is empty.

(the first question is: should Gitea support deleting LDAP accounts?)

[puni9869]

@lunny Need your help on this.

[lunny]

Maybe we need to check whether password is set before check popup the password dialog.

[puni9869]

You mean we need an api call from frontend that will check the password is set on the account?

[puni9869]

Thinking that, there are still some edge cases remaining before we implement the solution around this 500 error.
I'm keeping this PR open for the further discussion and will raise a complete new PR.