search

mikker / passwordless

🗝 Authentication for your Rails app without the icky-ness of passwords
MIT License
1.28k stars 89 forks source link

Pass request to `after-session-save` #49

Closed aurels closed 5 years ago

aurels commented 5 years ago

I would find it useful, what do you think ?

For example : my app is multi-domain and I'd like to know on which domain the user has initiated the login.

mikker commented 5 years ago

Sure. Did you break the tests? 😊

aurels commented 5 years ago

Yes I did :p Let me fix them !

aurels commented 5 years ago

Not really clear to me how to run them locally.

I did : 

rbenv shell 2.4.5
bundle
cd test/dummy
bin/rails db:migrate RAILS_ENV=test

and I get : ActiveRecord::StatementInvalid: SQLite3::SQLException: table "passwordless_sessions" already exists: CREATE TABLE "passwordless_sessions"

aurels commented 5 years ago

Okay, a bin/rails db:test:prepare did it ;-)

aurels commented 5 years ago

I fixed the specs ;-)

But I realise I introduced a breaking change in the API : Passwordless.after_session_save MUST now always have two parameters.

kinduff commented 5 years ago

@aurels you could make it optional by assigning _request = nil at the lambda. I like this change but I see it's not flexible. How about to optionally be able to pass anything to the after_session_save method?

aurels commented 5 years ago

@kinduff Yes I can do _request = nil.

How about to optionally be able to pass anything to the after_session_save method?

I thought about that initially but could not find a good way because it's called in SessionsController. Any idea ?

mikker commented 5 years ago

Perhaps we could use Proc#arity to determine how many arguments to pass? https://ruby-doc.org/core-2.2.0/Proc.html#method-i-arity

aurels commented 5 years ago

I'll try that !

aurels commented 5 years ago

What do you guys think of d4d0428fe81f1ff621592aabd550802dea5ea23b ?

mikker commented 5 years ago

LGTM 👍 Fix the rubocop warnings and I'll be happy to include this.

aurels commented 5 years ago

Mmm most of them come from code I didn't write, right ?

mikker commented 5 years ago

I’m not even sure at this point. I’ll find out and merge when I find a minute.

mikker commented 5 years ago

Merged this in #55 – thanks a bunch! 💙💚💛💜❤️