Allow reporting of malicious content #2946

arkirchner · 2025-06-04T15:58:00Z

RfCs and Comments on RfCs are user generated content that can be reviewed by other users. This feature can be misused. A simple email based reporting mechanism has been added allow users to report this malicious content.

The UI for the RfC comment are part of a separate change.

Relates to #2715

RfCs and Comments on RfCs are user generated content that can be reviewed by other users. This feature can be misused. A simple email based reporting mechanism has been added allow users to report this malicious content. The UI for the RfC comment are part of a separate change. Relates to #2715

arkirchner · 2025-06-04T16:06:28Z

docs/LOCAL_SETUP.md

@@ -158,6 +158,9 @@ do
    cp config/$f.example config/$f
  fi
 done
+
+sed -i.bak 's/^# - mail@example.com/- mail@example.com/' config/code_ocean.yml


I am not sure about this. I want to keep the email out of the default configuration to avoid setting this up accidentally,

I added this, added the email to the local DEV environment.

Creating the backup and then deleting is necessary because of the differences between Mac and Linux sed. Not thrilled with this solution. 🤷

I agree. Do we need this action at all? What is bad about having the comment in the config?

I am just really unconfutable that will make the tests fail. Debugging this will be a bad onboarding experience.

For now, I take it out. 😟

I am happy to take a look when you point me to the failing tests 👍

The mailer tests and the report tests are failing because the email is not configured by default. What I mean is with the guide for the local setup, the environment is not ready to pass all tests. You need to add the email first.

What I mean is with the guide for the local setup, the environment is not ready to pass all tests.

That shouldn't be the case, obviously. Can you specifiy whether following the local setup guide on main already causes test failures? Or is this statement only true once this PR has been merged?

In the latter case, we should consider injecting config values dynamically, as we've done on other occasions, too.

codecov · 2025-06-04T16:12:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.76%. Comparing base (df3ae19) to head (cf59d51).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
+ Coverage   69.60%   69.76%   +0.16%     
==========================================
  Files         214      215       +1     
  Lines        6820     6834      +14     
==========================================
+ Hits         4747     4768      +21     
+ Misses       2073     2066       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

MrSerth

Thank you for working on this PR and the feature to report inappropriate content! I am sure it will come handy for the next courses 😁

Please find some comments below; many are focussing on some style we used previously. Of course, they are up for discussion, but I wanted to point them out and make decisions explicit.
Also, I would strongly suggest to get a second review, e. g., from @kkoehn or @nenock.

One comment regarding your title: I am not much in favor of using the term "malicious content". In my terminology, "malicious" describes malware or other cybersecurity threats. Instead, I would refer to this content as being inappropriate, offensive, or explicit.

Many thanks also for writing and adding the specs; I really liked them and think you aligned well with the existing structure in this repo.

MrSerth · 2025-06-04T22:20:28Z

app/controllers/reports_controller.rb

+# frozen_string_literal: true
+
+class ReportsController < ApplicationController
+  def create


We touched on this topic previously: I would have chosen to add a new action to comments / RfCs, since someone is reporting an existing content. Of course, you can also see that as a new Report being generated; it's just not the style we used so far. Then, you wouldn't need to specify the policy below, since it would be automatically matched.

While I do not know the discussions you already had, I consider a report action to comments and RfCs more fitting to the context.

Is there some documentation about this? I know sometimes custom actions are used as a quick one off. Is there any architectural design behind this idea?

I feel a bit lost understanding when to create a controller and when to use a custom action with this approach?

For now, I have added a new report action in the controller.

I don't have any documentation at hand for that. Instead, I would say its some gut feeling for me.

MrSerth · 2025-06-04T22:21:37Z

app/controllers/reports_controller.rb

+
+class ReportsController < ApplicationController
+  def create
+    authorize(reported_content, policy_class: ReportPolicy)


It is discussable, but we previously used a before action to set the main resource and a custom authorize! action.

Interesting! I saw this and was confused.

What are the reasons behind this idea?

Part of the argument is consistency: We just have it in the other controllers.

