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

softDelete fix for double 'get' call is not ideal #163

Closed
NikitaVlaznev opened this issue May 1, 2017 · 7 comments
Closed

softDelete fix for double 'get' call is not ideal #163

NikitaVlaznev opened this issue May 1, 2017 · 7 comments

Comments

@NikitaVlaznev
Copy link
Contributor

NikitaVlaznev commented May 1, 2017

softDelete fix #160 have one more issue.

Currently it uses internal users.get result as final result. It's ok except that internal calls are made without hook.params.provider set, making some other hooks to be skipped, like restrictToRoles.

Thus, if restrictToRoles hook is registered after softDelete it will not be called at all, it is a security issue.

Somehow softDelete should call users.get like an external service, could someone give an advice about how to do it?

I see that it can be fixed by adding softDelete as after get hook, and by making softDelete before get hook just pass through with just setting some flag like $disableSoftDelete, but it will brake compatibility, everyone will need to add softDelete to after get hooks, may be there is a way that a hook can use to register after hook by it self?

Btw, current tests does not see that issue.

@eddyystop
Copy link
Collaborator

I believe this situation was introduced by your #160. I see 2 solutions

What are the ramifications of the first approach above? Some hooks could prevent that get from being successful.

softDelete is designed to be an all-before hook. I think its unwise to break it up into before & after hooks.

@NikitaVlaznev
Copy link
Contributor Author

NikitaVlaznev commented May 1, 2017

This service.get(id, { query: { $disableSoftDelete: true }, provider: hook.params.provider }) is exactly what i was looking for, if service methods really can accept provider then it is the solution i think.

@NikitaVlaznev
Copy link
Contributor Author

I think if some hooks can prevent that get, then it should be prevented.

What hooks can prevent it except access controlling hooks?

eddyystop added a commit that referenced this issue May 2, 2017
@eddyystop
Copy link
Collaborator

Fixed in #166 which will be published in v3.2.1.

eddyystop added a commit that referenced this issue May 2, 2017
@salttis
Copy link

salttis commented May 11, 2017

Now when you call softDelete on users service before all hook, and on client side straight after authentication you try to get the user, this will fail.
This could be fixed by including hook params user and authenticated in the service.get(id, { query: { $disableSoftDelete: true }, provider: hook.params.provider }) call.

You can try this error out at my example repo @ https://github.com/3bola/example-feathers-soft-delete-errors

@eddyystop
Copy link
Collaborator

Nice work!
@ekryski ^^^

@NikitaVlaznev
Copy link
Contributor Author

There is one more issue here, i have described it here: #188.

eddyystop added a commit that referenced this issue May 21, 2017
- #163 eliminated an unneeded `get` when doing a softDelete `get` call.
- However issues were introduced with authentication
- This is a focused fix which may be safer than the one proposed in pull #188
eddyystop added a commit that referenced this issue May 21, 2017
Resolved issues in softDelete caused by pull #163
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants