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

[Moderation] Add User Status Checks on Endpoints #352

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

AkshayPall
Copy link
Contributor

@AkshayPall AkshayPall commented Sep 11, 2022

1/3 PRs for #224, ensuring that a user who has been banned/deleted recently will no longer be able to interact with the site.

The next 2 PRs will be to (1) Update all views for other users so they no longer see the usernames of deleted/banned users and (2) Add support for banning + deleting users (as admin or yourself).

@AkshayPall AkshayPall changed the title [WIP][Moderation] Add User Status Checks [WIP][Moderation] Add User Status Checks on Endpoints Sep 12, 2022
@AkshayPall AkshayPall changed the title [WIP][Moderation] Add User Status Checks on Endpoints [Moderation] Add User Status Checks on Endpoints Sep 12, 2022
@AkshayPall AkshayPall requested review from a team and akprasad and removed request for a team September 12, 2022 07:29
@AkshayPall
Copy link
Contributor Author

I know we've stuck to lookup tables for Site Roles however I don't imagine the UserStatus field to grow in value. If anyone thinks there is a nonzero chance of this, please let me know and then I can create a separate UserStatus table.

Copy link
Contributor

@akprasad akprasad left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I want to think through the active/deleted/banned distinction a little more. It looks like a given user always has one of these three states, but I think there are other states we could consider as well, such as:

  • An unverified user who hasn't confirmed their email address yet.
  • A user who's banned for 1 week.

If we're very sure that this set won't meaningfully change, it might be better to have date flags like activated_at, deleted_at, banned_at, where the field is either set (= true) or null (= false)

I find it helpful to see how Discourse models their user data: https://github.com/discourse/discourse/blob/main/app/models/user.rb#L1988

ambuda/views/auth.py Outdated Show resolved Hide resolved
@AkshayPall
Copy link
Contributor Author

Adding unverified state is definitely useful - I can see us gating certain functionality behind it.

I don't think we'll have any other statuses beyond the 4 mentioned - I think the idea of adding temporal annotations statuses seems to suggest a creation of a UserStatus Log / HIstory Table, which is just a running log of all status changes for a user with the timestamp and potentially a "previous action" column that stores context as to why the status change occured.

So in total, the Table UserStatusHistory would have columns (id, timestamp, user_id, status, explanation).

Mentally, I am modelling the state machine of Users to have 4 states (statuses) that I don't expect to change much but I feel we may introduce a variety of policies for moving between them (edges) often enough to keep them separate. What are your thoughts on implementing this approach?

@akprasad
Copy link
Contributor

akprasad commented Sep 14, 2022

What are your thoughts on implementing this approach?

I like it in principle, but this view of the world assumes we can model user states as a simple finite state automaton where 1 state = 1 node. But I think there are reasonable semantics we can associate with combinations of states, and having just one state can potentially lose information.

For example, I think it's meaningful to distinguish between deleted and banned and deleted and not banned if a user wishes to have their account reinstated for whatever reason. Likewise I think it's possible to have combinations like banned and not verified (e.g. if we allow limited activity without verification).

Using dates as a truth value is something I haven't done before, but it's a way to get bookkeeping for cheap without much complexity elsewhere in the stack (e.g. with a separate audit log). I do think we should have an audit log, but that's less urgent.

So although it feels clumsy, what do you think about having simple columns like:

# using booleans for illustration, but a similar principle applies if we use dates

# whether the user has a verified email 
is_verified
# whether the user is banned
is_banned
# whether the user's account has been deleted (deactivated?)
is_deleted

and managing the state this way?

@AkshayPall
Copy link
Contributor Author

Ah I see, you make a fair point about having "multiple states at once". Let's stick to the columnwise approach and, like you said, create the audit log of "state changes" later on once the banning/removing functionality gets added.

Copy link
Contributor

@akprasad akprasad left a comment

Choose a reason for hiding this comment

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

Thanks for waiting -- this broadly LGTM.

#: All roles available for this user.
roles = relationship("Role", secondary="user_roles")

def set_password(self, raw_password: str):
"""Hash and save the given password."""
self.password_hash = generate_password_hash(raw_password)

def set_is_deleted(self, is_deleted: bool):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use setters instead of user.is_deleted = True?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be consistent with set_password as a property that is modified after-the-fact (and I'm biased with my previous Java experience haha). I can remove it if it's too verbose.

Copy link
Contributor

Choose a reason for hiding this comment

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

set_password is kind of a special case to abstract away the password hashing logic. Otherwise I think plain attributes are more manageable long-term

test/ambuda/conftest.py Outdated Show resolved Hide resolved
ambuda/utils/user_mixins.py Outdated Show resolved Hide resolved
ambuda/views/auth.py Outdated Show resolved Hide resolved
@akprasad
Copy link
Contributor

akprasad commented Sep 23, 2022

@epicfaace ^ looks like the docker-setup is failing here -- can you help debug?

Merging because fixing the docker issue might take a while and it seems unrelated to this PR.

@akprasad akprasad merged commit 55948de into ambuda-org:main Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants