Skip to content

feat(auth): Add ORCID Login - #7664

Open
Neilk1021 wants to merge 7 commits into
apache:mainfrom
Neilk1021:task/addOrcidLogin
Open

feat(auth): Add ORCID Login#7664
Neilk1021 wants to merge 7 commits into
apache:mainfrom
Neilk1021:task/addOrcidLogin

Conversation

@Neilk1021

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Closes #7516 by adding ORCID login as an optional feature, disabled by default.

ORCID differs from the existing OIDC provider (Google) in two ways, and those two differences drive nearly all of this diff.

1. No email address. This PR uses ORCID's authorization-code flow with the /authenticate scope, which returns an iD and a name and no email. (ORCID does support OpenID Connect — there's an openid scope and an id_token — but even under openid it doesn't assert an address.) Many Texera features require a valid email, so after signing in with ORCID the user is prompted by a modal to attach one.

The rules that prompt enforces:

  • an address held by an account that already has a credential (LOCAL/Google/ORCID)
    is refused with 409
  • an address held by a contributor placeholder is claimed: the ORCID identity
    moves onto the placeholder's uid, and the row created at login is discarded
  • an account that already has an address can't replace it

The attach is single-step, following repo precedent: AuthResource.register already claims a placeholder on an unverified, typed address, and there is no email verification anywhere in the codebase today. Adding verification is out of scope here
but worth doing.

2. Not a single-step handoff. Because this is a plain OAuth 2.0 authorization-code flow rather than the OIDC path Google takes, we can't resolve login in one clean step. The frontend gets a dedicated callback component that resolves the code and passes it to the backend before routing to the homepage. The CSRF state parameter is now verified there — the login page was already writing it to sessionStorage, but nothing read it back.

Schema change required to build this branch

sql/updates/38.sql adds ORCID to provider_type_enum, with the matching changelog.xml changeSet and the texera_ddl.sql enum. Anyone building this branch must apply the migration and re-run DAO/jooqGenerate, or ProviderTypeEnum.ORCID won't exist and the build fails. (Migration was renumbered 36 → 38 after upstream took
both 36 and 37.)

Shared code this touches

These are edits to existing paths, not new ORCID-only code, so they're the parts worth
review attention:

  • ExternalProfile.email widens to Option[String]. Signature change in shared
    provisioning code that Google flows through as well. Google still passes
    Some(email); its linking behaviour is unchanged.
  • loginWithExistingToken ordering. It no longer logs out immediately for an
    emailless INACTIVE user; it asks for the address first, and the reissued token
    re-enters and takes the registration path. This affects all providers. Motivation is
    concrete: bin/k8s/values.yaml ships USER_SYS_INVITE_ONLY=true, and without the
    reorder the admin receives a request with a null address, which
    /gmail/notify-unauthorized rejects.
  • User.email is now optional in TypeScript, which surfaced two real spots:
    FlarumService (identifies accounts by email) and the admin edit field.
  • AdminUserResource.updateUser refuses to activate an account with no email. New
    guard on an existing admin endpoint — a REGULAR account without an address builds
    dataset paths as null/<name>/….

Config and how to enable

user-sys.orcid.{clientId,clientSecret,baseUrl,redirectUri}, GUI_LOGIN_ORCID_LOGIN, and both k8s values files. /auth/orcid/config returns 503 when unconfigured, so the button stays disabled.

To try it locally: register a sandbox app with the redirect URI. Note that ORCID rejects localhost, so local testing needs ng serve --host 127.0.0.1.

Any related issues, documentation, discussions?

Closes #7516

How was this PR tested?

