-
Notifications
You must be signed in to change notification settings - Fork 58
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: Access logs #198
base: main
Are you sure you want to change the base?
Feat: Access logs #198
Conversation
|
||
class AccesslogsAppConfig(AppConfig): | ||
|
||
name = "zelthy.apps.accesslogs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use the app name as access_logs
from django.db import models | ||
from axes.models import AccessBase | ||
|
||
from ..appauth.models import AppUserModel, UserRoleModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the full import here ?
from zelthy.apps.appauth.models import AppUserModel, UserRoleModel
if anything needs to import within the app then it's still fine, but across the app we can use the full import
Let me know your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can follow that
from ..appauth.models import AppUserModel, UserRoleModel | ||
|
||
|
||
class AppAccessLogs(AccessBase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can name this model as AppAccessLog
id=getattr(request, "selected_role_id") | ||
).last() | ||
|
||
elif getattr(request, "parser_context", ""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is role_name
getting set anywhere in parser_context ?
<div class="error-content"> | ||
<div class="not-found">Account Locked</div> | ||
<div class="not-found-title"> | ||
Oops, your account/ IP address has been temporarily locked as there has been too many incorrect password attempts. Don't worry, this will be automatically reset after 15 minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the cool off time dynamic here instead of hardcoded 15 mins ?
@@ -0,0 +1,48 @@ | |||
from django.contrib.auth import signals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reorder the import statements, pls check in all the files
from ..appauth.models import AppUserModel | ||
|
||
|
||
def capture_failed_login_attempt(request, credentials): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add doc string in this method, as end user might be using it.
|
||
|
||
# Axes Lockout | ||
env = environ.Env( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this initialization is not part of core settings, it's in the project template settings file zelthy3/backend/src/zelthy/cli/project_template/project_name/settings.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the core settings the default value can be present, and in the project settings we will have values which can be read from env
@@ -0,0 +1,40 @@ | |||
{% load i18n %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a base template and this can be used for other error pages as well, should we move this to some other location let's say core/templates/exceptions
?
|
||
|
||
def user_authentication_failed(request, credentials): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add doc string in this method, check in all the methods
capture_failed_login_attempt(request, credentials) | ||
|
||
if not AxesProxyHandler.is_allowed(request): | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use get_api_response
method here, to maintain the consistent format of API response
feat: cooloff time dynamic in lockout template
@@ -116,3 +116,15 @@ | |||
STATIC_ROOT = os.path.join(BASE_DIR, "static") | |||
STATIC_URL = "static/" | |||
STATICFILES_DIRS += [os.path.join(BASE_DIR, "assets")] | |||
|
|||
# Axes Lockout | |||
env = environ.Env( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to initialize it over here, it's initialized above (line no 9)
you can just update your default values in it
@@ -1,6 +1,7 @@ | |||
import sys | |||
import os | |||
|
|||
import environ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused import
No description provided.