Adapt authorizations controller to be scope agnostic, refs #23 #24

Open
wants to merge 11 commits into
from

Conversation

Projects
None yet
4 participants

BRMatt commented Nov 27, 2011

Just starting a pull request to keep track of this (not ready to merge just yet).

Currently doesn't include tests (noticed that the routing for the authorization controller is failing in routing tests so not sure if integration test is possible) or documentation (currently need to wrap the mount in a devise_scope)

  devise_scope :member do
    mount Devise::Oauth2Providable::Engine => '/oauth2'
  end

This is currently a breaking change, need to make it backwards compatible.

Contributor

wireframe commented Nov 28, 2011

+1

I'm absolutely in favor of decoupling the gem from requiring a User model. if you can add some tests to verify behavior I'll pull it in.

re: routing specs...they are failing due to a rails bug loading the routes for tests. i'm not too concerned with it at the moment, but let me know if you see any patches.

Contributor

wireframe commented Nov 28, 2011

can you apply this change to the tokens_controller as well?

BRMatt commented Nov 28, 2011

can you apply this change to the tokens_controller as well?

Hehe whoops, forgot to do them! I've done a little bit of refactoring locally and will hopefully be able to push them up at some point.
There are some other things related to this that are a bit annoying (e.g. all the relationships between models also hardcode User)

re: routing specs...they are failing due to a rails bug loading the routes for tests. i'm not too concerned with it at the moment, but let me know if you see any patches.

Ok, do you happen to know what the issue number for that bug is?

Contributor

wireframe commented Nov 29, 2011

here's a blog post that details the issues testing routes with rails engines:
http://www.builtfromsource.com/2011/09/21/testing-routes-with-rails-3-1-engines

and here is the monkey patch i've applied to the engine routing specs:
https://github.com/socialcast/devise_oauth2_providable/blob/master/spec/support/inject_engine_routes_into_application.rb

BRMatt commented Dec 4, 2011

I've been running into some problems with routing etc., just thinking through options and was wondering if you had anything against dropping the rails engine mounting and using a devise-esque routing helper such as devise_oauth_for?

For a start it'd make it easier to view routes with the rake routes task and should solve the problems with spec routing not working. It'd also make it easier to use more than one oauth endpoint (e.g. if you have two different devise scopes) and if you need to customise behaviour (e.g. in our app only a few private apps are allowed, so if the client's credentials are valid we don't want to ask the user to authorize the app)

BRMatt added some commits Dec 4, 2011

A lot of breaking changes to routing, refs #23
This commit removes the rails engine routes and introduces a
devise_oauth_for helper which constructs the necessary routes.

Benefits of this change:

* Can now have more than one oauth endpoint per app
* Can now safely override controllers
* All current tests are now passing!

Although there are tests for some aspects of the new routing system,
they're not 100% comprehensive and don't cover the authenticate_scope! method

BRMatt commented Dec 4, 2011

Ok, the middle commit basically rewrote the routes to be initialized in the same way as devise's, using devise_oauth_for.

Still not quite decoupled the User model, that's the next thing I'm going to hit.

BRMatt added some commits Dec 4, 2011

Fixing bug with AR belongs_to type mismatch
The bug in question was that AR was complaining about it had received
an object of type Client when it expected one of type Client.

Essentially this bug is caused when the Client model is loaded at some point in execution
then reloaded later on, redefining the class with the same name, but different #object_id

In this instance the Client class was being loaded via the routes.rb file when
`devise_oauth_for` created a mapping object which constantized all the model names,
causing Rails to load them all.

Solution is to only constantize them on demand. It also makes the syntax more managable.

Refs #23

Is this something that is still being worked on. This seemed like a promissing change for someone who doesn't use a User model...

BRMatt commented Apr 13, 2012

Everything started off fairly simply, but I ended up rewriting a fair amount of this gem's "core code" (mainly so that it could be tested without throwing off a few hundred errors) so I doubt it'll get merged in.

We're currently using our semi-rewrite in production, the modified version of the gem is available in jestro/devise_oauth2_providable (see jestro/master and 23-scope), however we haven't [yet] backported recent changes to this gem.

I see, thanks for the heads-up. I'll go check out your fork and see if I can get it to work.

update: I've ported over some easy-to-understand commits to your version of the gem, but I haven't tested this yet. Other changes/commits where a bit larger in scope and seemed to change things you already modified in your fork. I hope we can get this ball rolling again and get the different forks merged together in the near future.

@BRMatt are you still interested in getting this merged in? If so can you please re-pull master and I'll take a look.

BRMatt commented Feb 6, 2013

It's been a long time since I looked at this code, so it will probably require a cleanup in some areas before it can be merged. We also moved the repository a few months ago, so unfortunately this PR is out of date.

Unfortunately life's a bit busy atm, so it may be a few weeks before I can get time to check it.

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