Skip to content

Commit

Permalink
Add user status checks for moderation
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
AkshayPall committed Sep 23, 2022
1 parent 05ae73b commit 55948de
Show file tree
Hide file tree
Showing 11 changed files with 187 additions and 2 deletions.
3 changes: 2 additions & 1 deletion ambuda/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ def _load_user(user_id: int) -> Optional[User]:
import current_user`) and as a template variable injected into each template.
"""
session = get_session()
return session.query(User).get(int(user_id))
user = session.query(User).get(int(user_id))
return user if user and user.is_ok else None


def _unauthorized():
Expand Down
1 change: 1 addition & 0 deletions ambuda/checks.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ def _check_lookup_tables(session) -> list[str]:
def _check_bot_user(session) -> list[str]:
"""Check that the ambuda-bot user exists."""
username = consts.BOT_USERNAME
# Assume bot user is active
bot_user = session.query(db.User).filter_by(username=username).first()
if bot_user:
return []
Expand Down
21 changes: 21 additions & 0 deletions ambuda/models/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,34 @@ class User(AmbudaUserMixin, Base):
#: The user's self-description.
description = Column(Text_, nullable=False, default="")

#: If the user deleted their account.
is_deleted = Column(Boolean, nullable=False, default=False)

#: If the user was banned..
is_banned = Column(Boolean, nullable=False, default=False)

#: If the user has verified their email.
is_verified = Column(Boolean, nullable=False, default=False)

#: 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):
"""Update is_deleted."""
self.is_deleted = is_deleted

def set_is_banned(self, is_banned: bool):
"""Update is_banned."""
self.is_banned = is_banned

def set_is_verified(self, is_verified: bool):
"""Update is_verified."""
self.is_verified = is_verified

def check_password(self, raw_password: str) -> bool:
"""Check if the given password matches the user's hash."""
return check_password_hash(self.password_hash, raw_password)
Expand Down
6 changes: 5 additions & 1 deletion ambuda/queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,11 @@ def page(project_id, page_slug: str) -> Optional[db.Page]:

def user(username: str) -> Optional[db.User]:
session = get_session()
return session.query(db.User).filter_by(username=username).first()
return (
session.query(db.User)
.filter_by(username=username, is_deleted=False, is_banned=False)
.first()
)


def create_user(*, username: str, email: str, raw_password: str) -> db.User:
Expand Down
8 changes: 8 additions & 0 deletions ambuda/utils/user_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ def is_moderator(self) -> bool:
def is_admin(self) -> bool:
return False

@property
def is_ok(self) -> bool:
return True


class AmbudaUserMixin(UserMixin):
def has_role(self, role: SiteRole) -> bool:
Expand Down Expand Up @@ -59,3 +63,7 @@ def is_moderator(self) -> bool:
@property
def is_admin(self) -> bool:
return self.has_role(SiteRole.ADMIN)

@property
def is_ok(self) -> bool:
return not (self.is_deleted or self.is_banned)
9 changes: 9 additions & 0 deletions ambuda/views/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class ResetPasswordFromTokenForm(FlaskForm):
@bp.route("/register", methods=["GET", "POST"])
def register():
if current_user.is_authenticated:
logout_if_not_ok()
return redirect(url_for("site.index"))

form = SignupForm()
Expand All @@ -152,6 +153,7 @@ def register():
@bp.route("/sign-in", methods=["GET", "POST"])
def sign_in():
if current_user.is_authenticated:
logout_if_not_ok()
return redirect(url_for("site.index"))

form = SignInForm()
Expand All @@ -165,6 +167,13 @@ def sign_in():
return render_template("auth/sign-in.html", form=form)


def logout_if_not_ok():
# Check if user is now deleted or banned
user = q.user(username=current_user.username)
if user and not user.is_ok:
logout_user()


@bp.route("/sign-out")
def sign_out():
logout_user()
Expand Down
54 changes: 54 additions & 0 deletions migrations/versions/99379162619e_add_user_statuses.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
"""Add user status
Revision ID: 99379162619e
Revises: bc48af5ec2e6
Create Date: 2022-09-11 17:20:02.341713
"""
from alembic import op
import sqlalchemy as sa
from sqlalchemy import orm

# revision identifiers, used by Alembic.
revision = "99379162619e"
down_revision = "bc48af5ec2e6"
branch_labels = None
depends_on = None

Base = orm.declarative_base()


class User(Base):
__tablename__ = "users"
id = sa.Column(sa.Integer, primary_key=True)
is_deleted = sa.Column(sa.Boolean, nullable=False, default=False)
is_banned = sa.Column(sa.Boolean, nullable=False, default=False)
is_verified = sa.Column(sa.Boolean, nullable=False, default=False)


def upgrade() -> None:
op.add_column("users", sa.Column("is_deleted", sa.Boolean, nullable=True))
op.add_column("users", sa.Column("is_banned", sa.Boolean, nullable=True))
op.add_column("users", sa.Column("is_verified", sa.Boolean, nullable=True))

bind = op.get_bind()
session = orm.Session(bind=bind)
for user in session.query(User).all():
user.is_deleted = False
user.is_banned = False
user.is_verified = False
session.add(user)
session.commit()

with op.batch_alter_table("users") as batch_op:
batch_op.alter_column("is_deleted", existing_type=sa.BOOLEAN(), nullable=False)
batch_op.alter_column("is_banned", existing_type=sa.BOOLEAN(), nullable=False)
batch_op.alter_column("is_verified", existing_type=sa.BOOLEAN(), nullable=False)


def downgrade() -> None:
# ### commands auto generated by Alembic - please adjust! ###
op.drop_column("users", "is_deleted")
op.drop_column("users", "is_banned")
op.drop_column("users", "is_verified")
# ### end Alembic commands ###
31 changes: 31 additions & 0 deletions test/ambuda/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,19 @@ def initialize_test_db():
session.add(admin)
session.flush()

# Deleted and Banned
deleted_admin = db.User(username="sandrocottus-deleted", email="cgm@ambuda.org")
deleted_admin.set_password("maurya")
deleted_admin.set_is_deleted(True)

banned = db.User(username="sikander-banned", email="alex@ambuda.org")
banned.set_password("onesicritus")
banned.set_is_banned(True)

session.add(deleted_admin)
session.add(banned)
session.flush()

# Roles
p1_role = db.Role(name=db.SiteRole.P1.value)
p2_role = db.Role(name=db.SiteRole.P2.value)
Expand All @@ -93,9 +106,13 @@ def initialize_test_db():
rama.roles = [p1_role, p2_role]
moderator.roles = [p1_role, p2_role, moderator_role]
admin.roles = [p1_role, p2_role, admin_role]
deleted_admin.roles = [p1_role, p2_role, admin_role]
banned.roles = [p1_role]
session.add(rama)
session.add(moderator)
session.add(admin)
session.add(deleted_admin)
session.add(banned)
session.flush()

# Blog
Expand Down Expand Up @@ -179,3 +196,17 @@ def admin_client(flask_app):
session = get_session()
user = session.query(db.User).filter_by(username="u-admin").first()
return flask_app.test_client(user=user)


@pytest.fixture()
def deleted_client(flask_app):
session = get_session()
user = session.query(db.User).filter_by(username="sandrocottus-deleted").first()
return flask_app.test_client(user=user)


@pytest.fixture()
def banned_client(flask_app):
session = get_session()
user = session.query(db.User).filter_by(username="sikander-banned").first()
return flask_app.test_client(user=user)
31 changes: 31 additions & 0 deletions test/ambuda/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@ def _cleanup(session, *objects):
session.commit()


def test_user__is_ok_when_created(client):
session = get_session()
user = db.User(username="test", email="test@ambuda.org")
user.set_password("my-password")
session.add(user)
session.commit()

assert user.is_ok

_cleanup(session, user)


def test_user__set_and_check_password(client):
session = get_session()
user = db.User(username="test", email="test@ambuda.org")
Expand Down Expand Up @@ -38,6 +50,25 @@ def test_user__set_and_check_role(client):
_cleanup(session, user)


def test_user__deletion(client):
session = get_session()

# Check active user
user = db.User(username="test", email="test@ambuda.org")
user.set_password("my-password")
session.add(user)
session.commit()
assert user.is_ok

# Deleted
user.set_is_deleted(True)
session.add(user)
session.commit()
assert not user.is_ok

_cleanup(session, user)


def test_role__repr(client):
role = db.Role(name="foo")
assert repr(role) == "<Role(None, 'foo')>"
Expand Down
10 changes: 10 additions & 0 deletions test/ambuda/views/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ def test_admin_index__auth(admin_client):
assert resp.status_code == 200


def test_admin_index__inactive(deleted_client, banned_client):
assert deleted_client.get("/admin/").status_code == 404
assert banned_client.get("/admin/").status_code == 404


def test_admin_text__unauth(client):
resp = client.get("/admin/text/")
assert resp.status_code == 404


def test_admin_text__inactive(deleted_client, banned_client):
assert deleted_client.get("/admin/text/").status_code == 404
assert banned_client.get("/admin/text/").status_code == 404
15 changes: 15 additions & 0 deletions test/ambuda/views/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,21 @@ def test_register__unauth_post__ok(client):
r = client.post("/register", data=data)
assert r.status_code == 302
assert current_user.username == "krishna"
assert current_user.is_ok


def test_register__auth(rama_client):
r = rama_client.get("/register")
assert r.status_code == 302


def test_register__banned(banned_client):
with banned_client:
r = banned_client.get("/register")
assert r.status_code == 200
assert current_user.is_anonymous


def test_sign_in__unauth(client):
r = client.get("/sign-in")
assert ">Sign in to Ambuda<" in r.text
Expand Down Expand Up @@ -115,6 +123,13 @@ def test_sign_in__auth(rama_client):
assert r.status_code == 302


def test_sign_in__banned(banned_client):
with banned_client:
r = banned_client.get("/sign-in")
assert r.status_code == 200
assert current_user.is_anonymous


def test_sign_out__unauth(client):
with client:
r = client.get("/sign-out")
Expand Down

0 comments on commit 55948de

Please sign in to comment.