Custom authentication #1512

Closed
wants to merge 303 commits into
from

Conversation

Projects
None yet
@radar
Member

radar commented May 8, 2012

Another month, another large pull request. Not content with ripping out pieces from the components of Spree, I decided to rip out an entire component. Particularly, the auth component.

The reason for this was because attempting to use Spree in application that already had authentication proved troublesome. By ripping this component out and adding in hooks (Spree.user_class and Spree.current_user_method) to core for user things, we can allow for people to embed Spree into their applications and to use their application's authentication setup, rather than forcing them to use Devise.

I am about to start working on a guide to document how you would go about doing this, and hopefully that would make it more clear how you could integrate Spree and the custom authentication from an application.

Consider this the first draft for the custom authentication solution.

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen May 8, 2012

Contributor

YEEEEEEEEEEES! Epic! This is one huuge step!
❤️ @radar

Contributor

tvdeyen commented May 8, 2012

YEEEEEEEEEEES! Epic! This is one huuge step!
❤️ @radar

@Spaceghost

This comment has been minimized.

Show comment
Hide comment
@Spaceghost

Spaceghost May 8, 2012

@radar You are a gentleman and a scoundrel. I salute you.

@radar You are a gentleman and a scoundrel. I salute you.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ May 8, 2012

Member

This looks great, I love pull requests that delete 23 times more code than they add!

There's a couple of points we need to address:

  1. For people who don't have an existing application, and just want a "store", will still need to provide all the basics (like signup, login, password reset, etc). I think we need an external extension that provides this, and the spree installer should prompt to install or not (default to yes).

  2. How do we handle migrations for existing stores? I've been thinking we should start to include an upgrade generator with each release that can make some of the necessary changes, in this case install the external auth extension.

Member

BDQ commented May 8, 2012

This looks great, I love pull requests that delete 23 times more code than they add!

There's a couple of points we need to address:

  1. For people who don't have an existing application, and just want a "store", will still need to provide all the basics (like signup, login, password reset, etc). I think we need an external extension that provides this, and the spree installer should prompt to install or not (default to yes).

  2. How do we handle migrations for existing stores? I've been thinking we should start to include an upgrade generator with each release that can make some of the necessary changes, in this case install the external auth extension.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 8, 2012

Member

@BDQ:

For people who just want a store and the basics, there's radar/spree_auth_devise for that. We could make that an official extension as you suggest and prompt to install it or not. There's some failing tests on that project that I've not gotten around to fixing just yet.

For existing stores, all they'll need to do is use the spree_auth_devise gem. It will work as it did before. At least, that is the intention.

Member

radar commented May 8, 2012

@BDQ:

For people who just want a store and the basics, there's radar/spree_auth_devise for that. We could make that an official extension as you suggest and prompt to install it or not. There's some failing tests on that project that I've not gotten around to fixing just yet.

For existing stores, all they'll need to do is use the spree_auth_devise gem. It will work as it did before. At least, that is the intention.

@jipiboily

This comment has been minimized.

Show comment
Hide comment
@jipiboily

jipiboily May 8, 2012

Love you @radar.

@BDQ: what happened to your #2? 1 and 3 huh? :)

Point 3 will be quite important to manage in some way. +1 for the upgrade generator.

Love you @radar.

@BDQ: what happened to your #2? 1 and 3 huh? :)

Point 3 will be quite important to manage in some way. +1 for the upgrade generator.

@robyurkowski

This comment has been minimized.

Show comment
Hide comment
@robyurkowski

robyurkowski May 8, 2012

Contributor

This is astounding work, Radar.

Contributor

robyurkowski commented May 8, 2012

This is astounding work, Radar.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ May 8, 2012

Member

@radar - I'd say make that an official extension, and please do include it in the install generator.

@jipiboily - I never liked #2.

Member

BDQ commented May 8, 2012

@radar - I'd say make that an official extension, and please do include it in the install generator.

@jipiboily - I never liked #2.

@knewter

This comment has been minimized.

Show comment
Hide comment
@knewter

knewter May 8, 2012

Hot damn, this is huge. 👍

knewter commented May 8, 2012

Hot damn, this is huge. 👍

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil May 8, 2012

Member

👍

Member

JDutil commented May 8, 2012

👍

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 10, 2012

Member

All the tests are passing on Travis for this branch. I will do the guide writeup this afternoon and then people can look through it and suggest improvements.

Member

radar commented May 10, 2012

All the tests are passing on Travis for this branch. I will do the guide writeup this afternoon and then people can look through it and suggest improvements.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 10, 2012

Member

First draft of authentication guide is here. spree_auth_devise has one failing test (to do with the user signup promotion) and if someone else wants to be a champ and solve that before I do, that'd be awesome.

Other than that, I can't think of anything else that needs to go into this pull request. Can you (@schof, @BDQ, @LBRapid, @cmar, @geekoncoffeee) look over this and see if everything makes sense please?

Member

radar commented May 10, 2012

First draft of authentication guide is here. spree_auth_devise has one failing test (to do with the user signup promotion) and if someone else wants to be a champ and solve that before I do, that'd be awesome.

Other than that, I can't think of anything else that needs to go into this pull request. Can you (@schof, @BDQ, @LBRapid, @cmar, @geekoncoffeee) look over this and see if everything makes sense please?

dash/app/models/spree/user_decorator.rb
@@ -0,0 +1,5 @@
+if Spree.user_class

