From 66d2c649711c7e7001cc5d1c332f99308e4aaffa Mon Sep 17 00:00:00 2001 From: Marc Saleiko Date: Thu, 11 Jan 2024 11:51:57 +0100 Subject: [PATCH 1/2] Obfuscate external participants emails in system notes When managing external participants in an issue we add system notes to display which email was added or removed. On public issues or for guest users this information should not be visible. Adds obfuscation to issue email participant system notes, so no email addresses are disclosed if the user does not have the needed role. Changelog: added --- app/graphql/types/notes/note_type.rb | 24 ++++++---- app/models/system_note_metadata.rb | 1 + app/presenters/note_presenter.rb | 28 +++++++++++ app/serializers/note_entity.rb | 12 ++++- .../system_notes/issuables_service.rb | 2 +- doc/api/graphql/reference/index.md | 22 ++++----- lib/api/entities/note.rb | 6 ++- lib/api/notes.rb | 2 +- lib/gitlab/utils/email.rb | 13 ++++++ spec/graphql/types/notes/note_type_spec.rb | 34 ++++++++++++++ spec/lib/gitlab/utils/email_spec.rb | 16 +++++++ spec/presenters/note_presenter_spec.rb | 45 ++++++++++++++++++ spec/requests/api/notes_spec.rb | 32 +++++++++++++ spec/serializers/note_entity_spec.rb | 46 ++++++++++++++++--- .../create_service_spec.rb | 1 + ...tes_content_obfuscation_shared_examples.rb | 39 ++++++++++++++++ 16 files changed, 292 insertions(+), 31 deletions(-) create mode 100644 app/presenters/note_presenter.rb create mode 100644 spec/presenters/note_presenter_spec.rb create mode 100644 spec/support/shared_examples/graphql/notes_content_obfuscation_shared_examples.rb diff --git a/app/graphql/types/notes/note_type.rb b/app/graphql/types/notes/note_type.rb index 0f2a01d73901bc..d1e917b73c79b3 100644 --- a/app/graphql/types/notes/note_type.rb +++ b/app/graphql/types/notes/note_type.rb @@ -13,6 +13,8 @@ class NoteType < BaseObject implements Types::ResolvableInterface + present_using NotePresenter + field :max_access_level_of_author, GraphQL::Types::String, null: true, description: "Max access level of the note author in the project.", @@ -28,11 +30,11 @@ class NoteType < BaseObject field :author, Types::UserType, null: true, - description: 'User who wrote this note.' + description: 'User who wrote the note.' field :system, GraphQL::Types::Boolean, null: false, - description: 'Indicates whether this note was created by the system or by a user.' + description: 'Indicates whether the note was created by the system or by a user.' field :system_note_icon_name, GraphQL::Types::String, null: true, @@ -49,7 +51,7 @@ class NoteType < BaseObject field :confidential, GraphQL::Types::Boolean, null: true, - description: 'Indicates if this note is confidential.', + description: 'Indicates if the note is confidential.', method: :confidential?, deprecated: { reason: :renamed, @@ -59,7 +61,7 @@ class NoteType < BaseObject field :internal, GraphQL::Types::Boolean, null: true, - description: 'Indicates if this note is internal.', + description: 'Indicates if the note is internal.', method: :confidential? field :created_at, Types::TimeType, @@ -67,16 +69,16 @@ class NoteType < BaseObject description: 'Timestamp of the note creation.' field :discussion, Types::Notes::DiscussionType, null: true, - description: 'Discussion this note is a part of.' + description: 'Discussion the note is a part of.' field :position, Types::Notes::DiffPositionType, null: true, - description: 'Position of this note on a diff.' + description: 'Position of the note on a diff.' field :updated_at, Types::TimeType, null: false, description: "Timestamp of the note's last activity." field :url, GraphQL::Types::String, null: true, - description: 'URL to view this Note in the Web UI.' + description: 'URL to view the note in the Web UI.' field :last_edited_at, Types::TimeType, null: true, @@ -95,7 +97,10 @@ class NoteType < BaseObject null: true, description: 'Metadata for the given note if it is a system note.' - markdown_field :body_html, null: true, method: :note + field :body_html, GraphQL::Types::String, + method: :note_html, + null: true, + description: "GitLab Flavored Markdown rendering of the content of the note." def url ::Gitlab::UrlBuilder.build(object) @@ -118,7 +123,8 @@ def author def id return super unless object.is_a?(SyntheticNote) - ::Gitlab::GlobalId.build(object, model_name: object.class.to_s, id: object.discussion_id) + # object is a presenter, so object.object returns the concrete note object. + ::Gitlab::GlobalId.build(object, model_name: object.object.class.to_s, id: object.discussion_id) end end end diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 58b46a20386880..16a19de0f9dcfe 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -25,6 +25,7 @@ class SystemNoteMetadata < ApplicationRecord tag due_date start_date_or_due_date pinned_embed cherry_pick health_status approved unapproved status alert_issue_added relate unrelate new_alert_added severity contact timeline_event issue_type relate_to_child unrelate_from_child relate_to_parent unrelate_from_parent override + issue_email_participants ].freeze validates :note, presence: true, unless: :importing? diff --git a/app/presenters/note_presenter.rb b/app/presenters/note_presenter.rb new file mode 100644 index 00000000000000..00eeded05b7390 --- /dev/null +++ b/app/presenters/note_presenter.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class NotePresenter < Gitlab::View::Presenter::Delegated # rubocop:disable Gitlab/NamespacedClass -- Note is not namespaced + presents ::Note, as: :object # because we also have a #note method + + delegator_override :note + def note + obfuscate_participants_emails_in_system_note(object.note) + end + + delegator_override :note_html + def note_html + # Always use `redacted_note_html` because it removes references + # based on the current user context. + # But fall back to `note_html` if redacted is not available (same behavior as `markdown_field`) + obfuscate_participants_emails_in_system_note(object.redacted_note_html || object.note_html) + end + + private + + def obfuscate_participants_emails_in_system_note(text) + return text unless object.system? + return text if can?(current_user, :read_external_emails, object.project) + return text if object.system_note_metadata&.action != 'issue_email_participants' + + Gitlab::Utils::Email.obfuscate_emails_in_text(text) + end +end diff --git a/app/serializers/note_entity.rb b/app/serializers/note_entity.rb index a50d893d24459e..919d8f08a150c6 100644 --- a/app/serializers/note_entity.rb +++ b/app/serializers/note_entity.rb @@ -17,9 +17,13 @@ class NoteEntity < API::Entities::Note expose :author, using: NoteUserEntity unexpose :note, as: :body - expose :note + expose :note do |note| + note_presenter(note).note + end - expose :redacted_note_html, as: :note_html + expose :note_html do |note| + note_presenter(note).note_html + end expose :last_edited_at, if: -> (note, _) { note.edited? } expose :last_edited_by, using: NoteUserEntity, if: -> (note, _) { note.edited? } @@ -118,6 +122,10 @@ def external_author Gitlab::Utils::Email.obfuscated_email(object.note_metadata.external_author, deform: true) end end + + def note_presenter(note) + NotePresenter.new(note, current_user: current_user) # rubocop: disable CodeReuse/Presenter -- Directly instantiate NotePresenter because we don't have presenters for all subclasses of Note + end end NoteEntity.prepend_mod_with('NoteEntity') diff --git a/app/services/system_notes/issuables_service.rb b/app/services/system_notes/issuables_service.rb index a5aae83c32a3fb..876e694dabf042 100644 --- a/app/services/system_notes/issuables_service.rb +++ b/app/services/system_notes/issuables_service.rb @@ -440,7 +440,7 @@ def mark_duplicate_issue(canonical_issue) end def email_participants(body) - create_note(NoteSummary.new(noteable, project, author, body)) + create_note(NoteSummary.new(noteable, project, author, body, action: 'issue_email_participants')) end def discussion_lock diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index a0a8eb480a9570..3b2a6a6fe44131 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -2918,7 +2918,7 @@ Input type: `CreateDiffNoteInput` | `confidential` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated:** This was renamed. Please use `internal`. Deprecated in GitLab 15.3. | | `internal` | [`Boolean`](#boolean) | Internal flag for a note. Default is false. | | `noteableId` | [`NoteableID!`](#noteableid) | Global ID of the resource to add a note to. | -| `position` | [`DiffPositionInput!`](#diffpositioninput) | Position of this note on a diff. | +| `position` | [`DiffPositionInput!`](#diffpositioninput) | Position of the note on a diff. | #### Fields @@ -2993,7 +2993,7 @@ Input type: `CreateImageDiffNoteInput` | `confidential` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated:** This was renamed. Please use `internal`. Deprecated in GitLab 15.3. | | `internal` | [`Boolean`](#boolean) | Internal flag for a note. Default is false. | | `noteableId` | [`NoteableID!`](#noteableid) | Global ID of the resource to add a note to. | -| `position` | [`DiffImagePositionInput!`](#diffimagepositioninput) | Position of this note on a diff. | +| `position` | [`DiffImagePositionInput!`](#diffimagepositioninput) | Position of the note on a diff. | #### Fields @@ -7185,7 +7185,7 @@ Input type: `RepositionImageDiffNoteInput` | ---- | ---- | ----------- | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `id` | [`DiffNoteID!`](#diffnoteid) | Global ID of the DiffNote to update. | -| `position` | [`UpdateDiffImagePositionInput!`](#updatediffimagepositioninput) | Position of this note on a diff. | +| `position` | [`UpdateDiffImagePositionInput!`](#updatediffimagepositioninput) | Position of the note on a diff. | #### Fields @@ -8209,7 +8209,7 @@ Input type: `UpdateImageDiffNoteInput` | `body` | [`String`](#string) | Content of the note. | | `clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | | `id` | [`NoteID!`](#noteid) | Global ID of the note to update. | -| `position` | [`UpdateDiffImagePositionInput`](#updatediffimagepositioninput) | Position of this note on a diff. | +| `position` | [`UpdateDiffImagePositionInput`](#updatediffimagepositioninput) | Position of the note on a diff. | #### Fields @@ -24537,30 +24537,30 @@ Represents the network policy. | Name | Type | Description | | ---- | ---- | ----------- | -| `author` | [`UserCore`](#usercore) | User who wrote this note. | +| `author` | [`UserCore`](#usercore) | User who wrote the note. | | `authorIsContributor` | [`Boolean`](#boolean) | Indicates whether the note author is a contributor. | | `awardEmoji` | [`AwardEmojiConnection`](#awardemojiconnection) | List of emoji reactions associated with the note. (see [Connections](#connections)) | | `body` | [`String!`](#string) | Content of the note. | -| `bodyHtml` | [`String`](#string) | GitLab Flavored Markdown rendering of `note`. | +| `bodyHtml` | [`String`](#string) | GitLab Flavored Markdown rendering of the content of the note. | | `confidential` **{warning-solid}** | [`Boolean`](#boolean) | **Deprecated** in GitLab 15.5. This was renamed. Use: `internal`. | | `createdAt` | [`Time!`](#time) | Timestamp of the note creation. | -| `discussion` | [`Discussion`](#discussion) | Discussion this note is a part of. | +| `discussion` | [`Discussion`](#discussion) | Discussion the note is a part of. | | `id` | [`NoteID!`](#noteid) | ID of the note. | -| `internal` | [`Boolean`](#boolean) | Indicates if this note is internal. | +| `internal` | [`Boolean`](#boolean) | Indicates if the note is internal. | | `lastEditedAt` | [`Time`](#time) | Timestamp when note was last edited. | | `lastEditedBy` | [`UserCore`](#usercore) | User who last edited the note. | | `maxAccessLevelOfAuthor` | [`String`](#string) | Max access level of the note author in the project. | -| `position` | [`DiffPosition`](#diffposition) | Position of this note on a diff. | +| `position` | [`DiffPosition`](#diffposition) | Position of the note on a diff. | | `project` | [`Project`](#project) | Project associated with the note. | | `resolvable` | [`Boolean!`](#boolean) | Indicates if the object can be resolved. | | `resolved` | [`Boolean!`](#boolean) | Indicates if the object is resolved. | | `resolvedAt` | [`Time`](#time) | Timestamp of when the object was resolved. | | `resolvedBy` | [`UserCore`](#usercore) | User who resolved the object. | -| `system` | [`Boolean!`](#boolean) | Indicates whether this note was created by the system or by a user. | +| `system` | [`Boolean!`](#boolean) | Indicates whether the note was created by the system or by a user. | | `systemNoteIconName` | [`String`](#string) | Name of the icon corresponding to a system note. | | `systemNoteMetadata` | [`SystemNoteMetadata`](#systemnotemetadata) | Metadata for the given note if it is a system note. | | `updatedAt` | [`Time!`](#time) | Timestamp of the note's last activity. | -| `url` | [`String`](#string) | URL to view this Note in the Web UI. | +| `url` | [`String`](#string) | URL to view the note in the Web UI. | | `userPermissions` | [`NotePermissions!`](#notepermissions) | Permissions for the current user on the resource. | ### `NotePermissions` diff --git a/lib/api/entities/note.rb b/lib/api/entities/note.rb index c693edc611bbaf..97f4810866215f 100644 --- a/lib/api/entities/note.rb +++ b/lib/api/entities/note.rb @@ -8,7 +8,11 @@ class Note < Grape::Entity expose :id expose :type - expose :note, as: :body + + expose :body do |note| + NotePresenter.new(note, current_user: options[:current_user]).note + end + expose :attachment_identifier, as: :attachment expose :author, using: Entities::UserBasic expose :created_at, :updated_at diff --git a/lib/api/notes.rb b/lib/api/notes.rb index 70b4a3735e3cc7..138b2fddc8363f 100644 --- a/lib/api/notes.rb +++ b/lib/api/notes.rb @@ -51,7 +51,7 @@ class Notes < ::API::Base notes = paginate(raw_notes) notes = prepare_notes_for_rendering(notes) notes = notes.select { |note| note.readable_by?(current_user) } - present notes, with: Entities::Note + present notes, with: Entities::Note, current_user: current_user end # rubocop: enable CodeReuse/ActiveRecord diff --git a/lib/gitlab/utils/email.rb b/lib/gitlab/utils/email.rb index 5eb57a66c63d9f..dfaea1511987ef 100644 --- a/lib/gitlab/utils/email.rb +++ b/lib/gitlab/utils/email.rb @@ -5,6 +5,8 @@ module Utils module Email extend self + EMAIL_ADDRESS_REGEXP = /([\w\-._]+@[\w\-.]+\.{1}[a-zA-Z]{2,})/ + # Replaces most visible characters with * to obfuscate an email address # deform adds a fix number of * to ensure the address cannot be guessed. Also obfuscates TLD with ** def obfuscated_email(email, deform: false) @@ -14,6 +16,17 @@ def obfuscated_email(email, deform: false) masker_class.new(email).masked end + # Runs email address obfuscation on the given text. + def obfuscate_emails_in_text(text) + return text unless text.present? + + matches = text.scan(EMAIL_ADDRESS_REGEXP).flatten.index_with do |match| + obfuscated_email(match, deform: true) + end + + text.gsub(Regexp.union(matches.keys), matches) + end + class Masker attr_reader :local_part, :sub_domain, :toplevel_domain, :at, :dot diff --git a/spec/graphql/types/notes/note_type_spec.rb b/spec/graphql/types/notes/note_type_spec.rb index 8aabdb78562cfc..5750e73a4b5d42 100644 --- a/spec/graphql/types/notes/note_type_spec.rb +++ b/spec/graphql/types/notes/note_type_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' RSpec.describe GitlabSchema.types['Note'], feature_category: :team_planning do + include GraphqlHelpers + it 'exposes the expected fields' do expected_fields = %i[ author @@ -37,4 +39,36 @@ specify { expect(described_class).to expose_permissions_using(Types::PermissionTypes::Note) } specify { expect(described_class).to require_graphql_authorizations(:read_note) } + + context 'when system note with issue_email_participants action', feature_category: :service_desk do + let_it_be(:user) { build_stubbed(:user) } + let_it_be(:email) { 'user@example.com' } + let_it_be(:note_text) { "added #{email}" } + # Create project and issue separately because we need to public project. + # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Notes::RenderService updates #note and #cached_markdown_version + let_it_be(:project) { create(:project, :public) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:note) do + create(:note, + project: project, noteable: issue, system: true, author: Users::Internal.support_bot, note: note_text) + end + + let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } + # rubocop:enable RSpec/FactoryBot/AvoidCreate + + let(:prepared_note) { Notes::RenderService.new(user).execute([note]).first } + let(:obfuscated_email) { 'us*****@e*****.c**' } + + describe '#body' do + subject { resolve_field(:body, prepared_note, current_user: user) } + + it_behaves_like 'a note content field with obfuscated email address' + end + + describe '#body_html' do + subject { resolve_field(:body_html, prepared_note, current_user: user) } + + it_behaves_like 'a note content field with obfuscated email address' + end + end end diff --git a/spec/lib/gitlab/utils/email_spec.rb b/spec/lib/gitlab/utils/email_spec.rb index c81c2558f706b0..430002eb520fbf 100644 --- a/spec/lib/gitlab/utils/email_spec.rb +++ b/spec/lib/gitlab/utils/email_spec.rb @@ -51,4 +51,20 @@ end end end + + describe '.obfuscate_emails_in_text' do + where(:input, :output) do + nil | nil + '' | '' + 'added no email address' | 'added no email address' + 'added user@example.com' | 'added us*****@e*****.c**' + 'added user@example.com and hello@example.com' | 'added us*****@e*****.c** and he*****@e*****.c**' + 'removed user@example.com, hello@example.com and bye@example.com' | + 'removed us*****@e*****.c**, he*****@e*****.c** and by*****@e*****.c**' + end + + with_them do + it { expect(described_class.obfuscate_emails_in_text(input)).to eq(output) } + end + end end diff --git a/spec/presenters/note_presenter_spec.rb b/spec/presenters/note_presenter_spec.rb new file mode 100644 index 00000000000000..59362b993492ff --- /dev/null +++ b/spec/presenters/note_presenter_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe NotePresenter, feature_category: :team_planning do + let_it_be(:user) { build_stubbed(:user) } + let_it_be(:email) { 'user@example.com' } + let_it_be(:note_text) { "added #{email}" } + # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Notes::RenderService updates #note and #cached_markdown_version + let_it_be_with_reload(:note) { create(:note, system: true, author: Users::Internal.support_bot, note: note_text) } + let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } + # rubocop:enable RSpec/FactoryBot/AvoidCreate + + let(:obfuscated_email) { 'us*****@e*****.c**' } + + subject(:presenter) do + # The RenderService calls Banzai::ObjectRenderer which + # populates `redacted_note_html`. We also do this in + # API controllers. + prepared_notes = Notes::RenderService.new(user).execute([note]) + described_class.new(prepared_notes.first, current_user: user) + end + + describe '#note' do + subject { presenter.note } + + it_behaves_like 'a note content field with obfuscated email address' + end + + describe '#note_html' do + subject { presenter.note_html } + + it_behaves_like 'a note content field with obfuscated email address' + + context 'when redacted_note_html is not present' do + subject(:presenter) do + described_class.new(note, current_user: user).note_html + end + + it 'falls back to note_html and obfuscates emails' do + is_expected.to include(obfuscated_email) + end + end + end +end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 497bbeced38b4a..ae5e00136443c3 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -49,6 +49,38 @@ end end + context 'when system note with issue_email_participants action' do + let!(:email) { 'user@example.com' } + let!(:note_text) { "added #{email}" } + let!(:note) do + create(:note, + project: project, noteable: issue, system: true, author: Users::Internal.support_bot, note: note_text) + end + + let!(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } + let!(:another_user) { create(:user) } + + let(:obfuscated_email) { 'us*****@e*****.c**' } + + it 'returns obfuscated email' do + get api("/projects/#{project.id}/issues/#{issue.iid}/notes", another_user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response.first['body']).to include(obfuscated_email) + end + + context 'when user has at least the reporter role in project' do + it 'returns email' do + get api("/projects/#{project.id}/issues/#{issue.iid}/notes", user) + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_pagination_headers + expect(json_response.first['body']).to include(email) + end + end + end + context "when referencing other project" do # For testing the cross-reference of a private issue in a public project let(:private_project) do diff --git a/spec/serializers/note_entity_spec.rb b/spec/serializers/note_entity_spec.rb index d5c4a31a93785c..56e6040c997781 100644 --- a/spec/serializers/note_entity_spec.rb +++ b/spec/serializers/note_entity_spec.rb @@ -2,14 +2,17 @@ require 'spec_helper' -RSpec.describe NoteEntity do +RSpec.describe NoteEntity, feature_category: :team_planning do include Gitlab::Routing + # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Persisted records required + let_it_be(:note) { create(:note) } + let_it_be(:user) { create(:user) } + # rubocop:enable RSpec/FactoryBot/AvoidCreate + let(:request) { double('request', current_user: user, noteable: note.noteable) } let(:entity) { described_class.new(note, request: request) } - let(:note) { create(:note) } - let(:user) { create(:user) } subject { entity.as_json } @@ -53,9 +56,8 @@ end end - describe 'with email participant' do - let_it_be(:note) { create(:note) } - let_it_be(:note_metadata) { create(:note_metadata, note: note) } + describe 'with email participant', feature_category: :service_desk do + let_it_be(:note_metadata) { create(:note_metadata, note: note) } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Persisted records required subject { entity.as_json[:external_author] } @@ -66,4 +68,36 @@ it_behaves_like 'external author' end end + + context 'when system note with issue_email_participants action', feature_category: :service_desk do + let_it_be(:email) { 'user@example.com' } + let_it_be(:note_text) { "added #{email}" } + # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Notes::RenderService updates #note and #cached_markdown_version + let_it_be(:note) { create(:note, system: true, author: Users::Internal.support_bot, note: note_text) } + let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } + # rubocop:enable RSpec/FactoryBot/AvoidCreate + + let(:obfuscated_email) { 'us*****@e*****.c**' } + let(:expected_text) { obfuscated_email } + + subject(:entity_hash) do + # The RenderService calls Banzai::ObjectRenderer which + # populates `redacted_note_html`. We also do this in + # API controllers. + prepared_notes = Notes::RenderService.new(user).execute([note]) + described_class.new(prepared_notes.first, request: request).as_json + end + + describe 'note' do + subject { entity_hash[:note] } + + it_behaves_like 'a note content field with obfuscated email address' + end + + describe 'note_html' do + subject { entity_hash[:note_html] } + + it_behaves_like 'a note content field with obfuscated email address' + end + end end diff --git a/spec/services/issue_email_participants/create_service_spec.rb b/spec/services/issue_email_participants/create_service_spec.rb index dc8d5a6ea74ec4..05c9162dde1487 100644 --- a/spec/services/issue_email_participants/create_service_spec.rb +++ b/spec/services/issue_email_participants/create_service_spec.rb @@ -12,6 +12,7 @@ note = issue.notes.last expect(note.system?).to be true expect(note.author).to eq(user) + expect(note.system_note_metadata.action).to eq('issue_email_participants') participants_emails = issue.email_participants_emails_downcase diff --git a/spec/support/shared_examples/graphql/notes_content_obfuscation_shared_examples.rb b/spec/support/shared_examples/graphql/notes_content_obfuscation_shared_examples.rb new file mode 100644 index 00000000000000..0f8f24a034f2e8 --- /dev/null +++ b/spec/support/shared_examples/graphql/notes_content_obfuscation_shared_examples.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'a note content field with obfuscated email address' do + context 'when anonymous' do + let(:user) { nil } + + it { is_expected.to include(obfuscated_email) } + end + + context 'with signed in user' do + before do + stub_member_access_level(note.project, access_level => user) if access_level + end + + context 'when user has no role in project' do + let(:access_level) { nil } + + it { is_expected.to include(obfuscated_email) } + end + + context 'when user has guest role in project' do + let(:access_level) { :guest } + + it { is_expected.to include(obfuscated_email) } + end + + context 'when user has reporter role in project' do + let(:access_level) { :reporter } + + it { is_expected.to include(email) } + end + + context 'when user has developer role in project' do + let(:access_level) { :developer } + + it { is_expected.to include(email) } + end + end +end -- GitLab From 2eb5005558a9c348b167874dcf51e3f4c74a3fe4 Mon Sep 17 00:00:00 2001 From: Marc Saleiko Date: Tue, 9 Apr 2024 12:57:33 +0200 Subject: [PATCH 2/2] Apply reviewer suggestions Unify email/custom email regexps and add request spec for graphql --- app/models/service_desk_setting.rb | 2 +- lib/gitlab/email/service_desk/custom_email.rb | 3 +- lib/gitlab/utils/email.rb | 9 +++- spec/graphql/types/notes/note_type_spec.rb | 3 +- spec/presenters/note_presenter_spec.rb | 2 +- spec/requests/api/graphql/notes/note_spec.rb | 41 ++++++++++++++++++- spec/requests/api/notes_spec.rb | 3 +- spec/serializers/note_entity_spec.rb | 2 +- 8 files changed, 53 insertions(+), 12 deletions(-) diff --git a/app/models/service_desk_setting.rb b/app/models/service_desk_setting.rb index 095eb0b67f38a2..eba39cd5f2b7ab 100644 --- a/app/models/service_desk_setting.rb +++ b/app/models/service_desk_setting.rb @@ -23,7 +23,7 @@ class ServiceDeskSetting < ApplicationRecord length: { maximum: 255 }, uniqueness: true, allow_nil: true, - format: /\A[\w\-._]+@[\w\-.]+\.{1}[a-zA-Z]{2,}\z/ + format: Gitlab::Utils::Email::EMAIL_REGEXP_WITH_ANCHORS validates :custom_email_credential, presence: true, diff --git a/lib/gitlab/email/service_desk/custom_email.rb b/lib/gitlab/email/service_desk/custom_email.rb index 1828f71984b8df..2d2ca20aa115e8 100644 --- a/lib/gitlab/email/service_desk/custom_email.rb +++ b/lib/gitlab/email/service_desk/custom_email.rb @@ -8,7 +8,6 @@ module ServiceDesk # incoming_email and service_desk_email. module CustomEmail REPLY_ADDRESS_KEY_REGEXP = /\+([0-9a-f]{32})@/ - EMAIL_REGEXP = /\A[\w\-._]+@[\w\-.]+\.{1}[a-zA-Z]{2,}\z/ class << self def reply_address(issue, reply_key) @@ -40,7 +39,7 @@ def key_from_reply_address(email) def find_service_desk_setting_from_reply_address(email, key) potential_custom_email = email.sub("+#{key}", '') - return unless EMAIL_REGEXP.match?(potential_custom_email) + return unless Gitlab::Utils::Email::EMAIL_REGEXP_WITH_ANCHORS.match?(potential_custom_email) ServiceDeskSetting.find_by_custom_email(potential_custom_email) end diff --git a/lib/gitlab/utils/email.rb b/lib/gitlab/utils/email.rb index dfaea1511987ef..6decb3da8f87f8 100644 --- a/lib/gitlab/utils/email.rb +++ b/lib/gitlab/utils/email.rb @@ -5,7 +5,12 @@ module Utils module Email extend self - EMAIL_ADDRESS_REGEXP = /([\w\-._]+@[\w\-.]+\.{1}[a-zA-Z]{2,})/ + # Don't use Devise.email_regexp or URI::MailTo::EMAIL_REGEXP to be a bit more restrictive + # on the format of an email. Especially for custom email addresses which cannot contain a `+` + # in app/models/service_desk_setting.rb + EMAIL_REGEXP = /[\w\-._]+@[\w\-.]+\.{1}[a-zA-Z]{2,}/ + EMAIL_REGEXP_WITH_CAPTURING_GROUP = /(#{EMAIL_REGEXP})/ + EMAIL_REGEXP_WITH_ANCHORS = /\A#{EMAIL_REGEXP.source}\z/ # Replaces most visible characters with * to obfuscate an email address # deform adds a fix number of * to ensure the address cannot be guessed. Also obfuscates TLD with ** @@ -20,7 +25,7 @@ def obfuscated_email(email, deform: false) def obfuscate_emails_in_text(text) return text unless text.present? - matches = text.scan(EMAIL_ADDRESS_REGEXP).flatten.index_with do |match| + matches = text.scan(EMAIL_REGEXP_WITH_CAPTURING_GROUP).flatten.index_with do |match| obfuscated_email(match, deform: true) end diff --git a/spec/graphql/types/notes/note_type_spec.rb b/spec/graphql/types/notes/note_type_spec.rb index 5750e73a4b5d42..02f311785db2bf 100644 --- a/spec/graphql/types/notes/note_type_spec.rb +++ b/spec/graphql/types/notes/note_type_spec.rb @@ -49,8 +49,7 @@ let_it_be(:project) { create(:project, :public) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:note) do - create(:note, - project: project, noteable: issue, system: true, author: Users::Internal.support_bot, note: note_text) + create(:note, :system, project: project, noteable: issue, author: Users::Internal.support_bot, note: note_text) end let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } diff --git a/spec/presenters/note_presenter_spec.rb b/spec/presenters/note_presenter_spec.rb index 59362b993492ff..2a8ae38f08274c 100644 --- a/spec/presenters/note_presenter_spec.rb +++ b/spec/presenters/note_presenter_spec.rb @@ -7,7 +7,7 @@ let_it_be(:email) { 'user@example.com' } let_it_be(:note_text) { "added #{email}" } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Notes::RenderService updates #note and #cached_markdown_version - let_it_be_with_reload(:note) { create(:note, system: true, author: Users::Internal.support_bot, note: note_text) } + let_it_be_with_reload(:note) { create(:note, :system, author: Users::Internal.support_bot, note: note_text) } let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } # rubocop:enable RSpec/FactoryBot/AvoidCreate diff --git a/spec/requests/api/graphql/notes/note_spec.rb b/spec/requests/api/graphql/notes/note_spec.rb index daceaec0b94cd6..2a2ea8ce49b1db 100644 --- a/spec/requests/api/graphql/notes/note_spec.rb +++ b/spec/requests/api/graphql/notes/note_spec.rb @@ -6,6 +6,7 @@ include GraphqlHelpers let_it_be(:current_user) { create(:user) } + let_it_be(:reporter_user) { create(:user) } let_it_be(:project) { create(:project, :private) } let_it_be(:issue) { create(:issue, project: project) } let_it_be(:note) { create(:note, noteable: issue, project: project) } @@ -19,6 +20,10 @@ graphql_query_for('note', note_params, note_fields) end + before_all do + project.add_reporter(reporter_user) + end + it_behaves_like 'a working graphql query' do before do post_graphql(query, current_user: current_user) @@ -44,7 +49,7 @@ end context 'when the user has access to read the note' do - before do + before_all do project.add_guest(current_user) end @@ -62,6 +67,40 @@ expect(note_data['id']).to eq(global_id_of(system_note).to_s) end + + context 'with issue_email_participants action' do + let_it_be(:email) { 'user@example.com' } + let_it_be(:note_text) { "added #{email}" } + let_it_be(:issue_email_participants_system_note) do + create(:note, :system, + project: project, noteable: issue, author: Users::Internal.support_bot, note: note_text) + end + + let_it_be(:system_note_metadata) do + create(:system_note_metadata, note: issue_email_participants_system_note, action: :issue_email_participants) + end + + let(:obfuscated_email) { 'us*****@e*****.c**' } + let(:note_params) { { 'id' => global_id_of(issue_email_participants_system_note) } } + + it 'returns obfuscated email' do + post_graphql(query, current_user: current_user) + + expect(note_data['id']).to eq(global_id_of(issue_email_participants_system_note).to_s) + expect(note_data['body']).to include(obfuscated_email) + expect(note_data['bodyHtml']).to include(obfuscated_email) + end + + context 'when user has at least the reporter role in project' do + it 'returns email' do + post_graphql(query, current_user: reporter_user) + + expect(note_data['id']).to eq(global_id_of(issue_email_participants_system_note).to_s) + expect(note_data['body']).to include(email) + expect(note_data['bodyHtml']).to include(email) + end + end + end end context 'and notes widget is not available' do diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index ae5e00136443c3..e7ff7baaa6ce09 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -53,8 +53,7 @@ let!(:email) { 'user@example.com' } let!(:note_text) { "added #{email}" } let!(:note) do - create(:note, - project: project, noteable: issue, system: true, author: Users::Internal.support_bot, note: note_text) + create(:note, :system, project: project, noteable: issue, author: Users::Internal.support_bot, note: note_text) end let!(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } diff --git a/spec/serializers/note_entity_spec.rb b/spec/serializers/note_entity_spec.rb index 56e6040c997781..51335c5f73204b 100644 --- a/spec/serializers/note_entity_spec.rb +++ b/spec/serializers/note_entity_spec.rb @@ -73,7 +73,7 @@ let_it_be(:email) { 'user@example.com' } let_it_be(:note_text) { "added #{email}" } # rubocop:disable RSpec/FactoryBot/AvoidCreate -- Notes::RenderService updates #note and #cached_markdown_version - let_it_be(:note) { create(:note, system: true, author: Users::Internal.support_bot, note: note_text) } + let_it_be(:note) { create(:note, :system, author: Users::Internal.support_bot, note: note_text) } let_it_be(:system_note_metadata) { create(:system_note_metadata, note: note, action: :issue_email_participants) } # rubocop:enable RSpec/FactoryBot/AvoidCreate -- GitLab