Add login page route for auth0 #718

wi11 · 2023-06-21T04:23:22Z

With this change, we can access the login page at /auth/auth0. After login, we are successfully redirected to the callback page which i just set as /api-docs for now.

richardxia

Thanks for submitting this, and sorry for the late review! I haven't reviewed the OmniAuth gem or Auth0's docs, but I left initial some comments purely about the code itself.

I think there are probably a few questions that we may need to answer regarding how we want dev environments to work, and how this gets wired up in prod, but we probably need another group discussion about that.

richardxia · 2023-07-06T05:15:55Z

config/application.rb

@@ -44,6 +44,9 @@ class Application < Rails::Application
    # rubocop:enable Layout/LineLength
    config.x.authorization.cert = OpenSSL::X509::Certificate.new(pem)

+    config.middleware.use ActionDispatch::Cookies
+    config.middleware.use ActionDispatch::Session::CookieStore, key: '_namespace_key'


What exactly is this used for? It's not obvious how this interacts with the other stuff in this PR, and if it's something that the omniauth gem implicitly depends on, it'd be good to document it with a comment.

Also, does the key really need to be named _namespace_key? Is it possible to give it a more descriptive name?

This is required to use omniauth, added a comment. Also changed the key to 'sheltertech'

richardxia · 2023-07-06T05:16:46Z

app/controllers/auth0_controller.rb

@@ -0,0 +1,31 @@
+# ./app/controllers/auth0_controller.rb


Nit: Could we remove comments like this? I'm not sure if it's really helpful to have a comment at the top of each file with the file's path.

richardxia · 2023-07-06T05:25:20Z

app/controllers/auth0_controller.rb

+    redirect_to(
+      "https://#{AUTH0_CONFIG['auth0_domain']}/authorize?" +
+      "client_id=#{AUTH0_CONFIG['auth0_client_id']}&" +
+      "response_type=code&" + 
+      "redirect_uri=#{URI.encode(auth_auth0_callback_url)}"
+    )


It'll be safer and cleaner to build this URL programmatically rather than by concatenating and interpolating strings. See https://stackoverflow.com/a/33257393, but you can use a combination of Ruby's URI::HTTPS.build method along with Rails' #to_query method that they monkeypatch onto the Hash class. #to_query also handles URI encoding, so you won't need to explicitly do that here, either.

richardxia · 2023-07-06T05:26:17Z

app/controllers/auth0_controller.rb

@@ -0,0 +1,31 @@
+# ./app/controllers/auth0_controller.rb
+class Auth0Controller < ApplicationController
+  def authentication


Nit: Rails controller methods are generally named as verbs, e.g. create, update, etc. Could this be renamed to authenticate?

richardxia · 2023-07-06T05:28:54Z

app/controllers/auth0_controller.rb

Nit: Could this be renamed to auth_controller.rb, and the class renamed to AuthController? Even though this is tightly coupled to the Auth0 service, it'll make it easier to navigate the Rails app if the controller name matches the URL route name.

richardxia · 2023-07-06T05:35:04Z

config/auth0.yml

+development:
+  auth0_client_id: <%= ENV["AUTH0_CLIENT_ID"] %>
+  auth0_client_secret: <%= ENV["AUTH0_CLIENT_SECRET"] %>
+  auth0_domain: <%= ENV["AUTH0_DOMAIN"] %>
+test:
+  auth0_client_id: <%= ENV["AUTH0_CLIENT_ID"] %>


Would it be possible to do this more like our other environment variables, which are read in from config/application.rb?

Looks like we also set some config values that are specific to our dev, test, prod environments here: config/environments.rb

EDIT: I also see a database.yml file in the config directory. Looks like we are using a few different patterns for defining ENV vars

richardxia · 2023-07-06T05:37:59Z

config/initializers/auth0.rb

+# ./config/initializers/auth0.rb
+AUTH0_CONFIG = Rails.application.config_for(:auth0)
+
+Rails.application.config.middleware.use OmniAuth::Builder do


We should decide on how we want to support this for development, testing, and production environments, but if we decide to not have Auth0 enabled for development environments, we should guard all of this with a check to see whether the Auth0 integration should be enabled. You can see how we do this for other external integrations, but they generally involve checking an enabled attribute on something configured on the Rails.application.config object.

richardxia · 2023-07-06T05:52:12Z

config/routes.rb

+  get 'auth/auth0', to: 'auth0#authentication', as: :authentication
+  get '/auth/failure' => 'auth0#failure'
+  get '/auth/logout' => 'auth0#logout'
+  get '/auth/auth0/callback' => 'auth0#callback'


Could you group all the routes with the same prefix under the same block? I think you can either do this with a resources call, where you set the only keyword argument to the empty list to block the creation of the usual CRUD routes, or you may be able to do this with a namespace call. I think the resources call may be preferable, since you should be able declare that all the nested routes are associated with the same Auth0Controller and not have to explicitly declare that with each route.

I haven't tested it, but I think it would look like the following:

Suggested change

get 'auth/auth0', to: 'auth0#authentication', as: :authentication

get '/auth/failure' => 'auth0#failure'

get '/auth/logout' => 'auth0#logout'

get '/auth/auth0/callback' => 'auth0#callback'

resources :auth, controller: 'auth0' do

get :auth0, to: 'auth0#authentication', as: :authentication

get :failure

get :logout

get 'auth0/callback' => 'auth0#callback'

end

