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

Use DfE sign in for staff authentication #408

Merged
merged 13 commits into from
Dec 6, 2023

Conversation

steventux
Copy link
Contributor

@steventux steventux commented Oct 31, 2023

Context

We currently have a devise based login for the support console of AYTQ/Check.
This is complexity we probably don’t need now we have a DfE Sign in integration.

Switch to using DfE Signin.

Changes proposed in this pull request

  • Remove Devise configuration for Staff/Support authentication.
  • Add staff specific DSI omniauth configuration.
  • Reuse common auth methods across Check/Support controllers.
  • Add support specific sign in and sign out functionality similar to Check.
  • Refactor system specs to cover DSI authentication.
  • Refactor Check system specs to work with common authentication steps.

Guidance to review

Introduces a new role dependency which is configured in the env as DFE_SIGN_IN_API_STAFF_ROLE_CODES, awaiting confirmation of role code from the sign in team.
The env var DFE_SIGN_IN_STAFF_REDIRECT_URL will need to match a valid DSI redirect URI in the service configuration.
Likewise the post logout redirect URI will need to be configured in DSI to match the appropriate omniauth config.
This PR will need reviewing on the test environment with the appropriate configuration as review apps won't play nicely with DSI.

Link to Trello card

https://trello.com/c/1PcLM39J/117-use-dfe-signin-for-support-login

Checklist

  • Attach to Trello card
  • Rebased main
  • Cleaned commit history
  • Tested by running locally

@steventux
Copy link
Contributor Author

@steventux steventux changed the title Use Dfe sign in for support staff authentication Use DfE Sign In for support staff authentication Oct 31, 2023
@steventux steventux changed the title Use DfE Sign In for support staff authentication Use DfE sign in for staff authentication Oct 31, 2023
@steventux steventux marked this pull request as ready for review October 31, 2023 11:35
@steventux steventux force-pushed the 117-use-dfe-signin-for-support-login branch 2 times, most recently from b57df9e to c87e5a9 Compare October 31, 2023 15:29
Copy link
Contributor

@felixclack felixclack left a comment

Choose a reason for hiding this comment

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

Added a non-blocking comment.

@@ -29,14 +31,14 @@ def add_auth_attributes_to_session
end

def check_user_access_to_service
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking: Did you consider making this def role instead?

It would allow us to remove line 11 completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @felixclack - No I didn't spot that, like it, will amend.

@steventux steventux changed the title Use DfE sign in for staff authentication [Do not merge] Use DfE sign in for staff authentication Nov 14, 2023
@steventux steventux force-pushed the 117-use-dfe-signin-for-support-login branch from 70f2150 to 7093059 Compare December 5, 2023 15:28
@steventux steventux force-pushed the 117-use-dfe-signin-for-support-login branch from 7093059 to 196c7e5 Compare December 6, 2023 09:16
@steventux steventux changed the title [Do not merge] Use DfE sign in for staff authentication Use DfE sign in for staff authentication Dec 6, 2023
@steventux steventux merged commit 4a5d6f1 into main Dec 6, 2023
18 checks passed
@steventux steventux deleted the 117-use-dfe-signin-for-support-login branch December 6, 2023 10:49
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.

3 participants