Add API endpoint to delete a registered user (resolves joohoi/acme-dns#177) #209
Conversation
|
The code looks good generally. Only one thing pops up: I'm going to be doing a more thorough review later on, but I don't expect large change requests to come up. About the test coverage: there's plenty of error cases that are I'm not too concerned about testing, but I'd like you to add tests for one of them: the case where invalid subdomain is provided. |
webcompas
force-pushed
the
feature_unregister
branch
from
bc852f8
to
3e03928
Compare
|
I just reworked my changes and added the following improvements:
@joohoi Please review the changes and merge if you're OK with them. |
webcompas
force-pushed
the
feature_unregister
branch
3 times, most recently
from
d43b7e0
to
46a7c62
Compare
webcompas
force-pushed
the
feature_unregister
branch
from
46a7c62
to
7910db2
Compare
|
@joohoi Unfortunately the changes haven't been merged yet for two weeks. Please review them and merge if you're OK with them. |
|
@joohoi Would be great if this PR is approved or let us know what we can do to make it better. |
| @@ -55,6 +55,40 @@ func Auth(update httprouter.Handle) httprouter.Handle { | |||
| } | |||
| } | |||
|
|
|||
| // AuthUnregister middleware for unregister request | |||
| func AuthUnregister(unregister httprouter.Handle) httprouter.Handle { | |||
There was a problem hiding this comment.
I'm not very happy about implementing a new authentication middleware, as the core functionality stays the same. I do realize however that the code does need refactoring here and is suboptimal for many parts. I still think that they should be merged.
Another issue that I have is that AuthUnregister endpoint doesn't respect the IP based allowlist, which I think it should. Even though I see the use cases where it would be nice to be able to omit this check (like losing control over an IP address, and wanting to clear up the registrations for that), it's still a security option that the user chooses when registering an acme-dns account and I believe should be restricted in all the subsequent requests.
|
any updates on this? would be great to be able to de-register.... |
|
@webcompas can this be updated/merged? |
This PR implements an API endpoint to delete registered users if they aren't neded anymore. Please check and merge.