This comment has been minimized.

@BDQ

BDQ May 10, 2012

Member

Why does dash need this decorator?

@BDQ

BDQ May 10, 2012

Member

Why does dash need this decorator?

This comment has been minimized.

@radar

radar May 22, 2012

Member

The tests were complaining if it didn't have it, and so I added it. One of dash's tests checks for roles iirc.

@radar

radar May 22, 2012

Member

The tests were complaining if it didn't have it, and so I added it. One of dash's tests checks for roles iirc.

self.email = user.email if self.user and not user.anonymous?
- self.user ||= User.anonymous!

This comment has been minimized.

@BDQ

BDQ May 10, 2012

Member

So we're no longer creating an anonymous user for all orders? This may cause problems as I expect some stuff maybe expect an order to have a user?

@BDQ

BDQ May 10, 2012

Member

So we're no longer creating an anonymous user for all orders? This may cause problems as I expect some stuff maybe expect an order to have a user?

This comment has been minimized.

@radar

radar May 22, 2012

Member

I don't think this is the case. Users are now prompted to enter an email when they create an order (if they're not authenticated). Is there a case where an order could be created anonymously?

@radar

radar May 22, 2012

Member

I don't think this is the case. Users are now prompted to enter an email when they create an order (if they're not authenticated). Is there a case where an order could be created anonymously?

This comment has been minimized.

@radar

radar May 23, 2012

Member

Just checked in admin/orders backend and that doesn't seem to error out. Order emails are associated with order.email, not order.user

@radar

radar May 23, 2012

Member

Just checked in admin/orders backend and that doesn't seem to error out. Order emails are associated with order.email, not order.user

This comment has been minimized.

@BDQ

BDQ May 23, 2012

Member

Previously, when an item was added to a cart and there was no current_order, an anonymous user was created with that new order (I was never a fan of that approach, but @schof had reasons for it).

@BDQ

BDQ May 23, 2012

Member

Previously, when an item was added to a cart and there was no current_order, an anonymous user was created with that new order (I was never a fan of that approach, but @schof had reasons for it).

This comment has been minimized.

@tvdeyen

tvdeyen May 23, 2012

Contributor

Please get rid of this. This is polluting the database and is not necessary IMHO.
Even if the order object needs a user object we could use a fake / double object.

@radar: keep up the good work. You are kicking ass!

    self.email = user.email if self.user and not user.anonymous?
  •    self.user ||= User.anonymous!
    

Previously, when an item was added to a cart and there was no current_order, an anonymous user was created with that new order (I was never a fan of that approach, but @schof had reasons for it).


Reply to this email directly or view it on GitHub:
https://github.com/spree/spree/pull/1512/files#r866255

@tvdeyen

tvdeyen May 23, 2012

Contributor

Please get rid of this. This is polluting the database and is not necessary IMHO.
Even if the order object needs a user object we could use a fake / double object.

@radar: keep up the good work. You are kicking ass!

    self.email = user.email if self.user and not user.anonymous?
  •    self.user ||= User.anonymous!
    

Previously, when an item was added to a cart and there was no current_order, an anonymous user was created with that new order (I was never a fan of that approach, but @schof had reasons for it).


Reply to this email directly or view it on GitHub:
https://github.com/spree/spree/pull/1512/files#r866255

This comment has been minimized.

@JDutil

JDutil May 23, 2012

Member

I agree it would be nice to use a single dummy user record if necessary instead of a new user record for every guest order. Perhaps the new user record for guest orders has something to do with promotions though?

@JDutil

JDutil May 23, 2012

Member

I agree it would be nice to use a single dummy user record if necessary instead of a new user record for every guest order. Perhaps the new user record for guest orders has something to do with promotions though?

This comment has been minimized.

@schof

schof May 23, 2012

Member

One of the original reasons for having an anonymous order was user token access (so users were able to access their own in progress and completed orders only.) IIRC we moved the token to order now so maybe its safe to contemplate this.

@schof

schof May 23, 2012

Member

One of the original reasons for having an anonymous order was user token access (so users were able to access their own in progress and completed orders only.) IIRC we moved the token to order now so maybe its safe to contemplate this.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ May 10, 2012

Member

I've got a few more comments:

  1. The sandbox application seems borked, you can't access the admin UI.
undefined local variable or method `spree_signup_path'
  1. I commented on dash user_decorator.rb but it seems all core engines have the same basic decorator adding the habtm association at least, why is that? Feel like the association should be in core, if it's needed for all extensions.

  2. I'm concerned that the sample data is now incomplete, specifically the order won't have any user data which will cause problems with the admin/orders ui.

Member

BDQ commented May 10, 2012

I've got a few more comments:

  1. The sandbox application seems borked, you can't access the admin UI.
undefined local variable or method `spree_signup_path'
  1. I commented on dash user_decorator.rb but it seems all core engines have the same basic decorator adding the habtm association at least, why is that? Feel like the association should be in core, if it's needed for all extensions.

  2. I'm concerned that the sample data is now incomplete, specifically the order won't have any user data which will cause problems with the admin/orders ui.

WIP: adding spree_auth_devise to sandbox Gemfile
This is currently NOT working. Will investigate lateR
Remove sandbox_generator, replace with Rails template
The rails template's code is much simpler and allows us to enforce specific gem requirements that the dummy application should never have.

