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

Added authentication endpoints #72

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Added authentication endpoints #72

wants to merge 3 commits into from

Conversation

maunga-et
Copy link
Collaborator

No description provided.

…g Python, so its impossible to build C extensions afterwards, wcwidth==0.2.6 and cwcwidth==0.1.8 in this case
@terrameijar
Copy link
Collaborator

@maunga-et thank you for the work on this, I'll test out the changes today. Before, I review, could you please resolve the failing flake8 checks?

I recommend running a tool like flake8 or black automatically to format and lint your code when you save files and before creating PR, that way the code we write will be consistent and inline with PEP 8 standard.

@maunga-et
Copy link
Collaborator Author

@maunga-et thank you for the work on this, I'll test out the changes today. Before, I review, could you please resolve the failing flake8 checks?

I recommend running a tool like flake8 or black automatically to format and lint your code when you save files and before creating PR, that way the code we write will be consistent and inline with PEP 8 standard.

I will do that thanks.

httponly=settings.SIMPLE_JWT["AUTH_COOKIE_HTTP_ONLY"],
samesite=settings.SIMPLE_JWT["AUTH_COOKIE_SAMESITE"],
max_age=823396,
# domain='example.com'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove these unnecessary comments. We can add these back in later if we need them

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another point, I just thought of @maunga-et . Let's make the access token valid for 10 mins and then, once we have a refresh token, we can set it to be valid for 2 weeks.

This will allow users to keep logged in without having to reauthenticate as long as they are still using the site.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I am on it thanks.

response.set_cookie(
key=settings.SIMPLE_JWT["AUTH_COOKIE"],
max_age=0,
# domain='example.com',
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maunga-et Same as above, we can remove this comment.

@@ -25,4 +25,5 @@
path("api/posts/<int:pk>/", views.PostDetail.as_view(), name="post-detail"),
path("api/posts/delete/<int:pk>/", views.PostDelete.as_view(), name="post-delete"),
path("api/signup", views.SignUpView.as_view(), name="signup-view"),
path("api/v2/auth/", include("authentication.urls")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maunga-et let's not change the api version yet. I recommend we name the JWT endpoints something like api/token/. Then we can have endpoints like:

  • api/token/ for generating a token keypair or login
  • api/token/refresh/ to generate new refresh tokens and
  • api/token/logout to invalidate & blacklist tokens and then log the user out.

Copy link
Collaborator

@terrameijar terrameijar left a comment

Choose a reason for hiding this comment

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

@maunga-et, great work on this!

My ask is that you make the requested changes to lint the files, change the urlpattern and remove the unnecessary comments. Once we have that in place, this PR will be good to merge.

We can improve on the auth stuff by adding token refresh, logout and views to blacklist invalid tokens. @TaqsBlaze has started that work in #74

@maunga-et maunga-et closed this Nov 8, 2024
@terrameijar
Copy link
Collaborator

Hey @maunga-et, did you close this PR in error?

@terrameijar terrameijar reopened this Nov 11, 2024
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.

2 participants