What I like about the idea is that the actions get "smaller" with fewer repetitions, since they can focus on the actual work being required. You don't need any additional checks and have the most common "errors" extracted (mainly authorization missing, but also other things like param checks, etc.)

At the same time, this is also a disadvantage: It's less visible what each action performs, and some actions have a list of before actions that I think is too long.

app/controllers/reports_controller.rb

config/locales/en/report.yml

config/routes.rb

MrSerth · 2025-06-04T23:15:07Z

docs/LOCAL_SETUP.md

@@ -158,6 +158,9 @@ do
    cp config/$f.example config/$f
  fi
 done
+
+sed -i.bak 's/^# - mail@example.com/- mail@example.com/' config/code_ocean.yml


I agree. Do we need this action at all? What is bad about having the comment in the config?

MrSerth · 2025-06-04T23:24:02Z

spec/factories/comment.rb

+FactoryBot.define do
+  factory :comment do
+    file { create(:rfc).file }
+    user factory: :learner


So far, we have given associations explicitly, not implicitly through method_missing (despite being supported by FactoryBot).

I am sorry. I do not understand what you mean.

Sebastians refers to FactoryBots method_missing feature. However, afaik there is a RuboCop rule to prefer implicit definitions over explicit ones and as far as I can tell, this is active and applied in CodeOcean 🤷‍♀️

Indeed -- thanks, Nele! And obviously, I was making a mistake here: The examples I checked (and the recent factories I touched) required explicit ones, leading to this wrong statement. Sorry!

spec/system/request_for_comments_filter_system_spec.rb

Co-authored-by: Sebastian Serth <MrSerth@users.noreply.github.com>

arkirchner · 2025-06-05T15:33:18Z

app/views/report_mailer/report_content.text.erb