This fixes the sandbox's requirement of spree_signup_path as pointed out by BDQ here: #1512 (comment)
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 22, 2012

Member

BDQ: 1) on your list addressed by @948f0ba. I think we could probably find a better spot for sandbox.rb than just chucking it at root. Perhaps lib?

Member

radar commented May 22, 2012

BDQ: 1) on your list addressed by @948f0ba. I think we could probably find a better spot for sandbox.rb than just chucking it at root. Perhaps lib?

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt May 22, 2012

Contributor

What about invoking it?

Contributor

parndt commented on Rakefile in 948f0ba May 22, 2012

What about invoking it?

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 22, 2012

Member

What difference does it make?

Member

radar replied May 22, 2012

What difference does it make?

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt May 22, 2012

Contributor
Contributor

parndt replied May 22, 2012

@beneggett

This comment has been minimized.

Show comment
Hide comment
@beneggett

beneggett May 26, 2012

Contributor

Per my conversation with @radar in IRC, I'm interested and available for testing once this is getting close to being released in the master branch; as I'm planning on implementing it.

Contributor

beneggett commented May 26, 2012

Per my conversation with @radar in IRC, I'm interested and available for testing once this is getting close to being released in the master branch; as I'm planning on implementing it.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar May 26, 2012

Member

Regarding spree_social, this pull request will break this extension, but not in a terrible way.

The first problem is the user_decorator file. This will need to be changed to decorate Spree.user_class rather than Spree::User. Pretty easy change.

Secondly, user_authentication.rb will need to be changed from this:

 belongs_to :user, :class_name => Spree.user_class.to_s

Thirdly, the calls to current_user throughout the controllers will need to be replaced with spree_current_user calls instead.

Fourthly, spree.login_path references will need to change to spree_login_path. We may need to add in a route for the account path too...

Finally, the modifications to Spree::UserRegistrationsController will need to be placed elsewhere, but I don't know where for now.

Member

radar commented May 26, 2012

Regarding spree_social, this pull request will break this extension, but not in a terrible way.

The first problem is the user_decorator file. This will need to be changed to decorate Spree.user_class rather than Spree::User. Pretty easy change.

Secondly, user_authentication.rb will need to be changed from this:

 belongs_to :user, :class_name => Spree.user_class.to_s

Thirdly, the calls to current_user throughout the controllers will need to be replaced with spree_current_user calls instead.

Fourthly, spree.login_path references will need to change to spree_login_path. We may need to add in a route for the account path too...

Finally, the modifications to Spree::UserRegistrationsController will need to be placed elsewhere, but I don't know where for now.

@tvdeyen

This comment has been minimized.

Show comment
Hide comment
@tvdeyen

tvdeyen May 26, 2012

Contributor

I also volunteer for testing this with our engine Alchemy CMS. Would be great to couple these two great projects even tighter together.

Best

Thomas von Deyen
magic labs*

Contributor

tvdeyen commented May 26, 2012

I also volunteer for testing this with our engine Alchemy CMS. Would be great to couple these two great projects even tighter together.

Best

Thomas von Deyen
magic labs*

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

@rohitnair Thanks for bringing up the issue about the DELETE method needed for the logout link. I've fixed this in spree/spree-guides@cbf8d13 now.

Member

radar commented Jun 11, 2012

@rohitnair Thanks for bringing up the issue about the DELETE method needed for the logout link. I've fixed this in spree/spree-guides@cbf8d13 now.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

@laspluviosillas I've updated spree-auth-devise now with spree/spree_auth_devise@beff51c and spree/spree_auth_devise@d3413ea commits. It appears that the sandbox now works for me. Could you please try it out and validate this as well, just for a second opinion?

Member

radar commented Jun 11, 2012

@laspluviosillas I've updated spree-auth-devise now with spree/spree_auth_devise@beff51c and spree/spree_auth_devise@d3413ea commits. It appears that the sandbox now works for me. Could you please try it out and validate this as well, just for a second opinion?

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 11, 2012

Contributor

It works! Time to test other things as well, cheers.

Contributor

laspluviosillas commented Jun 11, 2012

It works! Time to test other things as well, cheers.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

Excellent, thanks for testing :)

Member

radar commented Jun 11, 2012

Excellent, thanks for testing :)

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 11, 2012

Contributor

Went to try things out with sorcery. Failing at the rails g spree:custom_user User step.

link to pastebin

Contributor

laspluviosillas commented Jun 11, 2012

Went to try things out with sorcery. Failing at the rails g spree:custom_user User step.

link to pastebin

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

Does your application contain a User class? Can you put the example app on GitHub somewhere so I can have a tinker? Also, come join #spree on Freenode.

Member

radar commented Jun 11, 2012

Does your application contain a User class? Can you put the example app on GitHub somewhere so I can have a tinker? Also, come join #spree on Freenode.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 11, 2012

Contributor

I was about to delete my post. Yes, that's exactly right. I was doing this before creating my user Model. facepalm. Always got to do a quick check at the error before knee jerk response. Sorry about that.

Contributor

laspluviosillas commented Jun 11, 2012

I was about to delete my post. Yes, that's exactly right. I was doing this before creating my user Model. facepalm. Always got to do a quick check at the error before knee jerk response. Sorry about that.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 11, 2012

Member

