Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to enable/disable users temporarily #41

Closed
awalias opened this issue Nov 20, 2020 · 8 comments · Fixed by #343
Closed

Ability to enable/disable users temporarily #41

awalias opened this issue Nov 20, 2020 · 8 comments · Fixed by #343
Labels
enhancement New feature or request released

Comments

@awalias
Copy link
Member

awalias commented Nov 20, 2020

We should add two endpoints accessible by the admin:

r.With(api.requireAdminCredentials).Post("/disable", api.DisableUser)
r.With(api.requireAdminCredentials).Post("/enable", api.EnableUser)

we would need an is_disabled column on the users table which is checked before a token is requested, and returns an error "this user has been disabled by the admin" if is true

@awalias awalias added the enhancement New feature or request label Nov 20, 2020
@icecream78
Copy link
Contributor

@awalias what to do in situations when user already got token and after that admin blocked him? do we need handle this case or it's OK and lets wait while his token expire?

@awalias
Copy link
Member Author

awalias commented Nov 22, 2020

I don't think it's possible for us to force expire an access token? I think we just have to wait for it to expire 🤷‍♂️

@icecream78
Copy link
Contributor

icecream78 commented Nov 22, 2020

@awalias maybe better make route like /admin/users/{user_id}/disable and /admin/users/{user_id}/enable and switch method from POST to PATCH? In my opinion, it seems to be more better choice. What do you think?
Sorry for such questions but it's better to ask them before task implementing rather then after finishing it

@awalias
Copy link
Member Author

awalias commented Nov 23, 2020

I wonder whether we should extend the existing POST /user endpoint with enable/disable functionality:

image

and make a new route for UserUpdate that is only accessible by Admin Users

what do you think? It will enable admins to change other data about a user (and can add more in the future)

@awalias
Copy link
Member Author

awalias commented Nov 23, 2020

but I also like your solution 🤔

@icecream78
Copy link
Contributor

Your approach is more scalable in future than mine. OK, I'll implement admin's analog of UserUpdate method=)

@mstade
Copy link

mstade commented Feb 13, 2021

I don't think it's possible for us to force expire an access token? I think we just have to wait for it to expire 🤷‍♂️

This is not necessarily true, and largely depends on how you verify the validity of tokens. Aside from checking the signature, and indeed even before checking the signature, you might want to check the sub (or other relevant claim) against a blacklist to see if the token should be disallowed.

The tricky bit is of course how you make sure your blacklist is accurate and up to date. It may not be palatable to look it up in the DB on every request, but on the other hand if you don't look it up on every request, there's a good chance you'll have a race condition where a request comes in and makes it all the way before the blacklist is propagated to wherever tokens are checked. To be fair though, depending on where in the flow you're checking the validity of a token you may well end up with a race condition anyway.

The way I've done this in the past is to have a mechanism for blacklisting tokens based on one or a combination of claims, e.g. for individual users it may be the sub claim, for groups of users it may be something else, a custom claim like grp or even individual role permissions like role. Maybe it's a combination of things, maybe a user should have general access, but lost admin privileges so any token with sub=1234 and role='admin' should be rejected. This blacklist lives at the API gateway, and that's the source of truth. Anything that gets passed the gateway is assumed to be valid (though signatures may still be checked, just not blacklist rules,) and anything behind the gateway only accepts requests coming from the gateway. Any request coming from another source is dropped without further consideration.

It's a decent compromise which allows for blocking tokens even if their expiration time is quite far in to the future. Blacklist rules generally don't expire, so care has to be taken to make it performant, and that of course largely depends on how complicated the rules can get. If all you're doing is field comparison it's a pretty fast lookup, but if you start doing fancy stuff like regex checks or multiple field comparisons you have to be real careful to make sure the expensive checks happen as late as possible, and the cheap checks act as circuit breakers first.

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 2.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants