You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
test(auth): add a Behat suite and CI workflow - #61
Adds a Behat suite for auth-command (features/: 156 scenarios, outline rows counted) and a Behat workflow that runs it on every pull request, on pushes to develop and on demand.
It covers command usage and site resolution, site auth, IP whitelists, auth across the site lifecycle, global auth, admin-tools auth and the auth files migration. features/COVERAGE.md maps each command, flag and behaviour to its scenarios.
The workflow runs the suite inside a fresh core checkout: core develop is cloned, every easyengine/* package is set to dev-develop, and this package is installed from the pull request's checkout through a Composer path repository, so the code under test is the PR's, not Packagist's.
It runs on PHP 7.4 and 8.5. @destructive scenarios run there too, since the runner is thrown away. Auth state is snapshotted and restored exactly around the scenarios that change global auth.
Scenarios written for open bugs are left out for now: they assert the fixed behaviour, so each comes back with its fix. Every scenario in this PR gates, and COVERAGE.md lists the bugs that aren't covered yet.
To run it locally (as root, on a test machine, since the suite creates and deletes *.test sites): replace a core checkout's features/ with this package's (rm -rf features && cp -R <package>/features features, as the workflow does; copying over it would keep core's own cli.feature, whose scenario runs ee cli self-uninstall --yes) and run sudo -E vendor/bin/behat from the core root; add --tags='~@destructive' to skip the scenarios that change global services or config.
Copy site-command's FeatureContext and add generic steps for command results, JSON output, sqlite rows, files, htpasswd verification, HTTP status checks and site setup. Anchor the 'I run' pattern, only clean up bh-auth-command-* sites, and snapshot/restore global auth state around @destructive scenarios.
Only Runner::init_ee() defines it, so every docker exec helper (htpasswd checks, admin-tools setup, the global state restore and proxy reload) hit an undefined constant: a fatal Error on PHP 8, a call to a container named EE_PROXY_TYPE on PHP 7.4.
…nder test
The @destructive restore rewrote only the global rows and regenerated files with develop's layout, after resetting test sites through `ee auth delete` while the scenario's global users and IPs were still in place. Test site files then kept those users/IPs into later scenarios, alias copies written by the wildcard work (EasyEngine#57) kept a known test password on real sites, the early return on an unchanged DB skipped the files after a failed restore, and a failed restore became the next scenario's snapshot.
Now the snapshot holds every non-test auth row plus the bytes and modes of htpasswd/* and vhost.d/*_acl, is saved to /opt/easyengine until restored (a killed run or failed restore is repaired before the next scenario), and is put back with PDO and the filesystem, followed by a strict proxy reload. Test sites and the "no global auth" setup are reset directly, and non-@destructive scenarios can't run `ee auth … global`. The ee-based AfterFeature safety net is gone.
Make the global list count deterministic, include the _acl file in the AUTH-13 scenario so nginx really parses it, cover invalid IPs on delete and update --ip on a disabled site, check auth_ips there too, reference AUTH-12..18 and mark the scenarios that pass with EasyEngine#57. AUTH-20 was refuted: every site's nginx container sets VIRTUAL_PATH=/, so /ee-admin/ always gets the admin-tools auth.
… against global auth
On a fresh host the first ee site create adds the admin-tools auth and the frontend-subnet whitelist; snapshotting before the Background made the restore remove them for good. The snapshot is now taken before the first step that isn't a site-exists step. A read-only step checks that non-destructive features run without global auth (their 401 checks would pass vacuously), and the public IP must be IPv4.
…en weak checks
Piped ee output uses the tab-separated table renderer, so the ASCII-pipe regexes never matched. Help is pinned to COLUMNS=200 and PAGER=cat. The URL-form scenario fails on develop (LOGCMD-8) and is now @known-bug. Add the missing positive checks (global users in the site file, site _acl after a global IP delete, removed IP after delete --ip) and setup exit codes.
…l path
The site's app/ directory is a symlink to the docker volume, and proc_open's cwd leaves PWD pointing at Behat's own directory, so ee's `pwd` saw the physical volume path and cwd site detection failed.
EasyEngine#57 is on develop now: AUTH-11 (site delete removes the _acl file) and AUTH-12 (shell metacharacters in passwords) pass, so they become regular regression scenarios, and a site without own users or IPs no longer keeps an htpasswd/_acl file (5f12c49), so the two scenarios that asserted the old files check that the files are gone.
…upgrade
With the registry blocked the upgrade fails after the auth migration ran: the old files must be back and the migration unrecorded; the next attempt writes the alias and _wildcard files.
…valid stored entries
Adds the older-version case: an unchecked whitelist entry is skipped when the _acl file is written, can still be deleted, and is dropped from existing _acl files by the upgrade migration.
…lobal auth
The AUTH-8 and AUTH-17 scenarios become regular ones, "Global create absorbs…" goes, a global easyengine user is expected to coexist with the admin-tools one, and the admin-tools password is expected to survive `ee auth delete global`; a new @admin-tools scenario keeps the regeneration for admin tools that used the global auth.
Run PHP 7.4 and 8.5, strip control characters from the junit files before building failures.txt, and cap the Behat step at 75 minutes so a hung run still uploads its logs. Diagnostics: the host addresses and routes, and the docker-compose version after the first run (MIG-20).
EE's first run downloads docker-compose 2.27.0 without --fail (MIG-10); on a GitHub error it moves the HTML error page over the binary, as on an admin-tools-command runner. Fetch 2.27.0 again with --fail and retries in that case.
…ng default.conf
The reset hook regenerates default.conf and runs nginx -t, but the proxy's own docker-gen watcher can be writing the same file then, and nginx -t fails on the truncated config (seen once on PHP 7.4 after all scenarios passed). The reload now retries the sequence up to five times.
The 5 scenario rows written for open bugs assert the fixed behaviour and fail today, so they stay out of the suite until their fixes land, and COVERAGE.md lists those bugs as not covered yet. Every scenario that ships now gates, and the non-gating known-bug CI lane goes with them.
The AfterFeature cleanup deleted ee.phar and ee-old.phar from the core checkout after every feature, although no scenario here builds them, so a local run removed a developer's own builds. The 'ee phar is generated' step now records that it built them, and the cleanup only removes them then.
Replace internal tracker IDs and local paths in the coverage notes and test comments with short descriptions of each bug or links to the pull requests that fixed it.
…line output
compareContents() compared list elements by position, so [1,3,5] wasn't found in [1,2,3,4,5], and a missing key or property raised a notice. The indexed exact-output step and both something-like steps only used the first line of the expected text.
…ailure reports
The htpasswd check and the authenticated curl requests passed the password to EE::launch() unmasked, so the machine's real admin-tools password was written to ee.log, and a failed htpasswd check printed it with the command. Both now pass it as an obfuscated value, and the command kept in the result is masked.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Failure diagnostics can expose retained plaintext credentials through database dumps and command output.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Redact credentials from describe() failure reports
features/bootstrap/FeatureContext.php:1281
describe() includes unredacted command output in failure reports. For example, if the final JSON assertion in “Global create keeps the admin-tools auth separate” fails, it prints the full ee auth list global response, including the machine's existing admin-tools password. This is a separate leak from command arguments and database dumps. Redact credentials when rendering failure diagnostics, while preserving the original output for assertions.
Redact passwords from auth_users assertion diagnostics
features/bootstrap/FeatureContext.php:1484
A failed auth_users row or count assertion dumps the entire table, including plaintext passwords for existing admin-tools/global accounts and unrelated sites. In the documented local run, those credentials remain valid after cleanup but are exposed in Behat's console and reports. Redact password fields before serializing diagnostic rows; the htpasswd/curl command masking does not protect this output path.
…dumps
describe() printed command output unredacted, so a failed check after `ee auth list global --format=json` showed the machine's admin-tools password, and a failed auth_users assertion dumped every row with its plaintext password. Passwords of rows outside the suite's sites are now collected before each scenario and in the snapshot, and masked in both.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Credential logging and an overly broad migration whitelist fixture remain unresolved.
No open issues 🎉
2 previously missed issues
In code that hasn't changed since last review
Use /024 to prevent LAN address whitelisting
features/auth-migration.feature:64
When {PUBLIC_IP} is a 192.168.x.x LAN address, this /08 entry normalizes to /8 and whitelists the client. The unauthenticated request at line 67 then returns 200 instead of 401 even when the migration works correctly. Use /024 and expect /24 to preserve the leading-zero regression check without whitelisting addresses outside the documentation subnet.
Redact admin passwords before logging listing output
features/bootstrap/FeatureContext.php:144
The table, CSV and YAML listings in auth-site.feature:296–315 include the machine's existing admin-tools password. EE::launch() logs captured STDOUT, but core's pattern masking does not cover these listing formats, and this context's redact() only protects failure reports. Thus even a successful local run with ~@destructive writes a still-valid password to ee.log. Refresh the known passwords before launching and pass them as the fourth argument, as the htpasswd/curl helpers already do.
…fixture
EE::launch() logs each command's STDOUT, and `ee auth list` prints the machine's real admin-tools and global passwords in table, CSV and YAML forms that core's masking doesn't cover; the snapshot restore also passed them to htpasswd unmasked. The suite's ee runs and the restore now pass the known passwords as obfuscated values. The leading-zero migration fixture /08 normalized to /8, which whitelists a 192.168.x.x client and breaks the 401 check; it is /024 now.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Privileged authentication-state restoration and host-networking changes warrant human validation of failure recovery.
No open issues 🎉
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a Behat suite for auth-command (
features/: 156 scenarios, outline rows counted) and aBehatworkflow that runs it on every pull request, on pushes todevelopand on demand.It covers command usage and site resolution, site auth, IP whitelists, auth across the site lifecycle, global auth, admin-tools auth and the auth files migration.
features/COVERAGE.mdmaps each command, flag and behaviour to its scenarios.developis cloned, everyeasyengine/*package is set todev-develop, and this package is installed from the pull request's checkout through a Composer path repository, so the code under test is the PR's, not Packagist's.@destructivescenarios run there too, since the runner is thrown away. Auth state is snapshotted and restored exactly around the scenarios that change global auth.To run it locally (as root, on a test machine, since the suite creates and deletes
*.testsites): replace a core checkout'sfeatures/with this package's (rm -rf features && cp -R <package>/features features, as the workflow does; copying over it would keep core's owncli.feature, whose scenario runsee cli self-uninstall --yes) and runsudo -E vendor/bin/behatfrom the core root; add--tags='~@destructive'to skip the scenarios that change global services or config.Green run on my fork: https://github.com/mrrobot47/auth-command/actions/runs/39294480957