We should probably check for the constant before then and show an error message telling the person to make sure they're referencing the right constant. 99% of people don't read Giant Error Messages of Doom.

Also: 90% of statistics are made up on the spot.

Member

radar commented Jun 11, 2012

We should probably check for the constant before then and show an error message telling the person to make sure they're referencing the right constant. 99% of people don't read Giant Error Messages of Doom.

Also: 90% of statistics are made up on the spot.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 11, 2012

Contributor

I know testing is more valuable right now than extra features, but would a default view generator be useful? It could just output the view code defined at the end of this guide. I could code it up.

Contributor

laspluviosillas commented Jun 11, 2012

I know testing is more valuable right now than extra features, but would a default view generator be useful? It could just output the view code defined at the end of this guide. I could code it up.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 12, 2012

Contributor

Okay, I was able to get this all setup with sorcery. Awesome stuff. The biggest stumbling block for me by far was understanding how to create custom controllers that would integrate into the "spree flow," so to speak. The solution is quite simple, which is to have your custom controllers inherit from Spree::BaseController, and put your respective routes in a Spree::Core::Engine.routes.prepend do block.

I'll add a pull request to the customization guides, but it would be good to reference this bit inside the actual custom authentication guide as well as I suspect 90% of users are going to want their custom login pages to integrate into the spree flow and automatically inherit the spree layout, since authentication is a core the system itself and not just a custom addon page. (That's a made up statistic as well.)

This is nothing more than a nitpick, but in the custom auth documentation, there is this code block at the end:

      def spree_current_user
        current_user
      end

      def spree_login_path
        main_app.login_path
      end

      def spree_signup_path
        main_app.signup_path
      end

      def spree_logout_path
        main_app.logout_path
      end

Why do the last 3 functions have main_app by default but the first one doesn't? Maybe Devise is different, but my experience with Sorcery is that if I did have my controllers inherit from Spree::BaseController, all the functions would be without the main_app prefix. On the other hand, if I left my controllers inherting from ApplicationController, all the functions would require the main_app prefix to work. It was trivial for me to figure out, but someone else might get confused.

Contributor

laspluviosillas commented Jun 12, 2012

Okay, I was able to get this all setup with sorcery. Awesome stuff. The biggest stumbling block for me by far was understanding how to create custom controllers that would integrate into the "spree flow," so to speak. The solution is quite simple, which is to have your custom controllers inherit from Spree::BaseController, and put your respective routes in a Spree::Core::Engine.routes.prepend do block.

I'll add a pull request to the customization guides, but it would be good to reference this bit inside the actual custom authentication guide as well as I suspect 90% of users are going to want their custom login pages to integrate into the spree flow and automatically inherit the spree layout, since authentication is a core the system itself and not just a custom addon page. (That's a made up statistic as well.)

This is nothing more than a nitpick, but in the custom auth documentation, there is this code block at the end:

      def spree_current_user
        current_user
      end

      def spree_login_path
        main_app.login_path
      end

      def spree_signup_path
        main_app.signup_path
      end

      def spree_logout_path
        main_app.logout_path
      end

Why do the last 3 functions have main_app by default but the first one doesn't? Maybe Devise is different, but my experience with Sorcery is that if I did have my controllers inherit from Spree::BaseController, all the functions would be without the main_app prefix. On the other hand, if I left my controllers inherting from ApplicationController, all the functions would require the main_app prefix to work. It was trivial for me to figure out, but someone else might get confused.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Jun 12, 2012

Member

@laspluviosillas I believe the last issue your referring to with the main_app prefixes is because the first method is not a routing method. It is asking for a user object rather than routing path.

The spree_current_user method is to return the "Current User". The other 3 methods are asking to return a URL path. so the first method is returning a user object and the the other 3 methods are returning a route. The "main_app" reference means your main applications routes. Not sure why they chose that naming convention, but it makes sense for the most part.

Member

JDutil commented Jun 12, 2012

@laspluviosillas I believe the last issue your referring to with the main_app prefixes is because the first method is not a routing method. It is asking for a user object rather than routing path.

The spree_current_user method is to return the "Current User". The other 3 methods are asking to return a URL path. so the first method is returning a user object and the the other 3 methods are returning a route. The "main_app" reference means your main applications routes. Not sure why they chose that naming convention, but it makes sense for the most part.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 12, 2012

Member

Thanks @JDutil for beating me to it! Here's the response I typed up before I saw his reply:


I am not sure I understand this. Why do you need to integrate with the Spree flow? Authentication should sit outside the context of Spree and then if it's available Spree should then hook back into it. It shouldn't require you to hook into Spree::BaseController at all, or draw on Spree::Core::Engine's routes. What's making you do that?

Regarding the helpers, current_user is a typical method that is available everywhere, made available by Devise/Sorcery/whatever inside of ApplicationController and friends. The other methods are routing methods, and due to how Rails engines are defined, you need to tell Rails in what context the routes can be found. By default, Rails will look in the current engine's routes for it. By putting main_app up front, we tell it expressly that the routes are located in the application.

I explain that (and a lot of other things about engines) in my engines screencast

Member

radar commented Jun 12, 2012

Thanks @JDutil for beating me to it! Here's the response I typed up before I saw his reply:


I am not sure I understand this. Why do you need to integrate with the Spree flow? Authentication should sit outside the context of Spree and then if it's available Spree should then hook back into it. It shouldn't require you to hook into Spree::BaseController at all, or draw on Spree::Core::Engine's routes. What's making you do that?

