-
Notifications
You must be signed in to change notification settings - Fork 49
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
Allow using auth filters on creation #321
base: master
Are you sure you want to change the base?
Conversation
if (opts?.filter && !applyFilter(record as Entity, opts.filter)) { | ||
throw new Error('Entity does not meet creation constraints') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just doing some type casting here to make this happy but I think raises a good question. What should be the behaviour for a filter like createdAt > "last_year"
when the createdAt is populated by the db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something we could do, but that would make it all a lot more complex is by doing inside a TypeORM query runner, doing the insert, getting the record, apply filter (can even then be done with the normal builder), if failed revert and throw error.
Otherwise I think this would also need to be documented as a limitation. (Preferred)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my preference too.
if (opts?.filter && !applyFilter(entity, opts.filter)) { | ||
throw new Error('Entity does not meet creation constraints') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'm thinking we should remove this check and only pass it down to the service, so that, when users want to they can use it there, main reason being: I would not want the service even beign called when the user does not have the right access (that's why on our API's we are using the before create).
This would also make it a breaking change so also not really in favor of that.
Would it work for you if we do pass it down (as that is a good idea to do!) but do not check the filter/throw a error? Especially as applyFilter
would not work with possible relations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be ignored as it will become opt-in, would be good to document on the flag that it could/would not work with relation filters though.
Just now saw the open questions, sorry. Open Questions:
Would indeed say the resolver options (better naming is welcome):
Agree, where it is now is indeed the best. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 4d0e472. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 4 targetsSent with 💌 from NxCloud. |
Following up on conversation here: #320
When getting started with nestjs-query I found it un-intuitive that my authorizers would be executed during the creation flow, but then not evaluated in any way to prevent creation of resources that didn't match my auth criteria.
This PR allows users to opt in to having their authorization filters used when hitting creation end points.
Open Questions:
opt-in
configuration be done?-- My feeling is in the resolver options
-- The original implementation I had done implemented this in the
typeorm-query-service
however, given that we are evaluating the filters inJS
we could move it to the resolver. My issue with that is I don't think this works very well with the assembler paradigm if you're CreateDTO is significantly different from core DTO/entity@TriPSs I'll start on test coverage once we're happy with the overall direction.