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

Feature: Added an account disabled page #3391

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

RafaelJohn9
Copy link
Contributor

this PR addresses issue #3153

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.

Project coverage is 79.00%. Comparing base (638ae4f) to head (08710a2).
Report is 3 commits behind head on main.

Additional details and impacted files

@RafaelJohn9 RafaelJohn9 marked this pull request as draft October 4, 2024 04:35
@RafaelJohn9 RafaelJohn9 marked this pull request as ready for review October 13, 2024 19:15
@RafaelJohn9 RafaelJohn9 marked this pull request as draft October 28, 2024 03:42
@mathjazz
Copy link
Collaborator

mathjazz commented Jan 6, 2025

Hi @RafaelJohn9, is this ready for review? Could you please rebase it? Do you mind if I do it?

@RafaelJohn9
Copy link
Contributor Author

uhmm yea sure @mathjazz , you can take on the issue 🤝 , sorry for the delay

@RafaelJohn9
Copy link
Contributor Author

RafaelJohn9 commented Jan 20, 2025

hey @mathjazz, on second thoughts, lemme complete this 🤝

@RafaelJohn9 RafaelJohn9 force-pushed the feat_account_disable_page branch from 0ebd442 to d76b3f9 Compare January 20, 2025 11:21
@RafaelJohn9 RafaelJohn9 marked this pull request as ready for review January 20, 2025 11:22
…to see if the reason its not being activated arrives from its possition in the middlewares\n

Signed-off-by: RafaelJohn9 <[email protected]>
@RafaelJohn9
Copy link
Contributor Author

hey @mathjazz , well I've configured everything correctly, issue arises in the AuthenticationMiddleware, it's weird how is_authenticated=true and is_active=false doesn't pass this middleware

@RafaelJohn9
Copy link
Contributor Author

RafaelJohn9 commented Jan 20, 2025

Looking for a way to by pass this 🤝

Signed-off-by: RafaelJohn9 <[email protected]>
@RafaelJohn9
Copy link
Contributor Author

oops, well is_authenticated=False && is_active=False works so yeah 😄

Copy link
Collaborator

@mathjazz mathjazz 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 the patch! Left some comments inline.

"default": {
"BACKEND": "django_bmemcached.memcached.BMemcached",
"OPTIONS": {},
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Please restore this change. :)

map(
int,
os.environ.get("BADGES_PROMOTION_THRESHOLDS", "1, 2, 5").split(","),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Please restore this change. :)

{% block title %}Account Disabled{% endblock %}
{% block description %}
Your account has been disabled. If you believe this is a mistake or need further assistance, please contact us at {{ DEFAULT_FROM_EMAIL }}.
{% endblock %}
Copy link
Collaborator

Choose a reason for hiding this comment

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


{% block title %}Account Disabled{% endblock %}
{% block description %}
Your account has been disabled. If you believe this is a mistake or need further assistance, please contact us at {{ DEFAULT_FROM_EMAIL }}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please make the email address a mailto: hyperlink.

return render(
request,
"account_disabled.html",
{"DEFAULT_FROM_EMAIL": settings.DEFAULT_FROM_EMAIL},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this line? I didn't check, but the context_processor should pass settings to all templates:
https://github.com/mozilla/pontoon/blob/08710a279ab2f2c52b7f1a1a1035b6778c5f7f95/pontoon/base/context_processors.py

We'd need to change DEFAULT_FROM_EMAIL to settings.DEFAULT_FROM_EMAIL in account_disabled.html though.

Comment on lines +175 to +188
# For non-authenticated users, check the session manually
user_id = request.session.get("_auth_user_id")
if user_id:
try:
user = User.objects.get(pk=user_id)
if not user.is_active:
return render(
request,
"account_disabled.html",
{"DEFAULT_FROM_EMAIL": settings.DEFAULT_FROM_EMAIL},
status=403,
)
except User.DoesNotExist:
pass # If the user ID is invalid, ignore it
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly does this code do? If the user is not authenticate, we shouldn't do anything.

Copy link
Contributor Author

@RafaelJohn9 RafaelJohn9 Jan 23, 2025

Choose a reason for hiding this comment

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

@mathjazz , I was having trouble running the code without this patch, mainly when the user is not active and wishes to login, it does not pass through the AuthMiddleware,

did some digging and found that one of the requirements for a user to be authenticated is that the user must be active

I tried checking if both is_active and is_authenticated is false, however, this sort of thing affects AnonymousUser's meaning they can't browse any pages if you aren't authenticated.

So I updated the patch to have two parts just in case:

  • User is_authenticated and not active
  • User is not active but is an existing user in the database

the path highlighted presents the second part of the patch,

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.

3 participants