Regarding the helpers, current_user is a typical method that is available everywhere, made available by Devise/Sorcery/whatever inside of ApplicationController and friends. The other methods are routing methods, and due to how Rails engines are defined, you need to tell Rails in what context the routes can be found. By default, Rails will look in the current engine's routes for it. By putting main_app up front, we tell it expressly that the routes are located in the application.

I explain that (and a lot of other things about engines) in my engines screencast

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Jun 12, 2012

Member

@radar I think the work on this is great, but I am still a bit hesitant about removing the auth component altogether. I think removing altogether was great for working out the issues with decoupling the components, but should that be the final solution? I almost think that spree_auth_devise should live on as part of "spree". Simply because a bulk of users will just want things to work out of the box, but it is great to offer the option to choose another auth source.

I'm just thinking that including spree_auth as part of spree may have prevented fully understanding all of the issues and solutions that were required to truly decouple the gems. It still may be beneficial to keep the auth gem as part of spree though for new users. Either way I'm glad the work was done, and think a lot of benefit will come from this. I'm just not sure breaking into its own repo will be the best option, but at the least it should prevent coupling by accident and raise errors w/decoupling sooner I would expect by separating the libraries.

Member

JDutil commented Jun 12, 2012

@radar I think the work on this is great, but I am still a bit hesitant about removing the auth component altogether. I think removing altogether was great for working out the issues with decoupling the components, but should that be the final solution? I almost think that spree_auth_devise should live on as part of "spree". Simply because a bulk of users will just want things to work out of the box, but it is great to offer the option to choose another auth source.

I'm just thinking that including spree_auth as part of spree may have prevented fully understanding all of the issues and solutions that were required to truly decouple the gems. It still may be beneficial to keep the auth gem as part of spree though for new users. Either way I'm glad the work was done, and think a lot of benefit will come from this. I'm just not sure breaking into its own repo will be the best option, but at the least it should prevent coupling by accident and raise errors w/decoupling sooner I would expect by separating the libraries.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Jun 12, 2012

Member

I should add that a big reason I am hesitant to break into it's own repo is that I've seen a lot of spree extensions fall behind the main platform. So while I think breaking auth out on it's own is great for the decoupling of the project I'm worried that it could cause the auth portion to fall behind the rest of the project as some extensions do.

Member

JDutil commented Jun 12, 2012

I should add that a big reason I am hesitant to break into it's own repo is that I've seen a lot of spree extensions fall behind the main platform. So while I think breaking auth out on it's own is great for the decoupling of the project I'm worried that it could cause the auth portion to fall behind the rest of the project as some extensions do.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 12, 2012

Contributor

@radar I want my login page to fit inside of the standard spree_application.html.erb layout. The spree_application.html.erb layout use a few variables/functions that are only available if the controller using that layout inherits from Spree::BaseController. I assume some other users would want to have their login page inside the spree layout as well.

Am I doing things massively the wrong way? That's 99% probable.

The routes bit was my own stupidity, so never mind on that account.

Contributor

laspluviosillas commented Jun 12, 2012

@radar I want my login page to fit inside of the standard spree_application.html.erb layout. The spree_application.html.erb layout use a few variables/functions that are only available if the controller using that layout inherits from Spree::BaseController. I assume some other users would want to have their login page inside the spree layout as well.

Am I doing things massively the wrong way? That's 99% probable.

The routes bit was my own stupidity, so never mind on that account.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 12, 2012

Member

@radar I think the work on this is great, but I am still a bit hesitant about removing the auth component altogether. I think removing altogether was great for working out the issues with decoupling the components, but should that be the final solution? I almost think that spree_auth_devise should live on as part of "spree". Simply because a bulk of users will just want things to work out of the box, but it is great to offer the option to choose another auth source.

I'm just thinking that including spree_auth as part of spree may have prevented fully understanding all of the issues and solutions that were required to truly decouple the gems. It still may be beneficial to keep the auth gem as part of spree though for new users. Either way I'm glad the work was done, and think a lot of benefit will come from this. I'm just not sure breaking into its own repo will be the best option, but at the least it should prevent coupling by accident and raise errors w/decoupling sooner I would expect by separating the libraries.

New users are prompted if they want to install the default authentication option as part of the installation process of Spree. If they select the default option of "Yes, please install this thing", then Spree will work (for all intents and purposes) exactly as it does now.

If they select "no", then, well... that's a different story. There's some differences involved, and that option is only for people who know what they're doing after they've read the authentication guide.

If there are errors introduced by this decoupling, I would have expected them to have appeared by now. Breaking the authentication into its own repo is the best way to go about this, as the auth component is now going to be maintained completely separately from the core components. This means that we would be able to do a new release of the auth extension without having to do another release of the remaining components, or vice versa.

It's my super-strong opinion that auth should've never been a part of an engine-ized Spree (or any engine, for that matter), as it's a concern of the application how it decides how users will authenticate against its system. This is the best solution I have come up with in my discussions with a lot of people on this issue. There's just simply too many differing factors in authentication systems these days to lock people to using a specific one.

I should add that a big reason I am hesitant to break into it's own repo is that I've seen a lot of spree extensions fall behind the main platform. So while I think breaking auth out on it's own is great for the decoupling of the project I'm worried that it could cause the auth portion to fall behind the rest of the project as some extensions do.

