Closed eddierubeiz closed 1 week ago
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!
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?
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 aget 'logout'
. If Microsoft SSO is turned on:passwords
controller to make it unusable (see below)devise_for :users
get 'login'
andpost 'login'
routes, which displayed and processed the login form, respectivelyuser_entra_id_omniauth_authorize_path
requires aPOST
request for security reasons, so we won't be pointing/login
to it in this PR.auth#sso_logout
, which logs the user out and then sends them to the Microsoft logout page.Changes to
env.rb
anddevise
:log_in_using_microsoft_sso
, turns the entire feature on and off;true
, you will also need values set for these three newenv
values which are needed to configure the new auth provider in thedevise.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.);Controllers
controllers/auth_controller.rb
entra_id
andlogout
.entra_id
is the callback method where users land after being successfully authenticatedDevise::PasswordsController
to make it unusable ifScihistDigicoll::Env.lookup(:log_in_using_microsoft_sso)
is true.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
omniauth-entra-id
andomniauth-rails_csrf_protection
;scihist.rake
spec/requests/auth_controller_spec.rb
which tests the new auth controller. It includes a bunch of tests that used to live inspec/system/login_spec.rb
but were slow and flaky, and adds more.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.login_spec.rb
is a test to show that post-login redirect works.Notes on logout
Currently, clicking on any of the logout buttons: