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

@@ -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

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,
         ]);
     }
 

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');
         });
     }

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',

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>

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,
+        ]);
+    }
 }