New specs on both sides. The consent screen and token exchange are the one part that cannot be unit
tested, so the exchange is a protected seam the specs override — as GoogleAuthResourceSpec does
with verifiedPayload — and the real flow was driven by hand against the ORCID sandbox.

  • OrcidAuthResourceSpec (new): provisioning from an authenticated iD — emailless INACTIVE
    account plus its auth_provider row, idempotent on a second login, the iD standing in for a
    private name. Refusals: a response naming no iD, a blank code, each missing config setting.
  • AuthResourceSpec: the PUT /auth/email contract — stores and reissues the token; refuses
    malformed, blank, already-set, and addresses owned by a credentialed account; adopts a contributor
    placeholder, including when the caller has a user_last_active_time row (that FK has no
    ON DELETE CASCADE, so the adoption used to throw).
  • ExternalAuthProvisionerSpec: identity-only provisioning, two emailless accounts staying
    separate, a later-collected address surviving a refresh. AdminUserResourceSpec: no activation
    without an address.
  • Frontend: orcid-callback.component.spec.ts (new) for the state check and every refusal
    path; auth.service.spec.ts for the prompt and its invite-only ordering; plus the modal,
    user.service.spec.ts, and the login page's redirect.
AMBER_TEST_FILTER=skip-integration sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.auth.* org.apache.texera.web.resource.dashboard.admin.user.*"
cd frontend && npx ng test --watch=false
sbt scalafmtCheckAll && cd frontend && npx tsc -p tsconfig.json --noEmit && yarn format:ci

By hand, against the ORCID sandbox. Register http://127.0.0.1:4200/callback/orcid on a sandbox
application (ORCID rejects localhost), then:

export USER_SYS_ORCID_CLIENT_ID=APP-XXXXXXXXXXXX
export USER_SYS_ORCID_CLIENT_SECRET=...          # read once per JVM, so export before starting
export GUI_LOGIN_ORCID_LOGIN=true
bin/local-dev.sh up                              # migrations + jOOQ codegen
cd frontend && npx ng serve --host 127.0.0.1     # ng serve binds localhost/::1 by default

Sign in with ORCID at http://127.0.0.1:4200/login, consent, supply an address at the prompt, and
reload to confirm you are not asked again. Refusals: a tampered state on the callback URL returns
you to /login; an address belonging to a credentialed account keeps the dialog open. With the
credentials unset, the button stays disabled and no error toast appears.

Migration: applied to a database whose enum lacked ORCID under both runners this repo uses
(bin/local-dev.sh keeps SET search_path; the Liquibase runner in sql/docker-compose.yml strips
it, which is why the type is schema-qualified), then re-applied to confirm idempotence.

Was this PR authored or co-authored using generative AI tooling?

Co-Authored with Claude Opus 4.8

Neilk1021 and others added 6 commits August 12, 2026 14:21
Upstream added sql/updates/36.sql (prefix dataset paths with "datasets/") and
37.sql (model metadata tables), so this branch's ORCID migration was renumbered
36 -> 38 and sql/changelog.xml now carries all three changeSets in order.

sql/texera_ddl.sql merged cleanly: provider_type_enum keeps 'ORCID' alongside
upstream's new model tables.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added engine ddl-change Changes to the TexeraDB DDL frontend Changes related to the frontend GUI infra common platform Non-amber Scala service paths labels Aug 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @Ma77Ball, @mengw15, @aicam
    You can notify them by mentioning @Ma77Ball, @mengw15, @aicam in a comment.

@codecov-commenter

codecov-commenter commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.11336% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (3cb3479) to head (d52652d).

Files with missing lines Patch % Lines
...e/texera/web/resource/auth/OrcidAuthResource.scala 50.90% 26 Missing and 1 partial ⚠️
...ontend/src/app/common/service/user/auth.service.ts 82.22% 5 Missing and 3 partials ⚠️
...l-request-modal/email-request-modal.component.html 54.54% 5 Missing ⚠️
frontend/src/app/common/util/email.ts 62.50% 1 Missing and 2 partials ⚠️
...pp/hub/component/login/texera-login.component.html 57.14% 1 Missing and 2 partials ⚠️
.../app/hub/component/login/texera-login.component.ts 81.25% 1 Missing and 2 partials ⚠️
...apache/texera/web/resource/auth/AuthResource.scala 91.66% 0 Missing and 2 partials ⚠️
...d/src/app/common/service/user/stub-auth.service.ts 60.00% 2 Missing ⚠️
.../web/model/http/request/auth/SetEmailRequest.scala 0.00% 1 Missing ⚠️
...ource/dashboard/admin/user/AdminUserResource.scala 75.00% 0 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7664      +/-   ##
============================================
- Coverage     90.12%   90.03%   -0.10%     
- Complexity     4420     4424       +4     
============================================
  Files          1176     1183       +7     
  Lines         47001    47233     +232     
  Branches       5271     5305      +34     
============================================
+ Hits          42359    42525     +166     
- Misses         2880     2925      +45     
- Partials       1762     1783      +21     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 98.62% <ø> (ø) Carriedforward from 1568b0b
amber 86.54% <67.02%> (-0.17%) ⬇️
computing-unit-managing-service 72.46% <ø> (ø)
config-service 77.55% <100.00%> (+0.23%) ⬆️
file-service 68.90% <ø> (ø)
frontend 91.37% <81.57%> (-0.07%) ⬇️
notebook-migration-service 78.89% <ø> (ø)
pyamber 97.57% <ø> (ø) Carriedforward from 1568b0b
workflow-compiling-service 57.89% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 0 better · 🔴 12 worse · ⚪ 3 noise (<±5%) · 0 without baseline

Compared against main 3cb3479 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🔴 bs=10 sw=10 sl=64 407 0.248 23,058/40,377/40,377 us 🔴 +29.5% / 🔴 +147.0%
🔴 bs=100 sw=10 sl=64 938 0.572 105,497/130,331/130,331 us 🔴 +12.4% / 🔴 +22.4%
🔴 bs=1000 sw=10 sl=64 1,071 0.654 921,024/1,044,473/1,044,473 us 🔴 +7.0% / 🟢 -5.7%
Baseline details

Latest main 3cb3479 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 407 tuples/sec 478 tuples/sec 779.28 tuples/sec -14.9% -47.8%
bs=10 sw=10 sl=64 MB/s 0.248 MB/s 0.292 MB/s 0.476 MB/s -15.1% -47.9%
bs=10 sw=10 sl=64 p50 23,058 us 18,704 us 12,712 us +23.3% +81.4%
bs=10 sw=10 sl=64 p95 40,377 us 31,183 us 16,345 us +29.5% +147.0%
bs=10 sw=10 sl=64 p99 40,377 us 31,183 us 19,050 us +29.5% +112.0%
bs=100 sw=10 sl=64 throughput 938 tuples/sec 1,010 tuples/sec 1,017 tuples/sec -7.1% -7.8%
bs=100 sw=10 sl=64 MB/s 0.572 MB/s 0.616 MB/s 0.621 MB/s -7.1% -7.9%
bs=100 sw=10 sl=64 p50 105,497 us 95,751 us 100,048 us +10.2% +5.4%
bs=100 sw=10 sl=64 p95 130,331 us 115,960 us 106,477 us +12.4% +22.4%
bs=100 sw=10 sl=64 p99 130,331 us 115,960 us 114,739 us +12.4% +13.6%
bs=1000 sw=10 sl=64 throughput 1,071 tuples/sec 1,104 tuples/sec 1,048 tuples/sec -3.0% +2.2%
bs=1000 sw=10 sl=64 MB/s 0.654 MB/s 0.674 MB/s 0.639 MB/s -3.0% +2.3%
bs=1000 sw=10 sl=64 p50 921,024 us 910,529 us 976,350 us +1.2% -5.7%
bs=1000 sw=10 sl=64 p95 1,044,473 us 976,287 us 1,022,084 us +7.0% +2.2%
bs=1000 sw=10 sl=64 p99 1,044,473 us 976,287 us 1,053,520 us +7.0% -0.9%
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,491.61,200,128000,407,0.248,23058.39,40377.41,40377.41
1,100,10,64,20,2132.28,2000,1280000,938,0.572,105497.40,130330.91,130330.91
2,1000,10,64,20,18674.88,20000,12800000,1071,0.654,921024.47,1044473.00,1044473.00

