From 887a79f130f7994cd2b5633218ca8eadddf55af7 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 26 Sep 2021 17:18:12 +0100 Subject: [PATCH] Reviewed adding IP recording to activity & audit log Review of #2936 - Added testing to cover - Added APP_PROXIES to .env.example.complete with details. - Renamed migration to better align the name and to set the migration date to fit with production deploy order. - Removed index from IP column in migration since an index does not yet provide any value. - Updated table header text label. - Prevented IP recording when in demo mode. --- .env.example.complete | 8 +++ app/Actions/ActivityService.php | 3 +- ...09_26_044614_add_activities_ip_column.php} | 5 +- resources/lang/en/settings.php | 2 +- resources/views/settings/audit.blade.php | 2 +- tests/AuditLogTest.php | 49 +++++++++++++++++++ 6 files changed, 62 insertions(+), 7 deletions(-) rename database/migrations/{2021_08_21_044614_add_column_ip_into_activities.php => 2021_09_26_044614_add_activities_ip_column.php} (80%) diff --git a/.env.example.complete b/.env.example.complete index 49d834ff7..5eb65c27f 100644 --- a/.env.example.complete +++ b/.env.example.complete @@ -42,6 +42,14 @@ APP_TIMEZONE=UTC # overrides can be made. Defaults to disabled. APP_THEME=false +# Trusted Proxies +# Used to indicate trust of systems that proxy to the application so +# certain header values (Such as "X-Forwarded-For") can be used from the +# incoming proxy request to provide origin detail. +# Set to an IP address, or multiple comma seperated IP addresses. +# Can alternatively be set to "*" to trust all proxy addresses. +APP_PROXIES=null + # Database details # Host can contain a port (localhost:3306) or a separate DB_PORT option can be used. DB_HOST=localhost diff --git a/app/Actions/ActivityService.php b/app/Actions/ActivityService.php index b2a4a132f..f8a0825bb 100644 --- a/app/Actions/ActivityService.php +++ b/app/Actions/ActivityService.php @@ -56,10 +56,11 @@ class ActivityService */ protected function newActivityForUser(string $type): Activity { + $ip = request()->ip() ?? ''; return $this->activity->newInstance()->forceFill([ 'type' => strtolower($type), 'user_id' => user()->id, - 'ip' => Request::ip(), + 'ip' => config('app.env') === 'demo' ? '127.0.0.1' : $ip, ]); } diff --git a/database/migrations/2021_08_21_044614_add_column_ip_into_activities.php b/database/migrations/2021_09_26_044614_add_activities_ip_column.php similarity index 80% rename from database/migrations/2021_08_21_044614_add_column_ip_into_activities.php rename to database/migrations/2021_09_26_044614_add_activities_ip_column.php index 97cc2c039..68391b1c2 100644 --- a/database/migrations/2021_08_21_044614_add_column_ip_into_activities.php +++ b/database/migrations/2021_09_26_044614_add_activities_ip_column.php @@ -4,7 +4,7 @@ use Illuminate\Database\Migrations\Migration; use Illuminate\Database\Schema\Blueprint; use Illuminate\Support\Facades\Schema; -class AddColumnIpIntoActivities extends Migration +class AddActivitiesIpColumn extends Migration { /** * Run the migrations. @@ -15,8 +15,6 @@ class AddColumnIpIntoActivities extends Migration { Schema::table('activities', function (Blueprint $table) { $table->string('ip', 45)->after('user_id'); - - $table->index(['ip'], 'user_ip_idx'); }); } @@ -28,7 +26,6 @@ class AddColumnIpIntoActivities extends Migration public function down() { Schema::table('activities', function (Blueprint $table) { - $table->dropIndex('user_ip_idx'); $table->dropColumn('ip'); }); } diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index d4feb2e7c..0ab168b66 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -117,9 +117,9 @@ return [ 'audit_deleted_item' => 'Deleted Item', 'audit_deleted_item_name' => 'Name: :name', 'audit_table_user' => 'User', - 'audit_table_user_ip' => 'User IP', 'audit_table_event' => 'Event', 'audit_table_related' => 'Related Item or Detail', + 'audit_table_ip' => 'IP Address', 'audit_table_date' => 'Activity Date', 'audit_date_from' => 'Date Range From', 'audit_date_to' => 'Date Range To', diff --git a/resources/views/settings/audit.blade.php b/resources/views/settings/audit.blade.php index b1de77567..84f180f3b 100644 --- a/resources/views/settings/audit.blade.php +++ b/resources/views/settings/audit.blade.php @@ -62,7 +62,7 @@ {{ trans('settings.audit_table_event') }} {{ trans('settings.audit_table_related') }} - {{ trans('settings.audit_table_user_ip') }} + {{ trans('settings.audit_table_ip') }} {{ trans('settings.audit_table_date') }} diff --git a/tests/AuditLogTest.php b/tests/AuditLogTest.php index bc36a184d..9f6576a46 100644 --- a/tests/AuditLogTest.php +++ b/tests/AuditLogTest.php @@ -140,4 +140,53 @@ class AuditLogTest extends TestCase $resp->assertSeeText($chapter->name); $resp->assertDontSeeText($page->name); } + + public function test_ip_address_logged_and_visible() + { + config()->set('app.proxies', '*'); + $editor = $this->getEditor(); + /** @var Page $page */ + $page = Page::query()->first(); + + $this->actingAs($editor)->put($page->getUrl(), [ + 'name' => 'Updated page', + 'html' => '

Updated content

', + ], [ + 'X-Forwarded-For' => '192.123.45.1' + ])->assertRedirect($page->refresh()->getUrl()); + + $this->assertDatabaseHas('activities', [ + 'type' => ActivityType::PAGE_UPDATE, + 'ip' => '192.123.45.1', + 'user_id' => $editor->id, + 'entity_id' => $page->id, + ]); + + $resp = $this->asAdmin()->get('/settings/audit'); + $resp->assertSee('192.123.45.1'); + } + + public function test_ip_address_not_logged_in_demo_mode() + { + config()->set('app.proxies', '*'); + config()->set('app.env', 'demo'); + $editor = $this->getEditor(); + /** @var Page $page */ + $page = Page::query()->first(); + + $this->actingAs($editor)->put($page->getUrl(), [ + 'name' => 'Updated page', + 'html' => '

Updated content

', + ], [ + 'X-Forwarded-For' => '192.123.45.1', + 'REMOTE_ADDR' => '192.123.45.2', + ])->assertRedirect($page->refresh()->getUrl()); + + $this->assertDatabaseHas('activities', [ + 'type' => ActivityType::PAGE_UPDATE, + 'ip' => '127.0.0.1', + 'user_id' => $editor->id, + 'entity_id' => $page->id, + ]); + } }