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

nsc-events-nestjs_Sprint-1_Restrict-Access-to-User-Routes_34 #35

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

Exochos
Copy link
Contributor

@Exochos Exochos commented Oct 26, 2023

Since all routes can be guarded from a top level authGuard I added the imports and then the authguard on line 20, since we also already have a jwt.strategy written I just used that for the auth. All of this trickles down to all of the below user routes. Proceeded to deleted un-needed authguards.
EDIT, added tests for these routes.
Closes #34

Here are tests,

  1. use the signup route and use the token retrieved to login to the next routes.
    1_signup

  2. Attempt to create a user without auth.
    2_createuser_without_auth

  3. Create a user with auth.
    3_create_user_with_auth

  4. Attempt to update a user without auth.
    4__2_update_user_withoutAuth

  5. Attempt to update a user with auth token.
    4_update_user

  6. Attempt to delete a user without Auth.
    5__1_delete_user_withoutAuth

  7. Attempt to delete a user with auth.
    5__2_delete_user_withAuth

Since all routes can be guarded from a top level authGuard which trickles down to all of the below user routes. Deleted un-needed authguards.
@Exochos Exochos added the enhancement New feature or request label Oct 26, 2023
@Exochos Exochos self-assigned this Oct 26, 2023
@Exochos Exochos changed the title nsc-events-nestjs_Sprint-5_Restrict-Access-to-User-Routes_30 nsc-events-nestjs_Sprint-5_Restrict-Access-to-User-Routes_34 Oct 28, 2023
@Exochos Exochos marked this pull request as ready for review October 28, 2023 00:02
Copy link
Contributor

@theGaryLarson theGaryLarson left a comment

Choose a reason for hiding this comment

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

Hi Jeremy, the code looks good but CI/CD is failing because the test is not implemented. Please uncomment and update the test. Review the activity controller test if needed.

Added Jest Mock testing for the user routes.
Added Jest Mock Tests
@Exochos
Copy link
Contributor Author

Exochos commented Oct 31, 2023

Hi Jeremy, the code looks good but CI/CD is failing because the test is not implemented. Please uncomment and update the test. Review the activity controller test if needed.

I have uploaded a Jest mock test which should hopefully fix this issue.

@Exochos Exochos requested a review from theGaryLarson November 1, 2023 00:03
@taylorpapke
Copy link
Contributor

@theGaryLarson looks like there was an update to this PR. Are the changes requested now satisfied?

Copy link
Contributor

@taylorpapke taylorpapke left a comment

Choose a reason for hiding this comment

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

Awesome! I ran through all of your screenshots and was able to get the same results. Pretty cool! So it seems like this pretty much gives admins control over user creation, updates and deletes while guarding these actions from other users.

Copy link
Contributor

@Robel-003 Robel-003 left a comment

Choose a reason for hiding this comment

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

This looks great, and thank you for providing some pictures!
I'm able to create/update/delete an account using the proper endpoints.
This is minor and can be added later but I'd recommend providing a message for the update and delete endpoints (primarily for the admin/creator)

@Exochos
Copy link
Contributor Author

Exochos commented Nov 7, 2023

This looks great, and thank you for providing some pictures! I'm able to create/update/delete an account using the proper endpoints. This is minor and can be added later but I'd recommend providing a message for the update and delete endpoints (primarily for the admin/creator)

Yeah I didn't know what best practice was in this case to I just left it with the response 200 code.

@brinkbrink
Copy link
Contributor

Looks like some checks still aren't going through with the CI/CD. I just re-ran the tests in case something got hung up (it happens sometimes) but that wasn't the case. Maybe troubleshooting this with @theGaryLarson tonight would be a good idea.

@Seiyaroo Seiyaroo closed this Nov 7, 2023
@Seiyaroo Seiyaroo reopened this Nov 7, 2023
@Seiyaroo
Copy link
Contributor

Seiyaroo commented Nov 7, 2023

Reopened and fixed the checks. Just need to confirm that @theGaryLarson is satisfied per the pending change request.

@taylorpapke taylorpapke dismissed theGaryLarson’s stale review November 8, 2023 05:47

Merging PR as per BC's instructions. Will need a re-review by lead dev.

@taylorpapke taylorpapke merged commit a7e63f6 into main Nov 8, 2023
@taylorpapke taylorpapke deleted the Restrict-Access-to-User-Routes-#34 branch November 8, 2023 05:48
@taylorpapke taylorpapke changed the title nsc-events-nestjs_Sprint-5_Restrict-Access-to-User-Routes_34 nsc-events-nestjs_Sprint-1_Restrict-Access-to-User-Routes_34 Nov 16, 2023
@Exochos Exochos restored the Restrict-Access-to-User-Routes-#34 branch November 30, 2023 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Sprint 1
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Restrict Access to User Routes
6 participants