Implement #27 #36

Merged
jordan merged 37 commits from lda/telodendria:implement-27 into master 2023-09-25 13:39:24 +00:00
7 changed files with 176 additions and 5 deletions

View file

@ -24,6 +24,11 @@ The following endpoints were added:
- Fixed a double-free in `RouteUserProfile()` that would cause errors with certain
Matrix clients. (#35)
### New Features
Implemented `/_telodendria/admin/deactivate/[localpart]` for admins to be able to
deactivate users.
## v0.3.0
**Saturday, June 10, 2023**

View file

@ -86,6 +86,7 @@ RouterBuild(void)
R("/_telodendria/admin/config", RouteConfig);
R("/_telodendria/admin/privileges", RoutePrivileges);
R("/_telodendria/admin/privileges/(.*)", RoutePrivileges);
R("/_telodendria/admin/deactivate/(.*)", RouteAdminDeactivate);
#undef R

View file

@ -0,0 +1,117 @@
/*
* Copyright (C) 2022-2023 Jordan Bancino <@jordan:bancino.net>
*
* Permission is hereby granted, free of charge, to any person
* obtaining a copy of this software and associated documentation files
* (the "Software"), to deal in the Software without restriction,
* including without limitation the rights to use, copy, modify, merge,
* publish, distribute, sublicense, and/or sell copies of the Software,
* and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be
* included in all copies or portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
* NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
* BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
* ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
* CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*/
#include <Routes.h>
#include <Json.h>
#include <HashMap.h>
#include <Str.h>
#include <User.h>
#include <Log.h>
ROUTE_IMPL(RouteAdminDeactivate, path, argp)
{
RouteArgs *args = argp;
HashMap *request = NULL;
HashMap *response = NULL;
JsonValue *val;
char *reason = "Deactivated by admin";
char *removedLocalpart = ArrayGet(path, 0);
char *token;
Db *db = args->matrixArgs->db;
User *user = NULL;
User *removed = NULL;
HttpRequestMethod method = HttpRequestMethodGet(args->context);
if ((method != HTTP_DELETE) && (method != HTTP_PUT))
{
char * msg = "Route only supports DELETE and PUT as for now.";
jordan marked this conversation as resolved
Review

Is there a reason for declaring a variable here instead of just passing the string literal into MatrixErrorCreate()?

Is there a reason for declaring a variable here instead of just passing the string literal into `MatrixErrorCreate()`?
Review

Honestly, it's merely just to keep it all under 80 columns without having to do line-breaks in the middle of a function call.

Honestly, it's merely just to keep it all under 80 columns without having to do line-breaks in the middle of a function call.
Review

Fair enough, I'm good with that. It doesn't look bad by any means, the code is still readable. Everyone has their own style and I have no real problems with this one.

Fair enough, I'm good with that. It doesn't look bad by any means, the code is still readable. Everyone has their own style and I have no real problems with this one.
HttpResponseStatus(args->context, HTTP_BAD_REQUEST);
return MatrixErrorCreate(M_UNRECOGNIZED, msg);
}
if (method == HTTP_DELETE)
{
request = JsonDecode(HttpServerStream(args->context));
if (!request)
{
HttpResponseStatus(args->context, HTTP_BAD_REQUEST);
return MatrixErrorCreate(M_NOT_JSON, NULL);
}
val = HashMapGet(request, "reason");
if (val && JsonValueType(val) == JSON_STRING)
{
reason = JsonValueAsString(val);
}
}
response = MatrixGetAccessToken(args->context, &token);
if (response)
{
goto finish;
}
user = UserAuthenticate(db, token);
removed = UserLock(db, removedLocalpart);
if (!user || !removed)
{
HttpResponseStatus(args->context, HTTP_BAD_REQUEST);
response = MatrixErrorCreate(M_UNKNOWN_TOKEN, NULL);
goto finish;
}
if (!(UserGetPrivileges(user) & USER_DEACTIVATE))
{
char * msg = "User doesn't have the DEACTIVATE privilege.";
HttpResponseStatus(args->context, HTTP_FORBIDDEN);
response = MatrixErrorCreate(M_FORBIDDEN, msg);
goto finish;
}
if (method == HTTP_DELETE)
{
UserDeactivate(removed, UserGetName(user), reason);
lda marked this conversation as resolved
Review

Again, why use an intermediate variable here?

Again, why use an intermediate variable here?
response = HashMapCreate();
JsonSet(response, JsonValueString(removedLocalpart), 1, "user");
JsonSet(response, JsonValueString(reason), 1, "reason");
JsonSet(response, JsonValueString(UserGetName(user)), 1, "banned_by");
}
else
{
UserReactivate(removed);
HttpResponseStatus(args->context, HTTP_NO_CONTENT);
}
finish:
UserUnlock(user);
UserUnlock(removed);
JsonFree(request);
return response;
}

View file

@ -126,7 +126,7 @@ ROUTE_IMPL(RouteDeactivate, path, argp)
goto finish;
}
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user))
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user, NULL, NULL))
jordan marked this conversation as resolved
Review

