Implement authentication #67

jazairi · 2024-07-26T21:05:28Z

This adds Touchstone authentication via Devise and Omniauth, along with fake auth for local development and PR builds. The initial auth setup and the fake auth config are in separate commits for ease of review. Please check each commit message for more detailed info on the changesets.

For the time being, I've opted to use Omniauth developer mode for fake auth. We might benefit from using Keycloak in local dev to better simulate a real authentication flow, but setting that proved somewhat complicated and is probably best saved for another time. I've created a spike ticket to investigate further.

To confirm functionality:

Fake auth logic can be tested in this PR app and locally. It's best to confirm both, because the logic that triggers fake auth is different for each environment. (Local checks for the development environment, whereas PR builds check for the HEROKU_APP_NAME env.)
Prod auth can be tested in staging. I've been doing that by deploying this branch, confirming that I can log in, then redeploying main.

db/schema.rb

Why these changes are being introduced: We will need user authentication on this app, as some features will be for authorized staff use only. Relevant ticket(s): * [TCO-29](https://mitlibraries.atlassian.net/browse/TCO-29) How this addresses that need: This adds and configures Devise, Omniauth, and the Omniauth OIDC strategy, so we can authenticate with Touchstone. Devise requires a minimal User model, to which I've added an email field and some validations. This commit does not include fake auth for local dev and PR builds. That feature will come in a subsequent commit. Side effects of this change: * The `sign_out` route is defined manually, as we are not using the `database_authenticatable` module that provides additional routes (including `destroy_user_session_path`). * This initial setup is pretty spare. We will likely need to add some more fields to the User model, and perhaps further Omniauth config as we continue to learn about Okta. * The sign-in/sign-out links are now buttons because Rails has dropped support for `method: :post`. It seems that [Turbo can replicate this behavior](https://discuss.rubyonrails.org/t/rails-7-0-4-link-to-method-post-no-longer-works/82321), but I was still seeing GET requests with Turbo. I'd still prefer to use links, but figuring out how to get them to work felt beyond the scope of this ticket.

Why these changes are being introduced: We need an easy way to bypass authentication in local dev and PR builds. Relevant ticket(s): * [TCO-29](https://mitlibraries.atlassian.net/browse/TCO-29) How this addresses that need: This implements fake auth in a similar way as we've done in ETD, using an initializer that checks the fake auth status as evaluated by a FakeAuthConfig module. Side effects of this change: We're missing an opportunity to use Keycloak for local authentication, which would better simulate the Touchstone process. That may be worth revisiting at some point.

jazairi · 2024-08-01T20:54:29Z

At this point, I'm comfortable putting this into review. The test coverage drop is related to the Omniauth developer strategy details, which we also don't test in ETD. To be clear, though, I'd be happy to add coverage for any/all of what's lacking if the reviewer feels it's appropriate.

JPrevost

This is one of those reviews in which there isn't really a ton of code but a heck of a lot is going on. Great job!

OIDC via state: works as expected
fake auth in PR build: only worked when I added FAKE_AUTH_ENABLED=true which is contrary to the docs (both in what variable is involved and that it doesn't need setting)
worked as advertised (no config changes, fake auth was working, users created as expected)

One test feels a bit misleading, but nothing else is blocking.

README.md

JPrevost · 2024-08-02T12:42:15Z

README.md

 ### Optional

+`HEROKU_APP_NAME`: Used by the FakeAuthConfig module to determine whether an app is a PR build.


This isn't actually something we set right? It just shows up in PR builds and we check it and compare it to our expected PR pattern for this app. If so, I think clarifying that here so someone doesn't think they need to set this someplace would be great.

Yeah, it's automatically added to PR builds. That's a good call, I'll update this to clarify.

app/controllers/users/omniauth_callbacks_controller.rb

app/models/concerns/fake_auth_config.rb

config/locales/devise.en.yml

JPrevost · 2024-08-02T13:07:40Z

test/integration/authentication_test.rb

+    OmniAuth.config.logger = previous_logger
+  end
+
+  test 'accessing callback without credentials redirects to signin' do


I'm not sure this tests for what it says it tests for. It says it should be redirected to the sign in path but only confirms any 200 was received right?

I agree, it could be clearer. We are following a redirect, but not confirming that we land anywhere in particular. I think I'll actually rewrite this to confirm that a new user isn't created.

The docs imply we can actually test what the name says it is testing:

https://api.rubyonrails.org/v7.1.3.4/classes/ActionDispatch/IntegrationTest.html (i.e. that we are on a signin path?)

If not, renaming the test to say what we are testing would be great.

I guess I'm not as concerned about the path as I am that logging in with invalid credentials doesn't create a user. So, I think renaming with some additional logic is the play here, unless you feel otherwise.

Works for me

JPrevost · 2024-08-02T13:12:04Z

test/models/fake_auth_test.rb

+  test 'fakeauth disabled' do
+    ClimateControl.modify(
+      FAKE_AUTH_ENABLED: 'false',
+      HEROKU_APP_NAME: 'thesis-submit-pr-123'


Probably better to use a tacos pattern to confirm specifically that FAKE_AUTH_ENABLED being false is enough to shut it off even if HEROKU_APP_NAME would be allowed for these two assertions.

What, you're criticizing my copypasta? 😅

JPrevost · 2024-08-02T13:12:19Z

test/models/fake_auth_test.rb

+    end
+    ClimateControl.modify(
+      FAKE_AUTH_ENABLED: 'false',
+      HEROKU_APP_NAME: 'thesis-dropbox-pr-123'


JPrevost · 2024-08-02T13:14:31Z

config/initializers/_fake_auth_configuration.rb

+# is available when needed.
+
+require "#{Rails.root}/app/models/concerns/fake_auth_config.rb"
+Rails.application.config.fake_auth_enabled = FakeAuthConfig.fake_auth_status


I'm somewhat curious why we used Rails.application.config.fake_auth_enabled instead of just calling FakeAuthConfig.fake_auth_status throughout. I know this is an ETD pattern carried over, but I'm not currently understanding the added value fwiw.

It does feel DRYer to me to do it in an initializer. That way, we don't have to include the module everywhere we use it. That's only in three places currently, so ymmv as to how much value it adds in practice.

One thing I did consider while copying this logic over, and am considering more seriously now, is whether we really need the FakeAuthConfig module at all. What's the real value of using that over, say, a Flipflop feature? We basically would just need to add ENV['FAKE_AUTH_CONFIG'] (or ENV['FAKE_AUTH_ENABLED'] as it will so be named) in the pipeline and in our local dev environments, right? That doesn't feel any different from how we handle, e.g., API endpoints across multiple environments.

Following some additional discussion in Slack, I think I prefer using the initializer and calling Rails.application.config.fake_auth_enabled rather than FakeAuthConfig for the sake of consistency and safety.

Further amending my take on this. 🫠 I'm dropping the initializer due to the filenaming kludge required (i.e., it must load before the devise initializer so the config will be available there). It feels less wonky to me to require the FakeAuthConfig module where we need it.

JPrevost · 2024-08-02T13:20:31Z

README.md

+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


I believe this should be FAKE_AUTH_ENABLED and it also reads like it is not required in PR builds, but in my testing I had to set FAKE_AUTH_ENABLED to true in the PR build to switch the Sign In button to fake auth

jazairi · 2024-08-02T16:53:14Z

@JPrevost I think the latest commit addresses all of your feedback, but let me know if I missed something. Ultimately, I decided to remove the fake auth initializer. This branch is currently deployed to staging if you'd like to confirm OIDC still works.

Also, while ENV has been updated in staging, I'm waiting to update in prod until this passes review. (Saying that mostly as a note to myself do it before I merge.)

JPrevost

Nice. I like the refactor. Approving, but also made a suggestion on a readme update to be extremely clear that nobody should ever set that value while still explaining what it is used for.

README.md

This adds test coverage and responds to 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`.

jazairi · 2024-08-02T17:40:38Z

@JPrevost Thanks for the detailed review!

mitlib temporarily deployed to tacos-api-pipeline-pr-67 July 26, 2024 21:07 Inactive

JPrevost reviewed Jul 31, 2024

View reviewed changes

db/schema.rb Outdated Show resolved Hide resolved

jazairi force-pushed the tco-29-add-devise branch from 6f49f82 to 14a297b Compare August 1, 2024 20:43

jazairi added 2 commits August 1, 2024 16:46

jazairi force-pushed the tco-29-add-devise branch from 14a297b to 6911eca Compare August 1, 2024 20:48

mitlib temporarily deployed to tacos-api-pipeline-pr-67 August 1, 2024 20:53 Inactive

jazairi requested review from JPrevost and matt-bernhardt August 1, 2024 20:57

JPrevost self-assigned this Aug 2, 2024

JPrevost temporarily deployed to tacos-stage August 2, 2024 13:22 Inactive

JPrevost reviewed Aug 2, 2024

View reviewed changes

mitlib temporarily deployed to tacos-stage August 2, 2024 16:32 Inactive

jazairi temporarily deployed to tacos-api-pipeline-pr-67 August 2, 2024 16:32 Inactive

jazairi requested a review from JPrevost August 2, 2024 16:51

JPrevost approved these changes Aug 2, 2024

View reviewed changes

README.md Show resolved Hide resolved

jazairi force-pushed the tco-29-add-devise branch from 2c15810 to 95d1b82 Compare August 2, 2024 17:37

jazairi temporarily deployed to tacos-api-pipeline-pr-67 August 2, 2024 17:37 Inactive

jazairi force-pushed the tco-29-add-devise branch from 95d1b82 to 1d29f7d Compare August 2, 2024 17:39

jazairi temporarily deployed to tacos-api-pipeline-pr-67 August 2, 2024 17:39 Inactive

jazairi merged commit 0ed43fa into main Aug 2, 2024
1 check passed

jazairi deleted the tco-29-add-devise branch August 2, 2024 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement authentication #67

Implement authentication #67

jazairi commented Jul 26, 2024 •

edited

Loading

jazairi commented Aug 1, 2024 •

edited

Loading

JPrevost left a comment

JPrevost Aug 2, 2024

jazairi Aug 2, 2024

JPrevost Aug 2, 2024

jazairi Aug 2, 2024

JPrevost Aug 2, 2024

jazairi Aug 2, 2024

JPrevost Aug 2, 2024

JPrevost Aug 2, 2024

jazairi Aug 2, 2024

JPrevost Aug 2, 2024

JPrevost Aug 2, 2024

jazairi Aug 2, 2024

jazairi Aug 2, 2024 •

edited

Loading

jazairi Aug 2, 2024

JPrevost Aug 2, 2024

jazairi commented Aug 2, 2024

JPrevost left a comment

jazairi commented Aug 2, 2024

		### Optional

		`HEROKU_APP_NAME`: Used by the FakeAuthConfig module to determine whether an app is a PR build.

Implement authentication #67

Implement authentication #67

Conversation

jazairi commented Jul 26, 2024 • edited Loading

jazairi commented Aug 1, 2024 • edited Loading

JPrevost 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

jazairi Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jazairi commented Aug 2, 2024

JPrevost left a comment

Choose a reason for hiding this comment

jazairi commented Aug 2, 2024

jazairi commented Jul 26, 2024 •

edited

Loading

jazairi commented Aug 1, 2024 •

edited

Loading

jazairi Aug 2, 2024 •

edited

Loading