Skip to content
Snippets Groups Projects

User admin approval - Default for new instances

Merged Serena Fang requested to merge 267568-user-admin-approval-default-for-new-instances into master

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

Availability and Testing

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)

Edited by Serena Fang

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Vitali Tatarintev
  • Vitali Tatarintev
  • unassigned @ck3g

  • Serena Fang added 208 commits

    added 208 commits

    Compare with previous version

  • Serena Fang added 1 commit

    added 1 commit

    Compare with previous version

  • Serena Fang added 1 commit

    added 1 commit

    Compare with previous version

  • Serena Fang changed the description

    changed the description

  • Serena Fang changed the description

    changed the description

  • Serena Fang added 1 commit

    added 1 commit

    Compare with previous version

  • assigned to @ck3g

  • Serena Fang added 30 commits

    added 30 commits

    Compare with previous version

  • Serena Fang added 1 commit

    added 1 commit

    • 1daa22b9 - Admin approval on signup default true

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading