search

songkick / oauth2-provider

Simple OAuth 2.0 provider toolkit
MIT License
529 stars 148 forks source link

Move authorization find/create for client to model #33

Closed pixeltrix closed 11 years ago

pixeltrix commented 11 years ago

By defining the class method and calling it through the association Active Record automatically assigns the owner so it's not needed in grant_access!

By using attr_accessible :client it allows passing the client through the attributes hash but prevents injected params assigning client_id. The is_a? check is not strictly necessary as Active Record will blow up when trying to assign a literal to a association, it's just there to provide a more helpful error message.

It may be preferable to use create! rather than create in find_or_create_for_client as no checking is done to see whether the save was successful.

jcoglan commented 11 years ago

This turns out not to be a backward-compatible change -- it has broken some of our internal code.

I already went though a round of discussion on a previous pull request about which things ought to be accessible on Authorization. The bone of contention there was that the previous request hid everything, but then reimplemented something that looked very much like find_or_create_by as a workaround, defeating the point of the access restriction.

This looks like the same thing, only halfway between -- it only covers client, not owner. What are you trying to do with this change?

pixeltrix commented 11 years ago

What are you trying to do with this change?

Manually constructing the association by assigning the owner directly bypasses the AR association code that handles caching of associations so oauth2_authorizations doesn't have the new authorization until it is saved and the association is reloaded. By moving it to Client it automatically gets added to the loaded association and makes testing it easier. The attr_accessible :client still prevents direct access to raw attribute methods but allows passing client instances via the attributes hash.

If you don't want to do it this way it's fine - I didn't see the previous pull request, is it #12? If so this is a little different as it's really meant to be called through the owner association (e.g. owner.grant_access!(client)), whereas in #12 its called on the authorization instance.

jcoglan commented 11 years ago

I found out why it broke our app -- it fails this spec I just pushed https://github.com/songkick/oauth2-provider/commit/6f959c1427de2e622f78086de166043fc218f870

Is it possible to make this spec pass using your approach?

jcoglan commented 11 years ago

I should point out: we moved your commits into pixeltrix-patches for now, since we found out they broke back-compat on master. If you were wondering where your commits went.

pixeltrix commented 11 years ago

Is it possible to make this spec pass using your approach?

If it was just AR 3.x then you could use :inverse_of on the association but we can use an association extension to handle it for backwards compatibility - see PR #38.

If you were wondering where your commits went.

I did wonder for a second, even to the point where I thought that someone had force-pushed by accident, however I spotted that branch before posting a message.