The extensions that have fallen behind need issues posted on them so that we know that a) people are actually using them and we should maintain them still and kinda related b) so we know that they're broken by newer releases and they need attention now. With the exception of spree_paypal_express, I can't think of any other official extension which has lagged terribly behind.

We won't let that happen to spree_auth_devise because it's a neat, easily-maintainable part of Spree that's critical to the default setup of Spree. Without it, Spree would be broken and that would reflect poorly on us.

Member

radar commented Jun 12, 2012

@radar I think the work on this is great, but I am still a bit hesitant about removing the auth component altogether. I think removing altogether was great for working out the issues with decoupling the components, but should that be the final solution? I almost think that spree_auth_devise should live on as part of "spree". Simply because a bulk of users will just want things to work out of the box, but it is great to offer the option to choose another auth source.

I'm just thinking that including spree_auth as part of spree may have prevented fully understanding all of the issues and solutions that were required to truly decouple the gems. It still may be beneficial to keep the auth gem as part of spree though for new users. Either way I'm glad the work was done, and think a lot of benefit will come from this. I'm just not sure breaking into its own repo will be the best option, but at the least it should prevent coupling by accident and raise errors w/decoupling sooner I would expect by separating the libraries.

New users are prompted if they want to install the default authentication option as part of the installation process of Spree. If they select the default option of "Yes, please install this thing", then Spree will work (for all intents and purposes) exactly as it does now.

If they select "no", then, well... that's a different story. There's some differences involved, and that option is only for people who know what they're doing after they've read the authentication guide.

If there are errors introduced by this decoupling, I would have expected them to have appeared by now. Breaking the authentication into its own repo is the best way to go about this, as the auth component is now going to be maintained completely separately from the core components. This means that we would be able to do a new release of the auth extension without having to do another release of the remaining components, or vice versa.

It's my super-strong opinion that auth should've never been a part of an engine-ized Spree (or any engine, for that matter), as it's a concern of the application how it decides how users will authenticate against its system. This is the best solution I have come up with in my discussions with a lot of people on this issue. There's just simply too many differing factors in authentication systems these days to lock people to using a specific one.

I should add that a big reason I am hesitant to break into it's own repo is that I've seen a lot of spree extensions fall behind the main platform. So while I think breaking auth out on it's own is great for the decoupling of the project I'm worried that it could cause the auth portion to fall behind the rest of the project as some extensions do.

The extensions that have fallen behind need issues posted on them so that we know that a) people are actually using them and we should maintain them still and kinda related b) so we know that they're broken by newer releases and they need attention now. With the exception of spree_paypal_express, I can't think of any other official extension which has lagged terribly behind.

We won't let that happen to spree_auth_devise because it's a neat, easily-maintainable part of Spree that's critical to the default setup of Spree. Without it, Spree would be broken and that would reflect poorly on us.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 12, 2012

Member

@radar I want my login page to fit inside of the standard spree_application.html.erb layout. The spree_application.html.erb layout use a few variables/functions that are only available if the controller using that layout inherits from Spree::BaseController. I assume some other users would want to have their login page inside the spree layout as well.

Am I doing things massively the wrong way? That's 99% probable.

I hadn't actually considered that people would put their login form inside of Spree's page, but that makes sense. What are the issues that you're running into regarding that? Perhaps we can work out a cleaner way to do that.

Member

radar commented Jun 12, 2012

@radar I want my login page to fit inside of the standard spree_application.html.erb layout. The spree_application.html.erb layout use a few variables/functions that are only available if the controller using that layout inherits from Spree::BaseController. I assume some other users would want to have their login page inside the spree layout as well.

Am I doing things massively the wrong way? That's 99% probable.

I hadn't actually considered that people would put their login form inside of Spree's page, but that makes sense. What are the issues that you're running into regarding that? Perhaps we can work out a cleaner way to do that.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 12, 2012

Contributor

@radar
What's required to get any page to use spree_application.html.erb layout is that the controller (in my app that's SessionsController) inherit from Spree::BaseController instead of the standard ApplicationController. Here's the catch:

_If_ your controller inherits from Spree::BaseController, then any routes you want your controller to use that are defined in your main application namespace have to be prepended with main_app. just like in the AuthenticationHelpers file. It makes using gems that automatically generate routes such as simple_form harder, so I half retract what I said about routing.

What are the issues that you're running into regarding that? Perhaps we can work out a cleaner way to do that.

It's not a huge deal in my opinion to inherit from Spree::BaseController, but the most annoying issue it is that I believe you're putting yourself inside the spree namespace when you do that (but not sure). It'd obviously be better if there were a way to keep your controller in the default namespace while having access to the spree layouts, but I'm not sure if this is possible.

Whatever the "official solution" is, I care about having some section in the guides that deals with this scenario. I'll more than happily [attempt to] write it myself, but I'll hold off on the writing until you confirm what the best solution to this scenario is.

Contributor

laspluviosillas commented Jun 12, 2012

@radar
What's required to get any page to use spree_application.html.erb layout is that the controller (in my app that's SessionsController) inherit from Spree::BaseController instead of the standard ApplicationController. Here's the catch:

_If_ your controller inherits from Spree::BaseController, then any routes you want your controller to use that are defined in your main application namespace have to be prepended with main_app. just like in the AuthenticationHelpers file. It makes using gems that automatically generate routes such as simple_form harder, so I half retract what I said about routing.