@Neilk1021

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

@github-actions
github-actions Bot requested a review from aglinxinyuan August 14, 2026 19:05
@Yicong-Huang

Copy link
Copy Markdown
Contributor

This PR is huge! 2000+ LOC and 50 files. Without breaking it into smaller PRs, can you check if all the changes are necessary? AI tend to write huge changes, let's try to compact it before anyone starts review

@Yicong-Huang

Copy link
Copy Markdown
Contributor

Many Texera features require a valid email, so after signing in with ORCID the user is prompted by a modal to attach one.

I think this is a big topic, as it changes the account serving model. I do believe we have many features that rely on emails. Especially we have communication features (comment, share, future email notifications for executions etc.) I would strongly suggest we make email NON optional for all accounts.

The current model essentially allows a partial account without an email. There are two concerns with the current model:

  1. What if after sign up, user never provides email? in our codebase we have to handle this scenario, which adds complexity. I really want to prevent that from happening, unless we really have the need to allow optional email.
  2. How do we know the email they entered is owned by the user? we will have to do some verification e.g., send a code to their email and ask them to enter it.

I feel this step could just be required before they sign up with ORCID. for example,

  • at registration form, provide an input box for email and a button for verification.
  • After clicking the verification, an email with a code will be sent to the one they entered. an input box with verification code will be shown for user to enter, and a button for verify will also be shown.
  • After they verified with the correct code, the URCID login button shows up.

Even further: a better way is to always require an email or an oauth with an email to login. And after that user can optionally link their ORCID in account settings.

@Yicong-Huang

Copy link
Copy Markdown
Contributor

In order to review this, we will also need a gif/recording to see how it works on the UI. Please attach one in the PR description.

@Yicong-Huang
Yicong-Huang self-requested a review August 15, 2026 00:57
@Neilk1021

Copy link
Copy Markdown
Contributor Author

This PR is huge! 2000+ LOC and 50 files. Without breaking it into smaller PRs, can you check if all the changes are necessary? AI tend to write huge changes, let's try to compact it before anyone starts review

Sure! But much of the PR was hand-rolled and then reviewed with AI, so idk how much can actually be cut, as most of it is dependent on each other.

@Neilk1021

Copy link
Copy Markdown
Contributor Author
  1. What if after sign up, user never provides email? in our codebase we have to handle this scenario, which adds complexity. I really want to prevent that from happening, unless we really have the need to allow optional email.

If I recall correctly, the way it's implemented signs out the user if they refuse to provide an email, thus preventing them from using the service until one is provided. However, the guards on this could be stronger.

  1. How do we know the email they entered is owned by the user? we will have to do some verification e.g., send a code to their email and ask them to enter it.

I should mention that this is already a problem we have with the current log-in system. Addition verification seemed out of scope for this specific PR, but I agree it needs to be added.

@Neilk1021

Copy link
Copy Markdown
Contributor Author

Even further: a better way is to always require an email or an oauth with an email to login. And after that user can optionally link their ORCID in account settings.

That is the intent with the Modal, it's not meant to be an optional step, rather its meant to basically say "hey, you're not done, do this to finish setting up your account."

@aglinxinyuan

aglinxinyuan commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

I agree with @Yicong-Huang. I'm pretty confident that you don't need 2000 lines to make it work. If you need 2000 lines for each provider, it will be very hard to scale.

@Neilk1021

Copy link
Copy Markdown
Contributor Author

I agree with @Yicong-Huang. I'm pretty confident that you don't need 2000 lines to make it work. If you need 2000 lines for each provider, it will be very hard to scale.

Wasn't saying that all future providers will need 2k lines, more so that there are required features that service similar to ORCID require to function, I'll split those into smaller PRs and submit them first.

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

Labels

common ddl-change Changes to the TexeraDB DDL engine frontend Changes related to the frontend GUI infra platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ORCID login

4 participants