Skip to content

feat: scaffold marko oauth package#57

Open
ps-carvalho wants to merge 1 commit intomarko-php:developfrom
ps-carvalho:feature/marko-oauth-package
Open

feat: scaffold marko oauth package#57
ps-carvalho wants to merge 1 commit intomarko-php:developfrom
ps-carvalho:feature/marko-oauth-package

Conversation

@ps-carvalho
Copy link
Copy Markdown
Contributor

@ps-carvalho ps-carvalho commented May 4, 2026

Summary

Adds the first implementation slice for marko/oauth, a native Marko OAuth2 authorization server package inspired by Laravel Passport but shaped around Marko's module, database, config, CLI, packaging, docs, and testing conventions.

This PR intentionally establishes the package foundation before implementing the actual OAuth grant controllers and league/oauth2-server adapters. The goal is to land the package boundary, PRD, storage model, config surface, key-management command, package metadata, docs entry, and tests as a stable base for the next slices.

Included in this slice:

  • Adds packages/oauth as a Marko module with Composer metadata for marko/oauth.
  • Adds dependencies on league/oauth2-server, PSR-7 bridge packages, Marko authentication, config, database, routing, and view packages.
  • Adds default config/oauth.php for routes, signing keys, token TTLs, refresh-token rotation, consent memory, scopes, and defaults.
  • Adds OAuthConfig typed accessors over the package config.
  • Adds OAuth persistence entities for clients, auth codes, access tokens, refresh tokens, approvals, and scopes.
  • Adds Marko repository interfaces and concrete repository classes for each OAuth entity, bound in module.php.
  • Adds oauth:keys via KeysCommand and KeyGenerator, with overwrite protection unless --force is passed and explicit Marko exceptions for key write/directory failures.
  • Adds #[RequiresScope] as the route-level scope declaration planned for OAuth-protected APIs.
  • Adds the GrantType enum for the v1 grant boundary: authorization code, client credentials, refresh token.
  • Adds docs/prd/marko-oauth.md capturing the product and implementation decisions from the design discussion.
  • Adds the public docs page at docs/src/content/docs/packages/oauth.md and keeps the package README as a slim installation/docs pointer.
  • Adds package distribution metadata: .gitattributes, package LICENSE, issue-template package dropdown entries, and package-structure tests.
  • Wires the package into root monorepo Composer path repositories, root require metadata, root test autoload, root README package list, and root Composer metadata tests.
  • Fixes the repo-management script test expectation to match the current optimized gh repo list implementation from develop.

The PRD documents the larger intended v1 scope: authorization-code with PKCE, client credentials, refresh-token rotation/reuse detection, signed JWT access tokens, database-backed revocation/audit, consent UI, configured scopes, and bearer-token protection. Those protocol flows are left as follow-up work on top of this foundation.

Type of Change

  • Bug fix
  • New feature
  • Docs
  • Breaking change
  • Refactor

Related Issues

N/A. The PRD is included in this branch at docs/prd/marko-oauth.md.

Verification

  • composer test
  • ./vendor/bin/pest packages/oauth/tests tests/RepoManagementScriptsTest.php packages/framework/tests/RootComposerJsonTest.php tests/PackagingTest.php tests/IntegrationVerificationTest.php --exclude-group=integration-destructive
  • ./vendor/bin/phpcs packages/oauth/src packages/oauth/tests packages/oauth/module.php tests/RepoManagementScriptsTest.php
  • ./vendor/bin/php-cs-fixer fix packages/oauth/src packages/oauth/tests --dry-run --diff --config=.php-cs-fixer.php
  • composer validate --strict --no-check-publish --no-check-lock
  • composer validate packages/oauth/composer.json --strict --no-check-publish --no-check-lock
  • npm --prefix docs ci
  • npm --prefix docs run build

Notes:

  • The local PHP CLI does not have the root-required ext-imagick extension enabled. I refreshed ignored local Composer artifacts with --ignore-platform-req=ext-imagick before running the full suite; composer.lock and vendor/ are ignored and are not part of this PR.
  • The docs build succeeds and reports existing warnings for unsupported latte/env code-block highlighters in unrelated docs pages.

Checklist

  • Tests pass, including full composer test
  • Lint passes for added package files and touched repo-management test
  • Composer manifests validate
  • Docs build passes
  • Package has README, LICENSE, .gitattributes, issue-template entries, and docs page
  • Follows Marko module and package standards