I think we should set these parameters instead of having them be NULL. The from should be the user, and the reason can just be hard-coded if necessary.

I think we should set these parameters instead of having them be `NULL`. The `from` should be the user, and the `reason` can just be hard-coded if necessary.
Review

Oh nevermind, I see that the default behavior of this function is to set it to the user's own name.

Oh nevermind, I see that the default behavior of this function is to set it to the user's own name.
{
HttpResponseStatus(args->context, HTTP_INTERNAL_SERVER_ERROR);
response = MatrixErrorCreate(M_UNKNOWN, NULL);

View file

@ -429,7 +429,40 @@ UserSetPassword(User * user, char *password)
}
int
UserDeactivate(User * user)
UserDeactivate(User * user, char * from, char * reason)
{
HashMap *json;
JsonValue *val;
if (!user)
{
return 0;
}
/* By default, it's the target's username */
if (!from)
{
from = UserGetName(user);
}
json = DbJson(user->ref);
JsonValueFree(HashMapSet(json, "deactivated", JsonValueBoolean(1)));
val = JsonValueString(from);
JsonValueFree(JsonSet(json, val, 2, "deactivate", "by"));
if (reason)
{
val = JsonValueString(reason);
JsonValueFree(JsonSet(json, val, 2, "deactivate", "reason"));
}
return 1;
}
int
UserReactivate(User * user)
{
HashMap *json;
@ -440,7 +473,10 @@ UserDeactivate(User * user)
json = DbJson(user->ref);
JsonValueFree(HashMapSet(json, "deactivated", JsonValueBoolean(1)));
JsonValueFree(HashMapSet(json, "deactivated", JsonValueBoolean(0)));
JsonValueFree(HashMapDelete(json, "deactivate"));
return 1;
}

View file

@ -103,6 +103,8 @@ ROUTE(RouteCreateRoom);
ROUTE(RouteAliasDirectory);
ROUTE(RouteRoomAliases);
ROUTE(RouteAdminDeactivate);
#undef ROUTE
#endif

View file

@ -199,14 +199,24 @@ extern int UserSetPassword(User *, char *);
* is to prevent future users from pretending to be a previous user
* of a given localpart. The user is logged out; all access tokens are
* invalidated.
* Additionally, it stores information on who deactivated the account
* (be it an admin or the user itself), and why. If the user
* responsible for deactivating the target user is NULL, then it is
* set to the target's own name.
*/
extern int UserDeactivate(User *);
extern int UserDeactivate(User *, char *, char *);
/**
* Reactivates the given user account if it has been deactvated with
lda marked this conversation as resolved
Review

Is a separate function really necessary? Shouldn't we just modify UserDeactivate() to take the additional params?

Is a separate function really necessary? Shouldn't we just modify `UserDeactivate()` to take the additional params?
Review

Considering how easy refactoring the code to use the proper UserDeactivate signature(which, to be frank, I should have done sooner), I think it's fine to change a few lines.

Considering how easy refactoring the code to use the proper `UserDeactivate` signature(which, to be frank, I should have done sooner), I think it's fine to change a few lines.
* .Fn UserDeactivate ,
* otherwise, it simply doesn't do anything.
*/
extern int UserReactivate(User *);
/**
* Return a boolean value indicating whether or not the user was
* deactivated using
* .Fn UserDeactivate .
* Note that once deactivated, users cannot be reactivated.
*/
extern int UserDeactivated(User *);