-
Notifications
You must be signed in to change notification settings - Fork 29
feat: Implement API to GDPR delete users #3224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| INTERNAL_API_TOKEN=supersecrettoken |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Api | ||
| class ApiController < ActionController::API | ||
| include ActionController::HttpAuthentication::Token::ControllerMethods | ||
|
|
||
| before_action :authenticate! | ||
|
|
||
| def authenticate! | ||
| authenticate_or_request_with_http_token do |token, _options| | ||
| ActiveSupport::SecurityUtils.secure_compare( | ||
| token, | ||
| ENV.fetch('INTERNAL_API_TOKEN', nil) | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Api | ||
| module Internal | ||
| class UsersController < Api::ApiController | ||
| def destroy | ||
| user = ExternalUser.where(external_id: params[:id]).first | ||
|
|
||
| return head :not_found unless user | ||
|
|
||
| user.soft_delete | ||
| head :ok | ||
| end | ||
| end | ||
| end | ||
| end | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| class AddDeletedAtToExternalUsers < ActiveRecord::Migration[8.0] | ||
| def change | ||
| add_column :external_users, :deleted_at, :datetime, null: true, default: nil | ||
| end | ||
| end |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | ||||||||
| # frozen_string_literal: true | ||||||||
|
|
||||||||
| require 'rails_helper' | ||||||||
|
|
||||||||
| RSpec.describe Api::ApiController do | ||||||||
| controller(described_class) do | ||||||||
| def index | ||||||||
| head :ok | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| before do | ||||||||
| routes.draw do | ||||||||
| get 'index' => 'api/api#index' | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| describe 'authentication' do | ||||||||
| context 'with valid token' do | ||||||||
| it 'allows the request' do | ||||||||
|
Comment on lines
+19
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this context follows the same pattern as the other contexts. E.g. context 'with valid token' do, context 'with invalid token' do, context 'without token' do |
||||||||
| request.headers['Authorization'] = 'Bearer supersecrettoken' | ||||||||
|
|
||||||||
| get :index | ||||||||
|
|
||||||||
| expect(response).to have_http_status(:ok) | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| context 'with invalid token' do | ||||||||
| it 'returns 401 Unauthorized' do | ||||||||
| request.headers['Authorization'] = 'Bearer invalid_token' | ||||||||
|
|
||||||||
| get :index | ||||||||
|
|
||||||||
| expect(response).to have_http_status(:unauthorized) | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| context 'without token' do | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
| it 'returns 401 Unauthorized' do | ||||||||
| get :index | ||||||||
|
|
||||||||
| expect(response).to have_http_status(:unauthorized) | ||||||||
| end | ||||||||
| end | ||||||||
|
|
||||||||
| context 'without the INTERNAL_API_TOKEN being set' do | ||||||||
| it 'returns 401 Unauthorized' do | ||||||||
| allow(ENV) | ||||||||
| .to receive(:fetch) | ||||||||
| .with('INTERNAL_API_TOKEN', nil) | ||||||||
| .and_return(nil) | ||||||||
|
|
||||||||
| request.headers['Authorization'] = "Bearer #{ENV.fetch('INTERNAL_API_TOKEN', nil)}" | ||||||||
| get :index | ||||||||
|
|
||||||||
| expect(response).to have_http_status(:unauthorized) | ||||||||
| end | ||||||||
| end | ||||||||
| end | ||||||||
| end | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| # frozen_string_literal: true | ||
|
arkirchner marked this conversation as resolved.
|
||
|
|
||
| include ActiveSupport::Testing::TimeHelpers | ||
|
Check warning on line 3 in spec/request/api/internal/users/destroy_spec.rb
|
||
|
|
||
| require 'rails_helper' | ||
|
|
||
| RSpec.describe 'DELETE Users API', type: :request do | ||
| let(:headers) { {'Authorization' => 'Bearer supersecrettoken'} } | ||
|
|
||
| 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.zone.now) | ||
| 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 | ||
Uh oh!
There was an error while loading. Please reload this page.