diff --git a/.env.example b/.env.example index 7ec0a7a8a..e8bd362d1 100644 --- a/.env.example +++ b/.env.example @@ -106,8 +106,13 @@ IPAPI_KEY= # --------------------------------------------------------------------------- # Certificates. PDFLATEX_PATH must point at a real pdflatex binary or the # whole certificate pipeline fails. See docs/handover/08-certificates.md +# +# CERTIFICATE_ADMIN_EMAILS is a comma-separated allowlist for +# /admin/certificate-backend/*. No role grants access, so if this is blank +# the certificate backend is closed to everyone. # --------------------------------------------------------------------------- PDFLATEX_PATH= +CERTIFICATE_ADMIN_EMAILS= # --------------------------------------------------------------------------- # Storage. RESOURCES_BUCKET is a second bucket for Learn & Teach PDFs, and is diff --git a/app/Console/Commands/SoftDeleteUsersWithoutConsent.php b/app/Console/Commands/SoftDeleteUsersWithoutConsent.php index 3580f03e5..4a91e03d7 100644 --- a/app/Console/Commands/SoftDeleteUsersWithoutConsent.php +++ b/app/Console/Commands/SoftDeleteUsersWithoutConsent.php @@ -6,6 +6,7 @@ use App\User; use Illuminate\Console\Command; use Illuminate\Support\Facades\Hash; +use Illuminate\Support\Str; class SoftDeleteUsersWithoutConsent extends Command { @@ -17,20 +18,19 @@ public function handle() $this->info('Creating or verifying legacy user...'); // Create legacy user if it doesn't exist - User::firstOrCreate( + $legacyUser = User::firstOrCreate( ['id' => 1000000], [ 'firstname' => 'Codeweek Legacy User', 'lastname' => 'Codeweek Legacy', 'username' => 'codeweek-legacy', - 'password' => Hash::make('some-secure-password-' . time()), + 'password' => Hash::make(Str::random(40)), 'email' => 'legacy@codeweek.eu', 'country_iso' => 'BE', - 'remember_token' => '5Pb4APP0CkOmQFzDtR960OlkdreLev2tvfUiywDawyJOmKDvFb3bmOAWlUL6', + 'remember_token' => Str::random(60), 'privacy' => 1, 'email_display' => 'legacy@codeweek.eu', 'receive_emails' => 0, - 'approved' => 1, 'magic_key' => 2520090911, 'consent_given_at' => now(), 'future_consent_given_at' => now(), @@ -40,6 +40,12 @@ public function handle() ] ); + // 'approved' is guarded against mass assignment, so set it explicitly. + if (! $legacyUser->approved) { + $legacyUser->approved = 1; + $legacyUser->save(); + } + $this->info('Starting to process users without consent...'); // Get users without consent in chunks to avoid memory issues diff --git a/app/Http/Controllers/EventController.php b/app/Http/Controllers/EventController.php index aed1cd11e..ccc7fe76a 100755 --- a/app/Http/Controllers/EventController.php +++ b/app/Http/Controllers/EventController.php @@ -7,7 +7,6 @@ use App\Queries\EventsQuery; use App\Queries\PendingEventsQuery; use App\User; -use Illuminate\Auth\Access\AuthorizationException; use Illuminate\Database\Eloquent\ModelNotFoundException; use Illuminate\Http\RedirectResponse; use Illuminate\Http\Request; @@ -231,10 +230,7 @@ public function reject(Request $request, Event $event) { $rejectionText = $request->get('rejectionText', null); - try { - $this->authorize('approve', $event); - } catch (AuthorizationException $e) { - } + $this->authorize('approve', $event); $event->reject($rejectionText); } diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 8d2c70315..afd93e358 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -23,7 +23,7 @@ public function update() 'lastname' => 'required|string', 'privacy' => 'required', 'receive_emails' => 'required', - 'country_iso' => 'nullable|exists:countries,iso', + 'country_iso' => 'required|exists:countries,iso', 'city_id' => 'nullable|exists:cities,id', 'twitter' => 'nullable', 'website' => 'nullable', diff --git a/app/Http/Middleware/EnsureSuperCertificateAdmin.php b/app/Http/Middleware/EnsureSuperCertificateAdmin.php index e707a43f6..2a900e5b7 100644 --- a/app/Http/Middleware/EnsureSuperCertificateAdmin.php +++ b/app/Http/Middleware/EnsureSuperCertificateAdmin.php @@ -4,18 +4,27 @@ use Closure; use Illuminate\Http\Request; +use Illuminate\Support\Facades\Log; use Symfony\Component\HttpFoundation\Response; class EnsureSuperCertificateAdmin { - private const ALLOWED_EMAIL = 'bernard@matrixinternet.ie'; - /** - * Handle an incoming request. Only the super certificate admin email can access. + * Handle an incoming request. Only a configured certificate admin can access. */ public function handle(Request $request, Closure $next): Response { - if (! $request->user() || $request->user()->email !== self::ALLOWED_EMAIL) { + $allowed = config('codeweek.certificate_admin_emails', []); + + if (empty($allowed)) { + Log::warning('Certificate backend access denied: CERTIFICATE_ADMIN_EMAILS is not set.'); + + abort(403, 'The certificate administrator list is not configured. Set CERTIFICATE_ADMIN_EMAILS.'); + } + + $email = $request->user()?->email; + + if ($email === null || ! in_array(strtolower($email), array_map('strtolower', $allowed), true)) { abort(403, 'Access denied. This area is restricted to the certificate administrator.'); } diff --git a/app/Listeners/AlertOnBusyQueue.php b/app/Listeners/AlertOnBusyQueue.php new file mode 100644 index 000000000..943a15712 --- /dev/null +++ b/app/Listeners/AlertOnBusyQueue.php @@ -0,0 +1,30 @@ +queue, + $event->connection, + $event->size + ); + + Log::error($message); + + \Sentry\captureMessage($message); + } +} diff --git a/app/Livewire/LeadingTeachersTable.php b/app/Livewire/LeadingTeachersTable.php index f50664d5f..d3f9e7895 100644 --- a/app/Livewire/LeadingTeachersTable.php +++ b/app/Livewire/LeadingTeachersTable.php @@ -46,6 +46,25 @@ public function filters(): array } }), + SelectFilter::make('City') + ->setFilterPillTitle('City') + ->setFilterPillValues([ + 'missing' => 'Not set', + 'set' => 'Set', + ]) + ->options([ + '' => 'All', + 'missing' => 'Not set', + 'set' => 'Set', + ]) + ->filter(function(Builder $builder, string $value) { + if ($value === 'missing') { + $builder->whereNull('city_id'); + } elseif ($value === 'set') { + $builder->whereNotNull('city_id'); + } + }), + ]; } @@ -101,6 +120,9 @@ public function columns(): array ->searchable(), Column::make('Country','country_iso') ->sortable(), + Column::make('City', 'city.city') + ->sortable() + ->searchable(), Column::make('email') ->sortable() ->searchable(), diff --git a/app/Nova/Actions/RejectEvent.php b/app/Nova/Actions/RejectEvent.php index 9dad8ee95..d04263476 100644 --- a/app/Nova/Actions/RejectEvent.php +++ b/app/Nova/Actions/RejectEvent.php @@ -8,6 +8,7 @@ use Illuminate\Support\Collection; use Laravel\Nova\Actions\Action; use Laravel\Nova\Fields\ActionFields; +use Laravel\Nova\Fields\Textarea; use Laravel\Nova\Http\Requests\NovaRequest; class RejectEvent extends Action @@ -22,7 +23,7 @@ class RejectEvent extends Action public function handle(ActionFields $fields, Collection $models) { foreach ($models as $model) { - $model->reject(); + $model->reject($fields->rejectionText); } } @@ -31,6 +32,10 @@ public function handle(ActionFields $fields, Collection $models) */ public function fields(NovaRequest $request): array { - return []; + return [ + Textarea::make('Rejection reason', 'rejectionText') + ->rules('required', 'string', 'max:2000') + ->help('Sent to the organiser in the rejection email and stored on the activity.'), + ]; } } diff --git a/app/User.php b/app/User.php index 8292554a4..ae9913e2f 100644 --- a/app/User.php +++ b/app/User.php @@ -107,7 +107,9 @@ class User extends Authenticatable implements MustVerifyEmail */ - protected $guarded = []; + // 'approved' controls whether a leading teacher is listed publicly, so it must + // never be settable from request data. Set it explicitly instead of mass-assigning. + protected $guarded = ['approved']; /** * The attributes that should be hidden for arrays. diff --git a/config/codeweek.php b/config/codeweek.php index 834baebcd..29a8808ff 100644 --- a/config/codeweek.php +++ b/config/codeweek.php @@ -24,4 +24,16 @@ 'CONTACT_FORM_RECIPIENT_EMAIL', env('ADMIN_EMAIL', 'admin@codeweek.test') ), + + // Pending-job count above which queue:monitor fires QueueBusy. Raise it if + // October traffic makes the alert noisy rather than switching the alert off. + 'queue_busy_threshold' => (int) env('QUEUE_BUSY_THRESHOLD', 100), + + // Who may use /admin/certificate-backend/*. Comma-separated list of email + // addresses; no role grants access. This used to be a single hardcoded + // address in the middleware, which locked out everyone else. + 'certificate_admin_emails' => array_values(array_filter(array_map( + 'trim', + explode(',', (string) env('CERTIFICATE_ADMIN_EMAILS', '')) + ))), ]; diff --git a/database/factories/CityFactory.php b/database/factories/CityFactory.php index d45c7cb23..49bfc8fcd 100644 --- a/database/factories/CityFactory.php +++ b/database/factories/CityFactory.php @@ -1,13 +1,24 @@ define(App\City::class, function () { - return [ - 'id' => $this->faker->numberBetween(1234567890, 9999999999), - 'city' => $this->faker->city(), - 'country' => $this->faker->country(), - 'country_iso' => $this->faker->countryCode(), - 'longitude' => $this->faker->longitude(), - 'latitude' => $this->faker->latitude(), - ]; -}); +namespace Database\Factories; + +use Illuminate\Database\Eloquent\Factories\Factory; + +class CityFactory extends Factory +{ + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + return [ + 'city' => $this->faker->city(), + 'country' => $this->faker->country(), + 'country_iso' => $this->faker->countryCode(), + 'longitude' => $this->faker->longitude(), + 'latitude' => $this->faker->latitude(), + ]; + } +} diff --git a/database/seeders/ResourceEditorRoleSeeder.php b/database/seeders/ResourceEditorRoleSeeder.php index 608c6b3da..f518db667 100644 --- a/database/seeders/ResourceEditorRoleSeeder.php +++ b/database/seeders/ResourceEditorRoleSeeder.php @@ -17,11 +17,11 @@ public function run(): void app()['cache']->forget('spatie.permission.cache'); // create permissions - Permission::create(['name' => 'moderate resource']); + Permission::firstOrCreate(['name' => 'moderate resource']); // create roles and assign created permissions - $role = Role::create(['name' => 'resource editor']); + $role = Role::firstOrCreate(['name' => 'resource editor']); $role->givePermissionTo(['moderate resource']); } diff --git a/docs/handover/00-access-checklist.md b/docs/handover/00-access-checklist.md index 296c9311b..c25c272c4 100644 --- a/docs/handover/00-access-checklist.md +++ b/docs/handover/00-access-checklist.md @@ -97,13 +97,17 @@ Ask for a `super admin` account on both environments. ### Day-one blocker you must resolve -The certificate administration area at `/admin/certificate-backend/*` is gated on a **single hardcoded email address** belonging to the outgoing team: +The certificate administration area at `/admin/certificate-backend/*` is not governed by roles at all. It reads an explicit allowlist of email addresses from the environment, and it **fails closed**: -```11:11:app/Http/Middleware/EnsureSuperCertificateAdmin.php - private const ALLOWED_EMAIL = 'bernard@matrixinternet.ie'; +```19:23:app/Http/Middleware/EnsureSuperCertificateAdmin.php + if (empty($allowed)) { + Log::warning('Certificate backend access denied: CERTIFICATE_ADMIN_EMAILS is not set.'); + + abort(403, 'The certificate administrator list is not configured. Set CERTIFICATE_ADMIN_EMAILS.'); + } ``` -No role will get you in. This constant has to be changed (ideally replaced with a role or permission check) before you can operate the certificate tooling at all. The same address is also the fallback recipient for the contact form in `app/Http/Controllers/ContactFormController.php`. Both are listed in [12](12-risks-and-known-issues.md). +So until somebody sets `CERTIFICATE_ADMIN_EMAILS` in Forge, **nobody can operate certificates** — not even a super admin. Add your own address to it on both environments as part of getting set up, and confirm you can load `/admin/certificate-backend`. This used to be a single hardcoded address belonging to the outgoing team; see [12](12-risks-and-known-issues.md). ## 9. A note on server addresses in git history diff --git a/docs/handover/02-environments-and-deployment.md b/docs/handover/02-environments-and-deployment.md index 8b078ddf7..7e24cbfc8 100644 --- a/docs/handover/02-environments-and-deployment.md +++ b/docs/handover/02-environments-and-deployment.md @@ -78,7 +78,7 @@ The `composer-test.json` swap is the part to remember. It exists to strip the No CI triggers on pushes and pull requests for both `master` and `dev`. It previously only ran for `master`, so pull requests into `dev` ran no tests at all. -Two tests fail on `master` for reasons unrelated to any recent change: `UserRestoreServiceTest` and `CommunityAmbassadorFilteringTest`, the latter because `database/factories/CityFactory.php` is still in the pre-Laravel-8 `$factory->define()` format. Everything else passes. Fix or quarantine these, because a suite that is normally red is a suite nobody reads. +The suite is green. It was not when this handover started — two tests failed and five files were silently skipped — so if you see red, it is something you changed rather than inherited noise. Keep it that way: a suite that is normally red is a suite nobody reads. [11](11-testing-and-local-dev.md) An abandoned Travis configuration targeting PHP 7.3 has been removed, along with `.env.travis`. GitHub Actions is the only pipeline. diff --git a/docs/handover/03-configuration.md b/docs/handover/03-configuration.md index fd6057ab2..35787e055 100644 --- a/docs/handover/03-configuration.md +++ b/docs/handover/03-configuration.md @@ -307,6 +307,26 @@ return [ Note that `blog_url` defaults to the live blog. On a dev or local environment the blog sync command will therefore pull from **production** unless `BLOG_URL` is set explicitly. See [09](09-wordpress-blog.md). +Three more entries live further down the same file and are easy to miss: + +| Key | Variable | Notes | +|-----|----------|-------| +| `contact_form_recipient` | `CONTACT_FORM_RECIPIENT_EMAIL` | Falls back to `ADMIN_EMAIL` | +| `queue_busy_threshold` | `QUEUE_BUSY_THRESHOLD` | Pending jobs above which the queue alert fires. Default 100. See [10](10-scheduled-jobs-and-runbooks.md) | +| `certificate_admin_emails` | `CERTIFICATE_ADMIN_EMAILS` | Comma-separated allowlist for the certificate backend. **Blank means nobody has access** | + +`certificate_admin_emails` is the one to set before anything else: + +```32:38:config/codeweek.php + // Who may use /admin/certificate-backend/*. Comma-separated list of email + // addresses; no role grants access. This used to be a single hardcoded + // address in the middleware, which locked out everyone else. + 'certificate_admin_emails' => array_values(array_filter(array_map( + 'trim', + explode(',', (string) env('CERTIFICATE_ADMIN_EMAILS', '')) + ))), +``` + ## Locales `LOCALES` currently lists 27 codes: diff --git a/docs/handover/04-domain-model.md b/docs/handover/04-domain-model.md index af3da7dc2..018b2fd03 100644 --- a/docs/handover/04-domain-model.md +++ b/docs/handover/04-domain-model.md @@ -224,7 +224,7 @@ Notes: ## Roles and permissions -Managed by `spatie/laravel-permission`. Seeded across four seeders. +Managed by `spatie/laravel-permission`. Seeded across four seeders. This section covers what the roles *are*; [14](14-accounts-and-moderation.md) covers how somebody gets one and what each role can actually do. From [database/seeders/RolesAndPermissionsSeeder.php](../../database/seeders/RolesAndPermissionsSeeder.php): diff --git a/docs/handover/05-nova-admin.md b/docs/handover/05-nova-admin.md index 339e050e5..2ad13d6d8 100644 --- a/docs/handover/05-nova-admin.md +++ b/docs/handover/05-nova-admin.md @@ -124,7 +124,7 @@ Ten actions in [app/Nova/Actions/](../../app/Nova/Actions). | Action | Attached to | Effect | |--------|-------------|--------| | `ApproveEvent` | `Event` | Calls `$model->approve()` — sets status and emails the organiser | -| `RejectEvent` | `Event` | Calls `$model->reject()`. **No message field** — see below | +| `RejectEvent` | `Event` | Calls `$model->reject($reason)` with a required reason — see below | | `BulkUploadMediaFiles` | `MediaUpload` | Multi-file upload to S3 | | `ExportHomeSlideLocaleOverrides` | `HomeSlide` | CSV export of slide translations | | `ImportHomeSlideLocaleOverrides` | `HomeSlide` | CSV import of slide translations | @@ -132,11 +132,23 @@ Ten actions in [app/Nova/Actions/](../../app/Nova/Actions). | `ImportMatchmakingProfiles` | `MatchmakingProfile` | Bulk profile import | | `ApproveSupportApproval` | `SupportApproval` | Support copilot workflow | | `RejectSupportApproval` | `SupportApproval` | Support copilot workflow | -### Rejecting through Nova loses the reason +### Rejecting through Nova -`RejectEvent` exposes no fields, so the `Moderation` record it creates has an empty `message`. The organiser gets a rejection with no explanation. The legacy Blade moderation screens at `/pending` and `/review` do capture a reason. +`RejectEvent` used to expose no fields at all, so the `Moderation` record it wrote had an empty `message` and the organiser received a rejection email with no explanation, while the Blade screens at `/pending` and `/review` captured a reason. Ambassadors use both paths, so the two behaved differently for no reason anyone had chosen. -**Guidance for ambassadors: use `/pending` and `/review` to reject, not Nova.** Or fix the action to include a message field. Either way, decide and communicate it, because right now the two paths behave differently and ambassadors use both. +The action now requires a reason: + +```33:39:app/Nova/Actions/RejectEvent.php + public function fields(NovaRequest $request): array + { + return [ + Textarea::make('Rejection reason', 'rejectionText') + ->rules('required', 'string', 'max:2000') + ->help('Sent to the organiser in the rejection email and stored on the activity.'), + ]; +``` + +One difference remains, and it is not fixable in the action: **editing the Status field directly on the Event resource bypasses `approve()` and `reject()` entirely**, so no email is sent and no moderation record is written. Use the actions, not the dropdown. [14](14-accounts-and-moderation.md) ## Filters diff --git a/docs/handover/08-certificates.md b/docs/handover/08-certificates.md index f1f1dee07..1f25422ad 100644 --- a/docs/handover/08-certificates.md +++ b/docs/handover/08-certificates.md @@ -204,13 +204,17 @@ A dedicated batch operations UI at `/admin/certificate-backend/*`, handled by `C Batch progress is tracked in the cache with a 24-hour TTL. -**This area is gated on a single hardcoded email address**, not a role: +**This area is gated on an explicit email allowlist**, not a role: -```11:11:app/Http/Middleware/EnsureSuperCertificateAdmin.php - private const ALLOWED_EMAIL = 'bernard@matrixinternet.ie'; +```25:29:app/Http/Middleware/EnsureSuperCertificateAdmin.php + $email = $request->user()?->email; + + if ($email === null || ! in_array(strtolower($email), array_map('strtolower', $allowed), true)) { + abort(403, 'Access denied. This area is restricted to the certificate administrator.'); + } ``` -A new developer, even as super admin, gets a 403. **Change this constant before the first certificate run** — ideally replacing it with a role or permission check. See [00](00-access-checklist.md) and [12](12-risks-and-known-issues.md). +The list comes from `CERTIFICATE_ADMIN_EMAILS`, comma-separated, and the comparison is case-insensitive. **Being a super admin grants nothing here.** If the variable is unset the middleware denies everyone and says so in the 403, so `Set CERTIFICATE_ADMIN_EMAILS` in a browser is your diagnosis. **Set it before the first certificate run.** See [00](00-access-checklist.md) and [12](12-risks-and-known-issues.md). ## Artisan commands diff --git a/docs/handover/10-scheduled-jobs-and-runbooks.md b/docs/handover/10-scheduled-jobs-and-runbooks.md index a1ec695c5..b90f9435d 100644 --- a/docs/handover/10-scheduled-jobs-and-runbooks.md +++ b/docs/handover/10-scheduled-jobs-and-runbooks.md @@ -21,9 +21,10 @@ php artisan schedule:list | Hourly :13 | `magic:key` | Regenerates user `magic_key` tokens for unsubscribe links | [04](04-domain-model.md) | | Hourly :15 | `api:germany-central --import` | The live German import | [07](07-partner-feeds-and-apis.md) | | Hourly :30 | `notify:administrators` | Emails admins about activities awaiting attention | | -| Hourly :30 | `relocate` | Fixes activities with missing or bad coordinates | | +| Hourly :30 | `relocate` | Repositions online activities stuck at `0,0` | | | Hourly :33 | `certificate:issues` | Detects certificates with generation problems | [08](08-certificates.md) | -| Every 2 min | `relocate:country` | Country-level coordinate repair | | +| Every 5 min | `queue:monitor default` | Fires the queue backlog alert. See below | | +| Every 2 min | `relocate:country` | Re-geocodes activities sitting on a country centroid | | | Every minute | `support:gmail:poll --max=10` | Support mailbox ingest. **Only if `support_gmail.enabled`** | | | Every minute | `support:ai:poll-agents` | Support copilot agent polling. **Only if both support AI flags are on** | | | Daily 01:00 | `app:sync-blogs` | Pulls WordPress posts into the `blogs` table | [09](09-wordpress-blog.md) | @@ -41,13 +42,27 @@ The schedule used to contain `app:export-search-data-to-json` at 02:00 daily. No Worth noting as a pattern: a permanently failing schedule entry trains everyone to ignore scheduler errors, which is how a real failure goes unnoticed. If you add a `Schedule::command()` line, confirm it appears in `php artisan schedule:list` and actually runs. -### Two entries with a name collision worth double-checking +### Two similarly named entries, worth not confusing -`relocate` (hourly at :30) and `relocate:country` (every two minutes) both resolve to `app/Console/Commands/RelocateCountry.php`. Confirm on the server with `php artisan schedule:list` which signature each actually binds to, and whether running a geocoding repair **every two minutes** is still intended. +`relocate` and `relocate:country` look like the same command and are not. `relocate` (hourly at :30, `app/Console/Commands/Relocate.php`) repositions online activities stuck at `0,0`. `relocate:country` (**every two minutes**, `app/Console/Commands/RelocateCountry.php`) re-geocodes activities sitting exactly on a country centroid. Confirm with `php artisan schedule:list` on the server, and ask whether a geocoding repair every two minutes is still intended — it is by far the most frequent entry in the schedule. + +### Queue monitoring + +```46:50:routes/console.php +// Fires Laravel's QueueBusy event when the backlog exceeds the threshold, which +// App\Listeners\AlertOnBusyQueue turns into a log line and a Sentry message. +// Nothing else watches the queue, so removing this makes a stalled worker silent. +Schedule::command('queue:monitor default --max='.config('codeweek.queue_busy_threshold')) + ->everyFiveMinutes(); +``` + +`queue:monitor` does not alert by itself — it only dispatches Laravel's `QueueBusy` event when the backlog exceeds `--max`. `App\Listeners\AlertOnBusyQueue` is what turns that into something you see, via `Log::error()` and a Sentry message. The threshold comes from `QUEUE_BUSY_THRESHOLD` and defaults to 100. + +If October traffic makes this noisy, **raise the threshold rather than removing the entry** — nothing else watches the queue, and a stalled worker is otherwise only noticed by someone wondering why activities have stopped appearing. ### Support subsystem entries are conditional -```47:48:routes/console.php +```53:54:routes/console.php $supportGmailPoll = Schedule::command('support:gmail:poll --max=10') ->when(fn () => (bool) config('support_gmail.enabled')); ``` @@ -174,7 +189,7 @@ Roughly a quarter of the year's visits land in October — October 2024 recorded A fortnight beforehand: - [ ] Confirm the schedule is registered: `php artisan schedule:list`. -- [ ] Confirm queue workers are alive and draining. See the note below — **`queue:monitor` needs arguments and will not alert anyone on its own.** +- [ ] Confirm queue workers are alive and draining, and that the queue alert reaches you — see [Checking the queue properly](#checking-the-queue-properly). Deliberately trigger it once so you know what the alert looks like before you need to recognise it. - [ ] Check for a backlog of failures: `php artisan queue:failed`. Clear anything stale before the spike so October failures are visible. - [ ] Confirm the German central import is succeeding: `php artisan api:germany-central`. **Dry-run is the default**; it only writes when you add `--import`, so this is safe to run against live. - [ ] Verify the database backup by restoring one. Backups are not configured in this repository — there is no `spatie/laravel-backup` — so they are managed in Forge/AWS. The commitment is nightly database and weekly file backups with restore tests. @@ -195,11 +210,17 @@ php artisan tinker --execute="echo config('queue.default');" php artisan queue:monitor database:default # or redis:default, matching the above ``` -Two caveats that make this weaker than it looks: +Two things to understand about what this does and does not tell you: -- `queue:monitor` only **dispatches a `QueueBusy` event** when a queue exceeds `--max`. There is no listener for that event anywhere in this application, so nothing is emailed or alerted. Run interactively and read the table, or register a listener if you want real alerting. +- `queue:monitor` only **dispatches a `QueueBusy` event** when a queue exceeds `--max`; the command itself alerts nobody. `App\Listeners\AlertOnBusyQueue` is what turns that event into a `Log::error()` line and a Sentry message, and the scheduled entry above is what runs the check every five minutes. All three pieces are needed — remove any one and a stalled worker goes silent again. - No job in this codebase calls `onQueue()`, so everything sits on the single default queue. There is no Horizon either. Worker health is therefore a Forge question, not an application one — check the daemon status in Forge alongside the commands above. +To prove the alerting path end to end without waiting for a real backlog, fire the event by hand and confirm it reaches Sentry: + +```bash +php artisan tinker --execute="event(new Illuminate\Queue\Events\QueueBusy(config('queue.default'), 'default', 9999));" +``` + A useful one-liner for confirming the effective drivers on a host: ```bash diff --git a/docs/handover/11-testing-and-local-dev.md b/docs/handover/11-testing-and-local-dev.md index babb348fc..fd1f1fe4b 100644 --- a/docs/handover/11-testing-and-local-dev.md +++ b/docs/handover/11-testing-and-local-dev.md @@ -39,7 +39,7 @@ If you are changing the filter layer, switch to it locally and run the suite aga ## Test layout -102 test files. +111 test files. | Location | Contents | |----------|----------| @@ -73,17 +73,36 @@ Every test inherits setup that is worth knowing about, because it changes what y - `Mail::fake()` is global — assert with `Mail::assertQueued()` rather than expecting real delivery. - A `signIn($user)` helper creates and authenticates a user in one call. -### Five test files that never run +### Two factory systems, side by side -These are in `tests/Feature/` but lack the `Test.php` suffix, so PHPUnit ignores them: +`database/factories/` contains **both** styles of Laravel factory, and which one you can use depends on the model: -- `GermanImports.php` -- `GermanUsersCreation.php` -- `RelocateCenteredActivities.php` -- `RelocateOnlineActivities.php` -- `NullEmails.php` +- **Modern class factories** (`class CountryFactory extends Factory`, resolved by `Model::factory()`). Seventeen models use these. +- **Legacy closure factories** (`$factory->define(App\Podcast::class, ...)`), which only work because `laravel/legacy-factories` is still installed. These are reached through the global `factory()` helper, or the `create()` and `make()` wrappers in `tests/utilities/functions.php`. Fourteen models still use these. -They look like tests and will show up when you grep, but they are dead weight. Some are probably worth reviving by renaming — the German import ones in particular, given [07](07-partner-feeds-and-apis.md) — but check they still pass before trusting them. +So `Podcast::factory()` fails with *"Class Database\Factories\PodcastFactory not found"*, while `create(\App\Country::class)` fails with *"Unable to locate factory for [App\Country]"*. Check which style a model has before writing a test against it, and if you convert one, convert its call sites in the same commit. + +`City` was converted from legacy to modern as part of this handover, because a community test needed `City::factory()`. Converting the remaining fourteen would be a reasonable tidy-up, but do it one model at a time and update the `create(...)` call sites with it. + +### Five files that used to be skipped + +`GermanImports`, `GermanUsersCreation`, `RelocateCenteredActivities`, `RelocateOnlineActivities` and `NullEmails` were in `tests/Feature/` without the `Test.php` suffix, so PHPUnit ignored them. They have been repaired and renamed, and now run. + +What they had drifted against is worth knowing, because the same drift is waiting in any old test you revive: + +- `->create([...], 6)` used to mean "make six". The modern API is `->count(6)->create([...])`, and passing an integer as the second argument now throws a `TypeError` about `$parent`. +- Two of them called `User::factory()` without importing `App\User`, which fails as `Class "Tests\Feature\User" not found`. +- `GermanImports` asserted that `cw22-leipzig`, `cw22-dresden` and `cw22-thueringen` count as imported, but those cities are no longer in `ImporterHelper::getGermanCities()`. Rather than add them back and change what `Event::imported()` matches, the test now iterates that helper, so the list stays the single source of truth. + +The lesson for the other thirteen legacy factories and anything else you exhume: a test that has not run for years is asserting the contract of a codebase that no longer exists. Read what it claims before you trust a green tick. + +### One flake, and why it mattered + +`OnlineEventsWorkflowTest` failed roughly one run in four, which is the worst possible failure rate — often enough to erode trust in the suite, rarely enough that re-running makes it go away. + +The cause is worth internalising because the pattern is everywhere in this codebase. `OnlineEventsQuery` selects activities with `start_date >= Carbon::now()->subDays(15)`, and the test created its fixture at **exactly** `Carbon::now()->subDays(15)`. The two `now()` calls happen milliseconds apart, so whenever the clock ticked a second between them the fixture fell a second outside its own window. + +Several queries use this fifteen-day window — `OnlineEventsQuery`, `CountriesQuery::withOnlineEvents()` and `EventHelper::getOnlineEvents()`. If you write a test against any of them, **put the fixture a day inside the boundary, not on it.** If you genuinely need to test the boundary, freeze time with `Carbon::setTestNow()` rather than relying on two clock reads agreeing. ## What the suite does not cover diff --git a/docs/handover/12-risks-and-known-issues.md b/docs/handover/12-risks-and-known-issues.md index c06649d6a..ebb68afa7 100644 --- a/docs/handover/12-risks-and-known-issues.md +++ b/docs/handover/12-risks-and-known-issues.md @@ -1,46 +1,24 @@ # 12 — Risks and known issues -This chapter used to be a long inventory. Most of it has since been fixed — see [what was fixed](#what-was-fixed) at the end for the list, so you are not surprised by references to it elsewhere. +Grouped by what you need to do about it: actions that need access or credentials we cannot use on your behalf, behaviour you must understand before touching imports or certificates, and a set of gaps left open deliberately. A record of what was fixed is at the end, so references to it elsewhere make sense. -What remains is grouped by what you need to do about it: one thing that will lock you out, a few open security actions, behaviour you must understand before touching imports or certificates, and a set of accepted gaps. +## Actions that need your credentials -## This will lock you out on day one +These are the only items that cannot be closed from the repository. Each needs someone with infrastructure access. -The certificate backend is gated on a single hardcoded email address belonging to the outgoing developer: +**Set `CERTIFICATE_ADMIN_EMAILS` before your next deploy.** Access to `/admin/certificate-backend/*` used to be a single hardcoded email address belonging to the outgoing developer. It is now a comma-separated allowlist read from the environment, and it **fails closed** — if the variable is blank, nobody gets in and the 403 says so. Set it in Forge for both dev and live, then confirm you can reach the certificate backend. Do this before October, because certificate reporting starts the moment October activities end. [08](08-certificates.md) -```11:11:app/Http/Middleware/EnsureSuperCertificateAdmin.php - private const ALLOWED_EMAIL = 'bernard@matrixinternet.ie'; -``` - -Everything under `/admin/certificate-backend/*` — batch generation, sending, retrying failures, manual creation — depends on it. **No role grants access.** A new super admin gets a 403 and cannot operate certificates at all. - -This was deliberately left unchanged so the incoming team can decide the access model rather than inherit ours. The options, roughly in order of preference: - -- an environment-driven allowlist, so access follows a variable rather than a deploy; -- a Spatie permission such as `generate certificate`, which already exists in the seeder; -- the `super admin` role, which is simplest but means every super admin can send certificate batches. - -Whichever you choose, do it before October, because certificate reporting starts the moment October activities end. [08](08-certificates.md) - -## Open security actions - -**The `forge` SSH keys should be rotated.** [docs/ops/learn-and-teach-resource-import.md](../ops/learn-and-teach-resource-import.md) contained the live and dev server IP addresses with working `ssh -i ~/.ssh/id_rsa forge@…` command lines, in the **public** `codeeu/codeweek` repository. The file now uses placeholders, but **redaction does not remove them from git history.** The real mitigation is rotating those keys, restricting SSH ingress at the AWS security group, or both. - -**The previously committed `APP_KEY` should be treated as compromised.** `.env.example` shipped a real base64 key for a long time. It is blank now, but any environment created by copying that file is using a publicly known encryption key. Rotate it wherever it was used, and remember that rotating `APP_KEY` invalidates existing encrypted values and sessions. +**Rotate the `forge` SSH keys.** [docs/ops/learn-and-teach-resource-import.md](../ops/learn-and-teach-resource-import.md) contained the live and dev server IP addresses with working `ssh -i ~/.ssh/id_rsa forge@…` command lines, in the **public** `codeeu/codeweek` repository. The file now uses placeholders, but **redaction does not remove them from git history.** The real mitigation is rotating those keys, restricting SSH ingress at the AWS security group, or both. -**`User` is fully mass-assignable.** +**Treat the previously committed `APP_KEY` as compromised.** `.env.example` shipped a real base64 key for a long time. It is blank now, but any environment created by copying that file is using a publicly known encryption key. Rotate it wherever it was used, and remember that rotating `APP_KEY` invalidates existing encrypted values and sessions. -```110:110:app/User.php - protected $guarded = []; -``` - -Every attribute is mass-assignable, including `approved` and `magic_key`. Any `fill()` or `update()` reached with unfiltered request data is a privilege-escalation path. This was left alone because moving to an explicit `$fillable` touches a lot of call sites and is exactly the kind of change that breaks quietly. Audit the call sites first, then narrow it. +**Confirm Turnstile actually verifies.** Bot protection on the contact form never ran, because the code read `TURNSTILE_SECRET_KEY` while deployed environments set `TURNSTILE_SECRET`. Both names now resolve, which means verification is switched on for the first time. If the configured secret is stale, contact form submissions will start failing. **Submit the form on dev before merging to live.** -**The public API is unauthenticated.** `/api/events/geobox`, `/api/events/germany`, and `/api/event-detail/{event}` have no API key, no token, and no per-consumer limit beyond a global 60-requests-per-minute throttle. That may well be right for open data, but it should be a decision someone has made rather than an accident. [07](07-partner-feeds-and-apis.md) +**Decide whether the public API should stay open.** `/api/events/geobox`, `/api/events/germany`, and `/api/event-detail/{event}` have no API key, no token, and no per-consumer limit beyond a global 60-requests-per-minute throttle. That may well be right for open data, but it should be a decision someone has made rather than an accident. [07](07-partner-feeds-and-apis.md) ## Behaviour to understand before you touch imports -None of these are bugs exactly. They are design choices with sharp edges, and each one has surprised somebody. +None of these are bugs. They are design choices with sharp edges, and each one has surprised somebody. **Restart workers after changing LaTeX templates.** The most important piece of inherited knowledge in this document. Long-running queue workers hold templates in memory, so if you skip `php artisan queue:restart` after a template change, certificates are generated **silently using last year's design** — no error, and by the time anyone notices they have been emailed. [08](08-certificates.md) @@ -65,55 +43,90 @@ So `jane@school-a.be` can be attributed to an existing `jane@somewhere-else.com` Cache::flush(); ``` -Not just the upload's own entries — everything, including the map cache and GeoIP lookups. Expect a performance dip on live after a large import. Narrowing this to targeted keys would be a cheap improvement, but it needs someone to enumerate the keys safely rather than guess. +Not just the upload's own entries — everything, including the map cache and GeoIP lookups. Expect a performance dip on live after a large import. Narrowing this to targeted keys would be a cheap improvement, but it needs someone to enumerate the keys safely rather than guess, and guessing wrong means stale data on the public map. **Duplicate detection is strict.** `BulkEventDuplicateFinder` matches on title, start date, country and organiser, plus address or coordinates. Change any one of those in a corrected spreadsheet and you get a second activity rather than an update. **Theme and audience IDs are a public contract.** Partners submit numeric theme and audience IDs taken from the [public wiki page](https://github.com/codeeu/codeweek/wiki/Publish-your-events-into-Codeweek). Theme IDs are deliberately non-contiguous after a past consolidation. **Renumbering them breaks historical activities and every partner's export script simultaneously.** [04](04-domain-model.md) -The column list is now pinned by `tests/Unit/BulkEventUploadColumnContractTest.php`, so changing `REQUIRED_COLUMNS` fails the suite with a reminder to update the wiki. Nothing pins the theme and audience IDs; treat them as frozen. +The column list is pinned by `tests/Unit/BulkEventUploadColumnContractTest.php`, so changing `REQUIRED_COLUMNS` fails the suite with a reminder to update the wiki. Nothing pins the theme and audience IDs; treat them as frozen. **The GDPR deletion script is irreversible.** [soft_delete_users_without_consent.sql](../../soft_delete_users_without_consent.sql) performs soft deletes and hard deletes in one pass, reassigning records to the legacy user with id `1000000`. There is no undo without a database backup. Take one first, every time. That literal `1000000` is load-bearing in several places. [04](04-domain-model.md) -**Blog sync never removes anything.** `app:sync-blogs` only upserts, so a post deleted or unpublished in WordPress stays in the `blogs` table forever and keeps appearing in site search, linking to a 404. There is no reconciliation pass. [09](09-wordpress-blog.md) +**Editing an activity's Status directly in Nova sends no email.** `Event::approve()` and `Event::reject()` are what queue the organiser's notification and write the moderation record. The Nova Status dropdown writes the column and nothing else. Ambassadors have full edit rights on activities in their own country, so this is easy to do by accident — use the Approve and Reject actions. [14](14-accounts-and-moderation.md) + +**Blog sync never removes anything.** `app:sync-blogs` only upserts, so a post deleted or unpublished in WordPress stays in the `blogs` table forever and keeps appearing in site search, linking to a 404. Adding a reconciliation pass is not hard, but making it delete rows on the strength of one API response is how you lose the archive if the WordPress API returns a partial page. If you build it, drive it from an explicit opt-in flag and log what it would remove before it removes anything. [09](09-wordpress-blog.md) + +## Gaps left open deliberately + +**`User` is still fully mass-assignable apart from one column.** + +```110:112:app/User.php + // 'approved' controls whether a leading teacher is listed publicly, so it must + // never be settable from request data. Set it explicitly instead of mass-assigning. + protected $guarded = ['approved']; +``` + +`approved` is now guarded, because that was the one genuine privilege-escalation path — it controls whether someone is listed publicly. Everything else remains mass-assignable. Narrowing this further to an explicit `$fillable` means auditing every `User::create()` and `fill()` call in the importers, the German sync, the social login service and the helpers, and the failure mode is a silently dropped column rather than an error. Worth doing, but do it with the suite in front of you, one call site at a time. Note that `id` **must** stay assignable: `SoftDeleteUsersWithoutConsent` relies on mass-assigning `1000000` to create the legacy placeholder user. -## Accepted gaps +**No Nova code is exercised by CI.** CI swaps in `composer-test.json`, which strips the Nova packages. Every Nova change needs manual QA on dev. [11](11-testing-and-local-dev.md) -These are known, and were left alone deliberately — either because fixing them needs access or decisions we do not have, or because the fix is riskier than the problem. +**Tests run on SQLite, production runs MySQL 8.** The `JSON_CONTAINS` and `FIND_IN_SET` paths in `app/Filters/EventFilters.php` cannot be covered by the default suite. A `mysql_testing` connection exists, commented out, in `phpunit.xml`; switch to it locally if you touch the filter layer. [11](11-testing-and-local-dev.md) -**Testing and CI.** CI swaps in `composer-test.json`, which strips the Nova packages, so **no Nova code is exercised by CI at all** — every Nova change needs manual QA on dev. Tests also run on SQLite while production is MySQL, so the JSON and `FIND_IN_SET` paths in `app/Filters/EventFilters.php` cannot be covered by the default suite; a `mysql_testing` connection exists, commented out, in `phpunit.xml`. Two tests currently fail on `master` for unrelated legacy reasons: `UserRestoreServiceTest`, and `CommunityAmbassadorFilteringTest` because `database/factories/CityFactory.php` is still in the pre-Laravel-8 `$factory->define()` format. Five further files in `tests/Feature/` lack the `Test.php` suffix and never run; renaming them would make them execute, so check they pass before doing it. [11](11-testing-and-local-dev.md) +**Two factory systems coexist.** `database/factories/` holds seventeen modern class factories and fourteen legacy `$factory->define()` closures, the latter working only because `laravel/legacy-factories` is still installed. `Model::factory()` works for one set, the `create()` and `make()` helpers for the other, and the error when you pick wrong is unhelpful. [11](11-testing-and-local-dev.md) -**Nothing alerts on a backed-up queue.** `queue:monitor` only dispatches Laravel's `QueueBusy` event above a threshold, and no listener for it exists. There is no Horizon, and no job calls `onQueue()`, so all work shares one uninstrumented queue. In practice a stalled worker during October is noticed by a human wondering why activities are not appearing. A `QueueBusy` listener that notifies the on-call address is a small, high-value change. Related: both `ValidateBulkEventUploadJob` and `ProcessBulkEventImportJob` set `tries = 1`, so a transient failure in a long import is final and you re-upload. [10](10-scheduled-jobs-and-runbooks.md) +**Environment and infrastructure are invisible from here.** The `schedule:run` cron entry, the queue workers, the server `.env` and the nginx configuration exist only in Forge — no amount of reading this repository tells you whether they are correct, so export them during handover. Dev commonly shares `RESOURCES_BUCKET` with live, meaning a resource import on dev uploads real PDFs into the production bucket while the rows stay on dev. `config/codeweek.php` defaults `blog_url` to the live blog, so a dev `app:sync-blogs` pulls production content unless the variable is set. `devspace.yaml` still pins a PHP 8.0 FPM image while `composer.json` requires `^8.2`; we left the tag alone because we cannot verify what exists in that private registry. [02](02-environments-and-deployment.md) -**Environment and infrastructure.** The `schedule:run` cron entry, the queue workers, the server `.env` and the nginx configuration exist only in Forge — no amount of reading this repository tells you whether they are correct, so export them during handover. Dev commonly shares `RESOURCES_BUCKET` with live, meaning a resource import on dev uploads real PDFs into the production bucket while the rows stay on dev. `config/codeweek.php` defaults `blog_url` to the live blog, so a dev `app:sync-blogs` pulls production content unless the variable is set. `devspace.yaml` still pins a PHP 8.0 FPM image while `composer.json` requires `^8.2`; we left the tag alone because we cannot verify what exists in that private registry. [02](02-environments-and-deployment.md) +**Half-finished features.** `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET` and `AZURE_REDIRECT` are set in the deployed environment, but `LoginController` allowlists only twitter, github, google and facebook — finish it or remove the variables. `SCOUT_DRIVER` and the Algolia variables exist but nothing uses them: all search is direct MySQL, so do not assume an index exists, and expect search to be a pressure point in October. `EventsQuery::trigger()` tests `status = 'FEATURED'`, but `FEATURED` is a value of `highlighted_status`, so that branch can never match; it is harmless, and left alone because removing a condition that filters live listings deserves a look at production data first. `App\Http\Controllers\ModerationController` is an empty scaffold, and the `moderate event` permission is seeded for ambassadors but never checked — every moderation check tests the role name instead, so granting that permission to another role achieves nothing. -**Half-finished and architectural.** `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET` and `AZURE_REDIRECT` are set in the deployed environment, but `LoginController` allowlists only twitter, github, google and facebook — finish it or remove the variables. Nova resources must be registered manually in `NovaServiceProvider::boot()`, because auto-discovery was missing them; add a resource and forget the array and it will not appear. `SCOUT_DRIVER` and the Algolia variables exist but nothing uses them: all search is direct MySQL, so do not assume an index exists, and expect search to be a pressure point in October. `RejectEvent` in Nova has no message field, so the `Moderation` record it writes has an empty `message` and the organiser gets no explanation, while the Blade screens at `/pending` and `/review` do capture a reason — two paths, two behaviours, and ambassadors use both. `EventPolicy::edit()` returns false once `reported_at` is set, with no explanatory message in the UI; intentional, but a steady source of support tickets. +**Nova resources must be registered by hand.** Auto-discovery was missing them, so `NovaServiceProvider::boot()` lists them explicitly. Add a resource, forget the array, and it will not appear in the sidebar. [05](05-nova-admin.md) -**Navigational debt.** Of 151 files in `app/Console/Commands/`, only about twenty are scheduled or operationally relevant; the rest are partner-specific importers and historical backfills that will never run again. `app/Console/Commands/excel/` is the obvious first candidate for a cleanup pass. Both `relocate` and `relocate:country` resolve to `app/Console/Commands/RelocateCountry.php` and one is scheduled every two minutes — confirm with `php artisan schedule:list` on the server what actually binds and whether that frequency is still wanted. Many long-lived branches are named after dates or import batches (`bulk_11_11_25`, `2-sept-imports`, `coderdojo-import-april`), so do not assume an unfamiliar branch is active. CI triggers on `master` while `docs/ops/` refers to "`main` / `master`"; confirm in Forge which branch each site deploys from. +**`EventPolicy::edit()` returns false once `reported_at` is set**, with no explanatory message in the UI. Intentional, but a steady source of support tickets. + +**Import jobs do not retry.** Both `ValidateBulkEventUploadJob` and `ProcessBulkEventImportJob` set `tries = 1`, so a transient failure in a long import is final and you re-upload. [10](10-scheduled-jobs-and-runbooks.md) + +**Navigational debt.** Of 151 files in `app/Console/Commands/`, only about twenty are scheduled or operationally relevant; the rest are partner-specific importers and historical backfills that will never run again. `app/Console/Commands/excel/` is the obvious first candidate for a cleanup pass. `relocate` and `relocate:country` are genuinely different commands — the first repositions online activities stuck at `0,0`, the second re-geocodes activities sitting on a country centroid — and the second is scheduled every two minutes, so confirm with `php artisan schedule:list` on the server that the frequency is still wanted. Many long-lived branches are named after dates or import batches (`bulk_11_11_25`, `2-sept-imports`, `coderdojo-import-april`), so do not assume an unfamiliar branch is active. **Documentation.** `docs/internal/` is gitignored and holds sample payloads, partner spreadsheets and past import reports — genuinely useful reference material that must be transferred **outside git**. One inherited document, `WP5 CodeWeek IT System Merged v0.5.md`, states that geocoding uses Nominatim; it does not, the code calls the ArcGIS World GeocodeServer through `GeocodeController`. Worth knowing if that document is circulated to stakeholders. ## What was fixed -Done in a single pass, with the test suite green apart from the two pre-existing failures noted above. Listed so that references elsewhere make sense, and so nobody re-reports them. +The test suite is green. Listed so that references elsewhere make sense, and so nobody re-reports them. + +### Security and access + +- **The certificate backend was gated on one hardcoded personal email address**, with no role granting access, so a new super admin got a 403 and could not operate certificates at all. It is now an environment-driven allowlist that fails closed, covered by `tests/Feature/CertificateBackendAccessTest.php`. +- **Any ambassador could reject any country's activities.** `EventController@reject` called `$this->authorize()` inside a `try` with an empty `catch`, so the country check was defeated while the identical check on `approve` was enforced. The exception is no longer swallowed, and two tests pin in-country and out-of-country behaviour. +- **`users.approved` was mass-assignable**, so any future `update($request->all())` could have published or hidden a leading teacher. Now guarded. +- **A hardcoded `remember_token` and a predictable password** were used when creating the legacy placeholder user in `SoftDeleteUsersWithoutConsent`. Both are now random. +- **Turnstile bot protection never ran.** The code read `TURNSTILE_SECRET_KEY` while deployed environments set `TURNSTILE_SECRET`, so verification was wrapped in a truthiness check on an always-null variable and the CAPTCHA response was validated as `nullable`. The widget rendered, so it looked healthy. Both names now resolve through `config/codeweek.php`. +- **`.env.example` shipped a real `APP_KEY`** plus live S3 bucket names, used the ignored `QUEUE_DRIVER` name instead of `QUEUE_CONNECTION`, and omitted about forty variables the application reads. Blanked, renamed, and filled in by name. +- **Committed debris deleted**: `cookies.txt` (a curl cookie jar with a live session cookie for meet-and-code.org), `texput.log`, `changed_files.txt`, `differences.diff`, `bom.json`, `phpunit.xml.bak`, `tailwind.js`, `server.php`, the LaTeX compile leftovers in `resources/latex/`, and the abandoned Travis configuration. `.gitignore` now covers the recurring ones. + +### Correctness -- **Turnstile bot protection never ran.** The code read `TURNSTILE_SECRET_KEY` while deployed environments set `TURNSTILE_SECRET`, so verification was wrapped in a truthiness check on an always-null variable and the CAPTCHA response was validated as `nullable`. The widget rendered, so it looked healthy. Both names now resolve through `config/codeweek.php`. **This needs a live submission test on dev** — it is the one change here that alters behaviour on a public form. -- **The contact form fell back to the outgoing team's address** when `CONTACT_FORM_RECIPIENT_EMAIL` was unset. It now falls back to `ADMIN_EMAIL`. - **Every ambassador saw only France** in the Nova `Country` resource, which was leftover debug code. Now scoped to the ambassador's own `country_iso`. - **`Ambassador` filtered on `model_has_roles.role_id = 4`**, which only held while the seeders had run in their original order. Now matched by role name. -- **A scheduled command that did not exist**, `app:export-search-data-to-json`, failed nightly at 02:00. Removed. +- **Rejecting from Nova told the organiser nothing.** The action called `reject()` with no argument, writing an empty moderation message and emailing a rejection with no reason, while the Blade screens at `/pending` and `/review` captured one. The action now has a required reason field. +- **The contact form fell back to the outgoing team's address** when `CONTACT_FORM_RECIPIENT_EMAIL` was unset. It now falls back to `ADMIN_EMAIL`. +- **Country was optional on the profile despite the form marking it required**, and the error block beneath it was bound to a field name that does not exist, so the validation message could never appear. Both fixed. - **`certificate:preflight` defaulted to `--edition=2025`**, so a bare run silently checked the wrong year. Now defaults to the current year. -- **`.env.example` shipped a real `APP_KEY`** plus live S3 bucket names, used the ignored `QUEUE_DRIVER` name instead of `QUEUE_CONNECTION`, and omitted about forty variables the application reads. Blanked, renamed, and filled in by name. +- **A scheduled command that did not exist**, `app:export-search-data-to-json`, failed nightly at 02:00. Removed. +- **`ResourceEditorRoleSeeder` threw on any re-run**, because it used `Permission::create` and `Role::create` for a role the main seeder already creates. Now idempotent. - **Dead code deleted**: the two `nova-components/` packages that were never installed, `PromoteAmbassador`, the unattached `UserStatus` filter, the unrouted `ImporterController`, and the `/map` route whose only view include was a zero-byte file. -- **Committed debris deleted**: `cookies.txt` (a curl cookie jar with a live session cookie for meet-and-code.org), `texput.log`, `changed_files.txt`, `differences.diff`, `bom.json`, `phpunit.xml.bak`, `tailwind.js`, `server.php`, the LaTeX compile leftovers in `resources/latex/`, and the abandoned Travis configuration. `.gitignore` now covers the recurring ones. + +### Visibility and operations + +- **Leading teachers with no city were invisible on `/community`** with nothing to tell them why. The community map groups teachers by `city_id` and skips any group whose city has no coordinates, so they were rendered nowhere at all. Their profile now warns them, and the leading-teachers admin list has a City column and a **Not set** filter so an admin can find and chase them. [14](14-accounts-and-moderation.md) +- **Nothing alerted on a backed-up queue.** `queue:monitor` was not scheduled and no listener for Laravel's `QueueBusy` event existed, so a stalled worker during October was noticed by a human wondering why activities were not appearing. `queue:monitor` now runs every five minutes against a configurable threshold, and `App\Listeners\AlertOnBusyQueue` logs and reports to Sentry. [10](10-scheduled-jobs-and-runbooks.md) - **CI ran only for `master`**, so pull requests into `dev` ran no tests. Both branches now trigger. -- **The bulk upload column contract is pinned** by a unit test, so changing `REQUIRED_COLUMNS` fails the suite with a reminder to update the partner-facing wiki. +- **The suite had two failing tests, one flake, and five files that never ran.** `CityFactory` was still in pre-Laravel-8 format, and `UserRestoreServiceTest` never disabled the `support_gmail.dry_run` guard that refuses writes. `OnlineEventsWorkflowTest` failed about one run in four because its fixture sat exactly on the query's fifteen-day boundary. The five files in `tests/Feature/` that lacked the `Test.php` suffix were repaired — they had drifted against the factory API — and renamed, so they now run. [11](11-testing-and-local-dev.md) ## Suggested order of work -1. **Decide the certificate access model** and replace the hardcoded email. Until this is done you cannot operate certificates. -2. **Close the security actions.** Rotate the `forge` SSH keys, rotate `APP_KEY` where the committed one was used, and confirm `TURNSTILE_SECRET` is valid by submitting the contact form on dev. -3. **Verify the invisible parts.** Prove the scheduler runs, the workers are alive, a backup restores, and Sentry is receiving. Export the Forge configuration while you have someone to ask. -4. **Make the suite trustworthy.** Fix the two failing tests, then decide about the five inert files. A green suite is what makes everything after this cheaper. +1. **Set `CERTIFICATE_ADMIN_EMAILS`** in Forge for dev and live, and confirm you can reach the certificate backend. Nothing else about certificates matters until this is done. +2. **Close the credential actions.** Rotate the `forge` SSH keys, rotate `APP_KEY` where the committed one was used, and submit the contact form on dev to prove the Turnstile secret is valid. +3. **Verify the invisible parts.** Prove the scheduler runs, the workers are alive, a backup restores, and Sentry is receiving — you should now be able to trigger a queue alert deliberately. Export the Forge configuration while you have someone to ask. +4. **Decide the open questions.** Whether the public API stays unauthenticated, whether blog sync should prune, and whether to finish or remove the Azure login variables. Then, before October: the readiness checklist in [10](10-scheduled-jobs-and-runbooks.md). diff --git a/docs/handover/13-visual-tour.md b/docs/handover/13-visual-tour.md index f7941068f..4e20a2878 100644 --- a/docs/handover/13-visual-tour.md +++ b/docs/handover/13-visual-tour.md @@ -40,6 +40,8 @@ The ambassador and community directory. `/ambassadors` redirects here, so do not ![`GET /community` — `CommunityController@index`](assets/07-community.jpg) +This page generates more support tickets than any other, because the two halves of it hide people for entirely different reasons: ambassadors need a bio and an avatar, leading teachers need approval and a city. Both fail silently. [14](14-accounts-and-moderation.md) walks through the diagnosis. + ### Matchmaking tool Connects volunteers offering digital-skills help with schools and organisations asking for it. It has its own spreadsheet template download at `/matchmaking-tool/download/template`, which is a useful reminder that this feature has a bulk path as well as a form. @@ -94,6 +96,8 @@ Remember that `CheckConsent` middleware runs on every web request: a signed-in u ![The shared login and register screen. `/leading-teachers/list`, `/certificates` and `/participation` all land here when signed out](assets/09-login-gate.jpg) +For what happens after this screen — what a new account actually contains, what the profile form requires, and how someone becomes an ambassador or leading teacher — see [14](14-accounts-and-moderation.md). + ## Certificates Four certificate types are generated as real PDFs by `pdflatex` from the templates in `resources/latex/`, then stored on S3. [08 — Certificates](08-certificates.md) covers the pipeline, the TeX Live dependencies and the annual rollover in full. diff --git a/docs/handover/14-accounts-and-moderation.md b/docs/handover/14-accounts-and-moderation.md new file mode 100644 index 000000000..a07e25e5e --- /dev/null +++ b/docs/handover/14-accounts-and-moderation.md @@ -0,0 +1,377 @@ +# 14 — Accounts, profiles, and moderation + +This chapter follows a person from the moment they hit **Register** to the moment their activity is approved, and explains who is allowed to approve what. Read it alongside [04](04-domain-model.md) for the data model and [05](05-nova-admin.md) for the Nova side. + +It exists mainly because of one recurring support question — *"why am I not showing on the community page?"* — which has several different causes that look identical from the outside. That diagnosis is in [Why am I not on the community page?](#why-am-i-not-on-the-community-page) below. If you are picking up support duty, start there. + +## The shape of an account + +There is no separate "profile" record. A person is a single row in `users`, and everything — ambassador, leading teacher, activity organiser — is that same row with different roles and columns filled in. This matters: there is no onboarding wizard that guarantees a complete profile, so most accounts are partially filled in, and the public pages silently hide incomplete ones. + +## Registration + +Authentication is **Laravel UI** (`Auth::routes()`), not Breeze, Jetstream, or Fortify. Do not expect Fortify conventions. There are two front doors. + +### Front door 1: email and password + +`GET /register` → `POST /register`, handled by `App\Http\Controllers\Auth\RegisterController`. The whole of `Auth::routes()` sits behind Spatie's honeypot middleware, and the register form renders `@honeypot`. + +Only four things are validated: + +```50:56:app/Http/Controllers/Auth/RegisterController.php + return Validator::make($data, [ + 'name' => 'required|string|max:255', + 'email' => 'required|string|email|max:255|unique:users', + 'password' => ['required', 'string', Password::defaults()], + 'privacy' => 'required', + ]); + } +``` + +`Password::defaults()` is configured in `AppServiceProvider` as minimum 10 characters, mixed case, letters, numbers, symbols, and checked against the compromised-password list. That is stricter than most sites, and it is a common source of "I can't register" complaints. + +Note what happens to `name` — it becomes `firstname`, and `lastname` is deliberately set to an empty string: + +```63:70:app/Http/Controllers/Auth/RegisterController.php + $user = User::create([ + 'firstname' => $data['name'], + 'lastname' => '', + 'username' => '', + 'email' => $data['email'], + 'password' => Hash::make($data['password']), + 'privacy' => 1, + ]); +``` + +So **every new account starts with no last name, no country, no city, no bio, no avatar, no role, and `approved = false`.** The profile form is where all of that gets filled in, and nothing forces the user to go there. + +### Front door 2: social login + +`GET /login/{provider}` → `GET /login/{provider}/callback`. Four providers are permitted at runtime: `google`, `facebook`, `twitter`, `github`, configured in [config/services.php](../../config/services.php). Azure or Microsoft login does **not** exist despite occasional questions about it. + +`App\Services\SocialUserLoginService` matches on `(provider, provider_id)` first, then falls back to a case-insensitive email match, and only creates a row if neither matches: + +```36:47:app/Services/SocialUserLoginService.php + return User::create([ + 'email' => $oauthEmail, + 'password' => bcrypt(Str::random()), + 'firstname' => $socialUser->getName() ?: $socialUser->getNickname(), + 'lastname' => '', + 'username' => $socialUser->getNickname() ?: '', + 'provider' => $provider, + 'provider_id' => $providerId, + 'magic_key' => random_int(1000000, 2000000) * random_int(1000, 2000), + 'email_verified_at' => Carbon::now(), + ]); + } +``` + +Two consequences worth knowing: + +- Social accounts are **created already email-verified**, because `email_verified_at` is set at creation. They never see the verification screen. +- If the provider does not return an email address, the callback logs it, emails the admin address, and `abort(500)`s. A user reporting "I get an error page when logging in with Facebook" almost always means a Facebook account with no shared email. + +### Email verification + +`App\User` implements `MustVerifyEmail`, but the `verified` middleware is applied to only two routes: `GET /profile` and `GET /participation`. Everything else, including submitting an activity, works unverified. Verification links land back on `/profile`. + +### The consent gate + +Every authenticated web request passes through `App\Http\Middleware\CheckConsent`: + +```18:22:app/Http/Middleware/CheckConsent.php + if (Auth::check() && !Auth::user()->hasGivenConsent()) { + if (!in_array($request->route()->getName(), $excludedRoutes)) { + return redirect()->route('consent.show'); + } + } +``` + +Anyone whose `consent_given_at` is null is redirected to `/consent` on every page until they accept. Declining logs them out. If a user reports being "stuck in a loop" or "always sent to the same page", this is why — including for social logins, which skip verification but not consent. + +## The profile + +| Purpose | Route | Middleware | +|---------|-------|------------| +| View and edit own profile | `GET /profile` | `auth`, `verified` | +| Save profile | `PATCH /user` (`user.update`) | `auth` | +| Delete own account | `GET /user/delete` | `auth` | +| Change login email | `POST /user/email-change/*` | `auth` | +| Upload avatar | `POST /api/users/{user}/avatar` | `auth` | + +`GET /profile` is a closure in [routes/web.php](../../routes/web.php) that passes the logged-in user to the `profile` view as `$profileUser`, with cache headers set to `no-store`. There is no `ProfileController`. + +**There is no public profile page for an individual.** Members are only ever exposed through the `/community` listings and the role-restricted `/badges/user/{user}` page. `AmbassadorController::profile()` exists but has no route pointing at it — dead code, do not rely on it. + +### What the form validates + +```21:33:app/Http/Controllers/UserController.php + $user->update(request()->validate([ + 'firstname' => 'required|string', + 'lastname' => 'required|string', + 'privacy' => 'required', + 'receive_emails' => 'required', + 'country_iso' => 'required|exists:countries,iso', + 'city_id' => 'nullable|exists:cities,id', + 'twitter' => 'nullable', + 'website' => 'nullable', + 'bio' => 'nullable', + 'email_display' => 'nullable|email', + 'tag' => 'nullable', + ])); +``` + +A few things to note: + +- **City is optional.** This is the root of the community-page problem below. It is deliberate — plenty of members have no reason to publish a city — but it has visible consequences for leading teachers. +- **Country is required**, and matches the asterisk on the form label. It was previously `nullable` while the label claimed otherwise, so the field could be silently saved empty; see [12](12-risks-and-known-issues.md). +- `bio` has no length rule here, but the column is `varchar(2500)`. A longer bio fails at the database, not in validation. +- The login email is **not** in this list and cannot be changed here. Email changes go through the separate confirm-by-signed-link flow in `UserEmailChangeController`. +- `email_display` is a different, optional, public-facing address. + +### City selection + +City is a plain `
- @component('components.validation-errors', ['field'=>'country'])@endcomponent + @component('components.validation-errors', ['field'=>'country_iso'])@endcomponent
+ @if ($profileUser->hasRole('leading teacher') && is_null($profileUser->city_id)) +

+ @lang('base.city_required_for_community_map') +

+ @endif