refactor(common/auth): move login credentials to auth_provider - #7055
Merged
Conversation
…xera into task/migrateBackend
github-actions
Bot
added
engine
ddl-change
Contributor
Automated Reviewer SuggestionsBased on the
|
Neilk1021
changed the title
Neilk1021
marked this pull request as draft
July 29, 2026 20:59
# Conflicts: # sql/changelog.xml # sql/updates/29.sql
# Conflicts: # amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7055 +/- ##
============================================
+ Coverage 86.75% 86.83% +0.08%
+ Complexity 4226 4213 -13
============================================
Files 1170 1169 -1
Lines 46807 46636 -171
Branches 5211 5185 -26
============================================
- Hits 40606 40497 -109
+ Misses 4496 4428 -68
- Partials 1705 1711 +6
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 365 | 0.223 | 27,646/36,116/36,116 us | 🔴 +140.1% / 🔴 +117.4% |
| 🔴 | bs=100 sw=10 sl=64 | 806 | 0.492 | 123,676/142,837/142,837 us | 🔴 +41.1% / 🔴 +26.2% |
| 🔴 | bs=1000 sw=10 sl=64 | 900 | 0.549 | 1,102,556/1,219,456/1,219,456 us | 🔴 +24.0% / 🔴 +12.4% |
Baseline details
Latest main 42d08a3 from 2026-08-10T13:03:59.622Z
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 365 tuples/sec | 818.75 tuples/sec | 740.3 tuples/sec | -55.4% | -50.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.223 MB/s | 0.5 MB/s | 0.452 MB/s | -55.4% | -50.6% |
| bs=10 sw=10 sl=64 | p50 | 27,646 us | 11,857 us | 13,173 us | +133.2% | +109.9% |
| bs=10 sw=10 sl=64 | p95 | 36,116 us | 15,045 us | 16,615 us | +140.1% | +117.4% |
| bs=10 sw=10 sl=64 | p99 | 36,116 us | 17,739 us | 19,272 us | +103.6% | +87.4% |
| bs=100 sw=10 sl=64 | throughput | 806 tuples/sec | 1,057 tuples/sec | 939.86 tuples/sec | -23.8% | -14.2% |
| bs=100 sw=10 sl=64 | MB/s | 0.492 MB/s | 0.645 MB/s | 0.574 MB/s | -23.8% | -14.2% |
| bs=100 sw=10 sl=64 | p50 | 123,676 us | 93,404 us | 106,062 us | +32.4% | +16.6% |
| bs=100 sw=10 sl=64 | p95 | 142,837 us | 101,214 us | 113,212 us | +41.1% | +26.2% |
| bs=100 sw=10 sl=64 | p99 | 142,837 us | 112,746 us | 123,853 us | +26.7% | +15.3% |
| bs=1000 sw=10 sl=64 | throughput | 900 tuples/sec | 1,081 tuples/sec | 966.11 tuples/sec | -16.8% | -6.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.549 MB/s | 0.66 MB/s | 0.59 MB/s | -16.8% | -6.9% |
| bs=1000 sw=10 sl=64 | p50 | 1,102,556 us | 923,474 us | 1,038,733 us | +19.4% | +6.1% |
| bs=1000 sw=10 sl=64 | p95 | 1,219,456 us | 983,118 us | 1,085,289 us | +24.0% | +12.4% |
| bs=1000 sw=10 sl=64 | p99 | 1,219,456 us | 1,015,764 us | 1,115,555 us | +20.1% | +9.3% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,548.30,200,128000,365,0.223,27646.10,36116.11,36116.11
1,100,10,64,20,2480.43,2000,1280000,806,0.492,123676.25,142836.89,142836.89
2,1000,10,64,20,22225.52,20000,12800000,900,0.549,1102556.29,1219456.39,1219456.39# Conflicts: # access-control-service/src/test/scala/org/apache/texera/service/activity/UserActivityEventListenerSpec.scala # amber/src/main/scala/org/apache/texera/web/ServletAwareConfigurator.scala # amber/src/main/scala/org/apache/texera/web/auth/GuestAuthFilter.scala # common/auth/src/main/scala/org/apache/texera/auth/JwtParser.scala
…eAuthResource.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com>
…AuthProvisioner.scala Co-authored-by: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Signed-off-by: Neil Ketteringham <53205839+Neilk1021@users.noreply.github.com>
…o be referenced by other files in Auth.
Yicong-Huang
changed the title
…stant The constant added last round sat in `AuthResource` as `private[auth]`, which left the one other file in that package — `ExternalAuthProvisioner` — still spelling `"23505"` inline, and put a property of the database inside an HTTP resource. It now lives beside `SqlServer` in `common/dao`, where every caller that catches the code can reach it regardless of package. Also drops the scaladoc the move orphaned in `LocalAuthProvisioner` (it had come to document the password encryptor), gives `ExternalAuthProvisioner`'s retry comment a referent for what happens on the second failure, and completes a sentence in `WorkflowExecutionsResourceSpec`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both `createLocalAccount` and `claimWithLocalCredential` map a unique violation onto a 409 whose text names which constraint fired, and neither branch had a test. Three review rounds each found a defect inside these two blocks — the handler running in an already-aborted transaction, the sibling method the fix skipped, and the wrong constraint being named — so the branches were being corrected by inspection with nothing pinning the result. Each case drives the real constraint against the real DDL under embedded Postgres: `uq_provider_identity` and `user_email_key` for the insert path, `PRIMARY KEY (uid, provider_type)` and `uq_provider_identity` for the claim path. The assertions cover the cause named as well as the causes *not* named, since telling a user their free handle is taken was the last defect here, and they also pin the transaction boundary: a lost race leaves behind neither a credential-less account nor a placeholder flipped to claimed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The migration raised `RAISE EXCEPTION` when any password-holding account's name was blank, whitespace-padded, or shared with another. Those are reachable states today — `AdminUserResource.updateUser` writes `user.getName` through without trimming, and nothing has ever constrained `"user".name` to be unique — so a single such row turned changeset 33 into a failed liquibase changeset and a deployment that cannot start until an operator hand-edits the database. Nothing about minting a handle for the first time requires the old name to be clean, so it is normalized: trimmed, replaced by `user-<uid>` when trimming leaves nothing, and suffixed with `-<uid>` when it still collides (the lowest uid keeps the unsuffixed form). The de-duplication runs inside the filling UPDATE rather than only in the following pass, because `uq_provider_identity` is already in force and two accounts named "john" otherwise collide with each other within that single statement. The bounded loop then resolves a suffixed handle that collides with a literal one, following 28.sql's dataset-name precedent. The LOCAL backfill now inserts the row without a handle and lets that UPDATE mint it, since NULLs do not collide under the unique constraint — inserting the raw name would abort before any normalization could run. Every minted handle that differs from the account name is reported via RAISE NOTICE, with a closing count: the handle is what the user types to log in, so an operator must be able to see which accounts got one they cannot guess. Verified against embedded Postgres on three paths — an old-schema database seeded with all of the previously-fatal shapes (padded, blank, three-way duplicate, and a trim that creates a fresh collision), a re-run over the already-migrated result, and a database built from this PR's own DDL. All three commit; afterwards no LOCAL row has a null or duplicated handle, every GOOGLE row still carries its original google_id verbatim, and ck_provider_credential holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Yicong-Huang
approved these changes
Aug 10, 2026
Yicong-Huang
left a comment
Yicong-Huang
left a comment
Contributor
There was a problem hiding this comment.
🟢 8 resolved · 0 open · 0 new
Round 7. Everything the previous round left open is closed: the claimWithLocalCredential conflict message, the unreferenced userDao, JwtAuthSpec's unread parameter, both scaladoc items, and the description's table and migration names. The title's scope was updated; the remaining question about its type was withdrawn by the maintainer.
Disclosure — three commits on this branch were pushed from this account rather than found and reported, so this review is not an independent judgment of them and a second reviewer should look: 7f057ea8c moves the unique-violation SQLSTATE to common/dao/SqlStates and picks up the wording fixes, 8be10a157 adds LocalAuthProvisionerSpec, and 75a8af14b makes migration 33 normalize login handles instead of refusing to run when a name is blank, padded or shared.
Outstanding verification — 7f057ea8c and 8be10a157 were only format-checked locally: stale jOOQ codegen made the dependent modules fail to compile here, so LocalAuthProvisionerSpec has never executed. CI is the first thing to run it.
Verification trace
Two claims were load-bearing this round.
Which constraints claimWithLocalCredential can actually reach, since the round-6 fix rests on naming them correctly: its only caller sets name and the placeholder fields but never email, and "user".name carries no UNIQUE constraint while email does, so user_email_key is unreachable on that path. What remains is PRIMARY KEY (uid, provider_type) for an already-claimed account and uq_provider_identity for a taken handle — exactly the two the handleExists split distinguishes.
That existing Google accounts keep working across the migration: the pre-PR resource stored payload.getSubject in "user".google_id, migration 33 copies that column into provider_id verbatim, and profileOf looks up payload.getSubject — one value end to end. The handle minting was then exercised on three databases: the old schema seeded with padded, blank, three-way-duplicate and trim-collision names; a re-run over the already-migrated result; and a database built from this PR's own DDL. All three commit, and afterwards no LOCAL row has a null or duplicated handle, every GOOGLE row still carries its original google_id, and ck_provider_credential holds.
Contributor
|
@aglinxinyuan can you check again as the PR has evolved a lot? |
AdminUserResource.createLocalAccount was a one-line delegate with a single caller and no test of its own; the collision path it claimed to expose is covered directly in LocalAuthProvisionerSpec. LocalAuthProvisioner's 2-arg createLocalAccount overload existed only to feed that wrapper, so both layers go and addUser calls the 3-arg form directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… handle addUser used one UUID for both the username and the password, so anyone able to read /admin/user/list could authenticate as any account created this way. Two independent UUIDs cost nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ExternalAuthProvisioner copied AuthResource's case-insensitive email lookup, and both copies carried the same rule in prose that had to stay in sync by hand. The copy's stated reason was wrong: createDSLContext() returns the shared context rather than opening one. The real reason is transaction scope, which a DSLContext overload satisfies without duplicating the query. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- map the joined record in place instead of re-reading the same user row by uid, saving a query on the returning-identity path - rename `claimed` to `wasPlaceholder`; it holds "is an unclaimed placeholder", so `if (drifted || claimed)` read backwards - declare provision's `: User` return type rather than inferring it through a two-branch match - drop a stray blank line in loginOrProvision Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
googleId used to be set unconditionally, so a local-only user's token carried "googleId": null; it is now omitted entirely. Names the one reader that can tell the difference (flarum.service.ts, which passes it as a Flarum password) so the next person does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this PR?
As discussed in #6716 we'd like to split authentication handling into its own table
auth_provider.For future support of multiple sources of authentication (e.g. github, IEEE accounts, etc.). This PR migrate the Schema from the image on the left to the one on the right.This PR:
auth_providertable in texera_ddl.33.sqlto migrate old account data to the new schema.GoogleAuthServiceto a genericExternalAuthProvisionerto be used in future PRs adding new external logins.Any related issues, documentation, discussions?
Closes #7048
How was this PR tested?
Compiled and ran all tests as well as deployed locally to verify functionality.
Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8