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

Protect asset create endpoint #263

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jd2rogers2
Copy link
Contributor

Dev Summary

only a user that is part of an org should be able to create an asset/post
to do this i added a new auth guard and used it on the endpoint

still to do -> fix the test that's breaking locally

(later to come, checking that updating/deleting a post can only be done by user within poster's org)

Test Plan

repro steps:

for these tests you can see that i have the seeded users and orgs, but i also inserted into user_orgs userId, organizationsId values 1, 1 to create a relationship between my user 1 and the first org
Screen Shot 2023-01-04 at 7 04 03 PM

  1. in browser login with user 1 and get cookie from dev tools
  2. make curl request with user 1's cookie. expect success (because they have an org related)
curl -X POST localhost:3001/api/assets \
--cookie "NEH_is_cool=s%3AeyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6MSwiZmlyc3ROYW1lIjoidXNlcjFGaXJzdCIsImxhc3RfbmFtZSI6InVzZXIxTGFzdCIsImVtYWlsIjoidXNlcjFGaXJzdEBleGFtcGxlLmNvbSIsImVtYWlsX25vdGlmaWNhdGlvbl9vcHRfb3V0IjpmYWxzZSwiaWF0IjoxNjcyODg2NTk4LCJleHAiOjE2NzI4OTAxOTh9._QCbE3mJWcKa72l6vnCEO1hCrtgR2cwzm8CdyzuOjTE.FdOBENq0wBAtBcXENuEwGX7X4Wm2ctqlajkL1enxP64" \
-H "Content-Type: application/json" \
-d '{ "title": "title", "description": "description", "quantity": 1 }'
  1. repeat 1 and 2 with other user (with no org). expect fail
--cookie "NEH_is_cool=s%3AeyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6MiwiZmlyc3ROYW1lIjoidXNlcjJGaXJzdCIsImxhc3RfbmFtZSI6InVzZXIyTGFzdCIsImVtYWlsIjoidXNlcjJGaXJzdEBleGFtcGxlLmNvbSIsImVtYWlsX25vdGlmaWNhdGlvbl9vcHRfb3V0IjpmYWxzZSwiaWF0IjoxNjcyODg4NjM5LCJleHAiOjE2NzI4OTIyMzl9.bqtpcaXXf3Eg1plPHBsBtjOmpdpDY-nvV7jHZKwe-7Y.MVgCtcVqxDUkxSpBhGlUQVtQEWRte%2FQjpa8ijLGxZ6k" \
-H "Content-Type: application/json" \
-d '{ "title": "title", "description": "description", "quantity": 1 }'

Resources

@jd2rogers2 jd2rogers2 linked an issue Jan 5, 2023 that may be closed by this pull request
@jd2rogers2 jd2rogers2 self-assigned this Jan 5, 2023
@esteban-gs
Copy link
Contributor

My changes in #264 throws a wrench in your work. Happy to pair on merge conflicts

Copy link
Contributor

@skyfox93 skyfox93 left a comment

Choose a reason for hiding this comment

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

Aren't assets with that are "offers"/ "donations" created by users without orgs? It's only the "request" asset type that needs this guard.

@@ -59,6 +59,17 @@ export class UserOrganizationsService {
return userOrg;
}

async getAllByUserId(userId: number): Promise<UserOrganization[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder that relations do this for us. I.e, we already have a method on the user entity, user.organizations, that does the same thing as this method.

@esteban-gs
Copy link
Contributor

Aren't assets with that are "offers"/ "donations" created by users without orgs? It's only the "request" asset type that needs this guard.

This sounds right.

@esteban-gs esteban-gs added the documentation Improvements or additions to documentation label Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add auth/checks to nonprofit posting endpoints
3 participants