What are the issues that you're running into regarding that? Perhaps we can work out a cleaner way to do that.

It's not a huge deal in my opinion to inherit from Spree::BaseController, but the most annoying issue it is that I believe you're putting yourself inside the spree namespace when you do that (but not sure). It'd obviously be better if there were a way to keep your controller in the default namespace while having access to the spree layouts, but I'm not sure if this is possible.

Whatever the "official solution" is, I care about having some section in the guides that deals with this scenario. I'll more than happily [attempt to] write it myself, but I'll hold off on the writing until you confirm what the best solution to this scenario is.

@laspluviosillas

This comment has been minimized.

Show comment
Hide comment
@laspluviosillas

laspluviosillas Jun 12, 2012

Contributor

@radar

Apologies for flooding your inbox! But, I believe I have discovered another (more critical) issue. The last line of authentication_helpers.rb

ApplicationController.send :include, Spree::AuthenticationHelpers

Is only called when server boots up. As a result, if you do a save to any controller in development mode, when the controllers are reloaded, the Spree::AuthenticationHelpers are no longer available.

I confirmed that this is a one-load issue by making the include explicit in app controller. When my ApplicationController is explicitly set to include the AuthenticationHelpers module, the issue goes away.

Contributor

laspluviosillas commented Jun 12, 2012

@radar

Apologies for flooding your inbox! But, I believe I have discovered another (more critical) issue. The last line of authentication_helpers.rb

ApplicationController.send :include, Spree::AuthenticationHelpers

Is only called when server boots up. As a result, if you do a save to any controller in development mode, when the controllers are reloaded, the Spree::AuthenticationHelpers are no longer available.

I confirmed that this is a one-load issue by making the include explicit in app controller. When my ApplicationController is explicitly set to include the AuthenticationHelpers module, the issue goes away.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jun 18, 2012

Member

@radar - I've tried the latest branch with this [1] test app and I'm still having similar migration issues:

Maybe, I'm doing something wrong - could you test out installing Spree into this app and see if it works as expected for you?

[1] https://github.com/RailsApps/rails3-devise-rspec-cucumber

Member

BDQ commented Jun 18, 2012

@radar - I've tried the latest branch with this [1] test app and I'm still having similar migration issues:

Maybe, I'm doing something wrong - could you test out installing Spree into this app and see if it works as expected for you?

[1] https://github.com/RailsApps/rails3-devise-rspec-cucumber

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 19, 2012

Member

I've fixed the ResizeApiKeyFields migration tonight. I'm going to run-through the guide tomorrow morning when I'm more coherent... but I think that at least fixes your problem.

Member

radar commented Jun 19, 2012

I've fixed the ResizeApiKeyFields migration tonight. I'm going to run-through the guide tomorrow morning when I'm more coherent... but I think that at least fixes your problem.

@BDQ

This comment has been minimized.

Show comment
Hide comment
@BDQ

BDQ Jun 19, 2012

Member

Looks good, I say: merge.

GREAT JOB BTW!

Member

BDQ commented Jun 19, 2012

Looks good, I say: merge.

GREAT JOB BTW!

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 20, 2012

Member

This has been rebased onto master as of right now. Thanks to all who've helped out and poked and prodded the work. Great to see we've finally landed this thing!

🎈 😃

Member

radar commented Jun 20, 2012

This has been rebased onto master as of right now. Thanks to all who've helped out and poked and prodded the work. Great to see we've finally landed this thing!

🎈 😃

@radar radar closed this Jun 20, 2012

@greinacker

This comment has been minimized.

Show comment
Hide comment
@greinacker

greinacker Jun 20, 2012

Contributor

@radar just out of curiosity - when you rebased this, did you rebase your local auth-take-two branch on top of master, and then just push master (but not push auth-take-two)? I'm just wondering why the commits still exist in their original form in auth-take-two after a rebase - trying to get my head around what exactly happens when you rebase code that has already been pushed.

Contributor

greinacker commented Jun 20, 2012

@radar just out of curiosity - when you rebased this, did you rebase your local auth-take-two branch on top of master, and then just push master (but not push auth-take-two)? I'm just wondering why the commits still exist in their original form in auth-take-two after a rebase - trying to get my head around what exactly happens when you rebase code that has already been pushed.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 20, 2012

Member

I rebased auth-take-two onto master, and then did my alias gl which is effectively git pull --rebase which runs another rebase on the top of that, pulling in the changes from master.

I don't know if this is the "right" way of doing it -- usually I'd just merge, but there's a policy on this project that's "rebase or death" -- but it works for me.

Member

radar commented Jun 20, 2012

I rebased auth-take-two onto master, and then did my alias gl which is effectively git pull --rebase which runs another rebase on the top of that, pulling in the changes from master.

I don't know if this is the "right" way of doing it -- usually I'd just merge, but there's a policy on this project that's "rebase or death" -- but it works for me.

@greinacker

This comment has been minimized.

Show comment
Hide comment
@greinacker

greinacker Jun 20, 2012

Contributor

Got it, and then after that I'm guessing you pushed master, but did not push auth-take-two...right?

Contributor

greinacker commented Jun 20, 2012

Got it, and then after that I'm guessing you pushed master, but did not push auth-take-two...right?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Jun 20, 2012

Member

Correct. That branch is dead now. I will delete it.

