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

feat: add basic user banning functionality #343

Merged
merged 14 commits into from
Jan 19, 2022
Merged

Conversation

kangmingtay
Copy link
Member

@kangmingtay kangmingtay commented Jan 14, 2022

What kind of change does this PR introduce?

  • Adds a ban_until column in auth.users to restrict specific user access until a certain time.
  • GET /admin/users should allow filtering by users
  • PUT /admin/users/<user_id> should allow banning an existing user
    • Update adminUserParams struct to include a BanDuration field
    • Decided to use a duration rather than a timestamp or boolean since it seems like the best DX (users don't have to worry about following go's time format) and provides greater flexibility over a bool
  • fixes Ability to enable/disable users temporarily  #41

Considerations

  • Decided not to go with a middleware approach in view of having to make an extra query to the database to check if a user is banned or not.
  • The current implementation makes use of the existing query used to fetch the user and checks the ban_until column.
  • Even though the current implementation requires more code repetition (more lines of if user.IsBanned() { ... }), the api performance won't be compromised.

Todo

  • Add ban_until column in users table
  • Check if user is banned in /token
  • Check if user is banned in /verify
  • Check if user is banned in /callback
  • PUT /admin/users/<user_id> should allow banning an existing user
  • GET /admin/users should allow filtering by banned users (to be fixed in another PR)

@kangmingtay kangmingtay self-assigned this Jan 14, 2022
@kangmingtay
Copy link
Member Author

kangmingtay commented Jan 16, 2022

current filtering syntax is too basic to support filtering by banned users:

  • To filter by email - /admin/users?filter=email
  • To filter by full name - /admin/users?filter=fullname

I'm thinking of changing this to the following but it involves making a breaking change to the API (so i'll do it in a separate PR):

/admin/users?email=email
/admin/users?fullname=fullname
/admin/users?ban_until=timestamp

# of course, the following will be possible too
/admin/users?email=email&fullname=fullname&ban_until=timestamp

@kangmingtay kangmingtay changed the title [WIP] feat: add basic user banning functionality feat: add basic user banning functionality Jan 16, 2022
-- updates users_instance_id_email_idx definition

DROP INDEX IF EXISTS users_instance_id_email_idx;
CREATE INDEX IF NOT EXISTS users_instance_id_email_idx on users using btree (instance_id, lower(email));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to this PR but this addresses #247

Copy link
Contributor

@darora darora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally seems fine. Not the biggest fan of using the magic string of "0" to un-ban a user-your call on whether you want that more spec'ed out or to go ahead with this.

@kangmingtay
Copy link
Member Author

Generally seems fine. Not the biggest fan of using the magic string of "0" to un-ban a user-your call on whether you want that more spec'ed out or to go ahead with this.

hmm would it be better to create an enum like unban instead of "0"? or should we make it such that if ban_duration isn't specified, we always reset the BanUntil field?

@darora
Copy link
Contributor

darora commented Jan 17, 2022

I'd probably go the route of having a separate enum, rather than relying on the ban_duration field missing

Copy link
Member

@awalias awalias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs some documentation but otherwise looks good 🥇

migrations/20220114185340_add_ban_until.up.sql Outdated Show resolved Hide resolved
"email": "test4@example.com",
"phone": "",
"password": "test1",
"ban_duration": "24h",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you confirm the format for what is accepted by ban_duration and also document it in the README 👌

README.md Outdated
"phone_confirm": true,
"user_metadata": {},
"app_metadata": {},
"ban_duration": "24h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry this is pedantic but is there a link to the format spec - e.g. can I do 24h 32m 6s or 24h 3s or 1y3m2s (with no spaces) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this is the format spec it follows
https://pkg.go.dev/time#ParseDuration

@kangmingtay kangmingtay merged commit cc94302 into master Jan 19, 2022
@kangmingtay kangmingtay deleted the km/feat-ban-users branch January 19, 2022 17:13
@github-actions
Copy link
Contributor

🎉 This PR is included 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to enable/disable users temporarily
3 participants