From 408d75dda10cf995a9669f8e876147600c6fdb19 Mon Sep 17 00:00:00 2001 From: Lyra Bot Date: Tue, 11 Aug 2026 16:37:23 -0400 Subject: [PATCH] chore: nice-to-have improvements for v1.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code quality: - Create AppConstants class with shared constants: - FAR_FUTURE_DATE (replaces duplicated '2999-12-31' strings) - MAX_INPUT_LENGTH (replaces duplicated 128 in Payload and StringTrait) - Extract duplicated 'hi $id' response body into StringTrait::authSuccessResponse() method, used by AcceptListener, AllowListener, and LoginManager - Add missing @throws InvalidArgumentException annotations to MonitorCacheKeys (getItem, hasItem, deleteItem, deleteItems, commit) Configuration: - Add proper env var type casting in services.yaml: - COOKIE_TTL → env(int:) - SUBDOMAIN_REDIRECT → env(bool:) - IP_TTL → env(int:) - TEAPOT → env(bool:) Documentation: - Create CONTRIBUTING.md with development setup, code style, testing guidelines, and PR process --- CONTRIBUTING.md | 74 +++++++++++++++++++++++++++++++ config/services.yaml | 8 ++-- src/AppConstants.php | 25 +++++++++++ src/Data/Payload.php | 7 +-- src/Listener/AcceptListener.php | 6 +-- src/Listener/AllowListener.php | 6 +-- src/MonitorCacheKeys.php | 5 +++ src/Service/BackupCodeManager.php | 5 ++- src/Service/LoginManager.php | 5 +-- src/Trait/StringTrait.php | 17 ++++++- src/Utilities.php | 2 +- 11 files changed, 135 insertions(+), 25 deletions(-) create mode 100644 CONTRIBUTING.md create mode 100644 src/AppConstants.php diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..3100f7a --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,74 @@ +# Contributing to Preauth + +Thank you for your interest in contributing to Preauth! This document +outlines the process for contributing to the project. + +## Development Setup + +1. Clone the repository +2. Install dependencies: `composer install` +3. Copy `.env.example` to `.env` and configure as needed +4. Run tests: `vendor/bin/phpunit` + +## Code Style + +This project follows [PSR-12](https://www.php-fig.org/psr/psr-12/) and +includes `php-cs-fixer` as a dev dependency. + +```bash +# Check for style violations +vendor/bin/php-cs-fixer fix --dry-run --diff + +# Auto-fix +vendor/bin/php-cs-fixer fix +``` + +All code must pass the style check before it can be merged. + +## Testing + +All code changes must include tests. The project maintains 100% code +coverage — new code must be fully tested. + +```bash +# Run tests +vendor/bin/phpunit + +# Run with coverage (requires Xdebug) +XDEBUG_MODE=coverage vendor/bin/phpunit --coverage-text +``` + +### Test Structure + +- **Unit tests** go in `tests/Unit/` and mirror the `src/` directory structure +- **Functional tests** go in `tests/Functional/` and test the full HTTP kernel +- Use the support traits (`TotpTestHelper`, `ListenerTestHelper`) for + reusable test fixtures + +## Pull Request Process + +1. Create a feature branch from `main` +2. Make your changes, ensuring tests pass and code style is clean +3. Update documentation if needed (README, CHANGELOG, docs/) +4. Submit a pull request to `main` + +### Commit Messages + +Use conventional commit format: + +- `feat:` new feature +- `fix:` bug fix +- `docs:` documentation only +- `refactor:` code change that neither fixes a bug nor adds a feature +- `test:` adding or correcting tests +- `chore:` build process, tooling, etc. + +## Architecture + +Preauth is an event-listener-driven Symfony application (no controllers). +See `ROADMAP.md` for the full architecture overview and design decisions. + +## License + +By contributing, you agree that your contributions will be licensed under +the MIT License. diff --git a/config/services.yaml b/config/services.yaml index 844f338..8b85eaa 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -56,12 +56,12 @@ parameters: # --- application variables --- app.totp_uri: '%env(TOTP_URI)%' - app.cookie_ttl: '%env(COOKIE_TTL)%' - app.subdomain_redirect: '%env(SUBDOMAIN_REDIRECT)%' + app.cookie_ttl: '%env(int:COOKIE_TTL)%' + app.subdomain_redirect: '%env(bool:SUBDOMAIN_REDIRECT)%' app.auth_subdomain: '%env(AUTH_SUBDOMAIN)%' - app.ip_ttl: '%env(IP_TTL)%' - app.teapot: '%env(TEAPOT)%' + app.ip_ttl: '%env(int:IP_TTL)%' + app.teapot: '%env(bool:TEAPOT)%' app.error_message: '%env(ERROR_MESSAGE)%' app.teapot_title: '%env(TEAPOT_TITLE)%' diff --git a/src/AppConstants.php b/src/AppConstants.php new file mode 100644 index 0000000..8bbe172 --- /dev/null +++ b/src/AppConstants.php @@ -0,0 +1,25 @@ +id = mb_substr(trim($data->id), 0, 128); - $payload->nonce = mb_substr(trim($data->nonce), 0, 128); + $payload->id = mb_substr(trim($data->id), 0, AppConstants::MAX_INPUT_LENGTH); + $payload->nonce = mb_substr(trim($data->nonce), 0, AppConstants::MAX_INPUT_LENGTH); $payload->json = ($data->json ?? true); $payload->scope = Scope::tryFrom($data->scope ?? '') ?? Scope::Cookie; - $payload->token = mb_substr(trim($data->token), 0, 128); + $payload->token = mb_substr(trim($data->token), 0, AppConstants::MAX_INPUT_LENGTH); return Payload::constrict($payload); } diff --git a/src/Listener/AcceptListener.php b/src/Listener/AcceptListener.php index 9df9f17..002d80a 100644 --- a/src/Listener/AcceptListener.php +++ b/src/Listener/AcceptListener.php @@ -11,7 +11,6 @@ use App\Trait\StringTrait; use Psr\Cache\CacheItemPoolInterface; use Psr\Cache\InvalidArgumentException; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; -use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\RequestEvent; final readonly class AcceptListener @@ -52,10 +51,7 @@ final readonly class AcceptListener $id = $item->get(); $this->logger->debug("has valid cookie-session: $id"); - $event->setResponse(new Response("hi $id", headers: [ - 'Content-Type' => 'text/plain', - 'Remote-User' => $id, - ])); + $event->setResponse($this->authSuccessResponse($id)); } catch (InvalidArgumentException $e) { /* cache failure — fail closed (don't authenticate) */ $this->logger->error("cache error in AcceptListener: {$e->getMessage()}"); diff --git a/src/Listener/AllowListener.php b/src/Listener/AllowListener.php index a524a23..7651524 100644 --- a/src/Listener/AllowListener.php +++ b/src/Listener/AllowListener.php @@ -10,7 +10,6 @@ use App\Trait\StringTrait; use Psr\Cache\CacheItemPoolInterface; use Psr\Cache\InvalidArgumentException; use Symfony\Component\EventDispatcher\Attribute\AsEventListener; -use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\RequestEvent; final readonly class AllowListener @@ -47,10 +46,7 @@ final readonly class AllowListener $id = $item->get(); $this->logger->debug("has valid ip-session: $id"); - $event->setResponse(new Response("hi $id", headers: [ - 'Content-Type' => 'text/plain', - 'Remote-User' => $id, - ])); + $event->setResponse($this->authSuccessResponse($id)); } catch (InvalidArgumentException $e) { /* cache failure — fail closed (don't authenticate) */ $this->logger->error("cache error in AllowListener: {$e->getMessage()}"); diff --git a/src/MonitorCacheKeys.php b/src/MonitorCacheKeys.php index 955c817..8497428 100644 --- a/src/MonitorCacheKeys.php +++ b/src/MonitorCacheKeys.php @@ -67,6 +67,7 @@ final readonly class MonitorCacheKeys implements CacheItemPoolInterface $this->cache->save($changeList); } + /** @throws InvalidArgumentException */ public function getItem(string $key): CacheItemInterface { return $this->cache->getItem($key); @@ -79,6 +80,7 @@ final readonly class MonitorCacheKeys implements CacheItemPoolInterface return $this->cache->getItems($keys); } + /** @throws InvalidArgumentException */ public function hasItem(string $key): bool { return $this->cache->hasItem($key); @@ -97,6 +99,7 @@ final readonly class MonitorCacheKeys implements CacheItemPoolInterface return true; } + /** @throws InvalidArgumentException */ public function deleteItem(string $key): bool { $this->isValid($key); @@ -113,6 +116,7 @@ final readonly class MonitorCacheKeys implements CacheItemPoolInterface return $this->cache->deleteItem($key); } + /** @throws InvalidArgumentException */ public function deleteItems(array $keys): bool { $this->allValid($keys); @@ -145,6 +149,7 @@ final readonly class MonitorCacheKeys implements CacheItemPoolInterface return $this->cache->saveDeferred($item); } + /** @throws InvalidArgumentException */ public function commit(): bool { return $this->cache->commit(); diff --git a/src/Service/BackupCodeManager.php b/src/Service/BackupCodeManager.php index cf4d13b..2858d6d 100644 --- a/src/Service/BackupCodeManager.php +++ b/src/Service/BackupCodeManager.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Service; +use App\AppConstants; use App\MonitorCacheKeys; use App\Trait\HasLoggerTrait; use App\Trait\StringTrait; @@ -84,7 +85,7 @@ final readonly class BackupCodeManager implements BackupCodeInterface * we want this to keep forever, so a few hundred years should do it */ $backupItem->expiresAt(DateTimeImmutable::createFromFormat( 'Y-m-d', - '2999-12-31' + AppConstants::FAR_FUTURE_DATE )); $this->sessionCache->save($backupItem); @@ -104,7 +105,7 @@ final readonly class BackupCodeManager implements BackupCodeInterface * we want this to keep forever, so a few hundred years should do it */ $backupItem->expiresAt(DateTimeImmutable::createFromFormat( 'Y-m-d', - '2999-12-31' + AppConstants::FAR_FUTURE_DATE )); $this->sessionCache->saveDeferred($backupItem); } diff --git a/src/Service/LoginManager.php b/src/Service/LoginManager.php index e63ece8..8db61a6 100644 --- a/src/Service/LoginManager.php +++ b/src/Service/LoginManager.php @@ -63,10 +63,7 @@ final readonly class LoginManager implements LoginInterface $cleanId = $this->makeCacheKey($payload->id); /* if they just want this one page, return ok, to grant them access */ - $response = new Response("hi $cleanId", headers: [ - 'Content-Type' => 'text/plain', - 'Remote-User' => $cleanId, - ]); + $response = $this->authSuccessResponse($cleanId); if ($payload->scope !== Scope::None) { /* grant access based on the requested scope */ diff --git a/src/Trait/StringTrait.php b/src/Trait/StringTrait.php index 72e32bb..88797f9 100644 --- a/src/Trait/StringTrait.php +++ b/src/Trait/StringTrait.php @@ -4,6 +4,9 @@ declare(strict_types=1); namespace App\Trait; +use App\AppConstants; +use Symfony\Component\HttpFoundation\Response; + trait StringTrait { /* cache keys can safely use alphanumeric, "_", and ".", remove the rest */ @@ -11,6 +14,18 @@ trait StringTrait public function makeCacheKey(string $name): string { - return mb_substr(preg_replace(static::KEY_REGEX, '_', $name), 0, 128); + return mb_substr(preg_replace(static::KEY_REGEX, '_', $name), 0, AppConstants::MAX_INPUT_LENGTH); + } + + /** + * Build the plain-text success response body and headers for an authenticated request. + * The body is a simple greeting that includes the session id. + */ + public function authSuccessResponse(string $id): Response + { + return new Response("hi $id", headers: [ + 'Content-Type' => 'text/plain', + 'Remote-User' => $id, + ]); } } diff --git a/src/Utilities.php b/src/Utilities.php index 4aac02c..1bbd503 100644 --- a/src/Utilities.php +++ b/src/Utilities.php @@ -47,7 +47,7 @@ final readonly class Utilities * we want this to keep forever, so a few hundred years should do it */ $totpItem->expiresAt(DateTimeImmutable::createFromFormat( 'Y-m-d', - '2999-12-31' + AppConstants::FAR_FUTURE_DATE )); $this->appPool->save($totpItem); return $totp;