Member

radar commented Jun 20, 2012

Correct. That branch is dead now. I will delete it.

@greinacker

This comment has been minimized.

Show comment
Hide comment
@greinacker

greinacker Jun 20, 2012

Contributor

Cool, that clears up my question. Thanks!

Contributor

greinacker commented Jun 20, 2012

Cool, that clears up my question. Thanks!

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Aug 30, 2012

In case anyone else lands here trying to work out how to integrate Spree and RefineryCMS authentication, I have created a sample app and documented the process here:

https://github.com/adrianmacneil/refinery_spree

In case anyone else lands here trying to work out how to integrate Spree and RefineryCMS authentication, I have created a sample app and documented the process here:

https://github.com/adrianmacneil/refinery_spree

@kennyadsl

This comment has been minimized.

Show comment
Hide comment
@kennyadsl

kennyadsl Aug 30, 2012

Member

@adrianmacneil Do you think that create an extension for this integration should be possible/useful?

Member

kennyadsl commented Aug 30, 2012

@adrianmacneil Do you think that create an extension for this integration should be possible/useful?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Aug 30, 2012

Member

@netconstructor I don't understand the question?

Member

radar commented Aug 30, 2012

@netconstructor I don't understand the question?

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Aug 30, 2012

Member

I still don't understand the question. We already are using CanCan and you can customize it. Look at the register ability method within our Ability class.

BTW: Could you please not include your massive email signature in posts? It's just spammy.

On 30/08/2012, at 18:16, Christian Hochfilzer notifications@github.com wrote:

I guess the question should be - "What about integrating cancan -
https://github.com/ryanb/cancan

http://www.netconstructor.com/?utm_source=chris%40netconstructor.com&utm_medium=email&utm_campaign=Direct+Emails&utm_content=logo-link-click
Christian Hochfilzer - VP of Strategic Marketing & Development
www.netconstructor.comhttp://www.netconstructor.com/?utm_source=chris%40netconstructor.com&utm_medium=email&utm_campaign=Direct+Emails&utm_content=text-url-link-click
| chris@netconstructor.com
C: 858.205.2405 | T: 619.512.2114 | F: 858.777.3489
105 Cancha De Golf, Rancho Santa Fe, CA 92067
[image: >]Facebookhttps://www.facebook.com/pages/NetConstructor/20638607272
[image:

]LinkedIn http://www.linkedin.com/in/netconstructor [image: >]Twitterhttp://twitter.com/#%21/netconstructor
CONFIDENTIALITY NOTICE: This e-mail message from NetConstructor (including
all attachments) is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review;
use, disclosure, copying or distribution is strictly prohibited. If you are
not the intended recipient, please contact the sender by reply e-mail and
destroy all copies of the original message.

On Thu, Aug 30, 2012 at 8:17 AM, Ryan Bigg notifications@github.com wrote:

@netconstructor https://github.com/netconstructor I don't understand
the question?


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/1512#issuecomment-8162751.


Reply to this email directly or view it on GitHub.

Member

radar commented Aug 30, 2012

I still don't understand the question. We already are using CanCan and you can customize it. Look at the register ability method within our Ability class.

BTW: Could you please not include your massive email signature in posts? It's just spammy.

On 30/08/2012, at 18:16, Christian Hochfilzer notifications@github.com wrote:

I guess the question should be - "What about integrating cancan -
https://github.com/ryanb/cancan

http://www.netconstructor.com/?utm_source=chris%40netconstructor.com&utm_medium=email&utm_campaign=Direct+Emails&utm_content=logo-link-click
Christian Hochfilzer - VP of Strategic Marketing & Development
www.netconstructor.comhttp://www.netconstructor.com/?utm_source=chris%40netconstructor.com&utm_medium=email&utm_campaign=Direct+Emails&utm_content=text-url-link-click
| chris@netconstructor.com
C: 858.205.2405 | T: 619.512.2114 | F: 858.777.3489
105 Cancha De Golf, Rancho Santa Fe, CA 92067
[image: >]Facebookhttps://www.facebook.com/pages/NetConstructor/20638607272
[image:

]LinkedIn http://www.linkedin.com/in/netconstructor [image: >]Twitterhttp://twitter.com/#%21/netconstructor
CONFIDENTIALITY NOTICE: This e-mail message from NetConstructor (including
all attachments) is for the sole use of the intended recipient(s) and may
contain confidential and privileged information. Any unauthorized review;
use, disclosure, copying or distribution is strictly prohibited. If you are
not the intended recipient, please contact the sender by reply e-mail and
destroy all copies of the original message.

On Thu, Aug 30, 2012 at 8:17 AM, Ryan Bigg notifications@github.com wrote:

@netconstructor https://github.com/netconstructor I don't understand
the question?


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/1512#issuecomment-8162751.


Reply to this email directly or view it on GitHub.

@amacneil

This comment has been minimized.

Show comment
Hide comment
@amacneil

amacneil Aug 31, 2012

@kennyadsl Sure thing! I moved the initializers, migrations and workarounds into a gem. It's my first ever gem, so any feedback is welcome :)

https://github.com/adrianmacneil/spree-refinery-authentication

@kennyadsl Sure thing! I moved the initializers, migrations and workarounds into a gem. It's my first ever gem, so any feedback is welcome :)

https://github.com/adrianmacneil/spree-refinery-authentication

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