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

WIP: Spike remove gds sso #769

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

WIP: Spike remove gds sso #769

wants to merge 4 commits into from

Conversation

thomasiles
Copy link
Contributor

This is a quick spike to look at what the minimum amount of needed to remove the gds sso gem from our application.

It's not complete and not very tested. It works fine but:

  • not all tests have been removed
  • nothing has been refactored
  • there are some odd bits from the gem which I've carried over but probably need questioning
  • I haven't removed any columns from the User table

Tom Iles added 4 commits November 15, 2023 16:49
We no longer use this method of authenticating users so can remove the
code and tests that enable it.
This Gem was used to support the cddo_sso authentication method and is
no longer used.
Remove the Gem, add bits missing from the gem itself.
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -85,6 +84,18 @@ def role_changed_to_editor?
role_changed_to_editor
end

def clear_remotely_signed_out!
# rubocop:disable Rails/SkipsModelValidations
update_attribute(:remotely_signed_out, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why update_attribute is used here - maybe to make sure it happens despite the state the user is in?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the remotely_signed_out! stuff? The existing stuff is reliant on the GOV.UK Signon servers calling an API exposed by gds_sso gem.

if user_provider == :gds
gds_sign_out_path
elsif %i[auth0 cddo_sso mock_gds_sso].include? user_provider
if %i[auth0 mock_gds_sso].include? user_provider
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be updated

end
end

Warden::Manager.serialize_from_session do |(uid, auth_timestamp)|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add more tests around these parts lifted from gds_sso

expect(controller_spy).to have_received :callback_from_omniauth
end
# expect(controller_spy).to have_received :callback_from_omniauth
# end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a gds_sso specific test which I'm not sure we need

expect(controller_spy).to have_received :callback_from_omniauth
end
# expect(controller_spy).to have_received :callback_from_omniauth
# end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a gds_sso specific test which I'm not sure we need

@thomasiles thomasiles marked this pull request as draft November 23, 2023 10:44
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