feat(permissions): Restrict user and profile details based on allowed fields #196
Conversation
| if (modelName === "api::profile.profile" && action === 'findOne' && response?.data?.attributes?.user?.data?.id === user.id) { | ||
| return response; | ||
| } | ||
| return this.findAndRestrictUserDetails(response, modelName); |
There was a problem hiding this comment.
Main algorithm here: take the response and introspect all attributes, if any attribute is a relation, we take that object and introspect its attributes. When relation points to user or profile, we replace that object with shrunk version, including only allowed fields.
| if (modelName === "user" && action === 'findOne' && response?.id === user.id) { | ||
| return response; | ||
| } | ||
| if (modelName === "api::profile.profile" && action === 'findOne' && response?.data?.attributes?.user?.data?.id === user.id) { |
There was a problem hiding this comment.
If profile was requested with additionally populated user, we can compare it with current user and return full profile, if current user is an owner of the profile. I noticed, that frontend doesn't call profiles API directly, maybe we can skip this check and always return limited version of the profile data?
There was a problem hiding this comment.
If the profile API is not called directly, should we remove the router?
There was a problem hiding this comment.
Probably, yes. I will add this to the list proposed permissions here https://docs.google.com/spreadsheets/d/1de3jpktjxFUdEUbmPmtizclECpFfak7Y_1GC3j-QfH0/edit?gid=1553006189#gid=1553006189
| if (modelName === "user") { | ||
| return this.restrictNonWrappedItem(item, this.config.allowedUserFields); | ||
| } | ||
| if (modelName === "plugin::users-permissions.user") { |
There was a problem hiding this comment.
When will we see this modelName instead of just user?
There was a problem hiding this comment.
When we call /api/users/1 we will get handler user.findOne, but let say we call api/idea-cards/1?populate=ideaOwner, that ideaOwner will be a relation type and will target not user but plugin::users-permissions.user. Other modelNames are more consistent api::profile.profile is the same in both handler and relation target type.
| const idx = handler.lastIndexOf("."); | ||
| const modelName = handler.substring(0, idx); | ||
| const action = handler.substring(idx + 1); | ||
| const metadata = strapi.db.metadata; |
There was a problem hiding this comment.
What are examples of metadata?
There was a problem hiding this comment.
Metadata is a Map-alike object where key is a modelName and value is MetaModel, which contains almost all the same info you can find under content-types/schema.json See https://github.com/strapi/strapi/blob/develop/packages/core/database/src/types/index.ts#L216 and https://github.com/strapi/strapi/blob/develop/packages/core/database/src/metadata/metadata.ts
becevka
requested review from
oriyomibadmus,
j-peace,
nikita-pardeshi-github,
dbradham and
chungthuang
| return restrictedItem; | ||
| } | ||
|
|
||
| restrictNonWrappedItem(item, allowedFields) { |
There was a problem hiding this comment.
Hey Wolfgang, the code looks really good. So a short refact comment about this conditional.
You are checking item[field] twice. Is there a way to check it just once and then check field === "profile" ?
|
@chungthuang just to I keep in touch, why isn't this pr merged yet? |
|
@nikita-pardeshi-github @chungthuang @tiagopazhs @oriyomibadmus is anyone here familiar with the file that has a conflict? I'd like to get this merged, as Yacine has fixed some breaking changes on the frontend side, so we can now merge and deploy this change. |
this was going to break some frontend interfaces that exposed PII (personally identifiable information) so we were waiting until those got updated. |
chungthuang
force-pushed
the
becevka/179-disable-user-details
branch
from
efdfe9e
to
1b51b68
Compare
| await next(); | ||
| try { | ||
| // Only need to filter if there is a response body | ||
| if (ctx.request.method === "GET" && ctx.request.url.includes("/api/") && ctx.response.body) { |
There was a problem hiding this comment.
@becevka I added a check for non empty ctx.response.body, otherwise https://github.com/dev-launchers/strapiv4/pull/196/files#diff-020f47be16701da9ac1d16017015d4d6d586e314e61850aa215c4c801450563eR27 will set it to an empty body and change the status code from 404 to 204.
chungthuang
force-pushed
the
becevka/179-disable-user-details
branch
from
1b51b68
to
75fd97c
Compare
|
Addresses #179
Please ignore commits from already squashed branch, I needed those while was waiting on PR approval.
Any suggestions how we can simplify this middleware are welcome.