richardxia · 2023-07-06T06:04:03Z

Gemfile

@@ -56,3 +56,6 @@ end

 gem 'pdfcrowd'
 gem 'google-cloud-translate'
+
+gem 'omniauth-auth0', '~> 3.0'
+gem 'omniauth-rails_csrf_protection', '~> 1.0' # prevents forged authentication requests


Do we need this for SPAs? I'm not actually even sure how we can make this work with our app, since we would need a way to extract the CSRF token from the API responses in our client-side app.

This was part of the example code, i can try it without and see if it works.

schroerbrian

Thanks for the contribution, Will! Looking mostly good to me. Thanks again for getting this started.

schroerbrian · 2023-07-11T22:27:21Z

config/auth0.yml

+development:
+  auth0_client_id: <%= ENV["AUTH0_CLIENT_ID"] %>
+  auth0_client_secret: <%= ENV["AUTH0_CLIENT_SECRET"] %>
+  auth0_domain: <%= ENV["AUTH0_DOMAIN"] %>
+test:
+  auth0_client_id: <%= ENV["AUTH0_CLIENT_ID"] %>


Looks like we also set some config values that are specific to our dev, test, prod environments here: config/environments.rb

EDIT: I also see a database.yml file in the config directory. Looks like we are using a few different patterns for defining ENV vars

schroerbrian · 2023-07-12T17:17:23Z

app/controllers/auth0_controller.rb

+    )
+  end
+
+    def callback


Looks like these subsequent method definitions have extra indentation. The linting tests seem to have caught this as well. If you run bundle exec rake rubocop in the app directory, it will point you to any syntactical issues with the new code

oops fixed!

lgarofalo · 2023-07-13T01:10:19Z

With this change, we can access the login page at /auth/auth0. After login, we are successfully redirected to the callback page which i just set as /api-docs for now.

We can access /auth/auth0 on the api server via the frontend path /api/auth/auth0, this seems to work by default, so if we were to deploy this, we would need no other changes to access it in staging.

Example from dev

askdarcel-api-api-1  | /usr/src/app/app/controllers/auth0_controller.rb:8: warning: URI.escape is obsolete
askdarcel-api-api-1  | method=GET path=/auth/auth0 format=html controller=Auth0Controller action=authentication status=302 duration=2.22 view=0.00 db=0.00 location=https:///authorize

lgarofalo · 2023-07-13T01:12:11Z

If we need to hush up robocop linting, we can do that, some examples are in the codebase, we can also add to .rubocop.yaml

lgarofalo · 2023-07-13T01:14:14Z

Seems good to me for testing, we'll need to make sure the env configs are wired in properly and I can add those to staging in advance if you share with me what key/values to use.

richardxia · 2023-07-24T03:43:44Z

If we need to hush up robocop linting, we can do that, some examples are in the codebase, we can also add to .rubocop.yaml

I would strongly request that we don't silence these lint failures; they're due to inconsistently indenting sibling methods with both 2 and 4 spaces. I would really prefer that all methods in the same scope be indented by the same amount (and I would also really prefer consistently using the same indentation style for the entire codebase).

With this change, we can access the login page at /auth/auth0. After login, we are successfully redirected to the callback page which i just set as /api-docs for now.

wi11 requested review from jjfreund, richardxia, schroerbrian and lgarofalo June 21, 2023 04:23

richardxia reviewed Jul 6, 2023

View reviewed changes

schroerbrian reviewed Jul 12, 2023

View reviewed changes

lgarofalo approved these changes Jul 13, 2023

View reviewed changes

Add login page route for auth0

d7663fa

With this change, we can access the login page at /auth/auth0. After login, we are successfully redirected to the callback page which i just set as /api-docs for now.

wi11 force-pushed the auth0 branch from 141bbee to d7663fa Compare August 22, 2023 05:38

Merge branch 'master' into auth0

279cbcf

wi11 closed this Sep 8, 2023

wi11 reopened this Sep 8, 2023

wi11 merged commit 35db181 into master Sep 8, 2023
8 checks passed

wi11 deleted the auth0 branch September 8, 2023 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add login page route for auth0 #718

Add login page route for auth0 #718

wi11 commented Jun 21, 2023

richardxia left a comment

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

schroerbrian Jul 11, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

richardxia Jul 6, 2023

wi11 Aug 22, 2023

schroerbrian left a comment

schroerbrian Jul 11, 2023

schroerbrian Jul 12, 2023

wi11 Aug 22, 2023

lgarofalo commented Jul 13, 2023

lgarofalo commented Jul 13, 2023

lgarofalo commented Jul 13, 2023

richardxia commented Jul 24, 2023

-  get 'auth/auth0', to: 'auth0#authentication', as: :authentication
-  get '/auth/failure' => 'auth0#failure'
-  get '/auth/logout' => 'auth0#logout'
-  get '/auth/auth0/callback' => 'auth0#callback'
+  resources :auth, controller: 'auth0' do
+    get :auth0, to: 'auth0#authentication', as: :authentication
+    get :failure
+    get :logout
+    get 'auth0/callback' => 'auth0#callback'
+  end

Add login page route for auth0 #718

Add login page route for auth0 #718

Conversation

wi11 commented Jun 21, 2023

richardxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schroerbrian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgarofalo commented Jul 13, 2023

lgarofalo commented Jul 13, 2023

lgarofalo commented Jul 13, 2023

richardxia commented Jul 24, 2023