+
+    <%= t('.take_action') %>
+
+    <%= rails_admin.show_url(


There is no link I can use for comments. I left the RailsAdmin link. I will reconsider this in the second half when I add UI support for comments.

Yeah, there is none. However, access to Rails Admin is (right now) strictly limited to platform admins, and some views are broken there (those using a duration as a column). Hence, I either thought of linking to the respective RfC (i.e., opening the modal?) or a simple view (such as for submission). Neither is ideal, but we can also postpone it for now and reconsider it later.

nenock

First of all, thanks for taking care of this feature!
Please feel free to raise any questions about the comments I made.

nenock · 2025-06-11T09:35:05Z

app/views/report_mailer/report_content.text.erb

https://slim-template.github.io/

nenock · 2025-06-11T09:43:11Z

app/controllers/reports_controller.rb

+# frozen_string_literal: true
+
+class ReportsController < ApplicationController
+  def create


While I do not know the discussions you already had, I consider a report action to comments and RfCs more fitting to the context.

app/controllers/reports_controller.rb

nenock · 2025-06-11T11:18:26Z

spec/system/request_for_comments_filter_system_spec.rb

+    expect do
+      accept_confirm do
+        click_on I18n.t('reports.report')
+      end
+
+      expect(page).to have_text(I18n.t('reports.reported'))
+    end.to have_enqueued_mail(ReportMailer, :report_content)
+      .with(params: {reported_content: request_for_comment}, args: [])


I know, we haven't switched from controller tests to request tests, yet, but IMO, this would be good moment to start. When having a request spec file for report#create or for comment#report and RfC#report, the enqueued mail could be expected there, which would make this system spec more clean:

Suggested change

expect do

accept_confirm do

click_on I18n.t('reports.report')

end

expect(page).to have_text(I18n.t('reports.reported'))

end.to have_enqueued_mail(ReportMailer, :report_content)

.with(params: {reported_content: request_for_comment}, args: [])

click_on I18n.t('reports.report')

expect(page).to have_text(I18n.t('reports.reported'))

I created a request test and moved the email test there.

spec/policies/report_policy_spec.rb

nenock · 2025-06-11T11:22:02Z

spec/policies/report_policy_spec.rb

+    it 'dose not allow reports when no report email is configured' do
+      user = build_stubbed(:external_user)
+
+      codeocean_config = instance_double(CodeOcean::Config)
+      allow(CodeOcean::Config).to receive(:new).with(:code_ocean).and_return(codeocean_config)
+      allow(codeocean_config).to receive(:read).and_return({})
+
+      expect(policy).not_to permit(user, Comment.new)
+    end


This should be in a context without a report email configured (same below)

spec/mailers/report_mailer_spec.rb

nenock · 2025-06-11T11:25:06Z

spec/mailers/report_mailer_spec.rb

+    it 'sets the correct sender' do
+      expect(mail.from).to include('codeocean@openhpi.de')
+    end
+
+    it 'sets the correct subject' do
+      expect(mail.subject).to eq(I18n.t('report_mailer.report_content.subject', content_name: Comment.model_name.human))
+    end
+
+    it 'sets the correct receiver' do
+      expect(mail.to).to include('report@example.com')
+    end
+
+    it 'includes the reported content' do
+      expect(mail.body).to include('Inappropriate content for Comment.')
+    end


This should be in a context block as well (for comment)

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

Bumps [@sentry/browser](https://github.com/getsentry/sentry-javascript) from 9.25.1 to 9.26.0. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@9.25.1...9.26.0) --- updated-dependencies: - dependency-name: "@sentry/browser" dependency-version: 9.26.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps the bundler group with 1 update: [rack](https://github.com/rack/rack). Updates `rack` from 3.1.15 to 3.1.16 - [Release notes](https://github.com/rack/rack/releases) - [Changelog](https://github.com/rack/rack/blob/main/CHANGELOG.md) - [Commits](rack/rack@v3.1.15...v3.1.16) --- updated-dependencies: - dependency-name: rack dependency-version: 3.1.16 dependency-type: indirect dependency-group: bundler ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [@sentry/browser](https://github.com/getsentry/sentry-javascript) from 9.26.0 to 9.27.0. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@9.26.0...9.27.0) --- updated-dependencies: - dependency-name: "@sentry/browser" dependency-version: 9.27.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [@babel/runtime](https://github.com/babel/babel/tree/HEAD/packages/babel-runtime) from 7.27.4 to 7.27.6. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.27.6/packages/babel-runtime) --- updated-dependencies: - dependency-name: "@babel/runtime" dependency-version: 7.27.6 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [webpack-assets-manifest](https://github.com/webdeveric/webpack-assets-manifest) from 6.1.0 to 6.2.1. - [Release notes](https://github.com/webdeveric/webpack-assets-manifest/releases) - [Changelog](https://github.com/webdeveric/webpack-assets-manifest/blob/master/release.config.mjs) - [Commits](webdeveric/webpack-assets-manifest@v6.1.0...v6.2.1) --- updated-dependencies: - dependency-name: webpack-assets-manifest dependency-version: 6.2.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [sass](https://github.com/sass/dart-sass) from 1.89.1 to 1.89.2. - [Release notes](https://github.com/sass/dart-sass/releases) - [Changelog](https://github.com/sass/dart-sass/blob/main/CHANGELOG.md) - [Commits](sass/dart-sass@1.89.1...1.89.2) --- updated-dependencies: - dependency-name: sass dependency-version: 1.89.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [@sentry/browser](https://github.com/getsentry/sentry-javascript) from 9.27.0 to 9.28.0. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@9.27.0...9.28.0) --- updated-dependencies: - dependency-name: "@sentry/browser" dependency-version: 9.28.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps the npm_and_yarn group with 1 update: [brace-expansion](https://github.com/juliangruber/brace-expansion). Updates `brace-expansion` from 2.0.1 to 2.0.2 - [Release notes](https://github.com/juliangruber/brace-expansion/releases) - [Commits](juliangruber/brace-expansion@v2.0.1...v2.0.2) --- updated-dependencies: - dependency-name: brace-expansion dependency-version: 2.0.2 dependency-type: indirect dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [@sentry/browser](https://github.com/getsentry/sentry-javascript) from 9.28.0 to 9.28.1. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@9.28.0...9.28.1) --- updated-dependencies: - dependency-name: "@sentry/browser" dependency-version: 9.28.1 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

app/controllers/test_sessions_controller.rb

MrSerth · 2025-06-12T18:43:48Z

Thanks for working on all the comments and improving the code!

Would you please mind improving the git history a little, please? I see some unrelated Dependabot commits in your history that were touched (compared to main). Also, I would appreciate clearer commit messages to various review comments 🙂

app/controllers/application_controller.rb

app/controllers/request_for_comments_controller.rb

app/controllers/test_sessions_controller.rb

MrSerth · 2025-06-12T18:49:22Z

config/locales/de/report.yml

+  report_mailer:
+    report_content:
+      prolog: 'Die folgenden Inhalte wurden als unangemessen gemeldet:'
+      subject: 'Spam Report: Ein %{content_name} in CodeOcean wurde als unangemessen markiert.'


We haven't done it consistently yet, but some translations bear a placeholder %{application_name} for CodeOcean. Nothing to change here, just for your information.

Currently, this is a bit difficult because this is a view helper and the subject is not generated in a view. Thank you for allowing me to ignore this once.

Including the helper would work, wouldn't it?

class ApplicationMailer < ActionMailer::Base include ApplicationHelper layout 'mailer' end

However, I would not continue bothering here, it is not consistent right now anyhow.

Co-authored-by: Sebastian Serth <MrSerth@users.noreply.github.com>

arkirchner self-assigned this Jun 4, 2025

arkirchner force-pushed the ak/2715-report-abuse branch from f17d4bb to 6e416f5 Compare June 4, 2025 16:00

arkirchner commented Jun 4, 2025

View reviewed changes

arkirchner marked this pull request as ready for review June 4, 2025 16:06

MrSerth reviewed Jun 5, 2025

View reviewed changes

arkirchner force-pushed the ak/2715-report-abuse branch from 19e3949 to f32a3e7 Compare June 5, 2025 14:46

Review corrections

a46a6d4

arkirchner force-pushed the ak/2715-report-abuse branch from f32a3e7 to a46a6d4 Compare June 5, 2025 14:47

arkirchner and others added 5 commits June 5, 2025 16:51

authorize!

3c548fd

rubocop

9ea4ac2

Noramlaize locals and fix button policy call

b2afed9

use config for condition

72cf79f

Update config/locales/de/report.yml

b2afece

Co-authored-by: Sebastian Serth <MrSerth@users.noreply.github.com>

arkirchner commented Jun 5, 2025

View reviewed changes

fix_view_test

52314ca

nenock requested changes Jun 11, 2025

View reviewed changes

arkirchner and others added 12 commits June 11, 2025 16:39

Update spec/policies/report_policy_spec.rb

61267b7

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

Update config/locales/en/report.yml

8c3ed6c

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

Update spec/system/request_for_comments_filter_system_spec.rb

00be745

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

arkirchner and others added 9 commits June 12, 2025 15:11

Explicit associations in factories

4b223a2

better method names

968f36d

translate email subject

d44c3bc

various review comments

7897f3c

Move report render condition into partial.

e78a1eb

Move view test to new partial

716be1b

remove js changes

782763c

Update spec/mailers/report_mailer_spec.rb

35e4901

Co-authored-by: Nele Sina Noack <nele.noack@hpi.de>

request test setup

5b691f8

github-advanced-security bot found potential problems Jun 12, 2025

View reviewed changes

app/controllers/test_sessions_controller.rb Fixed Show fixed Hide fixed

Test session brakeman fix

7270e48

MrSerth reviewed Jun 12, 2025

View reviewed changes

arkirchner and others added 4 commits June 13, 2025 07:44

remove_test_code

4455686

Update app/controllers/request_for_comments_controller.rb

852d44d

Co-authored-by: Sebastian Serth <MrSerth@users.noreply.github.com>

added context block

ddf4498

test with email and password signup

cf59d51

Allow reporting of malicious content #2946

Are you sure you want to change the base?

Allow reporting of malicious content #2946

Conversation

arkirchner commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MrSerth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkirchner Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nenock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2025 •

edited

Loading

arkirchner Jun 5, 2025 •

edited

Loading