feat: Implement API to GDPR delete users

[Janis4411]

This PR introduces a possible way to GDPR delete users via a API.

If we delete a user on openHPI we also have to delete the user on codeocean. Previously this was handled via a rake task that has to be triggered manually.

Part of SODEV-2997

[codecov]

Codecov Report

❌ Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.39%. Comparing base (7876111) to head (1d4b374).

Files with missing lines	Patch %	Lines
lib/tasks/gdpr_delete.rake	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3224      +/-   ##
==========================================
+ Coverage   70.33%   70.39%   +0.06%     
==========================================
  Files         214      216       +2     
  Lines        6839     6857      +18     
==========================================
+ Hits         4810     4827      +17     
- Misses       2029     2030       +1     

☔ 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.

[arkirchner]

I think it is better to blow up if the update did not work. soft_delete does not need a !. The ! indicates a volatile version of a method. Since we do not implement two versions of this method, we should not use !.

Suggested change

	def soft_delete!
	update(name: 'Deleted User', email: nil)
	def soft_delete
	update!(name: 'Deleted User', email: nil)

[arkirchner]

Maybe we can add a new column deleted_at and add a timestamp when the user was deleted. This is good for debugging.

[Janis4411]

Is the updated_at not serving the pretty much same purpose? Once a ExternalUser is updated with (name: 'Deleted User', email: nil) it won't be updated again, right?

[arkirchner]

It might be. The username Deleted User could be used by a user. No email could be a side effect from some bug. The deleted_at just makes it clearer and is a common practice for soft deletions.

[Janis4411]

I added the deleted_at column and update it now in the soft_delete method.

[arkirchner]

We can use some more Rails helper methods here.

Suggested change

	before_action :verify_token!
	def verify_token!
	token = request.headers['Authorization']&.remove('Bearer ')
	unless ActiveSupport::SecurityUtils.secure_compare(
	token.to_s,
	ENV.fetch('OPENHPI_API_TOKEN', nil)
	)
	head :unauthorized
	end
	before_action :authenticate
	def authenticate
	authenticate_or_request_with_http_token do |token, _options|
	ActiveSupport::SecurityUtils.secure_compare(
	token,
	ENV.fetch('OPENHPI_API_TOKEN', nil)
	)
	end
	end

[Janis4411]

I get this error:

undefined method 'authenticate_or_request_with_http_token' for #Api::Internal::UsersController:0x000000013e17c458

It should also be available for ActionController::API, I'll have another look.

[Janis4411]

I had to include the ActionController::HttpAuthentication::Token::ControllerMethods as they are set in the ActionController::API by default.

[arkirchner]

Can you add one more test where OPENHPI_API_TOKEN is not present? I want to see if no secret configuration is set and the API is still unreachable.

[arkirchner]

Please keep the tests simple. Your test setup is quite complex with all these let statements. One-off record creation should be done in the test directly and not assigned as let. This makes test easier to understand.

Suggested change

	let(:user_id) { '123456' }
	let(:token) { 'supersecrettoken' }
	let(:consumer) { create(:consumer, id: 1, name: 'openHPI') }
	let!(:user) { create(:external_user, external_id: user_id, consumer_id: consumer.id, name: 'Test User', email: 'testmail@gmail.com') }
	let(:body) do
	{user_id: user_id}.to_json
	end
	let(:headers) do
	{
	'Authorization' => "Bearer #{token}",
	'Content-Type' => 'application/json',
	}
	end
	describe 'DELETE /api/internal/users' do
	context 'with valid token and signature' do
	it 'anonymizes the user name to "Deleted User"' do
	expect { delete "/api/internal/users/#{user_id}", headers: headers }
	.to change { user.reload.name }.to('Deleted User').and change { user.reload.email }.to(nil)
	end
	it 'returns 200 OK' do
	delete "/api/internal/users/#{user_id}",
	headers: headers
	expect(response).to have_http_status(:ok)
	end
	end
	end
	let(:headers) { { 'Authorization' => "Bearer #{token}" } }
	describe 'DELETE /api/internal/users' do
	it 'soft deletes a user' do
	user = create(:external_user, external_id: '123456')
	freeze_time
	expect { delete "/api/internal/users/123456", headers: headers }
	.to change { user.reload.deleted_at }.to(Time.current)
	end
	it 'returns an error if the user is not found' do
	delete "/api/internal/users/123456",
	headers: headers
	expect(response).to have_http_status(:not_found)
	end
	end
	end

[arkirchner]

[nits] A context with only one example is not really worth it. I think a single it assertion here is clearer and more readable.

Suggested change

	context 'with valid token' do
	it 'allows the request' do
	it 'allows the request with valid token' do

[arkirchner]

We have a request test for this API. We don't need a controller test as well.