Improvements to authentication workflow

This commit responds to code review feedback for TCO-29. It
includes the following changes:

* Removal of the fake_auth_configuration initializer. We now
evaluate fake auth using the FakeAuthConfig module directly.
As part of this change, `FakeAuthConfig#fake_auth_status` has been
renamed to `FakeAuthConfig#fake_auth_enabled?` and lightly
refactored for clarity.
* Revised documentation.
* Removal of a superfluous `save` call in
`OmniauthCallBacksController#developer`.
* Updates to multiple tests.

README.md

@@ -16,24 +16,35 @@ To see a current list of commands, run `make help`.
 
 `UNPAYWALL_EMAIL`: email address to include in API call as required in their [documentation](https://unpaywall.org/products/api). Your personal email is appropriate for development. Deployed and for tests, use the timdex moira list email.
 
-#### Authentication
+### Optional
+
+`PLATFORM_NAME`: The value set is added to the header after the MIT Libraries logo. The logic and CSS for this comes
+from our theme gem.
+
+### Authentication
+
+#### Required in all environments
 
 Access to some of the config values below is limited. Please contact someone in the EngX team if you need help locating
 them.
 
 `BASE_URL`: The base url for the app. This is required for Omniauth config.
-`FAKE_AUTH_CONFIG`: Switches Omniauth to developer mode when set to `true`. Fake auth is also enabled whenever the
-app is started in the development environment, or if the app is a PR build (see `HEROKU_APP_NAME` under optional
-variables).
-`OP_HOST`: The OID provider hostname, required for authentication. (Do not include URL prefix.)
-`OP_SECRET_KEY`: The secret key for the OID client.
-`OP_CLIENT_ID`: The identifier for the OID client.
-`OP_ISSUER`: The URL for the OIDC issuer. This can be found in the Touchstone OpenID metadata.
-
-### Optional
-
-`HEROKU_APP_NAME`: Used by the FakeAuthConfig module to determine whether an app is a PR build.
-`PLATFORM_NAME`: The value set is added to the header after the MIT Libraries logo. The logic and CSS for this comes from our theme gem.
+`OPENID_HOST`: The OID provider hostname, required for authentication. (Do not include URL prefix.)
+`OPENID_SECRET_KEY`: The secret key for the OID client.
+`OPENID_CLIENT_ID`: The identifier for the OID client.
+`OPENID_ISSUER`: The URL for the OIDC issuer. This can be found in the Touchstone OpenID metadata.
+
+#### Required in PR builds
+
+The config below is needed to run Omniauth in developer mode in Heroku review apps. Rather than relying upon a single
+ENV value, we use the `FakeAuthConfig` module to perform additional checks that confirm whether developer mode should
+be enabled. This assures that developer mode is never enabled in staging or production apps.
+
+`FAKE_AUTH_ENABLED`: Switches Omniauth to developer mode when set. If unset, PR builds will attempt to authenticate with
+OIDC, which will fail as their domains are not registered with the provider. (Note: Developer mode is also enabled
+whenever the app is started in the development environment.)
+`HEROKU_APP_NAME`: Used by the FakeAuthConfig module to determine whether an app is a PR build. If this is set along
+with `FAKE_AUTH_ENABLED`, then Omniauth will use Developer mode.
 
 ## Documentation
 

app/controllers/users/omniauth_callbacks_controller.rb

@@ -2,6 +2,8 @@
 # [the Devise docs](https://www.rubydoc.info/gems/devise_token_auth/DeviseTokenAuth/OmniauthCallbacksController) for
 # additional information about this controller.
 class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController
+  include FakeAuthConfig
+
   def openid_connect
     @user = User.from_omniauth(request.env['omniauth.auth'])
     sign_in_and_redirect @user, event: :authentication
@@ -10,10 +12,9 @@ def openid_connect
 
   # Developer authentication is used in local dev and PR builds.
   def developer
-    raise 'Invalid Authentication' unless Rails.configuration.fake_auth_enabled
+    raise 'Invalid Authentication' unless FakeAuthConfig.fake_auth_enabled?
 
     @user = User.from_omniauth(request.env['omniauth.auth'])
-    @user.save
     sign_in_and_redirect @user, event: :authentication
     flash[:notice] = "Welcome, #{@user.email}!"
   end

app/models/concerns/fake_auth_config.rb

@@ -1,23 +1,20 @@
 module FakeAuthConfig
   # Used in an initializer to determine if the application is configured and allowed to use fake authentication.
-  def self.fake_auth_status
-    return true if fake_auth_enabled? && app_name_pattern_match?
-
-    false
+  def self.fake_auth_enabled?
+    fake_auth_env? && app_name_pattern_match?
   end
 
   # Default to fake auth in development unless FAKE_AUTH_ENABLED=false. This allows rake tasks to run without loading
   # ENV.
-  private_class_method def self.fake_auth_enabled?
+  private_class_method def self.fake_auth_env?
     if Rails.env.development? && ENV['FAKE_AUTH_ENABLED'].nil?
       true
     else
       ENV['FAKE_AUTH_ENABLED'] == 'true'
     end
   end
 
-  # Checks if the current app is a Heroku pipeline app, in which case fake_auth should be enabled.
-  # In test ENV we require setting a fake app name to allow for testing of the pattern.
+  # Check if the app is a PR build. This assures that fake auth is never enabled in staging or prod.
   private_class_method def self.app_name_pattern_match?
     return true if Rails.env.development?
 

app/models/user.rb

@@ -9,7 +9,9 @@
 #  updated_at :datetime         not null
 #
 class User < ApplicationRecord
-  if Rails.configuration.fake_auth_enabled
+  include FakeAuthConfig
+
+  if FakeAuthConfig.fake_auth_enabled?
     devise :omniauthable, omniauth_providers: [:developer]
   else
     devise :omniauthable, omniauth_providers: [:openid_connect]
@@ -22,7 +24,7 @@ class User < ApplicationRecord
   # traverse the response hash differently than with OIDC, as developer mode returns metadata in a different structure.
   # @param auth Hash The authentication response from Omniauth.
   def self.from_omniauth(auth)
-    if Rails.configuration.fake_auth_enabled
+    if FakeAuthConfig.fake_auth_enabled?
       uid = auth.uid
       email = auth.info.email
     else

app/views/layouts/_site_nav.html.erb

@@ -15,7 +15,7 @@
             <%= button_to("Sign out", destroy_user_session_path, class: 'action-auth', id: "sign_in",
                           method: :delete) %>
           <% else %>
-            <% if Rails.configuration.fake_auth_enabled %>
+            <% if FakeAuthConfig.fake_auth_enabled? %>
               <%= button_to("Sign in", user_developer_omniauth_authorize_path, id: "sign_in", class: 'action-auth',
                             method: :post) %>
             <% else %>

config/initializers/devise.rb

@@ -4,25 +4,26 @@
   # :mongoid (bson_ext recommended) by default. Other ORMs may be
   # available as additional gems.
   require 'devise/orm/active_record'
+  require "#{Rails.root}/app/models/concerns/fake_auth_config.rb"
 
   config.sign_out_via = :delete
 
-  if Rails.configuration.fake_auth_enabled
+  if FakeAuthConfig.fake_auth_enabled?
     config.omniauth :developer
   else
     # OIDC configuration
     config.omniauth :openid_connect, {
       name: :openid_connect,
       scope: ['openid', 'email', 'profile'],
       claims: ['name', 'nickname', 'preferred_username', 'given_name', 'middle_name', 'family_name', 'email', 'profile'],
-      issuer: ENV['OP_ISSUER'],
+      issuer: ENV['OPENID_ISSUER'],
       discovery: true,
       response_type: :code,
       uid_field: 'kerberos_id',
       client_options: {
-        host: ENV['OP_HOST'],
-        identifier: ENV['OP_CLIENT_ID'],
-        secret: ENV['OP_CLIENT_SECRET'],
+        host: ENV['OPENID_HOST'],
+        identifier: ENV['OPENID_CLIENT_ID'],
+        secret: ENV['OPENID_CLIENT_SECRET'],
         redirect_uri: [ENV['BASE_URL'], '/users/auth/openid_connect/callback'].join
       },
     }

test/integration/authentication_test.rb

@@ -17,13 +17,15 @@ def silence_omniauth
     OmniAuth.config.logger = previous_logger
   end
 
-  test 'accessing callback without credentials redirects to signin' do
+  test 'accessing callback with bad credentials does not create a new user' do
+    user_count = User.count
     OmniAuth.config.mock_auth[:openid_connect] = :invalid_credentials
     silence_omniauth do
       get '/users/auth/openid_connect/callback'
       follow_redirect!
     end
     assert_response :success
+    assert_equal(user_count, User.count)
   end
 
   test 'new users can authenticate' do

test/models/fake_auth_test.rb

@@ -5,16 +5,18 @@ class FakeAuthTest < ActiveSupport::TestCase
 
   test 'fakeauth disabled' do
     ClimateControl.modify(
-      FAKE_AUTH_ENABLED: 'false',
-      HEROKU_APP_NAME: 'thesis-submit-pr-123'
+      FAKE_AUTH_ENABLED: 'false'
     ) do
-      assert_equal(false, FakeAuthConfig.fake_auth_status)
+      assert_equal(false, FakeAuthConfig.fake_auth_enabled?)
     end
+  end
+
+  test 'fakeauth disabled pr apps' do
     ClimateControl.modify(
       FAKE_AUTH_ENABLED: 'false',
-      HEROKU_APP_NAME: 'thesis-dropbox-pr-123'
+      HEROKU_APP_NAME: 'tacos-api-pipeline-pr-1'
     ) do
-      assert_equal(false, FakeAuthConfig.fake_auth_status)
+      assert_equal(false, FakeAuthConfig.fake_auth_enabled?)
     end
   end
 
@@ -23,33 +25,33 @@ class FakeAuthTest < ActiveSupport::TestCase
       FAKE_AUTH_ENABLED: 'true',
       HEROKU_APP_NAME: 'tacos-api-pipeline-pr-1'
     ) do
