chore: absorb auth into packages/auth - #13890
Conversation
…algorithms and proofs
Co-authored-by: Matej Bačo <[email protected]>
Co-authored-by: Matej Bačo <[email protected]>
Co-authored-by: Matej Bačo <[email protected]>
- Set default algorithm in Password class when initializing - Update tests to use new algorithm methods and default settings - Modify token generation to explicitly set SHA algorithm - Adjust test assertions to match new default hashing methods
- Update Proof classes to generate random values without input parameter - Add password generation with configurable length and charset - Modify README.md with comprehensive usage examples for different proof types - Update tests to reflect new generation and configuration methods - Improve code flexibility and security for authentication proofs
Move common hash and verify implementations from individual Proof subclasses to the abstract Proof base class, reducing code duplication and simplifying the class hierarchy
Algorithms -> Hashes
Introduce a new section demonstrating the usage of the Utopia\Auth\Store class, showcasing key features like setting, getting, encoding, and decoding data with practical code examples
Add auth Store class
docs: centralize CODE_OF_CONDUCT and CONTRIBUTING at the monorepo root
…ocuments Add OAuth Client ID Metadata Documents
chore(auth): align phpunit config with the other packages
chore: stop committing package lock files
feat(auth): add AuthorizationDetails reader for RFC 9396 grants
…s-list-shape fix(auth): reject non-list authorization_details shapes
feat(auth): add a generic HS256 JWT issuer
…ls-restrict feat(auth): narrow authorization_details to what a resolver still allows
ChiragAgg5k
added
the
absorb
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Mirror of IdToken::leftHalfHash for assertion purposes. | ||
| */ | ||
| private function expectedLeftHalfHash(string $value): string | ||
| { | ||
| return rtrim(strtr(base64_encode(substr(hash('sha256', $value, true), 0, 16)), '+/', '-_'), '='); |
There was a problem hiding this comment.
expectedLeftHalfHash() repeats the production hashing and encoding steps. If both copies change in the same incorrect way, the test can still pass. The Argon2 tests similarly derive expected values from the object's options. The repository requires tests to check observable behavior rather than mirror source code or configuration; this requirement should be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/tests/Issuers/Asymmetric/IdTokenTest.php
Line: 262-266
Comment:
**Tests mirror production logic**
`expectedLeftHalfHash()` repeats the production hashing and encoding steps. If both copies change in the same incorrect way, the test can still pass. The Argon2 tests similarly derive expected values from the object's options. The repository requires tests to check observable behavior rather than mirror source code or configuration; this requirement should be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| // Verify all data was preserved | ||
| foreach ($data as $key => $value) { | ||
| $this->assertEquals($value, $store->getProperty($key)); | ||
| } |
There was a problem hiding this comment.
The test decodes into $newStore but then checks values on $store. It would pass even if decoding lost every value, so it cannot catch a regression in Store serialization.
| // Verify all data was preserved | |
| foreach ($data as $key => $value) { | |
| $this->assertEquals($value, $store->getProperty($key)); | |
| } | |
| // Verify all data was preserved | |
| foreach ($data as $key => $value) { | |
| $this->assertEquals($value, $newStore->getProperty($key)); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/tests/StoreTest.php
Line: 87-90
Comment:
**Round-trip test checks original**
The test decodes into `$newStore` but then checks values on `$store`. It would pass even if decoding lost every value, so it cannot catch a regression in Store serialization.
```suggestion
// Verify all data was preserved
foreach ($data as $key => $value) {
$this->assertEquals($value, $newStore->getProperty($key));
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| $store->set('userId', '12345') | ||
| ->set('name', 'John Doe') | ||
| ->set('isActive', true) | ||
| ->set('preferences', ['theme' => 'dark', 'notifications' => true]); | ||
|
|
||
| // Get values with optional defaults | ||
| $userId = $store->get('userId'); | ||
| $missing = $store->get('missing', 'default value'); |
There was a problem hiding this comment.
The example uses set() and get(), but Store provides setProperty() and getProperty(). A reader running the example will get an undefined-method error, including at the final decoded-value lookup. Please use the public method names throughout.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/docs/store.md
Line: 15-22
Comment:
**Store example calls missing methods**
The example uses `set()` and `get()`, but `Store` provides `setProperty()` and `getProperty()`. A reader running the example will get an undefined-method error, including at the final decoded-value lookup. Please use the public method names throughout.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| }, | ||
| "replace": { | ||
| "utopia-php/agents": "*", | ||
| "utopia-php/auth": "*", |
There was a problem hiding this comment.
Replacing the locked auth dependency removes its ext-scrypt requirement from the root Composer dependency graph. If a deployment lacks that extension, an ordinary Composer platform check no longer flags it, while the scrypt user-creation path throws when it calls Scrypt::hash(). The package still declares the requirement, but the root project needs to declare it for Composer to enforce it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: composer.json
Line: 181
Comment:
**Scrypt requirement no longer enforced**
Replacing the locked auth dependency removes its `ext-scrypt` requirement from the root Composer dependency graph. If a deployment lacks that extension, an ordinary Composer platform check no longer flags it, while the scrypt user-creation path throws when it calls `Scrypt::hash()`. The package still declares the requirement, but the root project needs to declare it for Composer to enforce it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
# Conflicts: # composer.lock
| throw new \InvalidArgumentException('Salt cannot be empty'); | ||
| } | ||
|
|
||
| if (! preg_match('/^[A-Za-z0-9+\/]+={0,2}$/', $salt)) { |
There was a problem hiding this comment.
The imported salt validator uses preg_match(). The repository guide says not to add regular expressions: use string operations or an existing validator, or explain in the PR why neither works. The same pattern appears in setSaltSeparator() and setSignerKey(), as well as imported auth tests. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/src/Hashes/ScryptModified.php
Line: 117
Comment:
**Regex validation violates repository guide**
The imported salt validator uses `preg_match()`. The repository guide says not to add regular expressions: use string operations or an existing validator, or explain in the PR why neither works. The same pattern appears in `setSaltSeparator()` and `setSignerKey()`, as well as imported auth tests. This repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/appwrite/appwrite/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| $token = $this->refreshToken->issue('user-123', 'aud', 'client-abc', 1209600); | ||
|
|
||
| $parts = explode('.', $token); | ||
| $expected = $this->base64UrlEncode(hash_hmac('sha256', $parts[0] . '.' . $parts[1], $this->secret, true)); |
There was a problem hiding this comment.
This test calculates its expected signature with the same HMAC and base64url steps as the issuer; the wrong-secret assertion repeats that approach. If both copies change incorrectly together, the test can still pass. The repository requires tests of observable behavior rather than tests that mirror source code. This requirement must be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/tests/Issuers/Symmetric/RefreshTokenTest.php
Line: 81
Comment:
**Signature test mirrors issuer**
This test calculates its expected signature with the same HMAC and base64url steps as the issuer; the wrong-secret assertion repeats that approach. If both copies change incorrectly together, the test can still pass. The repository requires tests of observable behavior rather than tests that mirror source code. This requirement must be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
# Conflicts: # composer.lock
# Conflicts: # composer.lock # rfc/monorepo.md
# Conflicts: # composer.lock
# Conflicts: # composer.lock # rfc/monorepo.md
# Conflicts: # composer.json # composer.lock
# Conflicts: # composer.lock # rfc/monorepo.md
# Conflicts: # composer.lock
|
|
||
| $this->assertNotEmpty($hash); | ||
| $this->assertSame(32, \strlen($hash)); | ||
| $this->assertSame(md5($password), $hash); |
There was a problem hiding this comment.
This assertion calculates the expected hash with md5(), the same function production uses. The test repeats that pattern for other inputs. If the implementation and assertions change together, the tests could pass despite a compatibility regression. The repository requires tests to check observable behavior rather than mirror source code; that requirement must be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/auth/tests/Algorithms/MD5Test.php
Line: 26
Comment:
**MD5 tests mirror production**
This assertion calculates the expected hash with `md5()`, the same function production uses. The test repeats that pattern for other inputs. If the implementation and assertions change together, the tests could pass despite a compatibility regression. The repository requires tests to check observable behavior rather than mirror source code; that requirement must be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
7 participants
Summary
This moves
utopia-php/authintopackages/auth, so Appwrite loads it directly. It's wave 1 of #13828 and follows the playbook inrfc/monorepo.md. There are three commits plus a whitespace fix:Add 'packages/auth/' from commit '580e41d6…': subtree import from the mirror with full history. The mirror head is the0.12.0commit Appwrite already locks (580e41d6), so Appwrite runs the same code.chore(auth): mirror plumbing:mirror.ymlnow points at this repository'smirror-redirect.yml, the README banner is refreshed, andabsorbremoves theDockerfile, which only built a local test container.refactor: load auth from packages/:replaceentries, removes theutopia-php/auth: ^0.12require line and thehttps://github.com/utopia-php/authvcsrepository entry, and removes the package from the lock.src/Auth/*moves up tosrc/.tests/Auth/*moves up totests/. The tests move fromUtopia\Tests\AuthtoUtopia\Auth\Tests, with a matchingautoload-deventry.tests/StoreTest.phpalready declaredUtopia\Auth\Testsbut wasn't autoloadable before; now it is.tests/E2E.docker-compose.ymlis removed. It only ran the unit tests in a container built from the deletedDockerfile, and the standard shape keeps a compose file only whencomposer test:e2eexists. The README's test section now sayscomposer install && composer testand lists the four extensions the tests need.phpstan.neonat level max oversrcandtests, and a standalonerector.php. The package had no PHPStan config of its own, so it ships with aphpstan-baseline.neonof 33 findings:mixedout-parameters and results fromopenssl_pkey_export(),openssl_pkey_get_details()andopenssl_sign()in the asymmetric issuer and verifier, integer arithmetic in the PHPass encoder, array shapes inAuthorizationDetailsandResourceIndicators, and decoded-claim arithmetic in the tests. The RFC's phase 8 burn-down list now includes them.bin/monorepo check auth --fix: empty-body braces,fn (spacing, blank lines after control blocks,protected→privateon final test classes. There's no behaviour change.style(auth): strip trailing whitespace from LICENSE: the upstreamLICENSEfailsgit diff --check.There's nothing to hoist. The package requires only PHP and
ext-hash,ext-openssl,ext-scryptandext-sodium.ext-opensslis already in the rootrequire, and extensions are exempt from the hoisting rule (same asimage'sext-gd). TheDockerfileand CI both install with--ignore-platform-reqs, so droppingext-scryptandext-sodiumfrom the lock changes no install. If we want the root manifest to declare them anyway (user password hashing depends on both), that's a one-line follow-up.Net for the load commit: the lock diff is the
utopia-php/authentry (−60 lines) plus the content hash.Validation
bin/monorepo validate: all packages validbin/monorepo check auth: Pint, PHPStan level max (with baseline) and Rector passbin/monorepo test auth: 283 tests / 572 assertionsvendor/bin/phpunit --testsuite packages --filter 'Utopia\\Auth': 283 tests / 572 assertions under the root autoloaderUtopia\Auth\Proofs\Passwordresolves topackages/auth/src/Proofs/Password.phpfrom the root autoloader, andvendor/utopia-php/authis goneUtopia\Auth(Documents/User.php,Auth/Validator/PasswordHistory.php,Auth/MFA/Type.php,Users/Base.php,Users/Http/Users/Scrypt/Modified/Create.php,Account/Http/Account/Sessions/IdToken/Create.php,Realtime/Message/Handlers/Authentication.php,app/init/resources/request.php)composer update --lock: removes onlyutopia-php/authcomposer validate --no-check-publish: valid (the existingutopia-php/platformexact-constraint warning only)bin/monorepo split auth --dry-run:1f261783, which fast-forwards from the mirror head580e41d6git diff --checkcomposer lint: passesMerge and follow-up
Merge with a merge commit.
The
utopia-php/authmirror already has the canonicalmainruleset (id9271238), with the split app as an always-bypass actor, so the absorb ran with--skip-ruleset.After merge:
Splitpushes toutopia-php/auth.packages/authfromutopia-php/monorepo.packages/auth. It moves Appwrite's identity validators and 47 OAuth2 provider adapters into the library. It needs a decision on whether to port it ontopackages/authhere or close it with a pointer. The mirror also has stale branches (dev,feat-oauth-helpers,cursor/auth-validators-oauth2-1299).🤖 Generated with Claude Code