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

Q: Why doesn't simple_token_authentication provide a Devise strategy? #57

Open
adamniedzielski opened this issue Apr 27, 2014 · 17 comments
Labels
Devise strategy enhancement feature request This issue requests a new feature. question When closed, this issue will become part of the FAQ.
Milestone

Comments

@adamniedzielski
Copy link
Contributor

I hope this not a silly question with a simple answer. When token authentication was built into Devise it was a Devise strategy - https://github.com/plataformatec/devise/blob/v3.0/lib/devise/strategies/token_authenticatable.rb

In simple_token_authentication some patterns are kind of duplicated, for example acts_as_token_authenticatable which could be devise :token_authenticatable, ... or acts_as_token_authentication_handler_for User which duplicates before_action :authenticate_user!

I believe that making simple_token_authentication a strategy will be coherent with other authentication strategies and possibly could simplify testing.

However, current implementation of simple_token_authentication allows specifying token authentication only for selected controllers. I don't know whether it is possible with a Devise strategy (Use strategy A in controller B and strategy C in controller D).

I know that it sounds like a complete redesign of the architecture with backwards incompatible changes but maybe it's worth considering.

@adamniedzielski
Copy link
Contributor Author

It looks like it's not a problem for anybody else. So I'm closing this for now - maybe somebody will back me up in the future.

@gonzalo-bulnes
Copy link
Owner

Hi Adam,

I didn't reply before because I was hoping somebody more appropriate would.

Simple Token Authentication was created because the Devise token authentication strategy was taken down by the Devise team. What are the reasons behind their decision? Really I don't know.
However, as you can see through this gem issues, token authentication is far from being trivial and involves significant amounts of time from several people, maybe more than available in a team who supports with great seriousness a great scope of different authentications strategies. In particular, remember that specific security issues shown up little time before the token authentication strategy was taken down (see the original gist José Valim mentioned in the README). That's what I can imagine; again, I don't know.

However, Devise maintainers know was they're doing and there is no doubt they will get in touch if token authentication fits again their plans for Devise. Until then, (and surely after that), what's important is that we, as the Devise, Ruby, FLOSS, write-here-what-you-want community, ensure the token authentication support we need.

Focusing our efforts is really what the Simple Token Authentication gem is for. ; )

@adamniedzielski I hope this reads more like the reply you expected. Thanks for feeling concerned. Best regards!

@gonzalo-bulnes gonzalo-bulnes changed the title Why simple_token_authentication is not a Devise strategy? Q: Why simple_token_authentication is not a Devise strategy? R: I have some ideas but don't know May 29, 2014
@gonzalo-bulnes gonzalo-bulnes changed the title Q: Why simple_token_authentication is not a Devise strategy? R: I have some ideas but don't know Q: Why simple_token_authentication is not a Devise strategy? (R: I have some ideas but don't know) May 29, 2014
@gonzalo-bulnes
Copy link
Owner

@adamniedzielski I just realized that you may have been suggesting that Simple Token Authentication could provide the strategy to Devise (as an extension, or a contributed strategy, versus official strategy). I love that idea, of course it makes sense. I have no idea right now on how that could be implemented, but there is surely a way. I'll explore a bit about Devise strategies, if you can build something up from your observations and give it a try, please share (!) and let's find an adequate structure for the gem to provide it.

If that was what you meant, no doubt: re-open this issue!

@adamniedzielski
Copy link
Contributor Author

Hi @gonzalo-bulnes,

I think that you misunderstood me a bit. Devise team removed token authentication because they didn't want to maintain it and they felt it is specific to a particular application. You took the responsibility of maintaining this part of code. This is really great and I'm thankful for that! What is more, Devise team should be happy too, because splitting big gems into bunch of smaller ones is a wonderful thing.

So - your second comment is exactly what I meant. Contributed strategy not an official strategy.

@adamniedzielski
Copy link
Contributor Author

Some time ago I was using a gem which provided a custom Devise strategy. Here is the link:
https://github.com/cschiewek/devise_ldap_authenticatable

