skip onboarding

[gumaerc]

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/6790

Description (What does it do?)

This PR adds the ability to skip onboarding when creating a new user, instead redirecting to the sanitized next query string param if available.

How can this be tested?

• In your .env file, set `SOCIAL_AUTH_ALLOWED_REDIRECT_HOSTS=["google.com"]
• Make sure you have Keycloak configured, and you have an email address not currently in use on the Keycloak instance you are using (If you have a GMail account, you can follow these instructions: https://github.com/mitodl/hq/issues/6790#issuecomment-2674795563)
• Spin up mit-learn on this branch
• Open an incognito window and browse to http://localhost:8062/login/ol-oidc?skip_onboarding=true&next=https://google.com/
• Click on "Sign Up"
• Fill out the details and click "Sign Up"
• Check the email address you typed in, and there should be a link there to verify your email, click it
• After clicking the link, you should be redirected to Google

[shanbady]

Holding off on my review until #2061 merges since it makes setting up keycloak locally easier

[shanbady]

/gemini review

[gemini-code-assist]

Code Review

This PR introduces the ability to skip onboarding for new users, redirecting them to a specified URL after email verification. The implementation appears well-structured, with added tests and configuration. However, there are a few areas where improvements can be made to enhance security and maintainability.

Summary of Findings

• Open Redirect Vulnerability: The current implementation relies on SOCIAL_AUTH_ALLOWED_REDIRECT_HOSTS for redirect sanitization. It's crucial to ensure that this list is comprehensive and that the sanitization logic is robust to prevent open redirect vulnerabilities.
• Code Clarity and Readability: The logic for determining allowed_hosts in user_onboarding could be simplified for better readability. Consider using a more concise approach to handle the conditional addition of request_host.
• Testing: The test case test_user_onboarding_actions could benefit from additional assertions to ensure that the sanitize_redirect function is called with the expected arguments.

Assessment

The code changes introduce a valuable feature for skipping onboarding. While the implementation is generally sound, addressing the identified security and maintainability concerns is crucial before merging. I recommend focusing on the open redirect vulnerability and improving code clarity. Please have others review and approve this code before merging.

[gemini-code-assist]

This logic for determining allowed_hosts can be simplified using the or operator for better readability.

Consider using a more concise approach to handle the conditional addition of request_host.

        allowed_hosts = [
            *allowed_hosts,
            request_host,
        ] if allowed_hosts else [request_host]

[gumaerc]

Not doing this, as our automatic formatting rules override it.

[gemini-code-assist]

Consider adding an assertion to verify that urlparse(next_qs).netloc is actually present in expected_allowed_hosts before proceeding with the rest of the test. This will help ensure that the test is actually testing what it intends to test.

Suggested change

	netloc = urlparse(next_qs).netloc if next_qs else None
	expected_allowed_hosts = (
	[*social_auth_allowed_redirect_hosts, "example.com"]
	if social_auth_allowed_redirect_hosts
	else ["example.com"]
	)
	netloc = urlparse(next_qs).netloc if next_qs else None
	expected_allowed_hosts = (
	[*social_auth_allowed_redirect_hosts, "example.com"]
	if social_auth_allowed_redirect_hosts
	else ["example.com"]
	)
	if netloc:
	assert netloc in expected_allowed_hosts

[gumaerc]

This is also kind of a weird suggestion, as further down in the test netloc is being used with:

    expected_next = None
    if is_new and not skip_onboarding:
        expected_next = "http://example.com"
    elif next_qs and netloc in expected_allowed_hosts:
        expected_next = next_qs

The test then asserts that backend.strategy.session_set is called to set the next query string param with the proper value.

[shanbady]

/gemini summary

[gemini-code-assist]

Summary of Changes

This pull request introduces the ability to skip the onboarding process for new users and redirect them to a specified URL after email verification. It modifies the authentication pipeline to check for a skip_onboarding flag in the session. If the flag is present, the user is redirected to the URL provided in the next query parameter, after sanitizing it against a list of allowed hosts. The settings file is updated to include skip_onboarding and next in the list of session stored fields. A test case is added to verify the new onboarding logic.

Highlights

• Skip Onboarding Feature: Adds functionality to bypass the onboarding flow for new users based on a skip_onboarding flag.
• Redirect Sanitization: Implements sanitization of the next URL to prevent redirects to untrusted hosts.
• Session Management: Stores skip_onboarding and next in the session to persist the skip onboarding preference and the redirect URL.
• Testing: Adds a comprehensive test case to verify the new onboarding logic, covering different scenarios with and without the skip_onboarding flag and next URL.

Changelog

• authentication/pipeline/user.py
  • Imports sanitize_redirect from social_core.utils.
  • Modifies user_onboarding function to check for skip_onboarding flag in the session.
  • If skip_onboarding is false, the user is redirected to the onboarding flow.
  • If skip_onboarding is true, the user is redirected to the sanitized next URL.
  • Adds request_host to the allowed hosts for redirect sanitization.
• authentication/pipeline/user_test.py
  • Imports urlparse from urllib.parse.
  • Adds a new test case test_user_onboarding_actions to verify the new onboarding logic.
  • The test case covers different scenarios with and without the skip_onboarding flag and next URL.
  • Mocks the backend and strategy objects to simulate the authentication flow.
  • Asserts that the user is redirected to the correct URL based on the skip_onboarding flag and next URL.
• main/settings.py
  • Adds skip_onboarding and next to SOCIAL_AUTH_FIELDS_STORED_IN_SESSION.

Activity

• shanbady held off on review until APISIX integration #2061 merges.
• shanbady requested a review using /gemini review.
• shanbady requested a summary using /gemini summary.
• gemini-code-assist[bot] suggested simplifying the logic for determining allowed_hosts using the or operator.
• gemini-code-assist[bot] suggested adding an assertion to verify that urlparse(next_qs).netloc is actually present in expected_allowed_hosts.