Conversation
dburkhart07
changed the title
maxn990
left a comment
maxn990
left a comment
There was a problem hiding this comment.
looks really good over all! i left a few comments, but love the general approach :he-is-brighter:
| .findOrderFoodRequest(entityId); | ||
|
|
||
| if (!request) { | ||
| console.log('Request not found on order'); |
There was a problem hiding this comment.
there are a handful of console.logs, are these necessary? and if so, might be better to replace them with proper logging
|
|
||
| Finally, run `yarn run typeorm:migrate` to load all the tables into your database. If everything is set up correctly, you should see "Migration ... has been executed successfully." in the terminal. | ||
|
|
||
| # AWS Setup |
| path: '/landing-page', | ||
| element: ( | ||
| <Authenticator components={components}> | ||
| <LandingPage /> | ||
| </Authenticator> | ||
| ), | ||
| }, | ||
| { | ||
| path: '/pantry-overview', | ||
| element: ( | ||
| <Authenticator components={components}> | ||
| <PantryOverview /> | ||
| </Authenticator> | ||
| ), | ||
| }, | ||
| { | ||
| path: '/pantry-dashboard/:pantryId', | ||
| element: ( | ||
| <Authenticator components={components}> |
There was a problem hiding this comment.
i don't think we need to have authenticator wrapped around every route, here's the react documentation on how to do this with significantly less repeat code: https://reactrouter.com/start/declarative/routing
| // useEffect(() => { | ||
| // document.title = 'SSF'; | ||
| // apiClient.getHello().then((res) => console.log(res)); | ||
| // }, []); |
|
|
||
| async validate(payload) { | ||
| return { idUser: payload.sub, email: payload.email }; | ||
| const dbUser = await this.usersService.findUserByCognitoId(payload.sub); |
There was a problem hiding this comment.
since find user by cognito id throws an exception i think we should add some sort of error handling here
| if (error.response?.status === 403) { | ||
| // TODO: For a future ticket, figure out a better method than renavigation on failure (or a better place to check than in the api requests) | ||
| window.location.replace('/unauthorized'); | ||
| } |
There was a problem hiding this comment.
react router has navigate() instead of replace, that might be a better solution. i'm also wondering if we should use 401 instead of 403 for this
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE users | ||
| ADD COLUMN IF NOT EXISTS user_cognito_sub VARCHAR(255) NOT NULL DEFAULT '';`, |
There was a problem hiding this comment.
existing users won't have cognito ids, will you add those manually?
There was a problem hiding this comment.
right now we do. i assumed this would be fine, as in production, a user would never be created without their cognito id there. this should be ensured when we go into creating user logic in a later ticket (i believe we have this planned)
3 participants
ℹ️ Issue
Closes #117
📝 Description
For this PR, we added in user-specific gated authentication to a few endpoints. The initial problem that required this user gate was that we need to restrict certain endpoints that incorporate the id parameter to specific users (for example, a pantry with the id of 3 should not really be able to do anything involving pantries with ids of 2).
To implement this, we are now marking all endpoints with a guard that allows us to pass in a lambda function. This lambda function uses services within the controller to define how we should go about getting the desired id to compare to the id of the currently signed in user (if they are not the same, access should be denied).
We implement a guard with a decorator, just as we did with the role-based authentication. For the decorator, we created 3 things:
moduleReffrom NestJs to get the appropriate service class so we can actually execute the lambda functions. We store them in a cache so we do not need to keep getting them each time,@CheckOwnershipdecorator. It contains the name of the id for us to use in the parameter, as well as the lambda function that will be resolved.For the guard, we use each attribute from the decorator to run the following set of logic steps:
✔️ Verification
For testing, I looked into 2 specific cases. One thing we want to make sure with this PR is that it can handle several services being used in verification (for example with orders, perhaps we are given an order id, and to verify it we need to get the corresponding pantry id, which allows us to get the corresponding pantry which allows us to get the pantry user id which we can check with the signed in user).
For this, I added lambda function guards to an endpoint in the pantries controller (the one called in the request-form page), and one in the orders controller (the one called when we open up the order modal on the admin donation board). I tested both of these pages with and without the corresponding user id being my signed in user. For both of these, I was able to verify that I was kicked out when I wasn't authorized, and allowed in otherwise.
Both these tests ultimately ended up using the
pantry_user_idfield, but just got to it in different ways.🏕️ (Optional) Future Work / Notes
This PR is primarily for infrastructure, and we will later need to adjust all of the endpoints accordingly when we decide who we want to give this endpoint access to.