This gem is doing exactly what I propose for simple_token_authentication

@gonzalo-bulnes
Copy link
Owner

@adamniedzielski Great! So it took me a month to understand your comment - sorry for that, it not something I'm proud of : /

To the point: I took a (very) quick look at Devise LDAP Authenticatable, it looks fine. I'll use it as a reference. I'll give it a try to familiarize with the installation steps, see how they test the Devise integration, then start experimenting. Keep me updated on your progress! (For now I re-open this issue.) Thanks for insisting on this topic.

@gonzalo-bulnes gonzalo-bulnes changed the title Q: Why simple_token_authentication is not a Devise strategy? (R: I have some ideas but don't know) Q: Why simple_token_authentication is not a Devise strategy? May 29, 2014
@gonzalo-bulnes gonzalo-bulnes changed the title Q: Why simple_token_authentication is not a Devise strategy? Q: Why doesn't simple_token_authentication provide a Devise strategy? May 29, 2014
@jbender
Copy link

jbender commented Jun 1, 2014

👍 for making it a drop-in replacement

@jbender
Copy link

jbender commented Jun 11, 2014

Just took my first stab at this (and my first look at how Devise strategies work) over on a feature branch (f72f6e5), but it's definitely a WIP, with no tests or anything yet. Any help would be appreciated if you have a chance @adamniedzielski.

@adamniedzielski
Copy link
Contributor Author

I will take a look @jbender

@gonzalo-bulnes
Copy link
Owner

@jbender the starting looks great. Feel welcome opening a PR to make sure we don't miss your next commits - see that one. Besides, that would ensure the discussion doesn't disappear if you remove your work branch someday.)

I'll start writing the Cucumber steps we'll need to run a "strategy-flavoured" set of regression tests.
The usage instructions will also need a deep re-writing (which is not a bad thing at all). Thanks @jbender!

@gonzalo-bulnes gonzalo-bulnes added this to the v2.0.0 milestone Jun 14, 2014
@adamniedzielski
Copy link
Contributor Author

@jbender - did you manage to get this code working in one of your projects?

I've finally managed and all my tests are passing, but I had to change a couple of things. I will divide them into small commits and try to convert this issue to pull request with your commits included.

@gonzalo-bulnes
Copy link
Owner

+1 for the small commits @adamniedzielski, that will make the integration more readable for people who would want to create a contributed strategy for Devise.

@adamniedzielski
Copy link
Contributor Author

This time I was not able to transform this issue to PR :(
I think that the discussion should now move to #69 and this issue can be closed.

@gonzalo-bulnes
Copy link
Owner

I agree, let's move the discussion to #69. I will however let this issue opened until we close it from the code. Tags will avoid ambiguity.

@gonzalo-bulnes
Copy link
Owner

@ishmael,

what's the best way to bring this up to speed?

Since you extendeed the available code for the Devise strategy, I think a PR would be welcome so we can see and comment about details there.

About the best way to bring the topic up to speed, the first thing to do is IMO finding how Devise strategies are supposed to be tested, and the second is writing the strategy specs, or tests.

Then, in order to determine which release strategy to apply (no wordplay), it would be necessary to referrence which Simple Token Authentication features can be provided by the new Devise strategy, and which ones would no longer be supported.

@ericpeters0n
Copy link

+1 Eagerly watching. Thanks, Gents.

@ishmael
Copy link

ishmael commented Dec 3, 2014

@gonzalo-bulnes,

I'll take a look at how Devise tests strategies but any help is appreciated. I am still trying to figure out how to best tie the code that exists in master right now to the strategy.

If you have any pointers, let me know.

@gonzalo-bulnes gonzalo-bulnes modified the milestones: v2.0.0, v3.0.0 Mar 25, 2016
@gonzalo-bulnes gonzalo-bulnes added the feature request This issue requests a new feature. label Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Devise strategy enhancement feature request This issue requests a new feature. question When closed, this issue will become part of the FAQ.
Projects
None yet
Development

No branches or pull requests

5 participants