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

Failing spec for garnered_find not respecting chained criteria #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mzikherman
Copy link
Contributor

Ahoy there!

Here is a failing spec that shows garnered_find not respecting chained criteria.

I would expect @monger_who_is_a_thug.cheeses.garnered_find("cheddar") to return nothing, since cheddar is not one of this monger's cheeses.

I'm realizing in our API, we do build up scopes/chain criteria and then call garnered_find at the end on some object. This means that someone can make an API call for something they are authorized for, but pass in an object id for something else and get it. Something like partner.artworks.garnered_find(id) means that if you pass in the id of any artwork, as long as you have access to the given partner, you'll be able to get the details of a completely different private work.

Let me know if you think this is something we should fix in Garner, I'll try it out. I am thinking that the criteria used for the lookup needs to be added to the identity.

@fancyremarker
Copy link
Contributor

Yeah, this was definitely never intended to work. 🍭

If you want to try to fix it, you'd be a mensch. However, I think that would be a major challenge, and (at least in the way I'm thinking about it) would greatly complicate some of the core object code.

@dblock dblock changed the title @fancyremarker => Failing spec for garnered_find not respecting chained criteria Failing spec for garnered_find not respecting chained criteria Jul 16, 2014
@dblock
Copy link
Contributor

dblock commented Jul 17, 2014

This is a very real problem. Sounds like @fancyremarker is challenging @mzikherman to fixing this. I say you show him!

@sweir27 sweir27 closed this Oct 28, 2015
@mzikherman mzikherman reopened this Oct 28, 2015
@mzikherman
Copy link
Contributor Author

Damn you GitHub. I PR'ed something in Gravity that says 'fixes' and linked to this issue (since we were chaining something I had noticed). That automagically closed this, which in fact remains a real issue.

Though I guess it's better off as an actual issue and not a PR.

@dblock
Copy link
Contributor

dblock commented Oct 28, 2015

Thx @mzikherman, I reopened this PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants