Enable SSO and ensure trial registrations perform onboarding steps
What does this MR do and why?
Let SSO sign ups experience the new user onboarding as we have this on normal sign ups for growth reasons in SaaS and want to add to the SSO experience as well. Before, due to the verification loop, the user is dropped out of the onboarding loop. This MR will address
- users dropping out of the onboarding loop during verification or by simply navigating away to another page
- intention to start a trial being dropped
- users not completing onboarding in general for SSO
This will also address trial registrations through regular signup being able to exit the specific trial onboarding and defaulting back to non-trial on welcome page redirection.
This should not affect Group Saml, LDAP or other Oauth strategies. Only the social sign ons for SaaS.
This feature is behind the ensure_onboarding
feature flag.
Screenshots or screen recordings
SSO
- before sso trial registration, toggle for trial seen(wrong)
Screen_Recording_2023-05-08_at_2.01.35_PM
- after in sso trial registration, toggle not seen and wording is correct
Screen_Recording_2023-05-08_at_2.14.12_PM
Regular
- before trial registration, toggle for trial seen(wrong)
Screen_Recording_2023-05-09_at_3.33.22_PM
- after in trial registration, toggle not seen and wording is correct
Screen_Recording_2023-05-09_at_3.28.58_PM
How to set up and validate locally
note basically trying to emulate feature spec steps in ee/spec/features/registrations/saas/standard_flow_just_me_creating_project_spec.rb
- Setup to simulate SaaS and restart GDK
- Enable ensure onboarding feature flag and
check_namespace_plan
inrails console
Feature.enable(:ensure_onboarding)
ApplicationSetting.first.update(check_namespace_plan: true)
- Sign up for non SSO(fill in user name and password) with trial(with
/-/trial_registrations/new
path) and notice differences in screenshots above. - Confirm email by finding the confirmation path from last User created in
rails console
Rails.application.routes.url_helpers.user_confirmation_path(confirmation_token: User.last.confirmation_token)
- Sign up for SSO(user github or google)
- set your
development
gitlab.yml
config values - mine looked like the below, setup for github by going on github under developer settings for my profile, redacted my info:
- set your
development:
<<: *base
omniauth:
block_auto_created_users: false
allow_single_sign_on: true
providers:
- { name: 'google_oauth2',
app_id: '',
app_secret: '',
args: { access_type: 'offline', approval_prompt: '' } }
- { name: 'github',
app_id: 'xxxx',
app_secret: 'xxxxx',
args: { scope: "user:email" } }
- sign in by clicking the sign in button the trial page for the SSO provider(with
/-/trial_registrations/new
path) and notice differences in screenshots above. - Validate when on welcome page/almost there that you can redirect to root path and it will redirect you back correctly, staying in the trial.
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #404926
Merge request reports
Activity
changed milestone to %16.0
assigned to @dstull
mentioned in issue #382219 (closed)
5 Warnings This merge request is quite big (662 lines changed), please consider splitting it into multiple merge requests. 1c3b47f3: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 316eca19: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
This merge request contains lines with QA selectors. Please ensure e2e:package-and-test
job is run.QA Selectors
The following changed lines in this MR contain QA selectors:
- file
app/views/devise/shared/_signup_omniauth_provider_list.haml
: -
-
= link_to omniauth_authorize_path(:user, provider, register_omniauth_params), method: :post, class: "btn gl-button btn-default gl-w-full gl-mb-4 js-oauth-login #{qa_selector_for_provider(provider)}", data: { provider: provider }, id: "oauth-login-#{provider}" do
-
-
-
= link_to omniauth_authorize_path(:user, provider, register_omniauth_params(local_assigns)), method: :post, class: "btn gl-button btn-default gl-w-full gl-mb-4 js-oauth-login #{qa_selector_for_provider(provider)}", data: { provider: provider }, id: "oauth-login-#{provider}" do
-
-
-
= link_to omniauth_authorize_path(:user, provider, register_omniauth_params), method: :post, class: "btn gl-button btn-default gl-w-full gl-mb-4 js-oauth-login #{qa_selector_for_provider(provider)}", data: { provider: provider }, id: "oauth-login-#{provider}" do
-
-
-
= link_to omniauth_authorize_path(:user, provider, register_omniauth_params(local_assigns)), method: :post, class: "btn gl-button btn-default gl-w-full gl-mb-4 js-oauth-login #{qa_selector_for_provider(provider)}", data: { provider: provider }, id: "oauth-login-#{provider}" do
-
Please ensure
e2e:package-and-test
job is run and the tests are passing.For the list of known failures please refer to the latest pipeline triage issue.
If your changes are under a feature flag, please check our Testing with feature flags documentation for instructions.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Valery Sizov (
@vsizov
) (UTC+1, 5 hours ahead of@dstull
)Drew Cimino (
@drew
) (UTC+0, 4 hours ahead of@dstull
)frontend Tomas Vik (
@viktomas
) (UTC+2, 6 hours ahead of@dstull
)Tristan Read (
@tristan.read
) (UTC+12, 16 hours ahead of@dstull
)test for spec/features/*
Valery Sizov (
@vsizov
) (UTC+1, 5 hours ahead of@dstull
)Maintainer review is optional for test for spec/features/*
~"group::authentication and authorization" Reviewer review is optional for ~"group::authentication and authorization" Smriti Garg (
@sgarg_gitlab
) (UTC+5.5, 9.5 hours ahead of@dstull
)To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangeradded bugfunctional typebug labels and removed typefeature label
added 295 commits
-
85a26dbc...a3080777 - 294 commits from branch
master
- ea796dd0 - Fix SSO onboarding completion failure
-
85a26dbc...a3080777 - 294 commits from branch
thought so i don't forget
def required_signup_info return unless current_user return unless current_user.role_required? store_location_for :user, request.fullpath redirect_to users_sign_up_welcome_path # do we need our glm params when user bails after signup then the `after_sign_up_path` may not be hit end
removed bugfunctional label
added featureenhancement typefeature labels and removed typebug label
removed keep confidential label
added 227 commits
-
af5c48a7...b10c95f8 - 226 commits from branch
master
- 01aedc99 - Fix SSO onboarding completion failure
-
af5c48a7...b10c95f8 - 226 commits from branch
added 162 commits
-
e75ef7da...6768769a - 160 commits from branch
master
- 17fdf411 - Fix SSO onboarding completion failure
- 42c4a83e - Revert changing for temp oauth flow
-
e75ef7da...6768769a - 160 commits from branch
- Resolved by Doug Stull
added 691 commits
-
69670e58...577469bd - 689 commits from branch
master
- 231b8270 - Fix SSO onboarding completion failure
- c1b6b63d - Revert changing for temp oauth flow
-
69670e58...577469bd - 689 commits from branch
- Resolved by Jessie Young