From 3c832256ae22405224b40159e099589ba826f234 Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Mon, 24 Nov 2025 12:26:14 -0500 Subject: [PATCH 1/7] Add first project onboarding for self-managed Implement a "Create your first project" registration step that appears for the root admin user on their second login to a new self-managed GitLab instance. This feature: - Redirects admins to project creation flow on second sign-in - Only triggers when first login cookie is set - Requires exactly one human user on the instance - Ensures the instance has no existing groups - Guides users through creating their first group and project The onboarding helps new self-managed administrators get started by walking them through creating their first project to store code. Changelog: added --- .../admin/registrations/groups/new/index.js | 53 +++ .../new/components/group_project_fields.vue | 337 ++++++++++++++++++ .../components/project_template_selector.vue | 175 +++++++++ .../registrations/groups/new/constants.js | 5 + .../registrations/groups/new/store/actions.js | 7 + .../registrations/groups/new/store/index.js | 14 + .../groups/new/store/mutation_types.js | 2 + .../groups/new/store/mutations.js | 10 + .../registrations/groups/new/store/state.js | 6 + .../admin/registrations/groups_controller.rb | 76 ++++ app/controllers/sessions_controller.rb | 50 +++ .../standard_namespace_create_service.rb | 80 +++++ .../admin/registrations/groups/new.html.haml | 37 ++ config/routes/admin.rb | 4 + ee/app/controllers/ee/sessions_controller.rb | 15 + ee/app/models/onboarding.rb | 4 + .../groups/new.html.haml_spec.rb | 6 + locale/gitlab.pot | 6 + spec/controllers/sessions_controller_spec.rb | 207 +++++++++-- .../registrations/groups_controller_spec.rb | 240 +++++++++++++ .../standard_namespace_create_service_spec.rb | 208 +++++++++++ .../groups/new.html.haml_spec.rb | 122 +++++++ 22 files changed, 1638 insertions(+), 26 deletions(-) create mode 100644 app/assets/javascripts/pages/admin/registrations/groups/new/index.js create mode 100644 app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue create mode 100644 app/assets/javascripts/registrations/groups/new/components/project_template_selector.vue create mode 100644 app/assets/javascripts/registrations/groups/new/constants.js create mode 100644 app/assets/javascripts/registrations/groups/new/store/actions.js create mode 100644 app/assets/javascripts/registrations/groups/new/store/index.js create mode 100644 app/assets/javascripts/registrations/groups/new/store/mutation_types.js create mode 100644 app/assets/javascripts/registrations/groups/new/store/mutations.js create mode 100644 app/assets/javascripts/registrations/groups/new/store/state.js create mode 100644 app/controllers/admin/registrations/groups_controller.rb create mode 100644 app/services/onboarding/self_managed/standard_namespace_create_service.rb create mode 100644 app/views/admin/registrations/groups/new.html.haml create mode 100644 spec/requests/admin/registrations/groups_controller_spec.rb create mode 100644 spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb create mode 100644 spec/views/admin/registrations/groups/new.html.haml_spec.rb diff --git a/app/assets/javascripts/pages/admin/registrations/groups/new/index.js b/app/assets/javascripts/pages/admin/registrations/groups/new/index.js new file mode 100644 index 00000000000000..0ea2cf01568e81 --- /dev/null +++ b/app/assets/javascripts/pages/admin/registrations/groups/new/index.js @@ -0,0 +1,53 @@ +import Vue from 'vue'; +import GroupProjectFields from '~/registrations/groups/new/components/group_project_fields.vue'; +import createStore from '~/registrations/groups/new/store'; +import GlFieldErrors from '~/gl_field_errors'; +import { parseBoolean } from '~/lib/utils/common_utils'; + +const mountGroupProjectFields = (el, store) => { + if (!el) { + return null; + } + + const { + groupPersisted, + groupId, + groupName, + projectName, + templateName, + initializeWithReadme, + rootUrl, + } = el.dataset; + + return new Vue({ + el, + store, + render(createElement) { + return createElement(GroupProjectFields, { + props: { + importGroup: false, + groupPersisted: parseBoolean(groupPersisted), + groupId: groupId || '', + groupName: groupName || '', + projectName: projectName || '', + templateName: templateName || '', + initializeWithReadme: parseBoolean(initializeWithReadme), + rootUrl, + trackActionForErrors: '', + }, + }); + }, + }); +}; + +const mountCreateGroupProjectFields = () => { + const store = createStore(); + + const elements = document.querySelectorAll('.js-create-group-project-fields'); + + [...elements].map((el) => mountGroupProjectFields(el, store)); + + return new GlFieldErrors(document.querySelectorAll('.gl-show-field-errors')); +}; + +mountCreateGroupProjectFields(); diff --git a/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue b/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue new file mode 100644 index 00000000000000..6a6bdb64032d07 --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue @@ -0,0 +1,337 @@ + + diff --git a/app/assets/javascripts/registrations/groups/new/components/project_template_selector.vue b/app/assets/javascripts/registrations/groups/new/components/project_template_selector.vue new file mode 100644 index 00000000000000..c87e3bc8aeef2c --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/components/project_template_selector.vue @@ -0,0 +1,175 @@ + + diff --git a/app/assets/javascripts/registrations/groups/new/constants.js b/app/assets/javascripts/registrations/groups/new/constants.js new file mode 100644 index 00000000000000..7ab720b3e1123a --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/constants.js @@ -0,0 +1,5 @@ +import { __, s__ } from '~/locale'; + +export const DEFAULT_GROUP_PATH = __('{group}'); +export const DEFAULT_PROJECT_PATH = __('{project}'); +export const DEFAULT_SELECTED_LABEL = s__('ProjectsNew|Select'); diff --git a/app/assets/javascripts/registrations/groups/new/store/actions.js b/app/assets/javascripts/registrations/groups/new/store/actions.js new file mode 100644 index 00000000000000..16c1e9436f777a --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/store/actions.js @@ -0,0 +1,7 @@ +import * as types from './mutation_types'; + +export const setStoreGroupName = ({ commit }, groupName) => + commit(types.SET_STORE_GROUP_NAME, groupName); + +export const setStoreGroupPath = ({ commit }, groupPath) => + commit(types.SET_STORE_GROUP_PATH, groupPath); diff --git a/app/assets/javascripts/registrations/groups/new/store/index.js b/app/assets/javascripts/registrations/groups/new/store/index.js new file mode 100644 index 00000000000000..d711038a4b8b72 --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/store/index.js @@ -0,0 +1,14 @@ +import Vue from 'vue'; +import Vuex from 'vuex'; // eslint-disable-line no-restricted-imports +import * as actions from './actions'; +import mutations from './mutations'; +import createState from './state'; + +Vue.use(Vuex); + +export default () => + new Vuex.Store({ + actions, + mutations, + state: createState(), + }); diff --git a/app/assets/javascripts/registrations/groups/new/store/mutation_types.js b/app/assets/javascripts/registrations/groups/new/store/mutation_types.js new file mode 100644 index 00000000000000..192dcf58e9dc32 --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/store/mutation_types.js @@ -0,0 +1,2 @@ +export const SET_STORE_GROUP_NAME = 'SET_STORE_GROUP_NAME'; +export const SET_STORE_GROUP_PATH = 'SET_STORE_GROUP_PATH'; diff --git a/app/assets/javascripts/registrations/groups/new/store/mutations.js b/app/assets/javascripts/registrations/groups/new/store/mutations.js new file mode 100644 index 00000000000000..c77dc97a9585c2 --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/store/mutations.js @@ -0,0 +1,10 @@ +import * as types from './mutation_types'; + +export default { + [types.SET_STORE_GROUP_NAME](state, groupName) { + state.storeGroupName = groupName; + }, + [types.SET_STORE_GROUP_PATH](state, groupPath) { + state.storeGroupPath = groupPath; + }, +}; diff --git a/app/assets/javascripts/registrations/groups/new/store/state.js b/app/assets/javascripts/registrations/groups/new/store/state.js new file mode 100644 index 00000000000000..c133f246f92af8 --- /dev/null +++ b/app/assets/javascripts/registrations/groups/new/store/state.js @@ -0,0 +1,6 @@ +import { DEFAULT_GROUP_PATH } from '../constants'; + +export default () => ({ + storeGroupName: '', + storeGroupPath: DEFAULT_GROUP_PATH, +}); diff --git a/app/controllers/admin/registrations/groups_controller.rb b/app/controllers/admin/registrations/groups_controller.rb new file mode 100644 index 00000000000000..e885af88c471fe --- /dev/null +++ b/app/controllers/admin/registrations/groups_controller.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Admin + module Registrations + class GroupsController < ApplicationController + skip_before_action :set_confirm_warning + before_action :authorize_create_group!, only: :new + + layout 'minimal' + + feature_category :onboarding + + urgency :low, [:create] + + def new + @group = Group.new( + visibility_level: Gitlab::CurrentSettings.default_group_visibility, + name: "#{current_user.username}-group" + ) + @project = Project.new(namespace: @group, name: "#{current_user.username}-project") + @initialize_with_readme = true + end + + def create + result = service_instance.execute + + if result.success? + actions_after_success(result.payload) + else + @group = result.payload[:group] + @project = result.payload[:project] + @template_name = project_params[:template_name] + @initialize_with_readme = project_params[:initialize_with_readme] + + render :new + end + end + + private + + def authorize_create_group! + access_denied! unless current_user.can_admin_all_resources? + end + + def service_instance + Onboarding::SelfManaged::StandardNamespaceCreateService.new(current_user, group_params: group_params, + project_params: project_params) + end + + def actions_after_success(payload) + redirect_to project_path(payload[:project]) + end + + def group_params + params.require(:group).permit( + :id, + :name, + :path, + :visibility_level, + :organization_id + ).with_defaults(organization_id: Current.organization.id) + end + + def project_params + params.require(:project).permit( + :initialize_with_readme, + :name, + :namespace_id, + :path, + :template_name, + :visibility_level + ) + end + end + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1fd3999f1c1e44..4147f929153190 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -36,6 +36,7 @@ class SessionsController < Devise::SessionsController before_action :save_failed_login, if: :action_new_and_failed_login? before_action :load_recaptcha before_action :set_invite_params, only: [:new] + before_action :set_first_sign_in_cookie_before_redirect, only: [:create] after_action :log_failed_login, if: :action_new_and_failed_login? after_action :verify_known_sign_in, only: [:create] @@ -59,6 +60,7 @@ class SessionsController < Devise::SessionsController CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha' MAX_FAILED_LOGIN_ATTEMPTS = 5 PRESERVE_COOKIES = %w[current_signin_tab preferred_language].freeze + FIRST_SIGN_IN_COOKIE = :first_sign_in_completed def new set_minimum_password_length @@ -360,6 +362,54 @@ def set_invite_params # overridden by EE module def onboarding_status_tracking_label; end + + override :after_sign_in_path_for + + def after_sign_in_path_for(resource) + if should_show_self_managed_project_onboarding?(resource) + cookies.delete(FIRST_SIGN_IN_COOKIE) + new_admin_sign_up_group_path + else + super + end + end + + def should_show_self_managed_project_onboarding?(user) + return false unless user.can_admin_all_resources? + return false unless self_managed? + return false unless first_sign_in_completed? + return false unless user.reset.sign_in_count == 2 + return false unless User.human.one? + return false if Group.any? + + true + end + + def set_first_sign_in_cookie(resource) + return unless resource.can_admin_all_resources? + return if cookies[FIRST_SIGN_IN_COOKIE] + + cookies[FIRST_SIGN_IN_COOKIE] = { + value: true, + expires: 1.year.from_now, + httponly: true, + secure: Rails.env.production? + } + end + + def self_managed? + true + end + + def first_sign_in_completed? + cookies[FIRST_SIGN_IN_COOKIE].present? + end + + def set_first_sign_in_cookie_before_redirect + return unless request.post? && warden.authenticated?(:user) + + set_first_sign_in_cookie(warden.user) + end end SessionsController.prepend_mod_with('SessionsController') diff --git a/app/services/onboarding/self_managed/standard_namespace_create_service.rb b/app/services/onboarding/self_managed/standard_namespace_create_service.rb new file mode 100644 index 00000000000000..8379a074614d33 --- /dev/null +++ b/app/services/onboarding/self_managed/standard_namespace_create_service.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +module Onboarding + module SelfManaged + class StandardNamespaceCreateService + include BaseServiceUtility + + def initialize(user, group_params:, project_params:) + @current_user = user + @group_params = group_params.dup + @project_params = project_params.dup + end + + def execute + normalize_group_path + + group = find_or_create_group + return error_response(group: group) unless group.persisted? + + project = create_project(group) + return error_response(group: group, project: project) unless project.persisted? + + ServiceResponse.success(payload: { project: project, group: project.namespace }) + end + + private + + attr_reader :current_user, :group_params, :project_params + + def normalize_group_path + return if group_params[:path].present? + return unless group_params[:name].present? + + group_params[:path] = group_params[:name].parameterize + end + + def find_or_create_group + if group_params[:id].present? + Group.find(group_params[:id]) + else + create_group + end + rescue ActiveRecord::RecordNotFound + Group.new + end + + def create_group + result = Groups::CreateService.new(current_user, group_params).execute + + result.payload[:group] + end + + def create_project(group) + merged_params = project_params.dup + merged_params[:namespace_id] = group.id + merged_params[:organization_id] = group.organization_id if group.organization_id + + Projects::CreateService.new(current_user, merged_params).execute + end + + def error_response(group: nil, project: nil) + group ||= Group.new(group_params) + filtered_project_params = project_params.except(:initialize_with_readme) + project ||= Project.new(filtered_project_params.merge(namespace: group)) + + ServiceResponse.error( + message: build_error_message(group, project), + payload: { group: group, project: project } + ) + end + + def build_error_message(group, project) + errors = [] + errors.concat(group.errors.full_messages) if group&.errors&.any? + errors.concat(project.errors.full_messages) if project&.errors&.any? + errors.presence || "Group or project has errors" + end + end + end +end diff --git a/app/views/admin/registrations/groups/new.html.haml b/app/views/admin/registrations/groups/new.html.haml new file mode 100644 index 00000000000000..188e9c18ab404d --- /dev/null +++ b/app/views/admin/registrations/groups/new.html.haml @@ -0,0 +1,37 @@ +- @hide_flash = true +- page_title _('Your GitLab group') +- add_page_specific_style 'page_bundles/registrations' + +.row.gl-grow + .gl-flex.gl-flex-col.gl-items-center.gl-w-full.gl-px-5.gl-pb-5 + .new-project.gl-flex.gl-flex-col.gl-items-center.gl-w-full{ class: "@sm/panel:gl-w-auto" } + %h2.gl-text-center= _('Create your first project') + + %p.gl-text-center= _('Projects help you organize your work. They contain your file repository, issues, merge requests, and so much more.') + + .js-toggle-container.gl-w-full + .tab-content.gitlab-tab-content.gl-bg-default.js-group-project-tab-contents + #blank-project-pane.tab-pane.js-toggle-container.active{ role: 'tabpanel' } + = gitlab_ui_form_for :project_group, + url: admin_sign_up_groups_path, + html: { class: 'gl-show-field-errors gl-w-full gl-p-4 js-groups-projects-form' } do |f| + = form_errors(@group, type: "Group") + = form_errors(@project, type: "Project") + = render 'layouts/flash' + + .js-create-group-project-fields{ data: { + import_group: 'false', + group_persisted: @group.persisted?.to_s, + group_id: @group.id, + group_name: @group.name, + project_name: @project.name, + template_name: @template_name, + initialize_with_readme: @initialize_with_readme.to_s, + root_url: root_url, + track_action_for_errors: '' } } + + .gl-flex.gl-justify-end.gl-gap-3 + = render Pajamas::ButtonComponent.new(href: home_dashboard_path, variant: :default, button_options: { data: { testid: 'skip-button' }}) do + = _("Skip") + = render Pajamas::ButtonComponent.new(type: :submit, variant: :confirm, button_options: { data: { testid: 'submit-button' }}) do + = _("Continue") diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 6edba232c43da5..627b260935be37 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -32,6 +32,10 @@ end end + namespace :sign_up do + resources :groups, only: [:new, :create], controller: '/admin/registrations/groups' + end + resource :session, only: [:new, :create] do post 'destroy', action: :destroy, as: :destroy end diff --git a/ee/app/controllers/ee/sessions_controller.rb b/ee/app/controllers/ee/sessions_controller.rb index 6ef9115e495b0a..a0b62d97d03148 100644 --- a/ee/app/controllers/ee/sessions_controller.rb +++ b/ee/app/controllers/ee/sessions_controller.rb @@ -28,6 +28,11 @@ def new end end + override :self_managed? + def self_managed? + !::Gitlab::Saas.feature_available?(:onboarding) + end + private def gitlab_geo_logout @@ -90,14 +95,24 @@ def detect_and_notify_for_compromised_password end override :onboarding_status_tracking_label + def onboarding_status_tracking_label onboarding_status_presenter.preregistration_tracking_label end + override :should_show_self_managed_project_onboarding? + + def should_show_self_managed_project_onboarding?(user) + return false if ::Gitlab::Saas.feature_available?(:onboarding) + + super + end + def onboarding_status_presenter ::Onboarding::StatusPresenter .new(params.permit(:invite_email).to_h.deep_symbolize_keys, session['user_return_to'], resource) end + strong_memoize_attr :onboarding_status_presenter end end diff --git a/ee/app/models/onboarding.rb b/ee/app/models/onboarding.rb index 3714cd3211c163..8a7b80460a3c9d 100644 --- a/ee/app/models/onboarding.rb +++ b/ee/app/models/onboarding.rb @@ -14,6 +14,10 @@ def self.enabled? ::Gitlab::Saas.feature_available?(:onboarding) end + def self_managed? + !::Gitlab::Saas.feature_available?(:onboarding) + end + def self.user_onboarding_in_progress?(user, use_cache: false) return false unless user.present? return false unless enabled? diff --git a/ee/spec/views/registrations/groups/new.html.haml_spec.rb b/ee/spec/views/registrations/groups/new.html.haml_spec.rb index 14b9fb548d44a7..bd67d29fb44d84 100644 --- a/ee/spec/views/registrations/groups/new.html.haml_spec.rb +++ b/ee/spec/views/registrations/groups/new.html.haml_spec.rb @@ -70,6 +70,12 @@ expect(rendered).to have_css('#blank-project-pane') expect(rendered).to have_css('#import-project-pane') end + + it 'has submit button with Continue text' do + render + + expect(rendered).to have_button('Continue') + end end context 'with trial_registration_hierarchy_education_experiment experiment' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0b5a772fb91032..98ebd01edcf1cf 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -20552,6 +20552,9 @@ msgstr "" msgid "Create your first page" msgstr "" +msgid "Create your first project" +msgstr "" + msgid "Create, update, or delete a merge request." msgstr "" @@ -64473,6 +64476,9 @@ msgstr "" msgid "Size limit per repository (MiB)" msgstr "" +msgid "Skip" +msgstr "" + msgid "Skip to input" msgstr "" diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 1ee87c2309f32e..a456c09b9cd60b 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -16,7 +16,7 @@ before do stub_omniauth_setting(auto_sign_in_with_provider: :saml) allow(controller).to receive(:omniauth_authorize_path).with(:user, :saml) - .and_return('/saml') + .and_return('/saml') end context 'and no auto_sign_in param is passed' do @@ -264,7 +264,7 @@ it 'creates audit event records' do expect { post(:create, params: { user: user_params }) }.to change { AuditEvent.count }.by(1) - .and change { AuditEvents::UserAuditEvent.count }.by(1) + .and change { AuditEvents::UserAuditEvent.count }.by(1) audit_event = AuditEvent.last expect(audit_event.details[:with]).to eq('standard') @@ -344,8 +344,8 @@ def unsuccessful_login(user_params, sesion_params: {}) expect(counter).to receive(:increment) expect(Gitlab::Metrics).to receive(:counter) - .with(:failed_login_captcha_total, anything) - .and_return(counter) + .with(:failed_login_captcha_total, anything) + .and_return(counter) post(:create, params: { user: user_params }, session: sesion_params) end @@ -357,8 +357,8 @@ def successful_login(user_params, sesion_params: {}) expect(counter).to receive(:increment) expect(Gitlab::Metrics).to receive(:counter) - .with(:successful_login_captcha_total, anything) - .and_return(counter) + .with(:successful_login_captcha_total, anything) + .and_return(counter) expect(Gitlab::Metrics).to receive(:counter).at_least(1).time.and_call_original post(:create, params: { user: user_params }, session: sesion_params) @@ -571,7 +571,7 @@ def authenticate_2fa(otp_user_id: user.id, **user_params) authenticate_2fa(otp_attempt: code) expect(controller).to set_flash.now[:alert] - .to(/Invalid two-factor code/) + .to(/Invalid two-factor code/) expect(@request.env['warden']).not_to be_authenticated end @@ -665,38 +665,38 @@ def authenticate_2fa(otp_user_id: user.id, **user_params) expect(session[:otp_user_id]).to be_nil end end + end + + context 'when another user does not have 2FA enabled' do + let(:another_user) { create(:user) } + + it "does not authenticate", :aggregate_failures do + authenticate_2fa(login: another_user.username, otp_attempt: 'invalid') - context 'when another user does not have 2FA enabled' do - let(:another_user) { create(:user) } + expect(controller).to set_flash.now[:alert].to(/Invalid login or password/) + expect(@request.env['warden']).not_to be_authenticated + expect(subject.current_user).not_to eq another_user + end + + context 'with username/password authentication is disabled' do + before do + stub_application_setting(password_authentication_enabled_for_web: false) + end it "does not authenticate", :aggregate_failures do authenticate_2fa(login: another_user.username, otp_attempt: 'invalid') - expect(controller).to set_flash.now[:alert].to(/Invalid login or password/) + expect(response).to have_gitlab_http_status(:forbidden) expect(@request.env['warden']).not_to be_authenticated expect(subject.current_user).not_to eq another_user end - - context 'with username/password authentication is disabled' do - before do - stub_application_setting(password_authentication_enabled_for_web: false) - end - - it "does not authenticate", :aggregate_failures do - authenticate_2fa(login: another_user.username, otp_attempt: 'invalid') - - expect(response).to have_gitlab_http_status(:forbidden) - expect(@request.env['warden']).not_to be_authenticated - expect(subject.current_user).not_to eq another_user - end - end end end end it 'creates audit event records' do expect { authenticate_2fa(login: user.username, otp_attempt: user.current_otp) }.to change { AuditEvent.count }.by(1) - .and change { AuditEvents::UserAuditEvent.count }.by(1) + .and change { AuditEvents::UserAuditEvent.count }.by(1) audit_event = AuditEvent.last expect(audit_event.details[:with]).to eq('two-factor') @@ -776,7 +776,7 @@ def authenticate_2fa(user_params) end expect { authenticate_2fa(login: user.username, device_response: "{}") }.to change { AuditEvent.count }.by(1) - .and change { AuditEvents::UserAuditEvent.count }.by(1) + .and change { AuditEvents::UserAuditEvent.count }.by(1) audit_event = AuditEvent.last expect(audit_event.details[:with]).to eq('two-factor-via-webauthn-device') @@ -940,6 +940,161 @@ def authenticate_2fa_with_passkeys end end + describe '#after_sign_in_path_for' do + let(:user) { create(:user) } + + subject(:after_sign_in_path) { controller.send(:after_sign_in_path_for, user) } + + context 'when user should see first project onboarding' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(Group).to receive(:any?).and_return(false) + admin_user.update_column(:sign_in_count, 2) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it 'redirects to new admin sign up group path' do + expect(controller.send(:after_sign_in_path_for, admin_user)).to eq(new_admin_sign_up_group_path) + end + end + + context 'when user should not see first project onboarding' do + it 'calls super for normal redirect flow' do + expect(after_sign_in_path).not_to eq(new_admin_sign_up_group_path) + end + end + end + + describe '#should_show_self_managed_project_onboarding?' do + subject(:should_show_onboarding) do + controller.send(:should_show_self_managed_project_onboarding?, test_user) + end + + let(:test_user) { create(:user) } + + context 'when all conditions are met' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(Group).to receive(:any?).and_return(false) + admin_user.update_column(:sign_in_count, 2) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be true } + end + + context 'when user is not an admin' do + let(:regular_user) { create(:user, sign_in_count: 1) } + let(:test_user) { regular_user } + + it { is_expected.to be false } + end + + context 'when sign_in_count is not 2' do + let(:admin_user) { create(:admin, sign_in_count: 3) } + let(:test_user) { admin_user } + + it { is_expected.to be false } + end + + context 'when sign_in_count is 1' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + it { is_expected.to be false } + end + + context 'when there are multiple human users' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + let(:other_user) { create(:user) } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(false) + admin_user.update_column(:sign_in_count, 2) + other_user + end + + it { is_expected.to be false } + end + + context 'when user already owns groups' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + let(:group) { create(:group) } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + admin_user.update_column(:sign_in_count, 2) + group.add_owner(admin_user) + end + + it { is_expected.to be false } + end + + context 'when there are internal users but only one human user' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + let(:ghost_user) { create(:user, :ghost) } + let(:bot_user) { create(:user, :project_bot) } + + before do + ghost_user + bot_user + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(Group).to receive(:any?).and_return(false) + admin_user.update_column(:sign_in_count, 2) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be true } + end + end + + describe 'first-time root user login integration' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(admin_user).to receive_message_chain(:owned_groups, :any?).and_return(false) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + end + + context 'when first-time root user logs in' do + it 'redirects to new admin sign up group path' do + post :create, params: { user: { login: admin_user.username, password: admin_user.password } } + + expect(response).to redirect_to(new_admin_sign_up_group_path) + end + + it 'increments sign_in_count to 2' do + expect do + post :create, params: { user: { login: admin_user.username, password: admin_user.password } } + end.to change { admin_user.reload.sign_in_count }.from(1).to(2) + end + end + + context 'when user logs in third time' do + before do + admin_user.update_column(:sign_in_count, 2) + end + + it 'does not redirect to new admin sign up group path' do + post :create, params: { user: { login: admin_user.username, password: admin_user.password } } + + expect(response).not_to redirect_to(new_admin_sign_up_group_path) + end + end + end + context 'when login fails' do before do @request.env["warden.options"] = { action: 'unauthenticated' } diff --git a/spec/requests/admin/registrations/groups_controller_spec.rb b/spec/requests/admin/registrations/groups_controller_spec.rb new file mode 100644 index 00000000000000..732a992402c50f --- /dev/null +++ b/spec/requests/admin/registrations/groups_controller_spec.rb @@ -0,0 +1,240 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Admin::Registrations::GroupsController', :with_current_organization, feature_category: :onboarding do + include AdminModeHelper + + let_it_be(:user, reload: true) { create(:admin) } + let_it_be(:group) { create(:group) } + + describe 'GET /admin/sign_up/groups/new' do + subject(:get_new) { get new_admin_sign_up_group_path } + + context 'with an unauthenticated user' do + it 'redirects to sign in' do + get_new + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'with an authenticated admin user' do + before do + sign_in(user) + enable_admin_mode!(user) + end + + it 'returns success' do + get_new + + expect(response).to have_gitlab_http_status(:ok) + end + + it 'renders the new template' do + get_new + + expect(response.body).to include('Create your first project') + expect(response.body).to include('js-create-group-project-fields') + end + + context 'when user is not an admin' do + let(:non_admin_user) { create(:user) } + + before do + sign_out(user) + sign_in(non_admin_user) + end + + it 'returns not found' do + get_new + + expect(response).to have_gitlab_http_status(:not_found) + end + end + end + end + + describe 'POST /admin/sign_up/groups' do + subject(:post_create) { post admin_sign_up_groups_path, params: params } + + let(:params) { { group: group_params, project: project_params } } + let(:group_params) do + { + name: 'Group name', + path: 'group-path', + visibility_level: Gitlab::VisibilityLevel::PRIVATE.to_s + } + end + + let(:project_params) do + { + name: 'New project', + path: 'project-path', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + initialize_with_readme: 'true' + } + end + + context 'with an unauthenticated user' do + it 'redirects to sign in' do + post_create + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'with an authenticated admin user' do + before do + sign_in(user) + enable_admin_mode!(user) + end + + it 'creates a group and project' do + expect { post_create }.to change { Group.count }.by(1).and change { Project.count }.by(1) + end + + it 'redirects to the created project' do + post_create + + expect(response).to have_gitlab_http_status(:redirect) + expect(response).to redirect_to(project_path(Project.last)) + end + + context 'when user has no existing groups or projects' do + let(:new_admin) { create(:admin) } + + before do + sign_out(user) + sign_in(new_admin) + enable_admin_mode!(new_admin) + end + + it 'creates the first group and project' do + expect(new_admin.owned_groups).to be_empty + expect(new_admin.projects).to be_empty + + expect { post_create }.to change { Group.count }.by(1).and change { Project.count }.by(1) + + expect(new_admin.reload.owned_groups.count).to eq(1) + end + end + + context 'when there is no suggested path based from the name' do + let(:group_params) { { name: '⛄⛄⛄', path: '' } } + + it 'creates a group' do + expect { post_create }.to change { Group.count }.by(1) + end + end + + context 'when the group cannot be created' do + let(:group_params) { { name: '', path: '' } } + + it 'does not create a group' do + expect { post_create }.not_to change { Group.count } + end + + it 'returns success and re-renders the form' do + post_create + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('Create your first project') + end + + it 'displays error messages' do + post_create + + expect(response.body).to include('Group') + end + end + + context "when group can be created but the project can't" do + let(:project_params) { { name: '', path: '', visibility_level: Gitlab::VisibilityLevel::PRIVATE } } + + it 'does not create a project' do + expect { post_create }.to change { Group.count }.and not_change { Project.count } + end + + it 'returns success and re-renders the form' do + post_create + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('Create your first project') + end + + it 'displays error messages' do + post_create + + expect(response.body).to include('Project') + end + end + + context "when a group is already created but a project isn't" do + before_all do + group.add_owner(user) + end + + let(:group_params) { { id: group.id } } + + it 'creates a project and not another group' do + expect { post_create }.to change { Project.count }.and not_change { Group.count } + end + end + + context 'when redirecting' do + let_it_be(:project) { create(:project) } + + let(:success_path) { project_path(project) } + + before do + allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.success(payload: { project: project }) + ) + end + end + + it 'redirects to the project path' do + post_create + + expect(response).to redirect_to(success_path) + end + end + + context 'with template_name in the params' do + let(:project_params) { super().merge(template_name: 'plainhtml') } + + it 'includes template_name in the re-rendered form' do + allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'failed', payload: { group: Group.new, project: Project.new }) + ) + end + + post_create + + expect(response.body).to include('data-template-name="plainhtml"') + end + end + + context 'with initialize_with_readme in the params' do + let(:project_params) { super().merge(initialize_with_readme: 'false') } + + it 'includes initialize_with_readme in the re-rendered form' do + allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow(service).to receive(:execute).and_return( + ServiceResponse.error(message: 'failed', payload: { group: Group.new, project: Project.new }) + ) + end + + post_create + + expect(response.body).to include('data-initialize-with-readme="false"') + end + end + end + end +end diff --git a/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb new file mode 100644 index 00000000000000..7dc55d732964ff --- /dev/null +++ b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb @@ -0,0 +1,208 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Onboarding::SelfManaged::StandardNamespaceCreateService, :aggregate_failures, feature_category: :onboarding do + describe '#execute' do + let_it_be(:user) { create(:user, can_create_group: true) } + let_it_be(:organization) { create(:organization, users: [user]) } + + let(:group_params) do + { + name: 'Group name', + path: 'group-path', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + organization_id: organization.id + } + end + + let(:extra_project_params) { {} } + let(:project_params) do + { + name: 'New project', + path: 'project-path', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + initialize_with_readme: true + }.merge(extra_project_params) + end + + subject(:execute) do + described_class + .new(user, group_params: group_params, project_params: project_params).execute + end + + context 'when group and project can be created' do + it 'creates a group and project' do + expect { execute }.to change { Group.count }.by(1) + .and change { Project.count }.by(1) + end + + it 'returns the created project in the payload' do + result = execute + + expect(result).to be_success + expect(result.payload[:project]).to be_persisted + expect(result.payload[:project].namespace).to be_a(Group) + end + + it 'sets the correct organization_id on the project' do + result = execute + + expect(result.payload[:project].organization_id).to eq(organization.id) + end + + it 'allows for the project to be initialized with a README' do + create_params = project_params.merge( + organization_id: organization.id, + namespace_id: anything + ) + expect(::Projects::CreateService).to receive(:new).with(user, create_params).and_call_original + + expect(execute).to be_success + end + end + + context 'when the group cannot be created' do + let(:group_params) { { name: '', path: '', organization_id: organization.id } } + + it 'does not create a group' do + expect { execute }.not_to change { Group.count } + expect(execute).to be_error + expect(execute.payload[:group].errors).not_to be_blank + end + + it 'returns both group and project in the error payload' do + result = execute + + expect(result).to be_error + expect(result.payload[:group]).not_to be_persisted + expect(result.payload[:project]).not_to be_persisted + expect(result.payload[:project].name).to eq('New project') + end + + it 'does not create a project' do + expect { execute }.not_to change { Project.count } + end + end + + context 'when group can be created but not the project' do + let(:project_params) do + { + name: '', + path: '', + visibility_level: Gitlab::VisibilityLevel::PRIVATE + } + end + + it 'does not create a project' do + expect { execute }.to change { Group.count }.by(1) + .and not_change { Project.count } + + expect(execute).to be_error + expect(execute.payload[:project].errors).not_to be_blank + end + + it 'returns the created group and failed project in the payload' do + result = execute + + expect(result).to be_error + expect(result.payload[:group]).to be_persisted + expect(result.payload[:project]).not_to be_persisted + end + end + + context 'when a group already exists and project needs to be created' do + let_it_be(:existing_group) do + group = create(:group, organization: organization) + group.add_member(user, Gitlab::Access::OWNER) + group + end + + let(:group_params) { { id: existing_group.id } } + + it 'creates a project and not another group' do + group_count = Group.count + + expect { execute }.to change { Project.count }.by(1) + + expect(Group.count).to eq(group_count) + end + + it 'creates the project in the existing group' do + result = execute + + expect(result).to be_success + expect(result.payload[:project].namespace).to eq(existing_group) + end + end + + context 'when group path is blank but name is present' do + let(:group_params) do + { + name: 'My Test Group', + path: '', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + organization_id: organization.id + } + end + + it 'normalizes the group path from the name' do + result = execute + + expect(result).to be_success + expect(result.payload[:project].namespace.path).to eq('my-test-group') + end + end + + context 'when user cannot create groups' do + let(:user) { create(:user, can_create_group: false) } + + it 'returns an error response' do + result = execute + + expect(result).to be_error + expect(result.payload[:group]).not_to be_persisted + expect(result.payload[:project]).not_to be_persisted + end + end + + context 'when group_params has invalid id' do + let(:group_params) { { id: non_existing_record_id } } + + it 'returns an error response' do + result = execute + + expect(result).to be_error + expect(result.payload[:group]).not_to be_persisted + expect(result.payload[:project]).not_to be_persisted + end + end + + context 'with initialize_with_readme option' do + let(:group_params) do + { + name: 'Test Group README', + path: 'test-group-readme', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + organization_id: organization.id + } + end + + let(:project_params) do + { + name: 'Test Project', + path: 'test-project', + initialize_with_readme: true, + visibility_level: Gitlab::VisibilityLevel::PRIVATE + } + end + + it 'creates project with README' do + result = execute + + expect(result).to be_success + expect(result.payload[:project].repository.readme).to be_present + end + end + end +end diff --git a/spec/views/admin/registrations/groups/new.html.haml_spec.rb b/spec/views/admin/registrations/groups/new.html.haml_spec.rb new file mode 100644 index 00000000000000..a8062e2dc5ed4f --- /dev/null +++ b/spec/views/admin/registrations/groups/new.html.haml_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'admin/registrations/groups/new', feature_category: :onboarding do + let(:user) { build_stubbed(:user) } + let(:group) { build_stubbed(:group) } + let(:project) { build_stubbed(:project) } + + before do + assign(:group, group) + assign(:project, project) + assign(:initialize_with_readme, true) + allow(view).to receive(:current_user).and_return(user) + end + + context 'for form action path' do + subject { render && rendered } + + it 'has the correct form action path' do + is_expected.to have_css('form[action="/admin/sign_up/groups"]') + end + end + + context 'with expected DOM elements' do + it 'contains js-groups-projects-form class' do + render + + expect(rendered).to have_css('.js-groups-projects-form') + end + + it 'has the create project heading' do + render + + expect(rendered).to have_css('h2', text: 'Create your first project') + end + + it 'has the project description text' do + render + + expect(rendered).to have_content('Projects help you organize your work') + end + + it 'has project creation form tabs' do + render + + expect(rendered).to have_css('.js-toggle-container') + expect(rendered).to have_css('#blank-project-pane') + end + + it 'has the submit button' do + render + + expect(rendered).to have_button('Continue') + end + + it 'has the skip button' do + render + + expect(rendered).to have_link('Skip', href: home_dashboard_path) + end + end + + context 'with js-create-group-project-fields data attributes' do + before do + render + end + + it 'sets the correct data attributes', :aggregate_failures do + expect(rendered).to have_css('.js-create-group-project-fields') + expect(rendered).to have_css("[data-import-group='false']") + expect(rendered).to have_css("[data-group-persisted='#{group.persisted?}']") + expect(rendered).to have_css("[data-group-id='#{group.id}']") + expect(rendered).to have_css("[data-group-name='#{group.name}']") + expect(rendered).to have_css("[data-project-name='#{project.name}']") + expect(rendered).to have_css("[data-initialize-with-readme='true']") + end + end + + context 'with template_name assigned' do + before do + assign(:template_name, 'rails') + render + end + + it 'sets the template_name data attribute' do + expect(rendered).to have_css("[data-template-name='rails']") + end + end + + context 'with flash messages' do + before do + allow(view).to receive(:flash).and_return({ notice: 'Test notice' }) + render + end + + it 'renders flash messages' do + expect(view).to render_template(partial: 'layouts/_flash') + end + end + + context 'with form errors' do + let(:group_with_errors) { build(:group, name: '') } + let(:project_with_errors) { build(:project, name: '') } + + before do + group_with_errors.valid? + project_with_errors.valid? + assign(:group, group_with_errors) + assign(:project, project_with_errors) + render + end + + it 'displays group errors' do + expect(rendered).to have_content("Group") + end + + it 'displays project errors' do + expect(rendered).to have_content("Project") + end + end +end -- GitLab From b1e77fa8f37ac1770471eeca2f153725527f81bf Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Fri, 12 Dec 2025 10:07:28 -0500 Subject: [PATCH 2/7] Updating to use correct service --- spec/requests/admin/registrations/groups_controller_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/requests/admin/registrations/groups_controller_spec.rb b/spec/requests/admin/registrations/groups_controller_spec.rb index 732a992402c50f..d24b8fbe7b6d65 100644 --- a/spec/requests/admin/registrations/groups_controller_spec.rb +++ b/spec/requests/admin/registrations/groups_controller_spec.rb @@ -190,7 +190,7 @@ let(:success_path) { project_path(project) } before do - allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow_next_instance_of(Onboarding::SelfManaged::StandardNamespaceCreateService) do |service| allow(service).to receive(:execute).and_return( ServiceResponse.success(payload: { project: project }) ) @@ -208,7 +208,7 @@ let(:project_params) { super().merge(template_name: 'plainhtml') } it 'includes template_name in the re-rendered form' do - allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow_next_instance_of(Onboarding::SelfManaged::StandardNamespaceCreateService) do |service| allow(service).to receive(:execute).and_return( ServiceResponse.error(message: 'failed', payload: { group: Group.new, project: Project.new }) ) @@ -224,7 +224,7 @@ let(:project_params) { super().merge(initialize_with_readme: 'false') } it 'includes initialize_with_readme in the re-rendered form' do - allow_next_instance_of(Registrations::StandardNamespaceCreateService) do |service| + allow_next_instance_of(Onboarding::SelfManaged::StandardNamespaceCreateService) do |service| allow(service).to receive(:execute).and_return( ServiceResponse.error(message: 'failed', payload: { group: Group.new, project: Project.new }) ) -- GitLab From d7e61c5397c818ef82899929568a1de80293248a Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Fri, 12 Dec 2025 10:10:00 -0500 Subject: [PATCH 3/7] Updating w query limit --- app/controllers/admin/registrations/groups_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/admin/registrations/groups_controller.rb b/app/controllers/admin/registrations/groups_controller.rb index e885af88c471fe..571efb1bf4f6ba 100644 --- a/app/controllers/admin/registrations/groups_controller.rb +++ b/app/controllers/admin/registrations/groups_controller.rb @@ -22,6 +22,8 @@ def new end def create + ::Gitlab::QueryLimiting.disable!('https://gitlab.com/gitlab-org/gitlab/-/issues/583774') + result = service_instance.execute if result.success? -- GitLab From 3268fba414f1349cdd741a43f7c9e96f550be41a Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Fri, 12 Dec 2025 23:04:29 -0500 Subject: [PATCH 4/7] Fix for group create spec failure --- .../new/components/group_project_fields.vue | 17 +++++++++++++++-- .../standard_namespace_create_service.rb | 3 ++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue b/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue index 6a6bdb64032d07..eb22d70a01b6ce 100644 --- a/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue +++ b/app/assets/javascripts/registrations/groups/new/components/group_project_fields.vue @@ -93,6 +93,15 @@ export default { this.onProjectUpdate(this.projectName); } }, + beforeDestroy() { + if (this.debouncedOnGroupUpdate) { + this.debouncedOnGroupUpdate.cancel(); + } + + if (this.currentApiRequestController) { + this.currentApiRequestController.abort(); + } + }, methods: { ...mapActions(['setStoreGroupName', 'setStoreGroupPath']), groupInputAttr(name) { @@ -138,10 +147,13 @@ export default { const slug = slugify(value); this.setStoreGroupName(value); - if (!slug) return this.setStoreGroupPath(DEFAULT_GROUP_PATH); + if (!slug) { + this.setStoreGroupPath(DEFAULT_GROUP_PATH); + return; + } this.setStoreGroupPath(slug); - return this.debouncedOnGroupUpdate(slug); + this.debouncedOnGroupUpdate(slug); }, onProjectUpdate(value) { this.projectPath = slugify(convertUnicodeToAscii(value)) || DEFAULT_PROJECT_PATH; @@ -221,6 +233,7 @@ export default { name="group[path]" autocomplete="off" :value="storeGroupPath" + data-testid="group-path-input" /> diff --git a/app/services/onboarding/self_managed/standard_namespace_create_service.rb b/app/services/onboarding/self_managed/standard_namespace_create_service.rb index 8379a074614d33..ace935914570ec 100644 --- a/app/services/onboarding/self_managed/standard_namespace_create_service.rb +++ b/app/services/onboarding/self_managed/standard_namespace_create_service.rb @@ -31,7 +31,8 @@ def normalize_group_path return if group_params[:path].present? return unless group_params[:name].present? - group_params[:path] = group_params[:name].parameterize + parameterized_path = group_params[:name].parameterize + group_params[:path] = parameterized_path.presence || SecureRandom.hex(4) end def find_or_create_group -- GitLab From 2832c48baad50b244125e4d55e318bba5b15eda0 Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Sat, 13 Dec 2025 10:54:50 -0500 Subject: [PATCH 5/7] Fixing incorrect spec expectation --- ee/spec/views/registrations/groups/new.html.haml_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/spec/views/registrations/groups/new.html.haml_spec.rb b/ee/spec/views/registrations/groups/new.html.haml_spec.rb index bd67d29fb44d84..2c12c366ecda4c 100644 --- a/ee/spec/views/registrations/groups/new.html.haml_spec.rb +++ b/ee/spec/views/registrations/groups/new.html.haml_spec.rb @@ -71,10 +71,10 @@ expect(rendered).to have_css('#import-project-pane') end - it 'has submit button with Continue text' do + it 'has submit button with Create project text' do render - expect(rendered).to have_button('Continue') + expect(rendered).to have_button('Create project') end end -- GitLab From 54264d2e487774208be89e5409e1d84fb1352961 Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Tue, 16 Dec 2025 00:49:12 -0500 Subject: [PATCH 6/7] Updated to fix undercoverage issue --- app/controllers/sessions_controller.rb | 18 +-- .../standard_namespace_create_service.rb | 22 +++- ee/app/models/onboarding.rb | 2 +- ee/spec/models/onboarding_spec.rb | 36 ++++++ spec/controllers/sessions_controller_spec.rb | 30 +++++ .../registrations/groups_controller_spec.rb | 29 +++++ .../standard_namespace_create_service_spec.rb | 105 ++++++++++++++++++ 7 files changed, 225 insertions(+), 17 deletions(-) diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 4147f929153190..71fe61fbc3c20d 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -60,7 +60,7 @@ class SessionsController < Devise::SessionsController CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha' MAX_FAILED_LOGIN_ATTEMPTS = 5 PRESERVE_COOKIES = %w[current_signin_tab preferred_language].freeze - FIRST_SIGN_IN_COOKIE = :first_sign_in_completed + FIRST_SIGN_IN_COOKIE = 'first_sign_in' def new set_minimum_password_length @@ -375,26 +375,20 @@ def after_sign_in_path_for(resource) end def should_show_self_managed_project_onboarding?(user) - return false unless user.can_admin_all_resources? + return false unless user.admin? return false unless self_managed? return false unless first_sign_in_completed? - return false unless user.reset.sign_in_count == 2 + return false unless user.sign_in_count == 2 return false unless User.human.one? return false if Group.any? true end - def set_first_sign_in_cookie(resource) - return unless resource.can_admin_all_resources? - return if cookies[FIRST_SIGN_IN_COOKIE] + def set_first_sign_in_cookie(_resource) + return if cookies[FIRST_SIGN_IN_COOKIE].present? - cookies[FIRST_SIGN_IN_COOKIE] = { - value: true, - expires: 1.year.from_now, - httponly: true, - secure: Rails.env.production? - } + cookies[FIRST_SIGN_IN_COOKIE] = 'true' end def self_managed? diff --git a/app/services/onboarding/self_managed/standard_namespace_create_service.rb b/app/services/onboarding/self_managed/standard_namespace_create_service.rb index ace935914570ec..f93c31ae2feafb 100644 --- a/app/services/onboarding/self_managed/standard_namespace_create_service.rb +++ b/app/services/onboarding/self_managed/standard_namespace_create_service.rb @@ -46,7 +46,10 @@ def find_or_create_group end def create_group - result = Groups::CreateService.new(current_user, group_params).execute + filtered_params = group_params.dup + filtered_params.delete(:organization_id) if filtered_params[:organization_id].nil? + + result = Groups::CreateService.new(current_user, filtered_params).execute result.payload[:group] end @@ -71,10 +74,21 @@ def error_response(group: nil, project: nil) end def build_error_message(group, project) + return 'Group or project has errors' if group.nil? || project.nil? + errors = [] - errors.concat(group.errors.full_messages) if group&.errors&.any? - errors.concat(project.errors.full_messages) if project&.errors&.any? - errors.presence || "Group or project has errors" + + if group.persisted? == false && (group.name.blank? || group.path.blank?) + group.validate + errors.concat(group.errors.full_messages) if group.errors.any? + end + + if project.persisted? == false && (project.name.blank? || project.path.blank?) + project.validate + errors.concat(project.errors.full_messages) if project.errors.any? + end + + errors.any? ? errors.join(', ') : 'Group or project has errors' end end end diff --git a/ee/app/models/onboarding.rb b/ee/app/models/onboarding.rb index 8a7b80460a3c9d..e4c6ffb5ec9ff1 100644 --- a/ee/app/models/onboarding.rb +++ b/ee/app/models/onboarding.rb @@ -14,7 +14,7 @@ def self.enabled? ::Gitlab::Saas.feature_available?(:onboarding) end - def self_managed? + def self.self_managed? !::Gitlab::Saas.feature_available?(:onboarding) end diff --git a/ee/spec/models/onboarding_spec.rb b/ee/spec/models/onboarding_spec.rb index cb0dcdc3ec002a..12e6dda72448f2 100644 --- a/ee/spec/models/onboarding_spec.rb +++ b/ee/spec/models/onboarding_spec.rb @@ -144,6 +144,42 @@ end end + describe '.self_managed?' do + subject { described_class.self_managed? } + + context 'when onboarding feature is available' do + before do + stub_saas_features(onboarding: true) + end + + it { is_expected.to eq(false) } + end + + context 'when onboarding feature is not available' do + before do + stub_saas_features(onboarding: false) + end + + it { is_expected.to eq(true) } + end + + context 'when Gitlab::Saas.feature_available? returns false' do + before do + allow(::Gitlab::Saas).to receive(:feature_available?).with(:onboarding).and_return(false) + end + + it { is_expected.to eq(true) } + end + + context 'when Gitlab::Saas.feature_available? returns true' do + before do + allow(::Gitlab::Saas).to receive(:feature_available?).with(:onboarding).and_return(true) + end + + it { is_expected.to eq(false) } + end + end + describe '.add_on_seat_assignment_iterable_params' do let(:namespace) { build(:namespace, id: non_existing_record_id) } diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index a456c09b9cd60b..d14d918ec4023d 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -997,6 +997,30 @@ def authenticate_2fa_with_passkeys it { is_expected.to be false } end + context 'when self_managed? returns false' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + before do + allow(controller).to receive(:self_managed?).and_return(false) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + end + + it { is_expected.to be false } + end + + context 'when first_sign_in_completed? returns false' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + before do + allow(controller).to receive(:first_sign_in_completed?).and_return(false) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + end + + it { is_expected.to be false } + end + context 'when sign_in_count is not 2' do let(:admin_user) { create(:admin, sign_in_count: 3) } let(:test_user) { admin_user } @@ -1059,6 +1083,12 @@ def authenticate_2fa_with_passkeys end end + describe '#self_managed?' do + it 'returns true' do + expect(controller.send(:self_managed?)).to be true + end + end + describe 'first-time root user login integration' do let(:admin_user) { create(:admin, sign_in_count: 1) } diff --git a/spec/requests/admin/registrations/groups_controller_spec.rb b/spec/requests/admin/registrations/groups_controller_spec.rb index d24b8fbe7b6d65..3c3c9afcc0b40e 100644 --- a/spec/requests/admin/registrations/groups_controller_spec.rb +++ b/spec/requests/admin/registrations/groups_controller_spec.rb @@ -53,6 +53,21 @@ expect(response).to have_gitlab_http_status(:not_found) end end + + context 'when user is not an admin on POST' do + let(:non_admin_user) { create(:user) } + + before do + sign_out(user) + sign_in(non_admin_user) + end + + it 'returns not found' do + post admin_sign_up_groups_path, params: { group: { name: 'test' }, project: { name: 'test' } } + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end @@ -236,5 +251,19 @@ end end end + + context 'with a non-admin user' do + let(:non_admin_user) { create(:user) } + + before do + sign_in(non_admin_user) + end + + it 'returns not found' do + post_create + + expect(response).to have_gitlab_http_status(:not_found) + end + end end end diff --git a/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb index 7dc55d732964ff..5e40234c753b8d 100644 --- a/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb +++ b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb @@ -204,5 +204,110 @@ expect(result.payload[:project].repository.readme).to be_present end end + + context 'when group organization_id is nil' do + let(:group_params) do + { + name: 'Test Group', + path: 'test-group', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + organization_id: nil + } + end + + it 'does not set organization_id on the project' do + result = execute + + expect(result).to be_error + expect(result.payload[:group].errors[:organization]).to be_present + end + end + + context 'when group organization_id is present' do + let(:group_params) do + { + name: 'Test Group', + path: 'test-group', + visibility_level: Gitlab::VisibilityLevel::PRIVATE, + organization_id: organization.id + } + end + + it 'sets organization_id on the project' do + result = execute + + expect(result).to be_success + expect(result.payload[:project].organization_id).to eq(organization.id) + end + end + end + + describe '#build_error_message' do + let(:user) { create(:user, can_create_group: true) } + let(:organization) { create(:organization, users: [user]) } + let(:service) do + described_class.new(user, group_params: {}, project_params: {}) + end + + context 'when group has errors' do + it 'includes group errors in the message' do + group = Group.new(name: '', path: '', organization_id: organization.id) + project = Project.new + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + end + end + + context 'when project has errors' do + it 'includes project errors in the message' do + group = Group.new(name: 'Test', path: 'test', organization_id: organization.id) + project = Project.new(name: '', path: '') + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + end + end + + context 'when group is nil' do + it 'returns default message' do + project = Project.new + + message = service.send(:build_error_message, nil, project) + + expect(message).to eq('Group or project has errors') + end + end + + context 'when project is nil' do + it 'returns default message' do + group = Group.new + + message = service.send(:build_error_message, group, nil) + + expect(message).to eq('Group or project has errors') + end + end + + context 'when both group and project have no errors' do + it 'returns default message' do + group = Group.new(name: 'Test', path: 'test') + project = Project.new(name: 'Test', path: 'test') + + message = service.send(:build_error_message, group, project) + + expect(message).to eq('Group or project has errors') + end + end + + context 'when both group and project are nil' do + it 'returns default message' do + message = service.send(:build_error_message, nil, nil) + + expect(message).to eq('Group or project has errors') + end + end end end -- GitLab From e60242e58261334c5ac417c8e6879a86c321d50c Mon Sep 17 00:00:00 2001 From: Buck OLeary Date: Tue, 16 Dec 2025 12:36:06 -0500 Subject: [PATCH 7/7] Updated specs to fix undercoverage --- .../standard_namespace_create_service.rb | 9 +- spec/controllers/sessions_controller_spec.rb | 72 ++++++++++++ .../standard_namespace_create_service_spec.rb | 103 ++++++++++++++++++ 3 files changed, 181 insertions(+), 3 deletions(-) diff --git a/app/services/onboarding/self_managed/standard_namespace_create_service.rb b/app/services/onboarding/self_managed/standard_namespace_create_service.rb index f93c31ae2feafb..15ca667b2acdf3 100644 --- a/app/services/onboarding/self_managed/standard_namespace_create_service.rb +++ b/app/services/onboarding/self_managed/standard_namespace_create_service.rb @@ -57,7 +57,7 @@ def create_group def create_project(group) merged_params = project_params.dup merged_params[:namespace_id] = group.id - merged_params[:organization_id] = group.organization_id if group.organization_id + merged_params[:organization_id] = group.organization_id if group.organization_id.present? Projects::CreateService.new(current_user, merged_params).execute end @@ -73,17 +73,20 @@ def error_response(group: nil, project: nil) ) end + # @param group [Group, nil] The group object to validate + # @param project [Project, nil] The project object to validate + # @return [String] Error message containing validation errors or default message def build_error_message(group, project) return 'Group or project has errors' if group.nil? || project.nil? errors = [] - if group.persisted? == false && (group.name.blank? || group.path.blank?) + if group.name.blank? || group.path.blank? group.validate errors.concat(group.errors.full_messages) if group.errors.any? end - if project.persisted? == false && (project.name.blank? || project.path.blank?) + if project.name.blank? || project.path.blank? project.validate errors.concat(project.errors.full_messages) if project.errors.any? end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index d14d918ec4023d..b166ed76a17d94 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -1049,6 +1049,36 @@ def authenticate_2fa_with_passkeys it { is_expected.to be false } end + context 'when User.human.one? returns false' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(false) + allow(Group).to receive(:any?).and_return(false) + admin_user.update_column(:sign_in_count, 2) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be false } + end + + context 'when Group.any? returns true' do + let(:admin_user) { create(:admin, sign_in_count: 1) } + let(:test_user) { admin_user } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(Group).to receive(:any?).and_return(true) + admin_user.update_column(:sign_in_count, 2) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be false } + end + context 'when user already owns groups' do let(:admin_user) { create(:admin, sign_in_count: 1) } let(:test_user) { admin_user } @@ -1081,12 +1111,54 @@ def authenticate_2fa_with_passkeys it { is_expected.to be true } end + + context 'when all conditions are met but User.human.one? is false' do + let(:admin_user) { create(:admin, sign_in_count: 2) } + let(:test_user) { admin_user } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(false) + allow(Group).to receive(:any?).and_return(false) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be false } + end + + context 'when all conditions are met but Group.any? is true' do + let(:admin_user) { create(:admin, sign_in_count: 2) } + let(:test_user) { admin_user } + + before do + allow(User).to receive_message_chain(:human, :one?).and_return(true) + allow(Group).to receive(:any?).and_return(true) + allow(Gitlab::Auth::CurrentUserMode).to receive(:new).with(admin_user).and_return(instance_double(Gitlab::Auth::CurrentUserMode, admin_mode?: true)) + request.cookies[SessionsController::FIRST_SIGN_IN_COOKIE] = 'true' + end + + it { is_expected.to be false } + end end describe '#self_managed?' do it 'returns true' do expect(controller.send(:self_managed?)).to be true end + + it 'always returns true for self-managed instances' do + expect(controller.send(:self_managed?)).to eq(true) + end + + context 'when called multiple times' do + it 'consistently returns true' do + result1 = controller.send(:self_managed?) + result2 = controller.send(:self_managed?) + + expect(result1).to be true + expect(result2).to be true + end + end end describe 'first-time root user login integration' do diff --git a/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb index 5e40234c753b8d..916be024490258 100644 --- a/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb +++ b/spec/services/onboarding/self_managed/standard_namespace_create_service_spec.rb @@ -258,6 +258,16 @@ expect(message).to include('Name') end + + it 'includes multiple group errors' do + group = Group.new(name: '', path: '') + project = Project.new + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + expect(message).to include('Path') + end end context 'when project has errors' do @@ -269,6 +279,52 @@ expect(message).to include('Name') end + + it 'includes multiple project errors' do + group = Group.new(name: 'Test', path: 'test') + project = Project.new(name: '', path: '') + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + expect(message).to include('Path') + end + end + + context 'when both group and project have errors' do + it 'includes both group and project errors' do + group = Group.new(name: '', path: '', organization_id: organization.id) + project = Project.new(name: '', path: '') + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + expect(message).to include('Path') + end + end + + context 'when group.errors.any? returns true' do + it 'includes group errors in the message' do + group = Group.new(name: '', path: '', organization_id: organization.id) + project = Project.new(name: 'Valid', path: 'valid') + + message = service.send(:build_error_message, group, project) + + expect(message).not_to eq('Group or project has errors') + expect(message).to include('Name') + end + end + + context 'when project.errors.any? returns true' do + it 'includes project errors in the message' do + group = Group.new(name: 'Valid', path: 'valid', organization_id: organization.id) + project = Project.new(name: '', path: '') + + message = service.send(:build_error_message, group, project) + + expect(message).not_to eq('Group or project has errors') + expect(message).to include('Name') + end end context 'when group is nil' do @@ -309,5 +365,52 @@ expect(message).to eq('Group or project has errors') end end + + context 'when group is persisted but has blank name' do + it 'validates and includes errors' do + group = create(:group, name: 'Test', path: 'test', organization_id: organization.id) + group.update_column(:name, '') + project = Project.new + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + end + end + + context 'when project is persisted but has blank name' do + it 'validates and includes errors' do + group = create(:group, organization_id: organization.id) + project = create(:project, namespace: group, name: 'Test') + project.update_column(:name, '') + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + end + end + + context 'when group is persisted and has no blank fields' do + it 'does not include group errors' do + group = create(:group, organization_id: organization.id) + project = Project.new(name: '', path: '') + + message = service.send(:build_error_message, group, project) + + expect(message).to include('Name') + expect(message).not_to include(group.name) + end + end + + context 'when project is persisted and has no blank fields' do + it 'does not include project errors' do + group = create(:group, organization_id: organization.id) + project = create(:project, namespace: group) + + message = service.send(:build_error_message, group, project) + + expect(message).to eq('Group or project has errors') + end + end end end -- GitLab