-      assert_equal(true, FakeAuthConfig.fake_auth_status)
+      assert_equal(true, FakeAuthConfig.fake_auth_enabled?)
     end
     ClimateControl.modify(
       FAKE_AUTH_ENABLED: 'true',
       HEROKU_APP_NAME: 'tacos-api-pipeline-pr-500'
     ) do
-      assert_equal(true, FakeAuthConfig.fake_auth_status)
+      assert_equal(true, FakeAuthConfig.fake_auth_enabled?)
     end
   end
 
   test 'fakeauth enabled no HEROKU_APP_NAME' do
     ClimateControl.modify FAKE_AUTH_ENABLED: 'true' do
-      assert_equal(false, FakeAuthConfig.fake_auth_status)
+      assert_equal(false, FakeAuthConfig.fake_auth_enabled?)
     end
   end
 
   test 'fakeauth enabled production app name' do
     ClimateControl.modify FAKE_AUTH_ENABLED: 'true',
                           HEROKU_APP_NAME: 'tacos-prod' do
-      assert_equal(false, FakeAuthConfig.fake_auth_status)
+      assert_equal(false, FakeAuthConfig.fake_auth_enabled?)
     end
   end
 
   test 'fakeauth enabled staging app name' do
     ClimateControl.modify FAKE_AUTH_ENABLED: 'true',
                           HEROKU_APP_NAME: 'tacos-stage' do
-      assert_equal(false, FakeAuthConfig.fake_auth_status)
+      assert_equal(false, FakeAuthConfig.fake_auth_enabled?)
     end
   end
 end