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

Allow multiple authentication_keys on login #1004

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

danielneis
Copy link

Hello,

this is second attempt of #949
I rebased the patch to current master and added back the "binary" statement for mysql

@zachfeldman
Copy link
Contributor

Hey @danielneis looks like the build is failing - could you perhaps better explain what your pull request is doing? I read through it but I'm not sure if I understand. Thanks for taking the time!

@danielneis
Copy link
Author

Hi, @zachfeldman , what I am trying to do is to use all the resource_class.authentication_keys instead of only the first one. The users in my database uses two fields for authentication (account and agency, for example).
With the first patch I was able to get it working on postgresql, I didn't tested this new code, just rewrite it to fit the new structure but it looks like it not worked...

@zachfeldman
Copy link
Contributor

Got it @danielneis - well if the checks pass I'd be down to merge, I don't see a huge problem with that.

@MaicolBen
Copy link
Collaborator

You didn't define fields in this PR, maybe it's that. I am not sure about adding this feature, but make sure this doesn't break for the common scenario of one field.

For the next time, please put a PR here when the work is finished.

@danielneis danielneis force-pushed the multiple-fields branch 5 times, most recently from 28a06ce to 49d67a4 Compare November 6, 2017 20:25
@danielneis
Copy link
Author

Now the tests are passing =)

It would be nice to have some tests for those cases with multiple :authentication_keys
It would have to define it on the model and then try to login with and without those fields, with users that may have same and different values on the other fields.
If you have any ideas or directions on how to do it I can add the tests =)

@MaicolBen
Copy link
Collaborator

@ArielAleksandrus
Copy link

Isn't this branch merged yet? Can you please write a guide on how to use this new feature? Thank you in advance

@danielneis
Copy link
Author

Hello,

if you have on your devise configuration something like that:

config.authentication_keys = [:account, :domain]

Then you will have on your User model something like:

devise  :database_authenticatable, :authentication_keys => [:account, :domain]

This will use the two fields for comparison when logging someone in. The account and domain must match.

With this patch, it will also work with devise_token_auth =)

@MaicolBen
Copy link
Collaborator

Sounds good! thanks, this resolves #1128

Copy link
Collaborator

@MaicolBen MaicolBen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests?

conditions = []
values = {}
fields.each do |f|
q = " #{f.to_s} = :#{f.to_s} "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use more meaningful variables, maybe query_field ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! just did it. called q = condition and f = field.

conditions.push(' provider = :provider')
values[:provider] = provider.to_s

@resource = resource_class.where([conditions.join(" AND "), values]).first
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use single quotes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure! just did it.

@danielneis
Copy link
Author

I took a look at the current tests and I'm afraid I don't know where to begin.
Maybe it would need a new "user" class with those options?
If you could help with thats would be awesome.

@bettysteger
Copy link
Contributor

@danielneis @MaicolBen will this be resolved anytime soon?

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

Successfully merging this pull request may close these issues.

6 participants