User admin approval - Default for new instances
What does this MR do?
The default setting for newly created settings is to allow user registration. In order to increase security, we should make the admin approval workflow the default for any newly created instances. Settings for existing instances should not be changed.
Ran a migration to change column default, which broke some tests that currently assume that user has permission to register without admin approval, so stubbed require_admin_approval_after_user_signup: false
for those tests.
% rails db:migrate
== 20201107032257 AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings: migrating
-- change_column_default(:application_settings, :require_admin_approval_after_user_signup, {:from=>false, :to=>true})
-> 0.0352s
== 20201107032257 AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings: migrated (0.0396s)
rake db:migrate:status
shows that 20201107032257 AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings
is 5th from latest migration, so add STEP=5 to the rollback:
% rails db:rollback STEP=5
== 20201107032257 AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings: reverting
-- change_column_default(:application_settings, :require_admin_approval_after_user_signup, {:from=>true, :to=>false})
-> 0.0343s
== 20201107032257 AddDefaultTrueRequireAdminApprovalAfterUserSignupToApplicationSettings: reverted (0.0343s)
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related to #267568 (closed)
Merge request reports
Activity
changed milestone to %13.6
assigned to @serenafang
added backend label
1 Warning This MR has a Changelog file outside ee/
, but code changes inee/
. Consider moving the Changelog file intoee/
.1 Message This merge request adds or changes documentation files. A review from the Technical Writing team before you merge is recommended. Reviews can happen after you merge. Documentation review
The following files require a review from a technical writer:
doc/user/admin_area/settings/sign_up_restrictions.md
The review does not need to block merging this merge request. See the:
- Technical Writers assignments for the appropriate technical writer for this review.
- Documentation workflows for information on when to assign a merge request for review.
Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
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 the chosen person is unavailable.
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, mention them as you normally would! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Kassio Borges ( @kassio
) (UTC+0, 6 hours ahead of@serenafang
)Vitali Tatarintev ( @ck3g
) (UTC+1, 7 hours ahead of@serenafang
)database Vitali Tatarintev ( @ck3g
) (UTC+1, 7 hours ahead of@serenafang
)Patrick Bair ( @pbair
) (UTC-5, 1 hour ahead of@serenafang
)test Quality for spec/features/*
Sanad Liaquat ( @sliaquat
) (UTC+5, 11 hours ahead of@serenafang
)Maintainer review is optional for test Quality for spec/features/*
QA Joanna Shih ( @jo_shih
) (UTC-6, same timezone as@serenafang
)Maintainer review is optional for QA If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by 🤖 GitLab Bot 🤖added databasereview pending label
added database label
removed workflowready for development label
- Resolved by Serena Fang
@sliaquat Hi Sanad, in the issue for this MR you'd mentioned that changing the default column value would also require changing some tests that currently assume a user is able to register without approval: #267568 (comment 436560392)
change_column_default from: false, to: true
indeed broke the following specs:- /spec/controllers/registrations_controller_spec.rb - /spec/features/users/signup_spec.rb - /spec/features/invites_spec.rb - /ee/spec/features/trial_registrations/signup_spec.rb - /ee/spec/features/invites_spec.rb - /ee/spec/features/signup_spec.rb
so I added
stub_application_setting(require_admin_approval_after_user_signup: false)
to each of those files, which mostly fixed the failing specs (exceptspec/features/users/signup_spec.rb
, still figuring that one out)Before I go too far down this path, I wanted to ask if stubbing that column value was what you had in mind, or if there's a better way to do this. Thanks!
assigned to @sliaquat
added workflowin dev label and removed 1 deleted label
added 971 commits
-
0e03d480...7472a375 - 957 commits from branch
master
- ceb24cac - Admin approval on signup default true
- c7902b70 - Rebase with master
- 887e9ee5 - Add changelog entry
- 46a02d2b - Add stub require admin approval false
- b03a61d9 - Stub setting before all
- 3ec5a2b8 - Test new context
- 82aa2638 - Not ready for review just saving
- b10bf3a5 - Remove extra spaces
- 78a23c7d - Stub application setting in the right place
- f69ac5fe - Add new spec for admin approval true
- 0de2edaf - Add specs for require admin approval true
- bcb142da - Stub application setting
- 86d74cef - Change some specs
- 0d0476c9 - Reword some spcs
Toggle commit list-
0e03d480...7472a375 - 957 commits from branch
@manojmj Could you do the backend review? Thank you!
Edited by Serena Fang- Resolved by Serena Fang
@ck3g Could you do the database review on this? Thank you!
assigned to @ck3g
- Resolved by Serena Fang
- Resolved by Serena Fang
- Resolved by Serena Fang
unassigned @ck3g
added 208 commits
-
12415652...7e423209 - 189 commits from branch
master
- c975fd22 - Admin approval on signup default true
- 28c43ac1 - Rebase with master
- 8370247c - Add changelog entry
- 9828c5ee - Add stub require admin approval false
- e6584a6a - Stub setting before all
- 5050521c - Test new context
- b56b6aa7 - Not ready for review just saving
- 78585350 - Remove extra spaces
- 936d2e0b - Stub application setting in the right place
- 34bc1653 - Add new spec for admin approval true
- e9e24a9e - Add specs for require admin approval true
- eba53da9 - Stub application setting
- d1cf6830 - Change some specs
- c8f358c8 - Reword some spcs
- 6df9c8da - Remove extra new user signups cap
- 856e774e - Rerun db migrate
- 8908fc74 - Change settings spec
- 550edfcb - Use with lock retries
- 16af8e9a - Remove extra new line
Toggle commit list-
12415652...7e423209 - 189 commits from branch
assigned to @ck3g
added 30 commits
-
08029458...7145daf0 - 8 commits from branch
master
- 4ebf0a77 - Admin approval on signup default true
- 547a11a9 - Rebase with master
- f295fc9c - Add changelog entry
- 1dd665b8 - Add stub require admin approval false
- cb7c025a - Stub setting before all
- 6758f854 - Test new context
- 757bd687 - Not ready for review just saving
- d057ca13 - Remove extra spaces
- 1c45fe38 - Stub application setting in the right place
- 39b2426c - Add new spec for admin approval true
- 57bfe399 - Add specs for require admin approval true
- 9e8cd4a4 - Stub application setting
- 7ca379a4 - Change some specs
- 2d1fe477 - Reword some spcs
- 774cdafe - Remove extra new user signups cap
- a02c350d - Rerun db migrate
- 9056d6ab - Change settings spec
- 16c12769 - Use with lock retries
- bdb1eb16 - Remove extra new line
- a9a1655f - Rearrange db structure
- df2975ad - Readd deleted migration
- 1af58b23 - Remove with lock retries
Toggle commit list-
08029458...7145daf0 - 8 commits from branch