@github-actions github-actions Bot added enhancement New feature or request breaking Introduces a breaking change labels May 4, 2026
@ps-carvalho ps-carvalho force-pushed the feature/marko-oauth-package branch from bf76b1c to c65a174 Compare May 4, 2026 12:07
@ps-carvalho ps-carvalho force-pushed the feature/marko-oauth-package branch from c65a174 to 5c79a54 Compare May 4, 2026 12:38
@github-actions github-actions Bot added enhancement New feature or request and removed enhancement New feature or request labels May 4, 2026
@markshust markshust added the post-1.0 Deferred until after 1.0 release label May 4, 2026
@markshust
Copy link
Copy Markdown
Collaborator

Thanks Paulo — this is a thoughtful PR and the PRD is genuinely well thought out. I want to be upfront with where I'm at though:

Deferring to post-1.0

I'm going to mark this post-1.0 and not merge into develop right now. Reasons:

  • Scope vs. 1.0 focus. 1.0 is about stabilizing core primitives. A 1,500-line new package introducing a full OAuth2 authorization server is exactly the kind of large, additive surface I want to land after 1.0 ships, when I'm not actively making breaking decisions in core/auth. See the post-1.0 label — this fits that triage cleanly.
  • Reviewer bandwidth. Giant scaffold-style PRs cost a lot of review time relative to the value they deliver pre-launch, especially when no end-to-end flow is yet runnable. Right now I need that bandwidth on 1.0 work.
  • Most 1.0 apps consume tokens, they don't issue them. OAuth2 server is a real need but not a launch blocker.

The PRD is good; let's keep this PR open with the post-1.0 label so the design work isn't lost, and revisit after 1.0.

When this comes back, please use the HCF workflow

When we pick this back up, please drive it through the HCF plugin: https://github.com/markshust/hcf

Specifically:

  • hcf:plan-create to slice the work into vertical PRs (one grant flow at a time, not the whole package surface in one shot).
  • hcf:plan-orchestrate (or hcf:tdd-worker per task) to actually implement red-green-refactor — failing test first, then the minimal code to pass.

This is documented in CLAUDE.md under "Feature Development":

For any feature or request beyond simple ones, use the hcf:plan-create skill to trigger the autonomous development workflow.

This isn't optional housekeeping for me — it's how I keep PRs of this size reviewable, and it forces TDD by construction.

Specific feedback for the next iteration

So you have it for when this comes back, here's what I'd want to see different:

1. No scaffold-only slices. Marko's principle #5 in CLAUDE.md is "No pseudo-functionality — only build real functionality when core supports it. If there's nothing meaningful to build, build nothing." This PR adds 6 empty extends RepositoryInterface {} interfaces, 6 concrete repos that only set an ENTITY_CLASS const, and config keys for revocation/consent/refresh-rotation that nothing reads. Your own PRD (line 104) calls out client-credentials as the right first slice because it exercises keys + clients + scopes + token issuance end-to-end. Ship that working.

2. Tests weren't TDD'd, and that's why this PR is bloated. This is the root cause of point #1, not a separate issue. Going through the test files:

  • OAuthRepositoriesTest asserts interface_exists() and re-states module.php bindings — no persistence exercised.
  • OAuthEntitiesTest reads #[Table] attribute strings back at the line they're declared.
  • The "confidential clients with hashed secrets" test only verifies PHP's password_hash/password_verify, not package code.
  • KeysCommandTest is misnamed — both cases call new KeyGenerator() directly. The KeysCommand itself (Input/Output, exit codes, error formatting) is never exercised.

These all pass green from day one, which means they couldn't have driven the implementation — they were written after the code as mirrors of what was already there.

The cost of that: with no failing test forcing each line into existence, there's no signal telling you to stop adding code. So you naturally end up shipping 6 entities, 6 interfaces, 6 repositories, ~20 config keys, an attribute, an enum, and a key command — when actually only 1 entity, 1 repository, 1 config section, and 1 command are exercised by anything runnable. That's roughly 70%+ of this PR is unreachable code that exists only because no test required it. Red-first TDD prevents this by construction: if no failing test demands a class, you don't write the class. That single discipline would have collapsed this PR to a fraction of its size and made it mergeable.

When this comes back, every new file should trace to a failing test that demanded it.

3. Trim config to what's wired. ~20 keys in config/oauth.php, only keys.* is consumed. Grow the config surface alongside each grant flow as it lands. (Same root cause as above — these keys exist because the PRD said they would, not because code reads them.)

4. Verify locally end-to-end before opening the PR. The PR notes mention --ignore-platform-req=ext-imagick workarounds; please make sure composer test passes cleanly on a fresh checkout, and that there's at least one runnable end-to-end flow demonstrable from the CLI before posting.

Really do appreciate the work and the PRD — none of this is wasted, it's just timing + workflow. Marking post-1.0 and leaving open.

@ps-carvalho
Copy link
Copy Markdown
Contributor Author

I will look at HCF plugin. Not played with it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Introduces a breaking change enhancement New feature or request post-1.0 Deferred until after 1.0 release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants