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

Created API endpoint for GET request (list view) for USERS with proper JWT authentication #77

Open
wants to merge 42 commits into
base: dev
Choose a base branch
from

Conversation

Esterello2
Copy link

@Esterello2 Esterello2 commented Oct 30, 2023

Modified views.py, urls.py, and serialization.py to establish endpoint for GET User (List View) with JWT authentication. This included adding the simpleJWT module from rest framework, along with JWT token built-in API views to allow the for the generation and refresh of tokens.
A POST User request API endpoint was also created here, which was also originally meant to deal with JWT authentication, however, team decided to make it an open endpoint.

Specifies how user data should be serialized
added UserCreateView to allow for the POST of users
added a view to deal with creating a new User
Esterello2 and others added 3 commits November 6, 2023 12:00
Added a check for authenticated users only to be able to send GET, POST, and PATCH requests for User info.
@Esterello2 Esterello2 marked this pull request as draft November 13, 2023 06:42
@Esterello2 Esterello2 changed the title Apis/post users Created API endpoint for POST requests for users with proper authentication method Nov 13, 2023
@Esterello2 Esterello2 marked this pull request as ready for review November 13, 2023 07:03
@Esterello2
Copy link
Author

Updated the PR by deleting the old files from the "users" directory, and adding the serializers, views, urls, and permissions files in a directory called API.

@Esterello2
Copy link
Author

Made a big update here, I added simple JWT authentication by importing the simpleJWT module from the rest_framework app. I incorporated three new views that deal with generating, refreshing, and verifying JWT tokens to properly authenticate API requests for POST and GET for User info.

@majorsylvie majorsylvie changed the title Created API endpoint for POST requests for users with proper authentication method Created API endpoint for POST requests for users with proper JWT authentication Nov 27, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason you check for unauthentication as opposed to authentication?

Copy link
Author

@Esterello2 Esterello2 Dec 1, 2023

Choose a reason for hiding this comment

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

Thanks for pointing this out, it was originally done like this because the only POST requests for Users I had in mind were when new users were creating accounts. I will remove this and update with the new implementation we agreed on that only admin users can create new users through API request.

Copy link
Contributor

@cuyakonwu cuyakonwu left a comment

Choose a reason for hiding this comment

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

Looks good generally! Please try to get back to the comments I made on the code though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Glad you condensed the user serializers! Any reason for the notification serializer in this PR specifically?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing this out, I was actually testing this out by making a serializer for the Notification model, but I will actually look to remove this now to keep the PR organized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave the comments in, it helps with organization.

@Esterello2
Copy link
Author

Added proper JWT authentication to a POST User API endpoint that allows a successful POST only if a valid JWT token is provided. Also, incorporated in the User List View all to ensure only Authenticated entities can see all users info. Deleted the extraneous code Conrad mentioned.

@Esterello2 Esterello2 changed the title Created API endpoint for POST requests for users with proper JWT authentication Created API endpoint for POST requests for users and GET list view for USERS with proper JWT authentication Dec 1, 2023
@Esterello2
Copy link
Author

Made a final edit after consulting team to take off JWT Authentication method for POST new users, and only kept it for ListView of Users.

@Esterello2 Esterello2 changed the title Created API endpoint for POST requests for users and GET list view for USERS with proper JWT authentication Created API endpoint for GET request (list view) for USERS with proper JWT authentication Dec 6, 2023
@Esterello2
Copy link
Author

A quick overview of using Postman to test JWT authentication on the GET User listview API endpoint:

  1. To generate a token, you have to go to the api/token/ url endpoint in the DRF GUI endpoint.
  2. Enter superuser credentials (email: esteban@test.com pass: Estetheyan)
  3. It will return two token strings, one that says refresh and other that says accept
  4. Copy the "accept" one, and then this is the part that so far can only be done on Postman interface
  5. Postman lets you send a GET request to a protected endpoint with the token you copied.
  6. Enter the appropriate URL (~/api/users/)
  7. In the Auth tab in the Postman interface, select the dropdown under type that says "Bearer Token", and paste token string into the "Token" field on the right.
  8. Ensuring "GET" is selected as Request type, select Send.
jwt-postman.snip.mp4

Copy link
Contributor

@majorsylvie majorsylvie left a comment

Choose a reason for hiding this comment

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

I can successfully NOT access /api/users when I don't use JWT authentication.

Yet I can still POST users (as you've said the team has decided to allow)

Then I can successfully view the api/users when GET requesting the endpoint with a proper JWT token, as generated from api/token (and verifiable as a successful token by visiting api/token/verify and POSTing the access token I got from api/token.

There are multiple comments about extraneous code. Please resolve then we can merge your actual functionality!

src/chigame/api/urls.py Outdated Show resolved Hide resolved
# LOBBY API URLS
path("tournaments/create/", views.TournamentCreateView.as_view(), name="create-tournament"),
path("tournaments/", views.TournamentListView.as_view(), name="tournament-list"),
path("register/", views.UserRegistrationView.as_view(), name="user-registration"),
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of this register endpoint?
Upon trying to access it it, it doesn't allow get.
You don't mention it in your JWT walkthrough, nor have any code comments explaining this.
Please either remove this URL, or explain and justify why this endpoint should be in this PR!

Copy link
Author

Choose a reason for hiding this comment

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

The register endpoint is the POST new user endpoint that does not have JWT, should I name it something more clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please!

Copy link
Contributor

Choose a reason for hiding this comment

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

could also be api/usersregister`

src/chigame/api/views.py Outdated Show resolved Hide resolved
@majorsylvie majorsylvie self-requested a review December 7, 2023 20:23
Copy link
Contributor

@majorsylvie majorsylvie left a comment

Choose a reason for hiding this comment

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

Seeing that you removed the tournament code from this PR, this is almost good to merge! Please add a code comment and make the register endpoint named more clearly.

Other than that, given the JWT GET users functionality is working as it's supposed to, this will be good to merge!

# LOBBY API URLS
path("tournaments/create/", views.TournamentCreateView.as_view(), name="create-tournament"),
path("tournaments/", views.TournamentListView.as_view(), name="tournament-list"),
path("register/", views.UserRegistrationView.as_view(), name="user-registration"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please!

# LOBBY API URLS
path("tournaments/create/", views.TournamentCreateView.as_view(), name="create-tournament"),
path("tournaments/", views.TournamentListView.as_view(), name="tournament-list"),
path("register/", views.UserRegistrationView.as_view(), name="user-registration"),
Copy link
Contributor

Choose a reason for hiding this comment

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

could also be api/usersregister`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants