-
Notifications
You must be signed in to change notification settings - Fork 0
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
wire up passwordless authentication #1
base: docker
Are you sure you want to change the base?
Conversation
def authenticate | ||
config = OpenIDConnect::Discovery::Provider::Config.discover! "https://did.app" | ||
|
||
client = OpenIDConnect::Client.new({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Work out where to put the client and config.
Is the correct place as an initializer? Or is it somewhere in boot.
The discover!
function makes a webcall but there would be no harm hardcoding the authorization_endpoint etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can put it in an ENV variable still, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense to me.
I mentioned the discover!
call makes a web request. and should be done in some startup step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Peter — what's your specific question here? I can't tell if you're asking me something or telling me something. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am asking, where do I put code that makes a web request during startup?
"https://did.app" can go in the env variable. But then there is a call that should be performed only once to lookup the authorization endpoint etc. that is what discover does. The config =
line should not be part of the http handler but some boot or startup code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it!
Well, I think you want a new file under config/initializers
. See more here: https://guides.rubyonrails.org/configuring.html#using-initializer-files.
client = OpenIDConnect::Client.new({ | ||
identifier: ENV["CLIENT_ID"], | ||
secret: ENV["CLIENT_SECRET"], | ||
redirect_uri: "http://localhost:3000/session/callback", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we get the host url so that the redirect is dynamic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may need to set default_url_options
:
https://edgeguides.rubyonrails.org/action_controller_overview.html#default-url-options
https://www.benpickles.com/articles/70-dynamically-setting-rails-default_url_options-in-heroku-review-apps.
And then either interpolate in the host (and port) or make a route in config/routes.rb
, maybe something like:
get '/session/callback', to: 'session#callback', as: 'session_callback'
...and then refer to session_callback_url
.
config = OpenIDConnect::Discovery::Provider::Config.discover! "https://did.app" | ||
|
||
client = OpenIDConnect::Client.new({ | ||
identifier: ENV["CLIENT_ID"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is calling straight into ENV the right thing to do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
redirect_to root_path | ||
end | ||
def terminate | ||
session[:current_user_id] = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a satisfactory way to end a session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it! Or you can remove the entire session with reset_session
.
https://api.rubyonrails.org/classes/ActionController/Base.html#M000668
depends_on: | ||
- db | ||
environment: | ||
- RECAPTCHA_SITE_KEY=aaaaa | ||
- RAILS_ADMIN_USERNAME=rails | ||
- RAILS_ADMIN_PASSWORD=rails | ||
- CLIENT_ID=test_0xKvM6N9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are values for a test application so there is no harm them being in source control
This PR add's openid_connect gem and configuration for did.app passwordless authentication.