flarum / issue-archive

0 stars 0 forks source link

Deactivate users via API #307

Open tcheko opened 6 years ago

tcheko commented 6 years ago

Explanation

If one set isActivated user property to true with API, it works. While, setting isActivated to false fails.

Technical details

Code

When isActivated is set to true, user get activated while setting it to false fails to deactivated user.

    $ch = curl_init($this->api_url . '/users/' . $flarum_user_id);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
    curl_setopt($ch, CURLOPT_CUSTOMREQUEST, "PATCH");
    curl_setopt($ch, CURLOPT_HTTPHEADER, [
        'Content-Type: application/json',
        'Authorization: Token ' . $this->session->token
    ]);
    curl_setopt($ch, CURLOPT_POSTFIELDS, json_encode([
        'data' => [
            'attributes' => [
            "isActivated" => false
            ]
        ]    
    ]));
    $result = curl_exec($ch);
luceos commented 6 years ago

isActivated false is the default, you can activate it if you pass along the property (no matter what value) in the request and you're an admin.

The solution is to not pass the property along.

https://github.com/flarum/core/blob/master/src/Core/Command/RegisterUserHandler.php#L129

PS your hosting environment, is that heavy metal 😁

tcheko commented 6 years ago

Well. The behaviour isn't really consistent imo.

Anyway. How do I deactivate an user thru API if I can't set this attribute to false?

tobyzerner commented 6 years ago

@luceos this is for updating a user (PATCH), not registering a new one

luceos commented 6 years ago

I see, didn't notice this was a patch request ;)

tcheko commented 6 years ago

Ok. digged the code a bit. Looks like there is no provision for disabling an user. activate method takes no argument at all and just switches the isActivated from false to true.

So, not really a bug in the end. But it would make sense imo to be able to disable an user from API. At least, it would help keeping stuff in sync with main login system (outside Flarum scope for my case).

As a side note, If user is disabled from main login system, browser might still have a valid Flarum session cookie. User could then still post messages until the cookie expires.

matteocontrini commented 6 years ago

You could suspend the user if you don't want them to be able to post to the forum

franzliedke commented 6 years ago

@tobscure Do we want to provide this use-case, even though it is not part of Flarum's core feature set?

For now, I vote no, unless this request comes up more often.

tobyzerner commented 6 years ago

What is the use-case for needing to deactivate users?

franzliedke commented 6 years ago

Synchronisation with an existing authentication system, apparently.

luceos commented 6 years ago

I'm in favor of having the ability to deactivate users. Good use case would be an infinitewp kind of tool that would remotely check user information against a database and actively deactivates any fraudulent users.

tobyzerner commented 6 years ago

Alright it's not hard to implement so let's do it.

franzliedke commented 6 years ago

Good use case example, but why in core?

tobyzerner commented 6 years ago

I think core API should probably provide sane defaults like being able to switch a boolean both ways, even when we don't use it on our front-end. An extension that specifically "allows you to deactivate users via the API" just seems like poor design?

franzliedke commented 6 years ago

I don't want to maintain code that isn't used in core (or bundled extensions). And we have enough tickets already. ;)

That said, it is hard to draw the line, I know...

As this is about a custom integration with an existing authentication system, I guess there is an extension active somehow that could integrate this code? @tcheko Please clarify.

franzliedke commented 6 years ago

it is hard to draw the line

is_private is a good example.

luceos commented 6 years ago

It's about keeping the balance. We want Flarum to be open enough so it is easily extended, yet close enough so we have enough control over security and stability.

In this case, the cost doesn't outweigh the benefit.

franzliedke commented 6 years ago

If you both vote for it, I am fine with it. Technically, this behavior should be tested, though - that would be the only way to ensure we notice if it breaks. :see_no_evil:

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!