feat(routes): Auto-register Apple Sign In routes and controller#66
feat(routes): Auto-register Apple Sign In routes and controller#66mikebronner merged 5 commits intomasterfrom
Conversation
mikebronner
left a comment
There was a problem hiding this comment.
Review Checklist
The following items need to be addressed before this is ready:
- Callback controller returns raw Socialite user —
AppleSignInController::callback()returns the raw$userobject, which serializes to JSON. This isn't practical as a default behavior. Consider redirecting to a configurable route (e.g.,config('services.sign_in_with_apple.routes.callback_redirect', '/')) after firing an event, or at minimum document that this controller must be overridden. - Missing test for disabling routes — AC requires: "config option to disable package routes works correctly." The tests verify routes are registered but never test
'routes.enabled' => false. Add a test that setsservices.sign_in_with_apple.routes.enabledtofalseand verifies the routes are not registered. - Missing regression test for controller override — AC requires: "apps that override controllers still function." No test covers binding a custom controller to the route names.
- Route customization tests don't actually test customization —
testRedirectRoutePathCanBeCustomizedandtestCallbackRoutePathCanBeCustomizedonly assert the default config values. They don't set a custom path and verify the route uses it.
AC Verified:
- ✅ Default routes auto-registered via ServiceProvider
- ✅ Default controller in package
- ✅ Config option exists to disable routes
- ✅ Routes use web middleware
⚠️ Test coverage gaps noted above
Code quality observations (non-blocking):
loadRoutesIf()pattern is clean- Route file is well-structured with configurable paths
- Controller is minimal and focused
mikebronner
left a comment
There was a problem hiding this comment.
Review Checklist
-
Callback route needs CSRF exclusion. Apple sends a form POST to the callback. The route has
webmiddleware (which includesVerifyCsrfToken), so the callback will 419 unless CSRF is bypassed. TheDisableCsrfForAppleCallbackmiddleware exists but isn't applied to this route. Either add it to the route definition or usewithoutMiddleware. -
Tests for disabling routes are missing. AC says "config option to disable package routes works correctly" — need a test that sets
routes.enabled = falseand asserts the routes are not registered. -
Tests for custom route paths don't actually test customization.
testRedirectRoutePathCanBeCustomizedandtestCallbackRoutePathCanBeCustomizedonly assert the default config values. They need to change the config, re-register routes, and verify the new paths are used. -
Regression test for controller overrides is missing. AC says "apps that override controllers still function" — need a test demonstrating an app can bind its own controller to the routes.
-
Default
callback()returns a raw Socialite user. For a "zero-config" package, returning a SocialiteUserobject from a web route isn't useful — it'll dump a JSON blob. Consider at least redirecting to a configurable route with the user data flashed to session, or documenting very clearly in the README that apps must override the callback.
- Add AppleSignInController with redirect and callback actions - Auto-register routes (apple/redirect and apple/callback) via ServiceProvider - Routes can be disabled or customized via config - Apple callback expects form POST with user data - All AC boxes complete: routes auto-register, paths customizable, can be disabled via config
…aw user - Callback now dispatches AppleSignInCallback event with Socialite user - Redirects to configurable 'routes.callback_redirect' (defaults to '/') - Apps listen for the event to persist/process the authenticated user - Add callback_redirect to default config
32582fa to
2722ba8
Compare
mikebronner
left a comment
There was a problem hiding this comment.
Review Complete ✅
All acceptance criteria verified against the actual implementation:
- Auto-registered routes —
routes/web.phpregistersapple/redirect(GET) andapple/callback(POST) vialoadRoutesFromin the ServiceProvider - Package controllers —
AppleSignInControllerhandles redirect (Socialite driver) and callback (dispatchesAppleSignInCallbackevent, redirects to configurable URL) - Config override/disable —
routes.enabled,redirect_route,callback_route, andcallback_redirectall configurable - Migration path — Event-based callback (
AppleSignInCallback) decouples user persistence; apps override by registering their own routes - CSRF exclusion — Callback route correctly excludes
VerifyCsrfTokenmiddleware (Apple sends a form POST)
Tests: 39 passing (0 failures). Route registration, disable toggle, custom paths, controller override regression, CSRF exclusion — all covered.
CI: GitHub Actions workflow added for PHP 8.2–8.4 × Laravel 12.
No issues found. Ready for merge.
Summary
Moves default routes and controllers into the package for zero-config setup. Routes are auto-registered via the ServiceProvider and can be customized or disabled via config. After setup, users only need to set environment variables.
Changes
AppleSignInCallbackevent and redirects to a configurable URL (routes.callback_redirect, defaults to/). Apps listen for the event to persist users.VerifyCsrfTokenmiddleware since Apple sends a form POST.AppleSignInCallback— dispatched with the Socialite user on successful callback.callback_redirectoption toservices.sign_in_with_apple.routes.routes.enabled = false)Acceptance Criteria
apple/redirect,apple/callback) are moved into the package and auto-registeredTest Coverage
Fixes #26