search

ubc-biztech / serverless-biztechapp

biztech's backend. AWS lambda, cognito, dynamodb, written in Node js
2 stars 1 forks source link

restrict users from updating admin in the USER patch endpoint #307

Closed jerryxu99 closed 6 months ago

jerryxu99 commented 8 months ago

🎟️ Ticket(s): Closes # https://www.notion.so/ubcbiztech/b24732925a6745caa305a2c746fa5ba7?v=d557178d0a134acf85a3890882538364&p=dd9baf94e6c6405281ca822b4b45fb13&pm=s

👷 Changes: A brief summary of what changes were introduced.

Temp soln to the backend security. Problem: anyone is able to access our backend. The most dangerous is if someone changes their user.admin = True. The broader problem is that all our endpoints can be hit using tools like Postman... This makes it so that people can infiltrate our db with injections of data into all our tables.

This pr just addresses the simple problem of ppl setting admin = True.

💭 Notes: Any additional things to take into consideration.

Wait! Before you merge, have you checked the following:

jerryxu99 commented 8 months ago

should we make it so it just fails to update the admin user props, but succeeds the other props, or just error out the operation entirely. Im worried some update data might just include the entire user object, which would have admin in it as well

i was thinking abt this and as of now, no calls put admin in the object. Sample call:

image

However if in the future we add more props to IMMUTABLE_USER_PROPS then we'd have to be careful abt adding a property to this list that is passed in to existing calls on the frontend. I'll add a comment for this to the code

I think the benefit of not allowing the call to go through is that it would make debugging more difficult if we were to make a request to an endpoint, it says 200 OK, yet it doesn't do what you expected it to, as it didn't update some field. Lmk what u think, I'm fine with filtering out IMMUTABLE_USER_PROPS from the body as you said too.