diff options
21 files changed, 705 insertions, 39 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index c0aec99263f..7302d5c02af 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -202,6 +202,7 @@ variables: REVIEW_APPS_GCP_REGION: "us-central1" CACHE_ASSETS_AS_PACKAGE: "true" + REUSE_FRONTEND_FIXTURES_ENABLED: "true" BUILD_ASSETS_IMAGE: "true" # Set it to "false" to disable assets image building, used in `build-assets-image` SIMPLECOV: "true" diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index 03cd4693991..809d1f10705 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -995,6 +995,8 @@ ################# .caching:rules:cache-workhorse: rules: + - <<: *if-not-ee + when: never # That would run for any project that has a "maintenance" pipeline schedule # but in fact, the cache package is only uploaded for gitlab.com/gitlab-org/gitlab and jihulab.com/gitlab-cn/gitlab - <<: *if-schedule-maintenance @@ -1012,6 +1014,8 @@ # The new strategy to cache assets as generic packages is experimental and can be disabled by removing the `CACHE_ASSETS_AS_PACKAGE` variable - if: '$CACHE_ASSETS_AS_PACKAGE != "true"' when: never + - <<: *if-not-ee + when: never # That would run for any project that has a "maintenance" pipeline schedule # but in fact, the cache package is only uploaded for gitlab.com/gitlab-org/gitlab and jihulab.com/gitlab-cn/gitlab - <<: *if-schedule-maintenance @@ -1035,6 +1039,8 @@ # The new strategy to cache assets as generic packages is experimental and can be disabled by removing the `CACHE_ASSETS_AS_PACKAGE` variable - if: '$CACHE_ASSETS_AS_PACKAGE != "true"' when: never + - <<: *if-not-ee + when: never # That would run for any project that has a "maintenance" pipeline schedule # but in fact, the cache package is only uploaded for gitlab.com/gitlab-org/gitlab and jihulab.com/gitlab-cn/gitlab - <<: *if-schedule-maintenance @@ -1256,12 +1262,12 @@ # The new strategy to upload fixtures as generic packages is experimental and can be disabled by removing the `REUSE_FRONTEND_FIXTURES_ENABLED` variable - if: '$REUSE_FRONTEND_FIXTURES_ENABLED != "true"' when: never + - <<: *if-not-ee + when: never - <<: *if-merge-request-labels-pipeline-expedite when: never - <<: *if-dot-com-gitlab-org-default-branch changes: *code-backstage-patterns - - <<: *if-foss-default-branch - changes: *code-backstage-patterns - <<: *if-dot-com-gitlab-org-merge-request changes: - ".gitlab/ci/frontend.gitlab-ci.yml" @@ -1817,6 +1823,9 @@ # From .qa:rules:package-and-test-schedule - <<: *if-dot-com-gitlab-org-schedule when: never + # Do not run on unapproved MR + - <<: *if-merge-request-not-approved + when: never # From .qa:rules:code-merge-request-manual - <<: *if-merge-request changes: *code-patterns diff --git a/app/assets/javascripts/sentry/init_sentry.js b/app/assets/javascripts/sentry/init_sentry.js index afc143ff351..dbd12dc36ce 100644 --- a/app/assets/javascripts/sentry/init_sentry.js +++ b/app/assets/javascripts/sentry/init_sentry.js @@ -29,6 +29,10 @@ const initSentry = () => { : [gon.gitlab_url, 'webpack-internal://'], environment: gon.sentry_environment, + // Browser tracing configuration + tracePropagationTargets: [/^\//], // only trace internal requests + tracesSampleRate: gon.sentry_clientside_traces_sample_rate || 0, + // This configuration imitates the Sentry.init() default configuration // https://github.com/getsentry/sentry-javascript/blob/7.66.0/MIGRATION.md#explicit-client-options transport: makeFetchTransport, diff --git a/app/services/groups/ssh_certificates/create_service.rb b/app/services/groups/ssh_certificates/create_service.rb new file mode 100644 index 00000000000..6890901c306 --- /dev/null +++ b/app/services/groups/ssh_certificates/create_service.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Groups + module SshCertificates + class CreateService + def initialize(group, params) + @group = group + @params = params + end + + def execute + key = params[:key] + fingerprint = generate_fingerprint(key) + + return ServiceResponse.error(message: 'Group', reason: :forbidden) if group.has_parent? + + # return a key error instead of fingerprint error, as the user has no knowledge of fingerprint. + unless fingerprint + return ServiceResponse.error(message: 'Validation failed: Invalid key', + reason: :unprocessable_entity) + end + + result = group.ssh_certificates.create!( + key: key, + title: params[:title], + fingerprint: fingerprint + ) + + # title and key attributes are returned as [FILTERED] + # by config/application.rb#L181-233 + # make attributes unfiltered by running find + ssh_certificate = group.ssh_certificates.find(result.id) + ServiceResponse.success(payload: ssh_certificate) + + rescue ActiveRecord::RecordInvalid, ArgumentError => e + ServiceResponse.error( + message: e.message, + reason: :unprocessable_entity + ) + end + + private + + attr_reader :group, :params + + def generate_fingerprint(key) + Gitlab::SSHPublicKey.new(key).fingerprint_sha256&.delete_prefix('SHA256:') + end + end + end +end diff --git a/app/services/groups/ssh_certificates/destroy_service.rb b/app/services/groups/ssh_certificates/destroy_service.rb new file mode 100644 index 00000000000..7a450d5bee6 --- /dev/null +++ b/app/services/groups/ssh_certificates/destroy_service.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Groups + module SshCertificates + class DestroyService + def initialize(group, params) + @group = group + @params = params + end + + def execute + ssh_certificate = group.ssh_certificates.find(params[:ssh_certificates_id]) + + ssh_certificate.destroy! + ServiceResponse.success + + rescue ActiveRecord::RecordNotFound + ServiceResponse.error( + message: 'SSH Certificate not found', + reason: :not_found + ) + + rescue ActiveRecord::RecordNotDestroyed + ServiceResponse.error( + message: 'SSH Certificate could not be deleted', + reason: :method_not_allowed + ) + end + + private + + attr_reader :group, :params + end + end +end diff --git a/config/feature_flags/development/ssh_certificates_rest_endpoints.yml b/config/feature_flags/development/ssh_certificates_rest_endpoints.yml new file mode 100644 index 00000000000..29087a8631f --- /dev/null +++ b/config/feature_flags/development/ssh_certificates_rest_endpoints.yml @@ -0,0 +1,8 @@ +--- +name: ssh_certificates_rest_endpoints +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130866 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/424501 +milestone: '16.4' +type: development +group: group::source code +default_enabled: false diff --git a/config/initializers/8_devise.rb b/config/initializers/8_devise.rb index 2e0fc9dbf08..3682a391033 100644 --- a/config/initializers/8_devise.rb +++ b/config/initializers/8_devise.rb @@ -162,10 +162,10 @@ Devise.setup do |config| # Number of authentication tries before locking an account if lock_strategy # is failed attempts. - config.maximum_attempts = 3 + config.maximum_attempts = 10 # Time interval to unlock the account if :time is enabled as unlock_strategy. - config.unlock_in = 30.minutes + config.unlock_in = 10.minutes # ==> Configuration for :recoverable # diff --git a/danger/ignored_model_columns/Dangerfile b/danger/ignored_model_columns/Dangerfile new file mode 100644 index 00000000000..e11566f90f2 --- /dev/null +++ b/danger/ignored_model_columns/Dangerfile @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +ignored_model_columns.add_comment_for_ignored_model_columns diff --git a/danger/plugins/ignored_model_columns.rb b/danger/plugins/ignored_model_columns.rb new file mode 100644 index 00000000000..8e0024a94d6 --- /dev/null +++ b/danger/plugins/ignored_model_columns.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require_relative '../../tooling/danger/ignored_model_columns' + +module Danger + class IgnoredModelColumns < ::Danger::Plugin + include Tooling::Danger::IgnoredModelColumns + end +end diff --git a/doc/api/api_resources.md b/doc/api/api_resources.md index 8706e605c51..9f2a57dd84f 100644 --- a/doc/api/api_resources.md +++ b/doc/api/api_resources.md @@ -112,37 +112,38 @@ The following API resources are available in the project context: The following API resources are available in the group context: -| Resource | Available endpoints | -|:-----------------------------------------------------------------|:--------------------| -| [Access requests](access_requests.md) | `/groups/:id/access_requests/` (also available for projects) | -| [Access tokens](group_access_tokens.md) | `/groups/:id/access_tokens` (also available for projects) | -| [Custom attributes](custom_attributes.md) | `/groups/:id/custom_attributes` (also available for projects and users) | -| [Debian distributions](packages/debian_group_distributions.md) | `/groups/:id/-/packages/debian` (also available for projects) | -| [Deploy tokens](deploy_tokens.md) | `/groups/:id/deploy_tokens` (also available for projects and standalone) | -| [Discussions](discussions.md) (comments and threads) | `/groups/:id/epics/.../discussions` (also available for projects) | -| [Epic issues](epic_issues.md) **(PREMIUM ALL)** | `/groups/:id/epics/.../issues` | -| [Epic links](epic_links.md) **(PREMIUM ALL)** | `/groups/:id/epics/.../epics` | -| [Epics](epics.md) **(PREMIUM ALL)** | `/groups/:id/epics` | -| [Groups](groups.md) | `/groups`, `/groups/.../subgroups` | -| [Group badges](group_badges.md) | `/groups/:id/badges` | -| [Group issue boards](group_boards.md) | `/groups/:id/boards` | -| [Group iterations](group_iterations.md) **(PREMIUM ALL)** | `/groups/:id/iterations` (also available for projects) | -| [Group labels](group_labels.md) | `/groups/:id/labels` | -| [Group-level variables](group_level_variables.md) | `/groups/:id/variables` | -| [Group milestones](group_milestones.md) | `/groups/:id/milestones` | -| [Group releases](group_releases.md) | `/groups/:id/releases`| -| [Group wikis](group_wikis.md) **(PREMIUM ALL)** | `/groups/:id/wikis` | -| [Invitations](invitations.md) | `/groups/:id/invitations` (also available for projects) | -| [Issues](issues.md) | `/groups/:id/issues` (also available for projects and standalone) | -| [Issues Statistics](issues_statistics.md) | `/groups/:id/issues_statistics` (also available for projects and standalone) | -| [Linked epics](linked_epics.md) | `/groups/:id/epics/.../related_epics` | -| [Member Roles](member_roles.md) | `/groups/:id/member_roles` | -| [Members](members.md) | `/groups/:id/members` (also available for projects) | -| [Merge requests](merge_requests.md) | `/groups/:id/merge_requests` (also available for projects and standalone) | -| [Notes](notes.md) (comments) | `/groups/:id/epics/.../notes` (also available for projects) | -| [Notification settings](notification_settings.md) | `/groups/:id/notification_settings` (also available for projects and standalone) | -| [Resource label events](resource_label_events.md) | `/groups/:id/epics/.../resource_label_events` (also available for projects) | -| [Search](search.md) | `/groups/:id/search` (also available for projects and standalone) | +| Resource | Available endpoints | +|:----------------------------------------------------------------------|:---------------------------------------------------------------------------------| +| [Access requests](access_requests.md) | `/groups/:id/access_requests/` (also available for projects) | +| [Access tokens](group_access_tokens.md) | `/groups/:id/access_tokens` (also available for projects) | +| [Custom attributes](custom_attributes.md) | `/groups/:id/custom_attributes` (also available for projects and users) | +| [Debian distributions](packages/debian_group_distributions.md) | `/groups/:id/-/packages/debian` (also available for projects) | +| [Deploy tokens](deploy_tokens.md) | `/groups/:id/deploy_tokens` (also available for projects and standalone) | +| [Discussions](discussions.md) (comments and threads) | `/groups/:id/epics/.../discussions` (also available for projects) | +| [Epic issues](epic_issues.md) **(PREMIUM ALL)** | `/groups/:id/epics/.../issues` | +| [Epic links](epic_links.md) **(PREMIUM ALL)** | `/groups/:id/epics/.../epics` | +| [Epics](epics.md) **(PREMIUM ALL)** | `/groups/:id/epics` | +| [Groups](groups.md) | `/groups`, `/groups/.../subgroups` | +| [Group badges](group_badges.md) | `/groups/:id/badges` | +| [Group issue boards](group_boards.md) | `/groups/:id/boards` | +| [Group iterations](group_iterations.md) **(PREMIUM ALL)** | `/groups/:id/iterations` (also available for projects) | +| [Group labels](group_labels.md) | `/groups/:id/labels` | +| [Group-level variables](group_level_variables.md) | `/groups/:id/variables` | +| [Group milestones](group_milestones.md) | `/groups/:id/milestones` | +| [Group releases](group_releases.md) | `/groups/:id/releases` | +| [Group SSH certificates](group_ssh_certificates.md) **(PREMIUM ALL)** | `/groups/:id/ssh_certificates` | +| [Group wikis](group_wikis.md) **(PREMIUM ALL)** | `/groups/:id/wikis` | +| [Invitations](invitations.md) | `/groups/:id/invitations` (also available for projects) | +| [Issues](issues.md) | `/groups/:id/issues` (also available for projects and standalone) | +| [Issues Statistics](issues_statistics.md) | `/groups/:id/issues_statistics` (also available for projects and standalone) | +| [Linked epics](linked_epics.md) | `/groups/:id/epics/.../related_epics` | +| [Member Roles](member_roles.md) | `/groups/:id/member_roles` | +| [Members](members.md) | `/groups/:id/members` (also available for projects) | +| [Merge requests](merge_requests.md) | `/groups/:id/merge_requests` (also available for projects and standalone) | +| [Notes](notes.md) (comments) | `/groups/:id/epics/.../notes` (also available for projects) | +| [Notification settings](notification_settings.md) | `/groups/:id/notification_settings` (also available for projects and standalone) | +| [Resource label events](resource_label_events.md) | `/groups/:id/epics/.../resource_label_events` (also available for projects) | +| [Search](search.md) | `/groups/:id/search` (also available for projects and standalone) | ## Standalone resources diff --git a/doc/api/group_ssh_certificates.md b/doc/api/group_ssh_certificates.md new file mode 100644 index 00000000000..d6dc17a5c15 --- /dev/null +++ b/doc/api/group_ssh_certificates.md @@ -0,0 +1,115 @@ +--- +stage: Create +group: Source Code +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/product/ux/technical-writing/#assignments +--- + +# Group SSH certificates API **(PREMIUM ALL)** + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/421915) in GitLab 16.4 [with a flag](../user/feature_flags.md) named `ssh_certificates_rest_endpoints`. Disabled by default. + +FLAG: +On self-managed GitLab, by default this feature is not available. To make it available, an administrator can [enable the feature flag](../administration/feature_flags.md) named `ssh_certificates_rest_endpoints`. +On GitLab.com, this feature is not available. + +Use this API to create, read and delete SSH certificates for a group. +Only top-level groups can store SSH certificates. +To use this API you must [authenticate yourself](rest/index.md#authentication) as an Administrator. + +## Get all SSH certificates for a particular group + +```plaintext +GET groups/:id/ssh_certificates +``` + +Parameters: + +| Attribute | Type | Required | Description | +| ---------- | ------ | -------- |----------------------| +| `id` | integer | Yes | The ID of the group. | + +By default, `GET` requests return 20 results at a time because the API results are paginated. +Read more on [pagination](rest/index.md#pagination). + +Example request: + +```shell +curl --header "PRIVATE-TOKEN: <your_access_token>" "https://primary.example.com/api/v4/groups/90/ssh_certificates" +``` + +Example response: + +```json +[ + { + "id": 12345, + "title": "SSH Title 1", + "key": "ssh-rsa AAAAB3NzaC1ea2dAAAADAQABAAAAgQDGbLkF44ScxRQi2FfA7VsHgGqptguSbmW26jkJhEiRZpGS4/+UzaaSqc8Psw2OhSsKc5QwfrB/ANpO4LhOjDzhf2FuD8ACkv3R7XtaJ+rN6PlyzoBfLAiSyzxhEoMFDBprTgaiZKgg2yQ9dRH55w3f6XMZ4hnaUae53nQgfQLxFw== example@gitlab.com", + "created_at": "2023-09-08T12:39:00.172Z" + }, + { + "id":12346, + "title":"SSH Title 2", + "key": "ssh-rsa AAAAB3NzaC1ac2EAAAADAQABAAAAgQDTl/hHfu1F/KlR+QfgM2wUmyxcN5YeiaWluEGIrfXUeJuI+bK6xjpE3+2afHDYtE9VQkeL32KRjefX2d72Jeoa68ewt87Vn8CcGkUTOTpHNzeL8pHMKFs3m7ArSBxNg5vTdgAsq5dbDGNtat7b2WCHTNvtWoON1Jetne30uW2EwQ== example@gitlab.com", + "created_at": "2023-09-08T12:39:00.244Z" + } +] +``` + +## Create SSH Certificate + +Create a new SSH certificate in the group. + +```plaintext +POST /groups/:id/ssh_certificates +``` + +Parameters: + +| Attribute | Type | Required | Description | +|-----------|------------| -------- |---------------------------------------| +| `id` | integer | Yes | The ID of the group. | +| `key` | string | Yes | The public key of the SSH certificate.| +| `title` | string | Yes | The title of the SSH certificate. | + +Example request: + +```shell +curl --request POST \ + --header "PRIVATE-TOKEN: <your_access_token>" \ + --url "https://gitlab.example.com/api/v4/groups/5/ssh_certificates?title=newtitle&key=ssh-rsa+REDACTED+example%40gitlab.com" +``` + +Example response: + +```json +{ + "id": 54321, + "title": "newtitle", + "key": "ssh-rsa ssh-rsa AAAAB3NzaC1ea2dAAAADAQABAAAAgQDGbLkF44ScxRQi2FfA7VsHgGqptguSbmW26jkJhEiRZpGS4/+UzaaSqc8Psw2OhSsKc5QwfrB/ANpO4LhOjDzhf2FuD8ACkv3R7XtaJ+rN6PlyzoBfLAiSyzxhEoMFDBprTgaiZKgg2yQ9dRH55w3f6XMZ4hnaUae53nQgfQLxFw== example@gitlab.com", + "created_at": "2023-09-08T12:39:00.172Z" +} +``` + +## Delete group SSH certificate + +Delete a SSH certificate from a group. + +```plaintext +DELETE /groups/:id/ssh_certificate/:id +``` + +Parameters: + +| Attribute | Type | Required | Description | +|-----------|---------| -------- |-------------------------------| +| `id` | integer | Yes | The ID of the group | +| `id` | integer | Yes | The ID of the SSH certificate | + +Example request: + +```shell +curl --request DELETE \ + --header "PRIVATE-TOKEN: <your_access_token>" \ + --url "https://gitlab.example.com/api/v4/groups/5/ssh_certificates/12345" +``` diff --git a/doc/architecture/blueprints/cells/index.md b/doc/architecture/blueprints/cells/index.md index bbf33d6bbd5..28414f9b68c 100644 --- a/doc/architecture/blueprints/cells/index.md +++ b/doc/architecture/blueprints/cells/index.md @@ -399,6 +399,29 @@ TBD TBD +### How do I decide whether to move my feature to the cluster, Cell or Organization level? + +By default, features are required to be scoped to the Organization level. Any deviation from that rule should be validated and approved by Tenant Scale. + +The design goals of the Cells architecture describe that [all Cells are under a single domain](goals.md#all-cells-are-under-a-single-gitlabcom-domain) and as such, Cells are invisible to the user: + +- Cell-local features should be limited to those related to managing the Cell, but never be a feature where the Cell semantic is exposed to the customer. +- The Cells architecture wants to freely control the distribution of Organization and customer data across Cells without impacting users when data is migrated. + +Cluster-wide features are strongly discouraged because: + +- They might require storing a substantial amount of data cluster-wide which decreases [scalability headroom](goals.md#provides-100x-headroom). +- They might require implementation of non-trivial [data aggregation](goals.md#aggregation-of-cluster-wide-data) that reduces resilience to [single node failure](goals.md#high-resilience-to-a-single-cell-failure). +- They are harder to build due to the need of being able to run [mixed deployments](goals.md#cells-running-in-mixed-deployments). Cluster-wide features need to take this into account. +- They might affect our ability to provide an [on-premise like experience on GitLab.com](goals.md#on-premise-like-experience). +- Some features that are expected to be cluster-wide might in fact be better implemented using federation techniques that use trusted intra-cluster communication using the same user identity. User Profile is shared across the cluster. +- The Cells architecture limits what services can be considered cluster-wide. Services that might initially be cluster-wide are still expected to be split in the future to achieve full service isolation. No feature should be built to depend on such a service (like Elasticsearch). + +### Will Cells use the [reference architecture for 50,000 users](../../../administration/reference_architectures/50k_users.md)? + +The infrastructure team will properly size Cells depending on the load. +The Tenant Scale team sees an opportunity to use GitLab Dedicated as a base for Cells deployment. + ## Decision log - 2022-03-15: Google Cloud as the cloud service. For details, see [issue 396641](https://gitlab.com/gitlab-org/gitlab/-/issues/396641#note_1314932272). diff --git a/spec/features/users/email_verification_on_login_spec.rb b/spec/features/users/email_verification_on_login_spec.rb index 407311afbc9..d83040efd72 100644 --- a/spec/features/users/email_verification_on_login_spec.rb +++ b/spec/features/users/email_verification_on_login_spec.rb @@ -247,6 +247,7 @@ RSpec.describe 'Email Verification On Login', :clean_gitlab_redis_rate_limiting, end it_behaves_like 'email verification required' + it_behaves_like 'no email verification required when 2fa enabled or ff disabled' describe 'when waiting for the auto unlock time' do before do diff --git a/spec/frontend/sentry/init_sentry_spec.js b/spec/frontend/sentry/init_sentry_spec.js index 8cfbcc1cfca..e31068b935b 100644 --- a/spec/frontend/sentry/init_sentry_spec.js +++ b/spec/frontend/sentry/init_sentry_spec.js @@ -22,6 +22,7 @@ const mockVersion = '1.0.0'; const mockRevision = '00112233'; const mockFeatureCategory = 'my_feature_category'; const mockPage = 'index:page'; +const mockSentryClientsideTracesSampleRate = 0.1; jest.mock('sentrybrowser', () => { return { @@ -51,6 +52,7 @@ describe('SentryConfig', () => { version: mockVersion, revision: mockRevision, feature_category: mockFeatureCategory, + sentry_clientside_traces_sample_rate: mockSentryClientsideTracesSampleRate, }; document.body.dataset.page = mockPage; @@ -89,6 +91,8 @@ describe('SentryConfig', () => { release: mockVersion, allowUrls: [mockGitlabUrl, 'webpack-internal://'], environment: mockEnvironment, + tracesSampleRate: mockSentryClientsideTracesSampleRate, + tracePropagationTargets: [/^\//], transport: makeFetchTransport, stackParser: defaultStackParser, diff --git a/spec/lib/banzai/filter/inline_diff_filter_spec.rb b/spec/lib/banzai/filter/inline_diff_filter_spec.rb index 1388a9053d9..89ee17837e0 100644 --- a/spec/lib/banzai/filter/inline_diff_filter_spec.rb +++ b/spec/lib/banzai/filter/inline_diff_filter_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Banzai::Filter::InlineDiffFilter do +RSpec.describe Banzai::Filter::InlineDiffFilter, feature_category: :source_code_management do include FilterSpecHelper it 'adds inline diff span tags for deletions when using square brackets' do diff --git a/spec/models/concerns/require_email_verification_spec.rb b/spec/models/concerns/require_email_verification_spec.rb index bfce2e93c97..63312d4e1f1 100644 --- a/spec/models/concerns/require_email_verification_spec.rb +++ b/spec/models/concerns/require_email_verification_spec.rb @@ -57,9 +57,17 @@ RSpec.describe RequireEmailVerification, feature_category: :insider_threat do it { is_expected.to eq(false) } end - context 'when failed_attempts is GTE overridden amount' do + context 'when failed_attempts is GTE overridden amount but LT Devise default amount' do before do - instance.failed_attempts = 3 + instance.failed_attempts = 6 + end + + it { is_expected.to eq(overridden) } + end + + context 'when failed_attempts is GTE Devise default amount' do + before do + instance.failed_attempts = 10 end it { is_expected.to eq(true) } @@ -79,7 +87,7 @@ RSpec.describe RequireEmailVerification, feature_category: :insider_threat do context 'when locked longer ago than Devise default time but shorter ago than overriden time' do before do - instance.locked_at = 31.minutes.ago + instance.locked_at = 11.minutes.ago end it { is_expected.to eq(!overridden) } diff --git a/spec/tooling/danger/ignored_model_columns_spec.rb b/spec/tooling/danger/ignored_model_columns_spec.rb new file mode 100644 index 00000000000..737b6cce077 --- /dev/null +++ b/spec/tooling/danger/ignored_model_columns_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require 'danger' +require 'gitlab/dangerfiles/spec_helper' + +require_relative '../../../tooling/danger/ignored_model_columns' +require_relative '../../../tooling/danger/project_helper' + +RSpec.describe Tooling::Danger::IgnoredModelColumns, feature_category: :tooling do + subject(:ignored_model_columns) { fake_danger.new(helper: fake_helper) } + + let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) } + let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) } + let(:comment) { described_class::COMMENT.chomp } + let(:file_diff) do + File.read(File.expand_path("../fixtures/#{fixture}", __dir__)).split("\n") + end + + include_context "with dangerfile" + + describe '#add_comment_for_ignored_model_columns' do + let(:file_lines) { file_diff.map { |line| line.delete_prefix('+').delete_prefix('-') } } + + before do + allow(ignored_model_columns).to receive(:project_helper).and_return(fake_project_helper) + allow(ignored_model_columns.project_helper).to receive(:file_lines).and_return(file_lines) + allow(ignored_model_columns.helper).to receive(:all_changed_files).and_return([filename]) + allow(ignored_model_columns.helper).to receive(:changed_lines).with(filename).and_return(file_diff) + end + + context 'when table column is renamed in a regular migration' do + let(:filename) { 'db/migrate/rename_my_column_migration.rb' } + let(:fixture) { 'rename_column_migration.txt' } + let(:matching_lines) { [7, 11, 15, 19, 23, 27, 31, 35, 39] } + + it 'adds comment at the correct line' do + matching_lines.each do |line_number| + expect(ignored_model_columns).to receive(:markdown).with("\n#{comment}", file: filename, line: line_number) + end + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + + context 'when table column is renamed in a post migration' do + let(:filename) { 'db/post_migrate/remove_column_migration.rb' } + let(:fixture) { 'remove_column_migration.txt' } + let(:matching_lines) { [7, 8, 16, 24, 32, 40, 48, 56, 64, 72] } + + it 'adds comment at the correct line' do + matching_lines.each do |line_number| + expect(ignored_model_columns).to receive(:markdown).with("\n#{comment}", file: filename, line: line_number) + end + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + + context 'when table cleanup is performed in a post migration' do + let(:filename) { 'db/post_migrate/cleanup_conversion_big_int_migration.rb' } + let(:fixture) { 'cleanup_conversion_migration.txt' } + let(:matching_lines) { [7, 11, 15, 19, 23, 27, 31, 35, 39] } + + it 'adds comment at the correct line' do + matching_lines.each do |line_number| + expect(ignored_model_columns).to receive(:markdown).with("\n#{comment}", file: filename, line: line_number) + end + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + + context 'when a regular migration does not rename table column' do + let(:filename) { 'db/migrate/my_migration.rb' } + let(:file_diff) do + [ + "+ undo_cleanup_concurrent_column_rename(:my_table, :old_column, :new_column)", + "- cleanup_concurrent_column_rename(:my_table, :new_column, :old_column)" + ] + end + + let(:file_lines) do + [ + ' def up', + ' undo_cleanup_concurrent_column_rename(:my_table, :old_column, :new_column)', + ' end' + ] + end + + it 'does not add comment' do + expect(ignored_model_columns).not_to receive(:markdown) + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + + context 'when a post migration does not remove table column' do + let(:filename) { 'db/migrate/my_migration.rb' } + let(:file_diff) do + [ + "+ add_column(:my_table, :my_column, :type)", + "- remove_column(:my_table, :my_column)" + ] + end + + let(:file_lines) do + [ + ' def up', + ' add_column(:my_table, :my_column, :type)', + ' end' + ] + end + + it 'does not add comment' do + expect(ignored_model_columns).not_to receive(:markdown) + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + + context 'when a post migration does not convert table column' do + let(:filename) { 'db/migrate/my_migration.rb' } + let(:file_diff) do + [ + "+ restore_conversion_of_integer_to_bigint(TABLE, COLUMNS)", + "- cleanup_conversion_of_integer_to_bigint(TABLE, COLUMNS)" + ] + end + + let(:file_lines) do + [ + ' def up', + ' cleanup_conversion_of_integer_to_bigint(TABLE, COLUMNS)', + ' end' + ] + end + + it 'does not add comment' do + expect(ignored_model_columns).not_to receive(:markdown) + + ignored_model_columns.add_comment_for_ignored_model_columns + end + end + end +end diff --git a/spec/tooling/fixtures/cleanup_conversion_migration.txt b/spec/tooling/fixtures/cleanup_conversion_migration.txt new file mode 100644 index 00000000000..14a7937b469 --- /dev/null +++ b/spec/tooling/fixtures/cleanup_conversion_migration.txt @@ -0,0 +1,44 @@ ++# frozen_string_literal: true ++ ++class TestMigration < Gitlab::Database::Migration[2.1] ++ disable_ddl_transaction! ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint :my_table, :my_column ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint 'my_table', 'my_column' ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint "my_table", "my_column", "new_column" ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint TABLE_NAME, MY_COLUMN ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint(:my_table, :my_column) ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint('my_table', 'my_column') ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint("my_table", "my_column") ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint(TABLE_NAME, MY_COLUMN) ++ end ++ ++ def up ++ cleanup_conversion_of_integer_to_bigint( ++ :my_table, ++ :my_column ++ ) ++ end ++end diff --git a/spec/tooling/fixtures/remove_column_migration.txt b/spec/tooling/fixtures/remove_column_migration.txt new file mode 100644 index 00000000000..885f0060d92 --- /dev/null +++ b/spec/tooling/fixtures/remove_column_migration.txt @@ -0,0 +1,84 @@ ++# frozen_string_literal: true ++ ++class TestMigration < Gitlab::Database::Migration[2.1] ++ disable_ddl_transaction! ++ ++ def up ++ remove_column :my_table, :my_column ++ remove_column :my_other_table, :my_column ++ end ++ ++ def down ++ remove_column :my_table, :my_column ++ end ++ ++ def up ++ remove_column 'my_table', 'my_column' ++ end ++ ++ def down ++ remove_column 'my_table', 'my_column' ++ end ++ ++ def up ++ remove_column "my_table", "my_column", "new_column" ++ end ++ ++ def down ++ remove_column "my_table", "my_column", "new_column" ++ end ++ ++ def up ++ remove_column TABLE_NAME, MY_COLUMN ++ end ++ ++ def down ++ remove_column TABLE_NAME, MY_COLUMN ++ end ++ ++ def up ++ remove_column(:my_table, :my_column) ++ end ++ ++ def down ++ remove_column(:my_table, :my_column) ++ end ++ ++ def up ++ remove_column('my_table', 'my_column') ++ end ++ ++ def down ++ remove_column('my_table', 'my_column') ++ end ++ ++ def up ++ remove_column("my_table", "my_column") ++ end ++ ++ def down ++ remove_column("my_table", "my_column") ++ end ++ ++ def up ++ remove_column(TABLE_NAME, MY_COLUMN) ++ end ++ ++ def down ++ remove_column(TABLE_NAME, MY_COLUMN) ++ end ++ ++ def up ++ remove_column( ++ :my_table, ++ :my_column ++ ) ++ end ++ ++ def down ++ remove_column( ++ :my_table, ++ :my_column ++ ) ++ end ++end diff --git a/spec/tooling/fixtures/rename_column_migration.txt b/spec/tooling/fixtures/rename_column_migration.txt new file mode 100644 index 00000000000..e79029219a5 --- /dev/null +++ b/spec/tooling/fixtures/rename_column_migration.txt @@ -0,0 +1,45 @@ ++# frozen_string_literal: true ++ ++class TestMigration < Gitlab::Database::Migration[2.1] ++ disable_ddl_transaction! ++ ++ def up ++ cleanup_concurrent_column_rename :my_table, :old_column, :new_column ++ end ++ ++ def up ++ cleanup_concurrent_column_rename 'my_table', 'old_column', 'new_column' ++ end ++ ++ def up ++ cleanup_concurrent_column_rename "my_table", "old_column", "new_column" ++ end ++ ++ def up ++ cleanup_concurrent_column_rename TABLE_NAME, OLD_COLUMN_NAME, NEW_COLUMN_NAME ++ end ++ ++ def up ++ cleanup_concurrent_column_rename(:my_table, :old_column, :new_column) ++ end ++ ++ def up ++ cleanup_concurrent_column_rename('my_table', 'old_column', 'new_column') ++ end ++ ++ def up ++ cleanup_concurrent_column_rename("my_table", "old_column", "new_column") ++ end ++ ++ def up ++ cleanup_concurrent_column_rename(TABLE_NAME, OLD_COLUMN_NAME, NEW_COLUMN_NAME) ++ end ++ ++ def up ++ cleanup_concurrent_column_rename( ++ :my_table, ++ :old_column, ++ :new_column ++ ) ++ end ++end diff --git a/tooling/danger/ignored_model_columns.rb b/tooling/danger/ignored_model_columns.rb new file mode 100644 index 00000000000..50379eed85c --- /dev/null +++ b/tooling/danger/ignored_model_columns.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require_relative 'suggestor' + +module Tooling + module Danger + module IgnoredModelColumns + include ::Tooling::Danger::Suggestor + + METHODS = %w[remove_column cleanup_concurrent_column_rename cleanup_conversion_of_integer_to_bigint].freeze + MIGRATION_FILES_REGEX = %r{^db/(post_)?migrate} + MIGRATION_METHODS_REGEX = /^\+\s*(.*\.)?(#{METHODS.join('|')})[(\s]/ + UP_METHOD_REGEX = /^.+(def\sup)/ + END_METHOD_REGEX = /^.+(end)/ + DOC_URL = "https://docs.gitlab.com/ee/development/database/avoiding_downtime_in_migrations.html" + + COMMENT = <<~COMMENT.freeze + Column operations, like [dropping](#{DOC_URL}#dropping-columns), [renaming](#{DOC_URL}#renaming-columns) or + [primary key conversion](#{DOC_URL}#migrating-integer-primary-keys-to-bigint), require columns to be ignored in + the model. This step is necessary because Rails caches the columns and re-uses it in various places across the + application. Please ensure that columns are properly ignored in the model. + COMMENT + + def add_comment_for_ignored_model_columns + migration_files.each do |filename| + add_suggestion(filename: filename, regex: MIGRATION_METHODS_REGEX, comment_text: COMMENT) + end + end + + private + + # This method was overwritten to make use of +up_method_lines+. + # It's necessary to only match lines that are inside the +up+ block in a migration file. + # + # @return [Integer, Nil] the line number - only if the line is from within a +up+ method + def find_line_number(file_lines, searched_line, exclude_indexes: []) + lines_to_search = up_method_lines(file_lines) + + _, index = file_lines.each_with_index.find do |file_line, index| + next unless lines_to_search.include?(index) + + file_line == searched_line && !exclude_indexes.include?(index) # rubocop:disable Rails/NegateInclude + end + + index + end + + # Return the line numbers from within the up method + # + # @example + # line 0 def up + # line 1 cleanup_conversion_of_integer_to_bigint():my_table, :my_column) + # line 2 remove_column(:my_table, :my_column) + # line 3 other_method + # line 4 end + # + # => [1, 2, 3] + def up_method_lines(file_lines) + capture_up_block = false + up_method_content_lines = [] + + file_lines.each_with_index do |content, line_number| + capture_up_block = false if capture_up_block && END_METHOD_REGEX.match?(content) + up_method_content_lines << line_number if capture_up_block + capture_up_block = true if UP_METHOD_REGEX.match?(content) + end + + up_method_content_lines + end + + def migration_files + helper.all_changed_files.grep(MIGRATION_FILES_REGEX) + end + end + end +end |