Skip to content
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

Microsoft SSO ( keywords Entra / Azure / OmniAuth / Oauth ) #2769

Merged
merged 49 commits into from
Nov 12, 2024

Conversation

eddierubeiz
Copy link
Contributor

@eddierubeiz eddierubeiz commented Oct 25, 2024

Ref #2564
We want to make sure we get @Gabz73 's blessing before merging it.
General documentation is in the wiki; feel free to append or emend.

Changes to routes

Because we may need to redirect users to the logout page, we replaced the old delete 'logout' route with a get 'logout'.
If Microsoft SSO is turned on:

  • we override Devise's passwords controller to make it unusable (see below)
  • we implicitly define some Microsoft SSO routes by editing devise_for :users
  • we turn of the get 'login' and post 'login' routes, which displayed and processed the login form, respectively
    • Note: user_entra_id_omniauth_authorize_path requires a POST request for security reasons, so we won't be pointing /login to it in this PR.
  • the new logout will go to auth#sso_logout, which logs the user out and then sends them to the Microsoft logout page.

Changes to env.rb anddevise

  • A feature switch, :log_in_using_microsoft_sso, turns the entire feature on and off;
    • If you set it to true, you will also need values set for these three new env values which are needed to configure the new auth provider in the devise.rb initializer:
      • :microsoft_sso_client_id identifies the app to Microsoft SSO;
      • :microsoft_sso_client_secret authenticates the app to Microsoft SSO;
      • :microsoft_sso_tenant_id identifies the Microsoft SSO directory the app wants to check (namely the Institute one. This ID is the same for dev, staging and prod.);
  • We are not making any changes to the User class other than adding the above auth provider.

Controllers

  • A new controller, controllers/auth_controller.rb
  • handles the interaction with entra (via two non-private methods: entra_id and logout.
    • entra_id is the callback method where users land after being successfully authenticated
  • We subclass Devise::PasswordsController to make it unusable if ScihistDigicoll::Env.lookup(:log_in_using_microsoft_sso) is true.
  • We add a method to ApplicationController so Devise knows what to do with users who just logged in. (since you can now log in from any page in the app, we want you to go back to wherever you were before after successfully logging in).

Other changes

  • We add a login / logout button on the footer.
  • Two new gems, omniauth-entra-id and omniauth-rails_csrf_protection;
  • We change some comment verbiage in scihist.rake
  • A new test file spec/requests/auth_controller_spec.rb which tests the new auth controller. It includes a bunch of tests that used to live in spec/system/login_spec.rb but were slow and flaky, and adds more.
  • Another new test file, spec/requests/passwords_spec.rb, contains some extra smoke tests, just to make sure password workflows are still functioning when :log_in_using_microsoft_sso is not set .
    It should help guard against regression bugs that could creep into our old auth workflows while we are using the new ones. If and when we need to turn off Microsoft log in temporarily in the future for whatever reason, we have a bit more confidence that it will it works the way it's supposed to.
  • The only thing left in login_spec.rb is a test to show that post-login redirect works.

Notes on logout

Currently, clicking on any of the logout buttons:

  • logs you out of the app
  • if SSO is turned on, subsequently redirects you to the Microsoft SSO page.
  • From the SSO page, you are supposed to get redirected to the root path of the app. It's Microsoft, so your mileage may vary.

@eddierubeiz eddierubeiz marked this pull request as ready for review October 30, 2024 21:09
don t allow the app to boot if you say you want SSO but don t provide the settings for SSO
@eddierubeiz eddierubeiz changed the title Microsoft SSO (Entra Microsoft SSO (Entra) Oct 31, 2024
@eddierubeiz eddierubeiz changed the title Microsoft SSO (Entra) Microsoft SSO ( keywords Entra / Azure / OmniAuth / Oauth ) Nov 1, 2024
@jrochkind
Copy link
Contributor

Looks pretty great overall, thanks!

A few comments, some of them are questions for discusisons, I'm not sure if they need to be done or not just bringing it up to see what you think.

Can you link to wiki page to make it easier for me to find and review? Thanks!

@eddierubeiz
Copy link
Contributor Author

@jrochkind
Copy link
Contributor

Wiki looks good I think, but what do you think about putting it on it's own wiki page, and link to it from the heroku page?

It's kind of lengthy content, that could get lengthier if we need more notes, and isn't really about our heroku deployment.

I also wonder if you want to note that Entra registrations are tied to hostnames, so if we add/change a hostname they need to be edited?

Also -- can you add back to README the rake tasks for creating users with passwords, that we'll want in development environments?

@eddierubeiz
Copy link
Contributor Author

@eddierubeiz eddierubeiz merged commit 85532fc into master Nov 12, 2024
1 check passed
@eddierubeiz eddierubeiz deleted the microsoft_sso branch November 12, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants