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.

diff --git a/.env.example.complete b/.env.example.complete
index 49d834ff76df2e82f64433be25d31311a7c2111f..5eb65c27f09f346fa806d48253ab3e2a26c87337 100644 (file)
--- 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 b2a4a132fc9b5a4930aa49300aee50497f525d52..f8a0825bbd8794b9ae1c89a5986a21219f08fb02 100644 (file)
--- 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 97cc2c03901fffcf63751a6d94463c1397a2c894..68391b1c2ddee585b6cc532f15aeadb1e52c9d61 100644 (file)
--- 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 d4feb2e7c841b767b0af0a189ad1ab620bdf17dc..0ab168b66998bca0de4807f94d181b9b12ed1683 100755 (executable)
--- 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 b1de775676e29ab653b07f2c51220a185b208a80..84f180f3b41fbc43c07933aeab7a4b6910caa5d4 100644 (file)
--- a/resources/views/settings/audit.blade.php
+++ b/resources/views/settings/audit.blade.php
@@ -62,7 +62,7 @@
                     <a href="{{ sortUrl('/settings/audit', $listDetails, ['sort' => 'key']) }}">{{ trans('settings.audit_table_event') }}</a>
                 </th>
                 <th>{{ trans('settings.audit_table_related') }}</th>
-                <th>{{ trans('settings.audit_table_user_ip') }}</th>
+                <th>{{ trans('settings.audit_table_ip') }}</th>
                 <th>
                     <a href="{{ sortUrl('/settings/audit', $listDetails, ['sort' => 'created_at']) }}">{{ trans('settings.audit_table_date') }}</a></th>
             </tr>

diff --git a/tests/AuditLogTest.php b/tests/AuditLogTest.php
index bc36a184d129f3465a9e9fc855dfcabd21b98d5d..9f6576a46dadcf6ba0dab8e8f8342f947d3dcc71 100644 (file)
--- 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' => '<p>Updated content</p>',
+        ], [
+            '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' => '<p>Updated content</p>',
+        ], [
+            '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,
+        ]);
+    }
 }