From ca68f00926c85cc69e2796d93f0c31e66ef12ada Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Wed, 12 Aug 2026 14:21:15 -0700 Subject: [PATCH 1/6] feat(auth): add ORCID frontend logic --- .../texera/web/TexeraWebApplication.scala | 16 ++--- .../web/resource/auth/OrcidAuthResource.scala | 33 +++++++++++ common/config/src/main/resources/gui.conf | 4 ++ .../src/main/resources/user-system.conf | 11 ++++ .../texera/common/config/GuiConfig.scala | 2 + .../common/config/UserSystemConfig.scala | 2 + .../service/resource/ConfigResource.scala | 1 + frontend/src/app/app-routing.module.ts | 6 ++ .../common/service/gui-config.service.mock.ts | 1 + .../app/common/service/gui-config.service.ts | 5 +- .../app/common/service/user/auth.service.ts | 15 ++++- .../common/service/user/orcid-auth.service.ts | 51 ++++++++++++++++ .../app/common/service/user/user.service.ts | 5 ++ frontend/src/app/common/type/gui-config.ts | 1 + .../login/orcid-callback.component.ts | 40 +++++++++++++ .../login/texera-login.component.html | 14 ++++- .../login/texera-login.component.scss | 33 +++++++++++ .../component/login/texera-login.component.ts | 55 +++++++++++++++++- .../src/assets/logos/ORCID-iD_icon_24x24.png | Bin 0 -> 1358 bytes 19 files changed, 278 insertions(+), 17 deletions(-) create mode 100644 amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala create mode 100644 frontend/src/app/common/service/user/orcid-auth.service.ts create mode 100644 frontend/src/app/hub/component/login/orcid-callback.component.ts create mode 100755 frontend/src/assets/logos/ORCID-iD_icon_24x24.png diff --git a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala index 6dd624de603..d25f1b43432 100644 --- a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala +++ b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala @@ -33,25 +33,16 @@ import org.apache.texera.auth.SessionUser import org.apache.texera.dao.SqlServer import org.apache.texera.web.auth.JwtAuth.setupJwtAuth import org.apache.texera.web.resource._ -import org.apache.texera.web.resource.auth.{AuthResource, GoogleAuthResource} +import org.apache.texera.web.resource.auth.{AuthResource, GoogleAuthResource, OrcidAuthResource} import org.apache.texera.web.resource.dashboard.DashboardResource import org.apache.texera.web.resource.dashboard.admin.execution.AdminExecutionResource import org.apache.texera.web.resource.dashboard.admin.user.AdminUserResource import org.apache.texera.web.resource.dashboard.hub.HubResource import org.apache.texera.web.resource.dashboard.user.UserResource import org.apache.texera.web.resource.dashboard.user.warehouse.WarehouseResource -import org.apache.texera.web.resource.dashboard.user.project.{ - ProjectAccessResource, - ProjectResource, - PublicProjectResource -} +import org.apache.texera.web.resource.dashboard.user.project.{ProjectAccessResource, ProjectResource, PublicProjectResource} import org.apache.texera.web.resource.dashboard.user.quota.UserQuotaResource -import org.apache.texera.web.resource.dashboard.user.workflow.{ - WorkflowAccessResource, - WorkflowExecutionsResource, - WorkflowResource, - WorkflowVersionResource -} +import org.apache.texera.web.resource.dashboard.user.workflow.{WorkflowAccessResource, WorkflowExecutionsResource, WorkflowResource, WorkflowVersionResource} import org.eclipse.jetty.server.session.SessionHandler import org.eclipse.jetty.servlet.{ErrorPageErrorHandler, FilterHolder} import org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter @@ -143,6 +134,7 @@ class TexeraWebApplication environment.jersey.register(classOf[AuthResource]) environment.jersey.register(classOf[GoogleAuthResource]) + environment.jersey.register(classOf[OrcidAuthResource]) environment.jersey.register(classOf[UserConfigResource]) environment.jersey.register(classOf[FeedbackResource]) environment.jersey.register(classOf[AdminUserResource]) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala new file mode 100644 index 00000000000..b550d54c7bd --- /dev/null +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala @@ -0,0 +1,33 @@ +package org.apache.texera.web.resource.auth + +import org.apache.texera.common.config.UserSystemConfig +import org.apache.texera.common.config.UserSystemConfig.orcidBaseUrl +import org.apache.texera.web.model.http.response.TokenIssueResponse +import org.apache.texera.web.resource.auth.OrcidAuthResource.clientId + +import javax.ws.rs.core.MediaType +import javax.ws.rs.{Consumes, GET, POST, Path, Produces} + +object OrcidAuthResource { + final private lazy val clientId = UserSystemConfig.orcidClientId +} + +@Path("/auth/orcid") +class OrcidAuthResource { + @GET + @Path("/config") + @Produces(Array(MediaType.APPLICATION_JSON)) + def getConfig: Map[String, String] = Map( + "clientId" -> clientId, + "authorizeUrl" -> s"$orcidBaseUrl/oauth/authorize" + ) + + @POST + @Consumes(Array(MediaType.TEXT_PLAIN)) + @Produces(Array(MediaType.APPLICATION_JSON)) + @Path("/login") + def login(code: String): TokenIssueResponse = { + print("this works!") + throw new NotImplementedError("you haven't actually done sign in") + } +} diff --git a/common/config/src/main/resources/gui.conf b/common/config/src/main/resources/gui.conf index b9908304fb0..e3e2a5f4f45 100644 --- a/common/config/src/main/resources/gui.conf +++ b/common/config/src/main/resources/gui.conf @@ -34,6 +34,10 @@ gui { google-login = true google-login = ${?GUI_LOGIN_GOOGLE_LOGIN} + # whether orcid login is enabled + orcid-login = true + orcid-login = ${?GUI_LOGIN_ORCID_LOGIN} + # Can be configured as { username: "texera", password: "password" } # If configured, this will be automatically filled into the local login input box default-local-user { diff --git a/common/config/src/main/resources/user-system.conf b/common/config/src/main/resources/user-system.conf index ffda7e2435a..8ea19fb0a43 100644 --- a/common/config/src/main/resources/user-system.conf +++ b/common/config/src/main/resources/user-system.conf @@ -36,6 +36,17 @@ user-sys { } } + orcid { + clientId = "" + clientId = ${?USER_SYS_ORCID_CLIENT_ID} + + clientSecret = "" + clientSecret = ${?USER_SYS_ORCID_CLIENT_SECRET} + + baseUrl = "https://sandbox.orcid.org" + baseUrl = ${?USER_SYS_ORCID_BASE_URL} + } + domain = "" domain = ${?USER_SYS_DOMAIN} diff --git a/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala b/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala index 6897b8e5121..a9f6977ad12 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/GuiConfig.scala @@ -29,6 +29,8 @@ object GuiConfig { conf.getBoolean("gui.login.local-login") val guiLoginGoogleLogin: Boolean = conf.getBoolean("gui.login.google-login") + val guiLoginOrcidLogin: Boolean = + conf.getBoolean("gui.login.orcid-login") val guiLoginDefaultLocalUserUsername: String = if (conf.hasPath("gui.login.default-local-user.username")) conf.getString("gui.login.default-local-user.username") diff --git a/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala b/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala index ae41a75c2d2..469de744815 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala @@ -30,6 +30,8 @@ object UserSystemConfig { val adminUsername: String = conf.getString("user-sys.admin-username") val adminPassword: String = conf.getString("user-sys.admin-password") val googleClientId: String = conf.getString("user-sys.google.clientId") + val orcidClientId: String = conf.getString("user-sys.orcid.clientId") + val orcidBaseUrl: String = conf.getString("user-sys.orcid.baseUrl") val gmail: String = conf.getString("user-sys.google.smtp.gmail") val smtpPassword: String = conf.getString("user-sys.google.smtp.password") val inviteOnly: Boolean = conf.getBoolean("user-sys.invite-only") diff --git a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala index a8887cda92a..4af58cefd9a 100644 --- a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala +++ b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala @@ -63,6 +63,7 @@ class ConfigResource { Map( "localLogin" -> GuiConfig.guiLoginLocalLogin, "googleLogin" -> GuiConfig.guiLoginGoogleLogin, + "orcidLogin" -> GuiConfig.guiLoginOrcidLogin, "defaultLocalUser" -> Map( "username" -> GuiConfig.guiLoginDefaultLocalUserUsername, "password" -> GuiConfig.guiLoginDefaultLocalUserPassword diff --git a/frontend/src/app/app-routing.module.ts b/frontend/src/app/app-routing.module.ts index e255fe16710..e18e767e2ee 100644 --- a/frontend/src/app/app-routing.module.ts +++ b/frontend/src/app/app-routing.module.ts @@ -44,6 +44,7 @@ import { LandingPageComponent } from "./hub/component/landing-page/landing-page. import { USER_WORKFLOW } from "./app-routing.constant"; import { HubSearchResultComponent } from "./hub/component/hub-search-result/hub-search-result.component"; import { AdminSettingsComponent } from "./dashboard/component/admin/settings/admin-settings.component"; +import { OrcidCallbackComponent } from "./hub/component/login/orcid-callback.component"; const routes: Routes = []; @@ -55,6 +56,11 @@ routes.push({ component: TexeraLoginComponent, }); +routes.push({ + path: "callback", + children: [{ path: "orcid", component: OrcidCallbackComponent }], +}); + routes.push({ path: "", component: DashboardComponent, diff --git a/frontend/src/app/common/service/gui-config.service.mock.ts b/frontend/src/app/common/service/gui-config.service.mock.ts index 93c8eaf0fdf..dfa8dfd7a4d 100644 --- a/frontend/src/app/common/service/gui-config.service.mock.ts +++ b/frontend/src/app/common/service/gui-config.service.mock.ts @@ -33,6 +33,7 @@ export class MockGuiConfigService { selectingFilesFromDatasetsEnabled: false, localLogin: true, googleLogin: true, + orcidLogin: true, inviteOnly: false, userPresetEnabled: true, workflowExecutionsTrackingEnabled: false, diff --git a/frontend/src/app/common/service/gui-config.service.ts b/frontend/src/app/common/service/gui-config.service.ts index 7806f1a680b..86a9b895f69 100644 --- a/frontend/src/app/common/service/gui-config.service.ts +++ b/frontend/src/app/common/service/gui-config.service.ts @@ -28,7 +28,10 @@ import { AppSettings } from "../app-setting"; // least invasive fix. const ACCESS_TOKEN_KEY = "access_token"; -type PreLoginConfig = Pick; +type PreLoginConfig = Pick< + GuiConfig, + "localLogin" | "googleLogin" | "orcidLogin" | "defaultLocalUser" | "attributionEnabled" +>; // Fields served by /config/amber. type AmberConfig = Pick; type GuiOnlyConfig = Omit; diff --git a/frontend/src/app/common/service/user/auth.service.ts b/frontend/src/app/common/service/user/auth.service.ts index 15baa8a9233..6094d245ffc 100644 --- a/frontend/src/app/common/service/user/auth.service.ts +++ b/frontend/src/app/common/service/user/auth.service.ts @@ -46,6 +46,7 @@ export class AuthService { public static readonly REFRESH_TOKEN = "auth/refresh"; public static readonly REGISTER_ENDPOINT = "auth/register"; public static readonly GOOGLE_LOGIN_ENDPOINT = "auth/google/login"; + public static readonly ORCID_LOGIN_ENDPOINT = "auth/orcid/login"; private tokenExpirationSubscription?: Subscription; @@ -79,7 +80,6 @@ export class AuthService { /** * This method will handle the request for Google login. * It will automatically login, save the user account inside and trigger userChangeEvent when success - */ public googleAuth(credential: string): Observable> { return this.http.post>( @@ -94,6 +94,19 @@ export class AuthService { ); } + public orcidAuth(code: string): Observable> { + return this.http.post>( + `${AppSettings.getApiEndpoint()}/${AuthService.GOOGLE_LOGIN_ENDPOINT}`, + code, + { + headers: { + "Content-Type": "text/plain", + Accept: "application/json", + }, + } + ); + } + /** * This method will handle the request for user login. * It will automatically login, save the user account inside and trigger userChangeEvent when success diff --git a/frontend/src/app/common/service/user/orcid-auth.service.ts b/frontend/src/app/common/service/user/orcid-auth.service.ts new file mode 100644 index 00000000000..9ca63473139 --- /dev/null +++ b/frontend/src/app/common/service/user/orcid-auth.service.ts @@ -0,0 +1,51 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Injectable } from "@angular/core"; +import { Observable } from "rxjs"; +import { HttpClient } from "@angular/common/http"; +import { AppSettings } from "../../app-setting"; + +/** + * What the login page needs to send the browser to ORCID: the registered client id, and the + * authorize endpoint of whichever ORCID deployment this backend is configured against + * (sandbox or production). Both come from the server so the two can never disagree. + */ +export interface OrcidConfig { + clientId: string; + authorizeUrl: string; +} + +/** + * sessionStorage key holding the CSRF `state` value across the ORCID round trip. Written by the + * login page before it redirects, read back by the callback page — shared here so the two sides + * cannot drift apart. + */ +export const ORCID_STATE_KEY = "orcid_state"; + +@Injectable({ + providedIn: "root", +}) +export class OrcidAuthService { + constructor(private http: HttpClient) {} + + getConfig(): Observable { + return this.http.get(`${AppSettings.getApiEndpoint()}/auth/orcid/config`); + } +} diff --git a/frontend/src/app/common/service/user/user.service.ts b/frontend/src/app/common/service/user/user.service.ts index bdb5f1c279f..603e7c66378 100644 --- a/frontend/src/app/common/service/user/user.service.ts +++ b/frontend/src/app/common/service/user/user.service.ts @@ -25,6 +25,7 @@ import { Role, User } from "../../type/user"; import { AuthService } from "./auth.service"; import { GuiConfigService } from "../gui-config.service"; import { catchError, map, shareReplay, switchMap } from "rxjs/operators"; +import { UnimplementedException } from "@angular-devkit/schematics"; /** * User Service manages User information. It relies on different @@ -64,6 +65,10 @@ export class UserService { .pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); } + public orcidLogin(code: string): Observable { + return this.authService.orcidAuth(code).pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); + } + public isLogin(): boolean { return this.currentUser !== undefined; } diff --git a/frontend/src/app/common/type/gui-config.ts b/frontend/src/app/common/type/gui-config.ts index 5da549acd96..f2f09b17ac0 100644 --- a/frontend/src/app/common/type/gui-config.ts +++ b/frontend/src/app/common/type/gui-config.ts @@ -24,6 +24,7 @@ export interface GuiConfig { selectingFilesFromDatasetsEnabled: boolean; localLogin: boolean; googleLogin: boolean; + orcidLogin: boolean; inviteOnly: boolean; userPresetEnabled: boolean; workflowExecutionsTrackingEnabled: boolean; diff --git a/frontend/src/app/hub/component/login/orcid-callback.component.ts b/frontend/src/app/hub/component/login/orcid-callback.component.ts new file mode 100644 index 00000000000..6aff1e7f2a5 --- /dev/null +++ b/frontend/src/app/hub/component/login/orcid-callback.component.ts @@ -0,0 +1,40 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; +import { Component, OnInit } from "@angular/core"; +import { ActivatedRoute } from "@angular/router"; +import { UserService } from "../../../common/service/user/user.service"; + +@UntilDestroy() +@Component({ + selector: "texera-orcid-callback", + template: "...", + imports: [], +}) +export class OrcidCallbackComponent implements OnInit { + constructor( + private route: ActivatedRoute, + private userService: UserService + ) {} + + ngOnInit(): void { + const code = this.route.snapshot.queryParams.get("code"); + this.userService.orcidLogin(code).pipe(untilDestroyed(this)).subscribe(); + } +} diff --git a/frontend/src/app/hub/component/login/texera-login.component.html b/frontend/src/app/hub/component/login/texera-login.component.html index 7a675973158..e0250e2e4c3 100644 --- a/frontend/src/app/hub/component/login/texera-login.component.html +++ b/frontend/src/app/hub/component/login/texera-login.component.html @@ -47,10 +47,22 @@ size="large" [width]="328"> + } @if (config.env.orcidLogin){ + } - @if (config.env.localLogin && config.env.googleLogin) { + @if (config.env.localLogin && (config.env.googleLogin || config.env.orcidLogin)) { diff --git a/frontend/src/app/hub/component/login/texera-login.component.scss b/frontend/src/app/hub/component/login/texera-login.component.scss index 8315a72ca45..57bdbd60f58 100644 --- a/frontend/src/app/hub/component/login/texera-login.component.scss +++ b/frontend/src/app/hub/component/login/texera-login.component.scss @@ -86,6 +86,39 @@ $text-secondary: rgba(0, 0, 0, 0.45); margin: -6px 2px 0; } +.orcid-login { + display: flex; + align-items: center; + padding-inline: 12px; + width: 328px; + height: 40px; + border-radius: 4px; + gap: 6px; + transition-duration: 0.15s; + + .orcid-icon { + width: 20px; + height: 20px; + flex: none; + } + + .orcid-label { + flex: 1; + text-align: center; + } + + &:hover, + &:focus { + background-color: rgba(217, 217, 250, 0.3); + border-color: #d9d9d9; + color: rgba(0, 0, 0, 0.88); + transition-duration: 0.15s; + } + + &:active { + background-color: rgba(0, 0, 0, 0.08); + } +} .foot { text-align: center; font-size: 13px; diff --git a/frontend/src/app/hub/component/login/texera-login.component.ts b/frontend/src/app/hub/component/login/texera-login.component.ts index 578f7ddda20..7c80c50dadb 100644 --- a/frontend/src/app/hub/component/login/texera-login.component.ts +++ b/frontend/src/app/hub/component/login/texera-login.component.ts @@ -29,7 +29,7 @@ import { } from "@angular/forms"; import { ActivatedRoute, Router } from "@angular/router"; import { catchError, filter } from "rxjs/operators"; -import { throwError } from "rxjs"; +import { EMPTY, throwError } from "rxjs"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { SocialAuthService, GoogleSigninButtonModule, SocialUser } from "@abacritt/angularx-social-login"; import { UserService } from "../../../common/service/user/user.service"; @@ -42,6 +42,7 @@ import { NzInputDirective, NzInputGroupComponent, NzInputGroupWhitSuffixOrPrefix import { NzButtonComponent } from "ng-zorro-antd/button"; import { NzDividerComponent } from "ng-zorro-antd/divider"; import { NzTypographyComponent } from "ng-zorro-antd/typography"; +import { ORCID_STATE_KEY, OrcidAuthService, OrcidConfig } from "../../../common/service/user/orcid-auth.service"; type LoginMode = "signin" | "signup"; @@ -76,9 +77,12 @@ export class TexeraLoginComponent implements OnInit { public mode: LoginMode = "signin"; public passwordVisible = false; public errorMessage: string | undefined; - public form: FormGroup; + // Undefined until the fetch in ngOnInit lands; the ORCID button stays disabled until then. + // Protected rather than private because the template reads it for that disabled binding. + protected orcidConfig: OrcidConfig | undefined; + constructor( private formBuilder: FormBuilder, private userService: UserService, @@ -87,6 +91,7 @@ export class TexeraLoginComponent implements OnInit { private router: Router, private ngZone: NgZone, private socialAuthService: SocialAuthService, + private orcidAuthService: OrcidAuthService, protected config: GuiConfigService ) { this.form = this.formBuilder.group({ @@ -114,6 +119,24 @@ export class TexeraLoginComponent implements OnInit { }); } + // Fetched up front rather than on click so the redirect is instant; until it arrives the + // button is disabled. EMPTY rather than a rethrow because this is background setup with no + // caller to propagate to — a failure leaves the button disabled, which fails safe. + if (this.config.env.orcidLogin) { + this.orcidAuthService + .getConfig() + .pipe( + catchError(() => { + this.notificationService.error("ORCID sign-in is unavailable"); + return EMPTY; + }), + untilDestroyed(this) + ) + .subscribe(orcidConfig => { + this.orcidConfig = orcidConfig; + }); + } + // Google emits the signed-in user here after its own button completes the flow. // The null filter matters: logging out pushes null through this subject, and it is a // ReplaySubject, so that stale null is replayed into this subscription the moment it starts. @@ -235,4 +258,32 @@ export class TexeraLoginComponent implements OnInit { } return null; }; + + /** + * Hand the browser to ORCID's consent screen. Unlike Google — whose SDK runs the whole + * handshake in a popup and emits a token — ORCID is plain authorization-code OAuth, so this + * leaves the app entirely and comes back at `/callback/orcid` with a `code` to exchange. + */ + protected orcidLogin(): void { + // Unreachable while the template keeps the button disabled, but the narrowing is needed + // regardless, and the guard outlives whoever might drop that binding later. + const config = this.orcidConfig; + if (!config) { + this.notificationService.error("ORCID sign-in is unavailable"); + return; + } + + const state = crypto.randomUUID(); + sessionStorage.setItem(ORCID_STATE_KEY, state); + + const params = new URLSearchParams({ + client_id: config.clientId, + response_type: "code", + scope: "/authenticate", + redirect_uri: `${window.location.origin}/callback/orcid`, + state, + }); + + window.location.href = `${config.authorizeUrl}?${params}`; + } } diff --git a/frontend/src/assets/logos/ORCID-iD_icon_24x24.png b/frontend/src/assets/logos/ORCID-iD_icon_24x24.png new file mode 100755 index 0000000000000000000000000000000000000000..4447d462832094ef884577598dc5e4bd723112fc GIT binary patch literal 1358 zcmeAS@N?(olHy`uVBq!ia0vp^5+KaM3?#3wJbMaAl?M2PxB}^A=PZ|=vs`}OV*YuH zrDrXcowb~Q-T^4M;yh5?V)=P1AX;|LV#!&{WoONS;{X5ufBE_B>Wi)$uLOPh`RwkS zt$S`|9e>dL<;SyMe?Fak-2dY9xyK(4o_*4PDyx3Ul2E!%h{@ZGmt+pfo5dN$|A ztCdGz{7eU0EKm~U7tHW}zk)(Tfx!Lz{q^$~6cn7_BsU)@ce*Mhq9iD>T%n*SKP@vS zRiUJ^AXOo=pd^`rp<<5j1Z%#-3IZ;_ZCm>fq|Y=hT~(r4#K_$&q8YU4&i)e)A~_Rs z9-Ge>E!ulm|Mc(AS^eo#m+H?C>s-xtc9-?jdD_+SF&}2m7rfW>(&qUC0i8$}H`e=N z){{9!XBKRBk?}v~mL?|ge8VM~_HW`TE9&R&n5(dO!56X1iHRDw6s27EE(^S#c_=U? zMKmtM*JL^8Hbx6}bp^?9%0hqqziQ+>H!w7qKY2mqi-efK`cKE6aO(zpS0@`DW@lhv zI-d#jR)nvQRdRl=USdjqQmS4>ZUNB03=B5*6$OdO*{LN8NvY|XdA92BckfqM$V{$ES@GWpo&B*kqDoPEm@(W3>%1*XSQMb3_vZ=5F8jzb>lBiITo0C^;Rbi_RHrEQs z1_|q{D}a@hWZSBH<|d}6T3NYPWTu7W=jSLG8tIvus{;)wN=dT{a&dzi0p!`LXOxr_ zSn2DRmzV368|&p4rRy77T3YHG80i}s=>k>g7FXt#Bv$C=6)OWx;8Fma;gVXMTm+1y z%=|nBkeP`|`K2Yc>grqyP@_|l;U*W97Uh7=O-a^I%}LEo%_}L^H`FuK2O6iKV3U$; zRa}~sm6}`v;zEpfN=yfHAiSJRyTqar8-0*PKsHDfTtXqKv^X;_wYV6EbV_D%R%u=` zF5T%xi3J&%$qFf{#hK}OILye*$tf)^DM|$S7MGIZ{G7y+)D#@jd5I;ZMX9(X^A!qG zi;8iYU6fiPp zSp-ajZ;ZAt1E$|&o-U3d6}M6^x=#XTO^1tYQ&Owzw5@#nzS}?I>DZ+CB}_#A1*_22 zy}1W&Y?~00xYJr?rKyUKYM}qJ{^yO?6IBG?iLmW><#p(XrF+AB=_PAC8oyin@Z38+ zbHOzAJ?V1;_QtGR^;z9P^P6B=LR86(dq$oer$sYbY-(OeAFx}qtL4IzhcbCHYAhbF z5x(H*DRu1o`dk;)ugWa9?fkZF&pUnM(cd){?1F`DvX7tK@0{z^G|5NvV&?y?6RnG` jTYB!iCY7;kQ=fFbNvRx5-yww$pe*U>>gTe~DWM4fBYP6n literal 0 HcmV?d00001 From e44ba4cc0dab3cce7e7d2a6b9c6917194db0ddbc Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Thu, 13 Aug 2026 11:57:38 -0700 Subject: [PATCH 2/6] feat(auth): add ORCID backend logic / email verification --- .../web/resource/auth/AuthResource.scala | 127 +++++++++- .../auth/ExternalAuthProvisioner.scala | 35 ++- .../resource/auth/GoogleAuthResource.scala | 4 +- .../web/resource/auth/OrcidAuthResource.scala | 225 +++++++++++++++++- .../web/resource/auth/AuthResourceSpec.scala | 119 ++++++++- .../auth/ExternalAuthProvisionerSpec.scala | 66 ++++- .../src/main/resources/user-system.conf | 6 + .../common/config/UserSystemConfig.scala | 2 + .../app/common/service/user/auth.service.ts | 23 +- .../common/service/user/stub-auth.service.ts | 8 + .../common/service/user/stub-user.service.ts | 8 + .../common/service/user/user.service.spec.ts | 47 +++- .../app/common/service/user/user.service.ts | 87 ++++++- frontend/src/app/common/type/user.ts | 4 +- .../admin/user/admin-user.component.spec.ts | 10 +- .../admin/user/admin-user.component.ts | 4 +- .../service/user/flarum/flarum.service.ts | 10 +- .../login/orcid-callback.component.ts | 41 +++- sql/changelog.xml | 5 + sql/texera_ddl.sql | 2 +- 20 files changed, 783 insertions(+), 50 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index b05ed180bd9..708f0c6dd89 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -20,14 +20,21 @@ package org.apache.texera.web.resource.auth import com.typesafe.scalalogging.Logger +import io.dropwizard.auth.Auth import org.apache.texera.auth.JwtAuth.{jwtClaims, jwtToken} +import org.apache.texera.auth.SessionUser import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.common.util.EmailUtil import org.apache.texera.dao.SqlServer import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} +import org.apache.texera.dao.jooq.generated.tables.daos.UserDao import org.apache.texera.dao.jooq.generated.tables.pojos.User -import org.apache.texera.web.model.http.request.auth.{UserLoginRequest, UserRegistrationRequest} +import org.apache.texera.web.model.http.request.auth.{ + SetEmailRequest, + UserLoginRequest, + UserRegistrationRequest +} import org.apache.texera.web.model.http.response.TokenIssueResponse import org.apache.texera.web.resource.auth.AuthResource._ import org.jooq.DSLContext @@ -35,8 +42,9 @@ import org.jooq.impl.DSL import java.time.Instant import java.time.temporal.ChronoUnit +import javax.annotation.security.RolesAllowed import javax.ws.rs._ -import javax.ws.rs.core.MediaType +import javax.ws.rs.core.{MediaType, Response} object AuthResource { private val logger: Logger = Logger(classOf[AuthResource]) @@ -145,6 +153,121 @@ object AuthResource { @Produces(Array(MediaType.APPLICATION_JSON)) class AuthResource { + /** + * Give the signed-in account the email address it does not have yet, and reissue its token so + * the `email` claim stops being null. + * + * This exists because an identity-only provider (ORCID) authenticates someone without + * asserting an address, while email is what the rest of the product addresses a user by — + * dataset paths are built from it and every access grant names one. So the account is real and + * signed in, but inert until this runs. + * + * The address is whatever the user typed, so it buys nothing that a verified one would: + * + * - It may create the account's own identity (the ordinary case) or claim a contributor + * placeholder, both of which the register path already does on an unverified address + * (see `register`). + * - It may never attach the caller to an account that already holds a credential. That + * account's owner has not consented, and anyone can type their address — it is the takeover + * [[ExternalProfile]] describes. Those callers are told to sign in with that account + * instead, and can link ORCID to it afterwards. + * + * Filling a blank only: changing an address that is already set is a different operation, with + * a different threat model, and is refused here. + */ + @PUT + @Path("/email") + @RolesAllowed(Array("INACTIVE", "RESTRICTED", "REGULAR", "ADMIN")) + def setEmail(request: SetEmailRequest, @Auth sessionUser: SessionUser): TokenIssueResponse = { + val email = Option(request.email).getOrElse("").trim + if (email.isEmpty) throw new NotAcceptableException("Email cannot be empty") + if (!EmailUtil.isValid(email)) throw new NotAcceptableException("Email format is invalid.") + + val user = SqlServer.withTransaction(SqlServer.getInstance().createDSLContext()) { ctx => + val txUserDao = new UserDao(ctx.configuration()) + + // Re-read inside the transaction: the pojo on the session was built from the token and may + // be minutes old, so it is not evidence about the row as it stands now. + val current = txUserDao.fetchOneByUid(sessionUser.getUid) + if (current == null) throw new NotAuthorizedException("Login credentials are incorrect.") + if (current.getEmail != null) { + throw new WebApplicationException( + "This account already has an email address.", + Response.Status.CONFLICT + ) + } + + Option(fetchUserByEmailIgnoreCase(ctx, email)) match { + case None => + current.setEmail(email) + txUserDao.update(current) + current + + case Some(existing) if existing.getIsPlaceholder => + adoptPlaceholder(ctx, txUserDao, current, existing) + + case Some(_) => + throw new WebApplicationException( + "That email address already belongs to an account. Sign in to that account instead.", + Response.Status.CONFLICT + ) + } + } + + TokenIssueResponse( + jwtToken( + jwtClaims(user, ExternalAuthProvisioner.providerIdOf(user.getUid, ProviderTypeEnum.GOOGLE)) + ) + ) + } + + /** + * Move the caller's external identity onto the contributor placeholder that owns `email`, and + * drop the account the identity provider created moments ago. + * + * Keeping the placeholder's uid is the whole point: dataset contributor rows already reference + * it, and re-pointing those instead would mean touching every table that FKs to `"user"`. It + * mirrors what `register` does when a registration presents a placeholder's address. + * + * Discarding the caller's own row is only safe because of what it cannot have accumulated: it + * has no email, so nothing email-keyed can name it, and it is INACTIVE, so every + * content-creating endpoint (all of which require REGULAR or ADMIN) has refused it. A caller + * past that point keeps its account and is refused instead. + */ + private def adoptPlaceholder( + ctx: DSLContext, + txUserDao: UserDao, + current: User, + placeholder: User + ): User = { + val callerIsEmpty = current.getRole == UserRoleEnum.INACTIVE + val placeholderHasCredential = ctx.fetchExists( + ctx.selectFrom(AUTH_PROVIDER).where(AUTH_PROVIDER.UID.eq(placeholder.getUid)) + ) + if (!callerIsEmpty || placeholderHasCredential) { + throw new WebApplicationException( + "That email address already belongs to an account. Sign in to that account instead.", + Response.Status.CONFLICT + ) + } + + ctx + .update(AUTH_PROVIDER) + .set(AUTH_PROVIDER.UID, placeholder.getUid) + .where(AUTH_PROVIDER.UID.eq(current.getUid)) + .execute() + + // The provider's display name is the user's own, so it wins over the one whoever listed them + // as a contributor typed. + placeholder.setName(current.getName) + claimPlaceholder(placeholder) + txUserDao.update(placeholder) + + // Last, so the provider rows have already moved off it: auth_provider cascades on delete. + txUserDao.deleteById(current.getUid) + placeholder + } + @POST @Path("/login") def login(request: UserLoginRequest): TokenIssueResponse = { diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala index f5bb43cbf74..e2ca8cdc9ef 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisioner.scala @@ -32,11 +32,20 @@ import java.time.OffsetDateTime import scala.util.chaining.scalaUtilChainingOps /** - * A verified external identity (Google, Facebook, ...) reduced to the fields we persist. + * A verified external identity (Google, ORCID, ...) reduced to the fields we persist. * - * `email` must be non-blank and provider-verified: `loginOrProvision` links the identity to the - * account owning that address and claims its placeholder, so an unverified address is a - * takeover. Each provider checks this in its own mapping function (Google: `email_verified`). + * `email`, when present, must be non-blank and provider-verified: `loginOrProvision` links the + * identity to the account owning that address and claims its placeholder, so an unverified + * address is a takeover. Each provider checks this in its own mapping function (Google: + * `email_verified`). + * + * `None` means the provider authenticates an identity without asserting an address — ORCID, + * whose `/authenticate` scope yields an iD and a name and nothing else. Such a login provisions + * an account with a NULL email and is deliberately never matched to an existing one, because + * the only address available for matching would be one the user typed. The account is + * functional for signing in but not for the email-keyed parts of the product (dataset paths, + * access grants), so the frontend collects an address before it is useful — see + * `AuthResource.setEmail`. * * `avatar` is the complete URL the provider supplied, already sanitized by `AvatarUtil`. * `None` means the provider offered no avatar we would store, in which case the account keeps @@ -46,7 +55,7 @@ final case class ExternalProfile( providerType: ProviderTypeEnum, providerId: String, name: String, - email: String, + email: Option[String], avatar: Option[String] ) @@ -98,7 +107,9 @@ object ExternalAuthProvisioner extends LazyLogging { } case None => - val user = userByEmailIgnoreCase(ctx, profile.email) match { + // An identity-only provider (`email` is None) skips the lookup entirely rather than + // matching on nothing, so it always lands in the insert branch below. + val user = profile.email.flatMap(userByEmailIgnoreCase(ctx, _)) match { case Some(existing) => existing.tap { user => val wasPlaceholder = user.getIsPlaceholder @@ -109,7 +120,9 @@ object ExternalAuthProvisioner extends LazyLogging { case None => val created = new User() created.setName(profile.name) - created.setEmail(profile.email) + // Left NULL for an identity-only provider. The column is nullable and its UNIQUE + // index tolerates repeated NULLs, so several such accounts can coexist. + profile.email.foreach(created.setEmail) profile.avatar.foreach(created.setAvatar) created.setRole(UserRoleEnum.INACTIVE) txUserDao.insert(created) @@ -137,6 +150,10 @@ object ExternalAuthProvisioner extends LazyLogging { /** * Mutate `user` in place to match `profile`, returning true iff anything changed * (so the caller only issues an UPDATE when needed). + * + * A field the provider did not assert is left as it is rather than blanked: an identity-only + * provider carries no address, and on a returning login the account may well have one by then + * — collected through `AuthResource.setEmail` — which this must not undo. */ private def refresh(user: User, profile: ExternalProfile): Boolean = { var changed = false @@ -144,8 +161,8 @@ object ExternalAuthProvisioner extends LazyLogging { user.setName(profile.name) changed = true } - if (user.getEmail != profile.email) { - user.setEmail(profile.email) + profile.email.filter(_ != user.getEmail).foreach { email => + user.setEmail(email) changed = true } profile.avatar.filter(_ != user.getAvatar).foreach { url => diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala index 89f1cc50005..f739407cc20 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleAuthResource.scala @@ -59,7 +59,9 @@ object GoogleAuthResource { ProviderTypeEnum.GOOGLE, payload.getSubject, Option(payload.get("name").asInstanceOf[String]).filter(_.nonEmpty).getOrElse(googleEmail), - googleEmail, + // Always `Some`: the checks above refuse a payload without a verified address, so Google + // remains an email-asserting provider and keeps linking to existing accounts. + Some(googleEmail), avatar = AvatarUtil.sanitize(Option(payload.get("picture").asInstanceOf[String])) ) } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala index b550d54c7bd..88027683acd 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala @@ -1,33 +1,238 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + package org.apache.texera.web.resource.auth +import com.fasterxml.jackson.databind.{JsonNode, ObjectMapper} +import com.typesafe.scalalogging.Logger +import kong.unirest.Unirest +import org.apache.texera.auth.JwtAuth.{jwtClaims, jwtToken} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.common.config.UserSystemConfig.orcidBaseUrl -import org.apache.texera.web.model.http.response.TokenIssueResponse -import org.apache.texera.web.resource.auth.OrcidAuthResource.clientId +import org.apache.texera.common.util.EmailUtil +import org.apache.texera.dao.jooq.generated.enums.ProviderTypeEnum +import org.apache.texera.web.model.http.response.OrcidLoginResponse +import org.apache.texera.web.resource.auth.OrcidAuthResource._ +import java.net.URLEncoder +import java.nio.charset.StandardCharsets import javax.ws.rs.core.MediaType -import javax.ws.rs.{Consumes, GET, POST, Path, Produces} +import javax.ws.rs.{Consumes, GET, NotAuthorizedException, POST, Path, Produces} +import scala.jdk.CollectionConverters.IteratorHasAsScala object OrcidAuthResource { + private val logger: Logger = Logger(classOf[OrcidAuthResource]) + final private lazy val clientId = UserSystemConfig.orcidClientId + final private lazy val clientSecret = UserSystemConfig.orcidClientSecret + final private lazy val redirectUri = UserSystemConfig.orcidRedirectUri + + // A user is waiting on the callback page while these run, so both sit far below the browser's + // patience: ORCID either answers promptly or this login has failed. + private val CONNECT_TIMEOUT_MS = 5000 + private val SOCKET_TIMEOUT_MS = 10000 + + private val mapper = new ObjectMapper() + + /** + * The identity behind a redeemed authorization code. `orcidId` is the ORCID iD + * (`0000-0002-1825-0097`); `name` is absent when the record's owner keeps it private. + * + * Both arrived over the back channel, on a connection our client secret opened, which is what + * separates them from anything in the redirect URL: the browser cannot have chosen them. + */ + private[auth] final case class OrcidIdentity(orcidId: String, name: Option[String]) + + private def textOf(node: JsonNode, field: String): Option[String] = + Option(node.path(field).asText(null)).map(_.trim).filter(_.nonEmpty) + + /** `a=1&b=2` with both halves percent-encoded — the client secret in particular may need it. */ + private def formEncode(fields: Seq[(String, String)]): String = + fields + .map { + case (name, value) => + s"${URLEncoder.encode(name, StandardCharsets.UTF_8)}=${URLEncoder.encode(value, StandardCharsets.UTF_8)}" + } + .mkString("&") + + /** + * Read the identity out of a token-endpoint response body. + * + * A body with no `orcid` is refused rather than defaulted: it means the exchange authenticated + * nobody, and provisioning against a synthesized id would hand out an account. + */ + private[auth] def identityOf(body: String): OrcidIdentity = { + val tree = mapper.readTree(body) + OrcidIdentity( + textOf(tree, "orcid").getOrElse( + throw new NotAuthorizedException("Login credentials are incorrect.") + ), + textOf(tree, "name") + ) + } + + /** + * The address to offer as a prefill from ORCID's email response, preferring the one the record + * marks primary. Anything unparseable yields None, which costs a filled-in form field. + */ + private[auth] def prefillFrom(body: String): Option[String] = { + val entries = mapper.readTree(body).path("email") + if (!entries.isArray) None + else { + val all = entries.elements().asScala.toSeq + all + .find(_.path("primary").asBoolean(false)) + .orElse(all.headOption) + .flatMap(textOf(_, "email")) + .filter(EmailUtil.isValid) + } + } } +/** + * ORCID sign-in. Unlike Google — whose SDK runs the whole handshake in the browser and hands the + * frontend a signed id-token to post here — ORCID is plain authorization-code OAuth, so its + * second leg happens on this side: the frontend forwards the one-time `code` it was redirected to + * `/callback/orcid` with, and this trades it for the identity behind it. That code is useless + * without `clientSecret`, which is the only reason it may travel through a browser at all. + * + * ORCID asserts no email under the `/authenticate` scope the login page requests, so the account + * provisioned here has a NULL email and is deliberately not matched against any existing account. + * See [[ExternalProfile]] for why that is the safe reading, and `AuthResource.setEmail` for how an + * address is collected once the user is in. + */ @Path("/auth/orcid") class OrcidAuthResource { @GET @Path("/config") @Produces(Array(MediaType.APPLICATION_JSON)) - def getConfig: Map[String, String] = Map( - "clientId" -> clientId, - "authorizeUrl" -> s"$orcidBaseUrl/oauth/authorize" - ) + def getConfig: Map[String, String] = + Map( + "clientId" -> clientId, + "authorizeUrl" -> s"$orcidBaseUrl/oauth/authorize" + ) + + /** + * Trade `code` for ORCID's token response, returning the raw body. + * + * `redirect_uri` is read from configuration rather than the request: ORCID requires it to match + * the authorize call byte-for-byte, and honouring a caller-supplied one would let the browser + * choose which registered redirect an exchange is attributed to. + * + * One of the two seams that reach the network. Kept as a method rather than a constructor + * parameter for the same reason [[GoogleAuthResource.verifiedPayload]] is: Jersey instantiates + * this resource from `classOf[OrcidAuthResource]`, so tests override instead of injecting. + */ + protected def exchangeCode(code: String): String = { + // Encoded by hand rather than with Unirest's `.field()`, which switches the request to + // multipart/form-data under conditions that are not obvious from the call site. ORCID accepts + // only application/x-www-form-urlencoded here, so the encoding is stated outright. + val form = formEncode( + Seq( + "client_id" -> clientId, + "client_secret" -> clientSecret, + "grant_type" -> "authorization_code", + "code" -> code, + "redirect_uri" -> redirectUri + ) + ) + + val response = Unirest + .post(s"$orcidBaseUrl/oauth/token") + .header("Content-Type", MediaType.APPLICATION_FORM_URLENCODED) + .header("Accept", MediaType.APPLICATION_JSON) + .body(form) + .connectTimeout(CONNECT_TIMEOUT_MS) + .socketTimeout(SOCKET_TIMEOUT_MS) + .asString() + + if (response.getStatus != 200) { + // Status only. The body of a failed exchange quotes the request back, and the body of a + // successful one carries a bearer token; neither belongs in a log. + logger.warn(s"ORCID token exchange returned ${response.getStatus}") + throw new NotAuthorizedException("Login credentials are incorrect.") + } + response.getBody + } + + /** + * The email this ORCID record publishes, if any — a prefill for the address prompt, never a key + * anything is matched on. ORCID returns only addresses the owner chose to make public, and an + * address being public is no evidence the owner controls it, so linking on this would be + * exactly the takeover [[ExternalProfile]] warns about. + * + * Best-effort: a failure here costs a prefilled form field, so it is logged and swallowed + * rather than failing a login that has already succeeded. + */ + protected def publishedEmail(orcidId: String, accessToken: String): Option[String] = + try { + val response = Unirest + .get(s"$orcidBaseUrl/v3.0/$orcidId/email") + .header("Accept", MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer $accessToken") + .connectTimeout(CONNECT_TIMEOUT_MS) + .socketTimeout(SOCKET_TIMEOUT_MS) + .asString() + + if (response.getStatus != 200) { + logger.info(s"ORCID published-email lookup returned ${response.getStatus}") + None + } else prefillFrom(response.getBody) + } catch { + case e: Exception => + logger.info(s"ORCID published-email lookup failed: ${e.getClass.getSimpleName}") + None + } @POST @Consumes(Array(MediaType.TEXT_PLAIN)) @Produces(Array(MediaType.APPLICATION_JSON)) @Path("/login") - def login(code: String): TokenIssueResponse = { - print("this works!") - throw new NotImplementedError("you haven't actually done sign in") + def login(code: String): OrcidLoginResponse = { + val trimmedCode = Option(code).map(_.trim).filter(_.nonEmpty).getOrElse { + throw new NotAuthorizedException("Login credentials are incorrect.") + } + + val body = exchangeCode(trimmedCode) + val identity = identityOf(body) + + val user = ExternalAuthProvisioner.loginOrProvision( + ExternalProfile( + ProviderTypeEnum.ORCID, + identity.orcidId, + // The iD stands in for a private name: `"user".name` is NOT NULL, and an ORCID iD is at + // least a real handle rather than an invented placeholder. + identity.name.getOrElse(identity.orcidId), + email = None, + avatar = None + ) + ) + + // Only worth a second round trip on a login that will actually prompt for an address — a + // returning user who already supplied one is not asked again. + val suggestedEmail = Option(user.getEmail) match { + case Some(_) => None + case None => + textOf(mapper.readTree(body), "access_token") + .flatMap(token => publishedEmail(identity.orcidId, token)) + } + + OrcidLoginResponse(jwtToken(jwtClaims(user, Some(identity.orcidId))), suggestedEmail) } } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala index d323d4c9757..17f4337ea83 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala @@ -19,21 +19,25 @@ package org.apache.texera.web.resource.auth -import org.apache.texera.auth.JwtAuth +import org.apache.texera.auth.{JwtAuth, SessionUser} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.MockTexeraDB import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} -import org.apache.texera.web.model.http.request.auth.{UserLoginRequest, UserRegistrationRequest} +import org.apache.texera.web.model.http.request.auth.{ + SetEmailRequest, + UserLoginRequest, + UserRegistrationRequest +} import org.jasypt.util.password.StrongPasswordEncryptor import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} import java.util.UUID -import javax.ws.rs.{NotAcceptableException, NotAuthorizedException} +import javax.ws.rs.{NotAcceptableException, NotAuthorizedException, WebApplicationException} class AuthResourceSpec extends AnyFlatSpec @@ -378,4 +382,113 @@ class AuthResourceSpec AuthResource.createAdminUser() userDao.fetchByName(UserSystemConfig.adminUsername).size() shouldBe 1 } + + // ─── setEmail ─────────────────────────────────────────────────────────────── + + /** An account as an identity-only login leaves it: authenticated, INACTIVE, no address. */ + private def seedEmaillessUser(tag: String, role: UserRoleEnum = UserRoleEnum.INACTIVE): User = { + val user = new User + user.setName(uname(tag)) + user.setRole(role) + userDao.insert(user) + seedExternalProvider(user.getUid, ProviderTypeEnum.ORCID, s"0000-0002-0000-$tag") + user + } + + private def emailClaimOf(token: String): AnyRef = + JwtAuth.jwtConsumer.processToClaims(token).getClaimValue("email") + + private def statusOf(thrown: WebApplicationException): Int = thrown.getResponse.getStatus + + "setEmail" should "store the address and reissue a token carrying it" in { + val user = seedEmaillessUser("fill") + + val response = resource.setEmail(SetEmailRequest(uemail("fill")), new SessionUser(user)) + + userDao.fetchOneByUid(user.getUid).getEmail shouldBe uemail("fill") + emailClaimOf(response.accessToken) shouldBe uemail("fill") + } + + it should "reject a malformed address" in { + val user = seedEmaillessUser("bad") + + assertThrows[NotAcceptableException] { + resource.setEmail(SetEmailRequest("not-an-address"), new SessionUser(user)) + } + userDao.fetchOneByUid(user.getUid).getEmail shouldBe null + } + + it should "reject a blank address" in { + val user = seedEmaillessUser("blank") + + assertThrows[NotAcceptableException] { + resource.setEmail(SetEmailRequest(" "), new SessionUser(user)) + } + } + + // Filling a blank only — replacing an address that is already set is a different operation with + // a different threat model. + it should "refuse to replace an address that is already set" in { + val user = seedUser(uname("has"), "pw") + + val thrown = intercept[WebApplicationException] { + resource.setEmail(SetEmailRequest(uemail("other")), new SessionUser(user)) + } + + statusOf(thrown) shouldBe 409 + userDao.fetchOneByUid(user.getUid).getEmail shouldBe s"${uname("has")}@example.com" + } + + // Anyone can type someone else's address, so attaching to an account that already holds a + // credential would be a takeover of it. + it should "refuse an address owned by an account that holds a credential" in { + val owner = seedUser(uname("owner"), "pw") + val caller = seedEmaillessUser("intruder") + + val thrown = intercept[WebApplicationException] { + resource.setEmail(SetEmailRequest(s"${uname("owner")}@example.com"), new SessionUser(caller)) + } + + statusOf(thrown) shouldBe 409 + userDao.fetchOneByUid(caller.getUid).getEmail shouldBe null + hasProvider(owner.getUid, ProviderTypeEnum.ORCID) shouldBe false + hasProvider(owner.getUid, ProviderTypeEnum.LOCAL) shouldBe true + } + + // The placeholder keeps its uid because dataset contributor rows already reference it; the + // caller's own row is discarded, which is only safe while it is INACTIVE and emailless. + it should "move the identity onto a contributor placeholder owning the address" in { + val placeholder = seedPlaceholder(uname("ghost"), uemail("ghost")) + val caller = seedEmaillessUser("claimer") + + val response = resource.setEmail(SetEmailRequest(uemail("ghost")), new SessionUser(caller)) + + val claimed = userDao.fetchOneByUid(placeholder.getUid) + claimed.getIsPlaceholder shouldBe false + claimed.getComment should include("Claimed contributor placeholder at ") + claimed.getName shouldBe uname("claimer") + hasProvider(placeholder.getUid, ProviderTypeEnum.ORCID) shouldBe true + providerIdOf(placeholder.getUid, ProviderTypeEnum.ORCID) shouldBe "0000-0002-0000-claimer" + + // The account the provider created moments ago is gone, and the session continues as the + // claimed one. + userDao.fetchOneByUid(caller.getUid) shouldBe null + subjectOf(response.accessToken) shouldBe uname("claimer") + emailClaimOf(response.accessToken) shouldBe uemail("ghost") + } + + // Past INACTIVE the caller may own content, so its row cannot be discarded and the placeholder + // has to be left for someone who can prove the address. + it should "not discard a caller that is no longer INACTIVE to claim a placeholder" in { + val placeholder = seedPlaceholder(uname("kept"), uemail("kept")) + val caller = seedEmaillessUser("regular", role = UserRoleEnum.REGULAR) + + val thrown = intercept[WebApplicationException] { + resource.setEmail(SetEmailRequest(uemail("kept")), new SessionUser(caller)) + } + + statusOf(thrown) shouldBe 409 + userDao.fetchOneByUid(caller.getUid) should not be null + userDao.fetchOneByUid(placeholder.getUid).getIsPlaceholder shouldBe true + } } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala index 9db28a2bed0..78d33ce6213 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/ExternalAuthProvisionerSpec.scala @@ -60,9 +60,23 @@ class ExternalAuthProvisionerSpec override protected def beforeEach(): Unit = cleanup() override protected def afterEach(): Unit = cleanup() - // Case-insensitive so it also collects rows seeded with a differing casing. - private def cleanup(): Unit = + // Case-insensitive so it also collects rows seeded with a differing casing. The ORCID arm + // catches what the email predicate cannot: an identity-only login leaves `email` NULL, so those + // accounts are identified by the provider row that cascades from them. + private def cleanup(): Unit = { getDSLContext.deleteFrom(USER).where(DSL.lower(USER.EMAIL).like("%" + emailDomain)).execute() + getDSLContext + .deleteFrom(USER) + .where( + USER.UID.in( + getDSLContext + .select(AUTH_PROVIDER.UID) + .from(AUTH_PROVIDER) + .where(AUTH_PROVIDER.PROVIDER_TYPE.eq(ProviderTypeEnum.ORCID)) + ) + ) + .execute() + } // ---- helpers ------------------------------------------------------------- @@ -75,7 +89,11 @@ class ExternalAuthProvisionerSpec email: String, avatar: Option[String] = Some(avatarUrl("pic")) ): ExternalProfile = - ExternalProfile(ProviderTypeEnum.GOOGLE, providerId, name, email, avatar) + ExternalProfile(ProviderTypeEnum.GOOGLE, providerId, name, Some(email), avatar) + + /** An identity-only profile: ORCID's `/authenticate` scope asserts an iD and a name, no address. */ + private def orcidProfile(providerId: String, name: String): ExternalProfile = + ExternalProfile(ProviderTypeEnum.ORCID, providerId, name, None, None) /** Seed a user row directly; uid is DB-assigned and read back into the pojo. */ private def seedUser(name: String, localPart: String, avatar: String = null): User = @@ -259,6 +277,48 @@ class ExternalAuthProvisionerSpec claimed.getComment should include("Claimed contributor placeholder at ") } + // ---- identity-only providers (no email asserted) -------------------------- + + it should "provision an emailless INACTIVE account for an identity-only provider" in { + val user = + ExternalAuthProvisioner.loginOrProvision(orcidProfile("0000-0001-0000-0001", "Researcher")) + + user.getUid should not be null + user.getName shouldBe "Researcher" + user.getEmail shouldBe null + user.getRole shouldBe UserRoleEnum.INACTIVE + providerIdOf(user.getUid, ProviderTypeEnum.ORCID) shouldBe "0000-0001-0000-0001" + } + + // Two emailless accounts have to be able to coexist: `"user".email` is UNIQUE, which in Postgres + // does not constrain repeated NULLs. If that ever changed, the second login would 500 here + // rather than silently merging, but this pins the behavior either way. + it should "keep two identity-only accounts separate rather than merging them on a null email" in { + val first = + ExternalAuthProvisioner.loginOrProvision(orcidProfile("0000-0001-0000-0002", "First")) + val second = + ExternalAuthProvisioner.loginOrProvision(orcidProfile("0000-0001-0000-0003", "Second")) + + second.getUid should not be first.getUid + providerRowCount(first.getUid) shouldBe 1 + providerRowCount(second.getUid) shouldBe 1 + } + + // The address is collected after the first login (AuthResource.setEmail), so every subsequent + // login arrives with a profile that still asserts no email. Refreshing must not blank it. + it should "preserve a later-collected email when an identity-only login returns" in { + val created = + ExternalAuthProvisioner.loginOrProvision(orcidProfile("0000-0001-0000-0004", "Returner")) + created.setEmail("collected" + emailDomain) + userDao.update(created) + + val returning = + ExternalAuthProvisioner.loginOrProvision(orcidProfile("0000-0001-0000-0004", "Returner")) + + returning.getUid shouldBe created.getUid + userDao.fetchOneByUid(created.getUid).getEmail shouldBe "collected" + emailDomain + } + // ---- provider id rotation ------------------------------------------------- it should "update the stored provider id when the same user returns with a new one" in { diff --git a/common/config/src/main/resources/user-system.conf b/common/config/src/main/resources/user-system.conf index 8ea19fb0a43..ce9c8e1c00a 100644 --- a/common/config/src/main/resources/user-system.conf +++ b/common/config/src/main/resources/user-system.conf @@ -45,6 +45,12 @@ user-sys { baseUrl = "https://sandbox.orcid.org" baseUrl = ${?USER_SYS_ORCID_BASE_URL} + + # Must be byte-identical to the redirect_uri the login page sends to ORCID + # (`${window.location.origin}/callback/orcid`) and to a redirect URI registered on the ORCID + # client, or the token exchange is rejected. Defaults to the `ng serve` origin. + redirectUri = "http://127.0.0.1:4200/callback/orcid" + redirectUri = ${?USER_SYS_ORCID_REDIRECT_URI} } domain = "" diff --git a/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala b/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala index 469de744815..f58f1e84ae7 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/UserSystemConfig.scala @@ -31,7 +31,9 @@ object UserSystemConfig { val adminPassword: String = conf.getString("user-sys.admin-password") val googleClientId: String = conf.getString("user-sys.google.clientId") val orcidClientId: String = conf.getString("user-sys.orcid.clientId") + val orcidClientSecret: String = conf.getString("user-sys.orcid.clientSecret") val orcidBaseUrl: String = conf.getString("user-sys.orcid.baseUrl") + val orcidRedirectUri: String = conf.getString("user-sys.orcid.redirectUri") val gmail: String = conf.getString("user-sys.google.smtp.gmail") val smtpPassword: String = conf.getString("user-sys.google.smtp.password") val inviteOnly: Boolean = conf.getBoolean("user-sys.invite-only") diff --git a/frontend/src/app/common/service/user/auth.service.ts b/frontend/src/app/common/service/user/auth.service.ts index 6094d245ffc..1b5c10c616c 100644 --- a/frontend/src/app/common/service/user/auth.service.ts +++ b/frontend/src/app/common/service/user/auth.service.ts @@ -47,6 +47,7 @@ export class AuthService { public static readonly REGISTER_ENDPOINT = "auth/register"; public static readonly GOOGLE_LOGIN_ENDPOINT = "auth/google/login"; public static readonly ORCID_LOGIN_ENDPOINT = "auth/orcid/login"; + public static readonly SET_EMAIL_ENDPOINT = "auth/email"; private tokenExpirationSubscription?: Subscription; @@ -94,9 +95,17 @@ export class AuthService { ); } - public orcidAuth(code: string): Observable> { - return this.http.post>( - `${AppSettings.getApiEndpoint()}/${AuthService.GOOGLE_LOGIN_ENDPOINT}`, + /** + * Trades the authorization code from `/callback/orcid` for a Texera token. + * + * `suggestedEmail` comes back when the account has no address yet and the ORCID record publishes + * one: ORCID authenticates an iD without asserting an email, so the account is signed in but + * still needs one before the email-keyed parts of the product (dataset paths, access grants) + * work. It is a prefill for that prompt only — the backend has matched nothing on it. + */ + public orcidAuth(code: string): Observable> { + return this.http.post>( + `${AppSettings.getApiEndpoint()}/${AuthService.ORCID_LOGIN_ENDPOINT}`, code, { headers: { @@ -107,6 +116,14 @@ export class AuthService { ); } + /** Gives the signed-in account the address it lacks, returning the reissued token. */ + public setEmail(email: string): Observable> { + return this.http.put>( + `${AppSettings.getApiEndpoint()}/${AuthService.SET_EMAIL_ENDPOINT}`, + { email } + ); + } + /** * This method will handle the request for user login. * It will automatically login, save the user account inside and trigger userChangeEvent when success diff --git a/frontend/src/app/common/service/user/stub-auth.service.ts b/frontend/src/app/common/service/user/stub-auth.service.ts index 8557b2fcbee..707db46a4d2 100644 --- a/frontend/src/app/common/service/user/stub-auth.service.ts +++ b/frontend/src/app/common/service/user/stub-auth.service.ts @@ -53,6 +53,14 @@ export class StubAuthService implements PublicInterfaceOf { return of(MOCK_TOKEN); } + orcidAuth(code: string): Observable> { + return of(MOCK_TOKEN); + } + + setEmail(email: string): Observable> { + return of(MOCK_TOKEN); + } + loginWithExistingToken(): User | undefined { if (AuthService.getAccessToken() === MOCK_TOKEN.accessToken) { return MOCK_USER; diff --git a/frontend/src/app/common/service/user/stub-user.service.ts b/frontend/src/app/common/service/user/stub-user.service.ts index af9be1c0c64..2540a1250a5 100644 --- a/frontend/src/app/common/service/user/stub-user.service.ts +++ b/frontend/src/app/common/service/user/stub-user.service.ts @@ -57,6 +57,14 @@ export class StubUserService implements PublicInterfaceOf { throw new Error("Method not implemented."); } + orcidLogin(code: string): Observable { + throw new Error("Method not implemented."); + } + + setEmail(email: string): Observable { + throw new Error("Method not implemented."); + } + isLogin(): boolean { return this.user !== undefined; } diff --git a/frontend/src/app/common/service/user/user.service.spec.ts b/frontend/src/app/common/service/user/user.service.spec.ts index bee3facd6d2..f7aafcfe9d4 100644 --- a/frontend/src/app/common/service/user/user.service.spec.ts +++ b/frontend/src/app/common/service/user/user.service.spec.ts @@ -29,16 +29,27 @@ import { commonTestProviders } from "../../testing/test-utils"; import { HttpClientTestingModule } from "@angular/common/http/testing"; import { GuiConfigService } from "../gui-config.service"; import { Role, User } from "../../type/user"; +import { NzModalService } from "ng-zorro-antd/modal"; describe("UserService", () => { let service: UserService; let config: GuiConfigService; + let modal: { create: ReturnType }; beforeEach(() => { AuthService.removeAccessToken(); + modal = { create: vi.fn().mockReturnValue({ getContentComponent: () => ({}), updateConfig: vi.fn() }) }; + TestBed.configureTestingModule({ imports: [HttpClientTestingModule], - providers: [UserService, { provide: AuthService, useClass: StubAuthService }, ...commonTestProviders], + providers: [ + UserService, + { provide: AuthService, useClass: StubAuthService }, + // UserService prompts for an email when the signed-in account has none; the mock records + // whether it would have opened without pulling NzModalModule into the suite. + { provide: NzModalService, useValue: modal }, + ...commonTestProviders, + ], }); service = TestBed.inject(UserService); @@ -195,6 +206,40 @@ describe("UserService", () => { expect(await nextEmission).toMatchObject({ uid: 1, name: "alice" }); }); + // ─── the missing-email prompt ───────────────────────────────────────────── + + // Only an identity-only provider (ORCID) produces an emailless account, and it cannot own a + // dataset with a resolvable path or be named in an access grant until one is supplied. + it("prompts for an email when the signed-in account has none", () => { + (service as any).changeUser({ ...baseUser, email: undefined }); + + expect(modal.create).toHaveBeenCalledTimes(1); + expect(modal.create.mock.calls[0][0].nzData).toMatchObject({ name: "alice" }); + }); + + it("prefills the prompt with the address ORCID published", async () => { + await firstValueFrom(service.orcidLogin("auth-code")); + (service as any).changeUser({ ...baseUser, email: undefined }); + + // StubAuthService returns no suggestion, so the field is left blank rather than invented. + expect(modal.create.mock.calls[0][0].nzData.suggestedEmail).toBeUndefined(); + }); + + it("does not prompt when the account already has an address", () => { + (service as any).changeUser(baseUser); + + expect(modal.create).not.toHaveBeenCalled(); + }); + + // changeUser runs on every token refresh, and a second dialog stacking on the one still waiting + // for an answer would be unanswerable. + it("does not stack a second prompt while one is open", () => { + (service as any).changeUser({ ...baseUser, email: undefined }); + (service as any).changeUser({ ...baseUser, email: undefined }); + + expect(modal.create).toHaveBeenCalledTimes(1); + }); + it("isAdmin reflects only the ADMIN role of the current user", () => { expect(service.isAdmin()).toBe(false); // no user diff --git a/frontend/src/app/common/service/user/user.service.ts b/frontend/src/app/common/service/user/user.service.ts index 603e7c66378..5e8ebe94e18 100644 --- a/frontend/src/app/common/service/user/user.service.ts +++ b/frontend/src/app/common/service/user/user.service.ts @@ -18,14 +18,17 @@ */ import { Injectable } from "@angular/core"; -import { HttpClient } from "@angular/common/http"; +import { HttpClient, HttpErrorResponse } from "@angular/common/http"; import { AppSettings } from "../../app-setting"; -import { Observable, of, ReplaySubject } from "rxjs"; +import { firstValueFrom, Observable, of, ReplaySubject } from "rxjs"; import { Role, User } from "../../type/user"; import { AuthService } from "./auth.service"; import { GuiConfigService } from "../gui-config.service"; -import { catchError, map, shareReplay, switchMap } from "rxjs/operators"; +import { catchError, map, shareReplay, switchMap, tap } from "rxjs/operators"; import { UnimplementedException } from "@angular-devkit/schematics"; +import { NzModalService } from "ng-zorro-antd/modal"; +import { NotificationService } from "../notification/notification.service"; +import { EmailRequestModalComponent } from "./email-request-modal/email-request-modal.component"; /** * User Service manages User information. It relies on different @@ -39,11 +42,15 @@ export class UserService { private userChangeSubject: ReplaySubject = new ReplaySubject(1); private cache = new Map(); private readonly cacheDuration = 3600 * 1000; // cache duration: 1h + private suggestedEmail?: string; + private emailPromptOpen = false; constructor( private authService: AuthService, private config: GuiConfigService, - private http: HttpClient + private http: HttpClient, + private modal: NzModalService, + private notificationService: NotificationService ) { const user = this.authService.loginWithExistingToken(); this.changeUser(user); @@ -66,7 +73,18 @@ export class UserService { } public orcidLogin(code: string): Observable { - return this.authService.orcidAuth(code).pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); + return this.authService.orcidAuth(code).pipe( + tap(({ suggestedEmail }) => (this.suggestedEmail = suggestedEmail)), + switchMap(({ accessToken }) => this.handleAccessToken(accessToken)) + ); + } + + /** + * Gives the signed-in account the address it lacks. The backend reissues the token so the + * `email` claim stops being null, and handling it here refreshes the current user. + */ + public setEmail(email: string): Observable { + return this.authService.setEmail(email).pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); } public isLogin(): boolean { @@ -102,12 +120,71 @@ export class UserService { const sat = Math.floor(60 + Math.random() * 20); // Saturation (60%-80%) const light = 50; // Lightness (50%) this.currentUser = { ...user, color: `hsl(${hue}, ${sat}%, ${light}%)` }; + this.promptForEmailIfMissing(); } else { this.currentUser = user; } this.userChangeSubject.next(this.currentUser); } + /** + * Asks for an email address when the signed-in account has none, and does nothing otherwise. + * + * Only an identity-only provider (ORCID) can produce such an account: local registration and + * Google both assert an address. Until one is supplied the account cannot own a dataset with a + * resolvable path or be named in an access grant, so the prompt is not dismissable — cancelling + * signs out, matching how the invite-only registration request behaves. + * + * Reached from `changeUser`, so it covers both the login that created the account and every + * later page load that restores its token. + */ + private promptForEmailIfMissing(): void { + const user = this.currentUser; + if (!user || (user.email ?? "").length > 0 || this.emailPromptOpen) { + return; + } + this.emailPromptOpen = true; + + const modalRef = this.modal.create({ + nzContent: EmailRequestModalComponent, + nzData: { name: user.name, suggestedEmail: this.suggestedEmail }, + nzOkText: "Save", + nzCancelText: "Sign out", + nzMaskClosable: false, + nzClosable: false, + + nzOnOk: async () => { + const { email } = modalRef.getContentComponent().getValues(); + const validation = UserService.validateEmail(email); + if (!validation.result) { + this.notificationService.error(validation.message); + return false; + } + + try { + await firstValueFrom(this.setEmail(email)); + } catch (e: unknown) { + // A 409 means the address belongs to an account that already has a credential, so it + // cannot be attached here. Keep the modal open with the message rather than signing out. + this.notificationService.error( + (e as HttpErrorResponse)?.error?.message ?? "That email address could not be saved." + ); + return false; + } + this.emailPromptOpen = false; + return true; + }, + + nzOnCancel: () => { + this.emailPromptOpen = false; + this.logout(); + }, + }); + + const comp = modalRef.getContentComponent(); + modalRef.updateConfig({ nzTitle: comp.modalTitle }); + } + // Returns Observable rather than void so callers (login / googleLogin / // register) can switchMap through the post-login config fetch. The /config/gui // and /config/user-system endpoints are @RolesAllowed, so we must wait for the diff --git a/frontend/src/app/common/type/user.ts b/frontend/src/app/common/type/user.ts index ee4e1f781bd..9e3d3bdb583 100644 --- a/frontend/src/app/common/type/user.ts +++ b/frontend/src/app/common/type/user.ts @@ -38,7 +38,9 @@ export interface User extends Readonly<{ uid: number; name: string; - email: string; + // Absent until supplied: an ORCID login authenticates an iD and asserts no address, so the + // account exists with none until the email prompt is answered (see `UserService`). + email?: string; googleId?: string; role: Role; color?: string; diff --git a/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts b/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts index 5b26fe372bc..e76a7ada5a4 100644 --- a/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/user/admin-user.component.spec.ts @@ -238,7 +238,7 @@ describe("AdminUserComponent", () => { expect(component.editUid).toBe(userB.uid); expect(component.editAttribute).toBe("email"); expect(component.editName).toBe(userB.name); - expect(component.editEmail).toBe(userB.email); + expect(component.editEmail).toBe(userB.email!); expect(component.editRole).toBe(userB.role); expect(component.editComment).toBe(userB.comment); }); @@ -248,7 +248,7 @@ describe("AdminUserComponent", () => { component.listOfDisplayUser = [userA]; component.editUid = userA.uid; component.editName = "Alice Updated"; - component.editEmail = userA.email; + component.editEmail = userA.email!; component.editRole = userA.role; component.editComment = userA.comment; @@ -270,7 +270,7 @@ describe("AdminUserComponent", () => { component.userList = [userA]; component.editUid = userA.uid; component.editName = userA.name; - component.editEmail = userA.email; + component.editEmail = userA.email!; component.editRole = userA.role; component.editComment = userA.comment; @@ -287,7 +287,7 @@ describe("AdminUserComponent", () => { component.userList = [userA]; component.editUid = userA.uid; component.editName = "Changed"; - component.editEmail = userA.email; + component.editEmail = userA.email!; component.editRole = userA.role; component.editComment = userA.comment; @@ -460,7 +460,7 @@ describe("AdminUserComponent", () => { component.userList = [userA]; component.editUid = userA.uid; component.editName = "Changed"; - component.editEmail = userA.email; + component.editEmail = userA.email!; component.editRole = userA.role; component.editComment = userA.comment; diff --git a/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts b/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts index 1a7934a6d30..e40dbd90682 100644 --- a/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts +++ b/frontend/src/app/dashboard/component/admin/user/admin-user.component.ts @@ -175,7 +175,9 @@ export class AdminUserComponent implements OnInit { this.editUid = user.uid; this.editAttribute = attribute; this.editName = user.name; - this.editEmail = user.email; + // An identity-only login (ORCID) can leave an account with no address until its owner supplies + // one, and the edit field is a plain string. + this.editEmail = user.email ?? ""; this.editRole = user.role; this.editComment = user.comment; diff --git a/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts b/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts index 3e2722621bb..6a661b17600 100644 --- a/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts +++ b/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts @@ -32,11 +32,19 @@ export class FlarumService { register() { const user = this.userService.getCurrentUser(); + // Flarum identifies an account by email, so there is nothing to register without one. An + // account can lack one between an identity-only login (ORCID) and the prompt that collects it. + const email = user?.email; + if (!email) { + // Thrown rather than returned as a failing observable to match `auth()` below, whose + // non-null assertions throw synchronously for a caller with no user at all. + throw new Error("A Texera account needs an email address to use the forum."); + } return this.http.post( "forum/api/users", { data: { - attributes: { username: user!.email.split("@")[0] + user!.uid, email: user!.email, password: user!.googleId }, + attributes: { username: email.split("@")[0] + user!.uid, email: email, password: user!.googleId }, }, }, { headers: { Authorization: "Token hdebsyxiigyklxgsqivyswwiisohzlnezzzzzzzz;userId=1" } } diff --git a/frontend/src/app/hub/component/login/orcid-callback.component.ts b/frontend/src/app/hub/component/login/orcid-callback.component.ts index 6aff1e7f2a5..d5dcfc5e554 100644 --- a/frontend/src/app/hub/component/login/orcid-callback.component.ts +++ b/frontend/src/app/hub/component/login/orcid-callback.component.ts @@ -18,8 +18,12 @@ */ import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { Component, OnInit } from "@angular/core"; -import { ActivatedRoute } from "@angular/router"; +import { ActivatedRoute, Router } from "@angular/router"; +import { catchError } from "rxjs/operators"; +import { EMPTY } from "rxjs"; import { UserService } from "../../../common/service/user/user.service"; +import { NotificationService } from "../../../common/service/notification/notification.service"; +import { LOGIN, USER_WORKFLOW } from "../../../app-routing.constant"; @UntilDestroy() @Component({ @@ -30,11 +34,40 @@ import { UserService } from "../../../common/service/user/user.service"; export class OrcidCallbackComponent implements OnInit { constructor( private route: ActivatedRoute, - private userService: UserService + private router: Router, + private userService: UserService, + private notificationService: NotificationService ) {} ngOnInit(): void { - const code = this.route.snapshot.queryParams.get("code"); - this.userService.orcidLogin(code).pipe(untilDestroyed(this)).subscribe(); + const params = this.route.snapshot.queryParamMap; + + const error = params.get("error"); + if (error !== null) { + this.failBackToLogin(params.get("error_description") ?? "ORCID sign-in was not completed"); + return; + } + + const code = params.get("code"); + if (code === null) { + this.failBackToLogin("ORCID sign-in was not completed"); + return; + } + + this.userService + .orcidLogin(code) + .pipe( + catchError((e: unknown) => { + this.failBackToLogin((e as Error)?.message || "ORCID sign-in failed"); + return EMPTY; + }), + untilDestroyed(this) + ) + .subscribe(() => this.router.navigateByUrl(USER_WORKFLOW)); + } + + private failBackToLogin(message: string): void { + this.notificationService.error(message); + this.router.navigateByUrl(LOGIN, { replaceUrl: true }); } } diff --git a/sql/changelog.xml b/sql/changelog.xml index 42e40f7b83d..76c01fedcce 100644 --- a/sql/changelog.xml +++ b/sql/changelog.xml @@ -89,6 +89,11 @@ + + + + + + + + + +

+ You are signed in as {{ name }}. ORCID does not share an email address, and Texera uses yours to + identify you when datasets are shared with you. +

+ + + diff --git a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.scss b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.scss new file mode 100644 index 00000000000..fd57b09a350 --- /dev/null +++ b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.scss @@ -0,0 +1,30 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +.email-modal-title { + display: flex; + align-items: center; + justify-content: space-between; + padding-right: 12px; +} + +.email-modal-logo { + height: 28px; + margin-left: 12px; + opacity: 0.9; +} diff --git a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts new file mode 100644 index 00000000000..dea2654242d --- /dev/null +++ b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts @@ -0,0 +1,57 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Component, Inject, TemplateRef, ViewChild } from "@angular/core"; +import { NZ_MODAL_DATA } from "ng-zorro-antd/modal"; +import { NzInputDirective } from "ng-zorro-antd/input"; +import { FormsModule } from "@angular/forms"; + +/** + * Asks a signed-in user for the email address their account does not have. + * + * ORCID authenticates an iD and asserts no address, so an ORCID-only account arrives here with + * `email` unset — and email is what the rest of the product addresses a user by: dataset storage + * paths are built from it and every access grant names one. So this is not a profile nicety; the + * account cannot be shared with or own a dataset until it is answered. + * + * `suggestedEmail` prefills the field from what the ORCID record publishes, which is a convenience + * and not a verified fact — the user can replace it. + */ +@Component({ + selector: "texera-email-request-modal", + templateUrl: "./email-request-modal.component.html", + styleUrls: ["./email-request-modal.component.scss"], + imports: [NzInputDirective, FormsModule], +}) +export class EmailRequestModalComponent { + name = ""; + email = ""; + + @ViewChild("modalTitle", { static: true }) + modalTitle!: TemplateRef; + + constructor(@Inject(NZ_MODAL_DATA) public data: { name: string; suggestedEmail?: string }) { + this.name = data?.name ?? ""; + this.email = data?.suggestedEmail ?? ""; + } + + getValues() { + return { email: (this.email ?? "").trim() }; + } +} diff --git a/sql/updates/36.sql b/sql/updates/36.sql new file mode 100644 index 00000000000..68f1e5fa460 --- /dev/null +++ b/sql/updates/36.sql @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +-- Allow ORCID as an identity provider in auth_provider.provider_type. +-- +-- ORCID is authorization-code OAuth rather than Google's id-token flow, but the identity it +-- yields lands in the same place: one auth_provider row whose provider_id is the ORCID iD. +-- +-- `ADD VALUE IF NOT EXISTS` rather than a recreate: dropping and recreating the type would +-- require dropping the column that uses it. The new label is only added here, never used, so +-- this is safe inside the transaction on PG12+. + +\c texera_db + +SET search_path TO texera_db; + +BEGIN; + +ALTER TYPE provider_type_enum ADD VALUE IF NOT EXISTS 'ORCID'; + +COMMIT; \ No newline at end of file From 6cb626f7ca9f11fb2f2ac609bd60c80943ad7948 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Thu, 13 Aug 2026 12:15:05 -0700 Subject: [PATCH 4/6] fix(auth): revert angular.json --- frontend/angular.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frontend/angular.json b/frontend/angular.json index 6fa78644c03..014f8e36d86 100644 --- a/frontend/angular.json +++ b/frontend/angular.json @@ -80,8 +80,7 @@ "builder": "@angular-builders/custom-webpack:dev-server", "options": { "buildTarget": "gui:build", - "proxyConfig": "proxy.config.json", - "host": "127.0.0.1" + "proxyConfig": "proxy.config.json" }, "configurations": { "production": { From 5f71a021f7c65191b7e00274b514b2dbeac35062 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Fri, 14 Aug 2026 11:16:32 -0700 Subject: [PATCH 5/6] fix(auth): review pass --- .../texera/web/TexeraWebApplication.scala | 13 +- .../http/response/OrcidLoginResponse.scala | 33 --- .../web/resource/auth/AuthResource.scala | 21 +- .../web/resource/auth/OrcidAuthResource.scala | 176 +++++++--------- .../admin/user/AdminUserResource.scala | 10 + .../web/resource/auth/AuthResourceSpec.scala | 22 +- .../resource/auth/OrcidAuthResourceSpec.scala | 105 ++++------ .../admin/user/AdminUserResourceSpec.scala | 33 +++ bin/k8s/values-development.yaml | 16 ++ bin/k8s/values.yaml | 16 ++ common/config/src/main/resources/gui.conf | 7 +- .../src/main/resources/user-system.conf | 9 +- frontend/angular.json | 3 +- .../common/service/user/auth.service.spec.ts | 160 ++++++++++++++- .../app/common/service/user/auth.service.ts | 131 +++++++++++- .../email-request-modal.component.spec.ts | 64 ++++++ .../email-request-modal.component.ts | 6 +- .../common/service/user/stub-auth.service.ts | 17 +- .../common/service/user/stub-user.service.ts | 4 - .../common/service/user/user.service.spec.ts | 55 ++--- .../app/common/service/user/user.service.ts | 107 ++-------- frontend/src/app/common/util/email.ts | 41 ++++ .../service/user/flarum/flarum.service.ts | 28 ++- .../login/orcid-callback.component.spec.ts | 191 ++++++++++++++++++ .../login/orcid-callback.component.ts | 43 +++- .../login/texera-login.component.spec.ts | 92 ++++++++- .../component/login/texera-login.component.ts | 7 +- sql/updates/36.sql | 8 +- 28 files changed, 1025 insertions(+), 393 deletions(-) delete mode 100644 amber/src/main/scala/org/apache/texera/web/model/http/response/OrcidLoginResponse.scala create mode 100644 frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts create mode 100644 frontend/src/app/common/util/email.ts create mode 100644 frontend/src/app/hub/component/login/orcid-callback.component.spec.ts diff --git a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala index d25f1b43432..a54432836b1 100644 --- a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala +++ b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala @@ -40,9 +40,18 @@ import org.apache.texera.web.resource.dashboard.admin.user.AdminUserResource import org.apache.texera.web.resource.dashboard.hub.HubResource import org.apache.texera.web.resource.dashboard.user.UserResource import org.apache.texera.web.resource.dashboard.user.warehouse.WarehouseResource -import org.apache.texera.web.resource.dashboard.user.project.{ProjectAccessResource, ProjectResource, PublicProjectResource} +import org.apache.texera.web.resource.dashboard.user.project.{ + ProjectAccessResource, + ProjectResource, + PublicProjectResource +} import org.apache.texera.web.resource.dashboard.user.quota.UserQuotaResource -import org.apache.texera.web.resource.dashboard.user.workflow.{WorkflowAccessResource, WorkflowExecutionsResource, WorkflowResource, WorkflowVersionResource} +import org.apache.texera.web.resource.dashboard.user.workflow.{ + WorkflowAccessResource, + WorkflowExecutionsResource, + WorkflowResource, + WorkflowVersionResource +} import org.eclipse.jetty.server.session.SessionHandler import org.eclipse.jetty.servlet.{ErrorPageErrorHandler, FilterHolder} import org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter diff --git a/amber/src/main/scala/org/apache/texera/web/model/http/response/OrcidLoginResponse.scala b/amber/src/main/scala/org/apache/texera/web/model/http/response/OrcidLoginResponse.scala deleted file mode 100644 index cdcbc02a9d0..00000000000 --- a/amber/src/main/scala/org/apache/texera/web/model/http/response/OrcidLoginResponse.scala +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -package org.apache.texera.web.model.http.response - -/** - * [[TokenIssueResponse]] plus the address to prefill the email prompt with. - * - * ORCID authenticates an iD and asserts no email, so the account behind `accessToken` may have - * none yet and the frontend has to ask for one. `suggestedEmail` is whatever the ORCID record - * publishes — a convenience for that form and nothing more. It is not a claim about the address: - * the backend has not matched anything on it, and it is not in the token. - * - * Absent when the account already has an address, when the record publishes none, or when the - * lookup failed. - */ -case class OrcidLoginResponse(accessToken: String, suggestedEmail: Option[String]) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala index 708f0c6dd89..7cff3e81c50 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/AuthResource.scala @@ -26,7 +26,7 @@ import org.apache.texera.auth.SessionUser import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.common.util.EmailUtil import org.apache.texera.dao.SqlServer -import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} +import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER, USER_LAST_ACTIVE_TIME} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.UserDao import org.apache.texera.dao.jooq.generated.tables.pojos.User @@ -214,11 +214,7 @@ class AuthResource { } } - TokenIssueResponse( - jwtToken( - jwtClaims(user, ExternalAuthProvisioner.providerIdOf(user.getUid, ProviderTypeEnum.GOOGLE)) - ) - ) + TokenIssueResponse(jwtToken(jwtClaims(user))) } /** @@ -230,9 +226,10 @@ class AuthResource { * mirrors what `register` does when a registration presents a placeholder's address. * * Discarding the caller's own row is only safe because of what it cannot have accumulated: it - * has no email, so nothing email-keyed can name it, and it is INACTIVE, so every - * content-creating endpoint (all of which require REGULAR or ADMIN) has refused it. A caller - * past that point keeps its account and is refused instead. + * has no email, so nothing email-keyed can name it, and it is INACTIVE, which no endpoint in any + * service admits — `setEmail` above is the single `@RolesAllowed` that names INACTIVE, and all + * of the others require REGULAR or ADMIN. So such an account has been refused everywhere it + * could have created something. A caller past INACTIVE keeps its account and is refused instead. */ private def adoptPlaceholder( ctx: DSLContext, @@ -263,7 +260,11 @@ class AuthResource { claimPlaceholder(placeholder) txUserDao.update(placeholder) - // Last, so the provider rows have already moved off it: auth_provider cascades on delete. + ctx + .deleteFrom(USER_LAST_ACTIVE_TIME) + .where(USER_LAST_ACTIVE_TIME.UID.eq(current.getUid)) + .execute() + txUserDao.deleteById(current.getUid) placeholder } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala index 88027683acd..fab1d5c5ed7 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/auth/OrcidAuthResource.scala @@ -25,16 +25,20 @@ import kong.unirest.Unirest import org.apache.texera.auth.JwtAuth.{jwtClaims, jwtToken} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.common.config.UserSystemConfig.orcidBaseUrl -import org.apache.texera.common.util.EmailUtil import org.apache.texera.dao.jooq.generated.enums.ProviderTypeEnum -import org.apache.texera.web.model.http.response.OrcidLoginResponse +import org.apache.texera.web.model.http.response.TokenIssueResponse import org.apache.texera.web.resource.auth.OrcidAuthResource._ -import java.net.URLEncoder -import java.nio.charset.StandardCharsets import javax.ws.rs.core.MediaType -import javax.ws.rs.{Consumes, GET, NotAuthorizedException, POST, Path, Produces} -import scala.jdk.CollectionConverters.IteratorHasAsScala +import javax.ws.rs.{ + Consumes, + GET, + NotAuthorizedException, + POST, + Path, + Produces, + ServiceUnavailableException +} object OrcidAuthResource { private val logger: Logger = Logger(classOf[OrcidAuthResource]) @@ -43,33 +47,35 @@ object OrcidAuthResource { final private lazy val clientSecret = UserSystemConfig.orcidClientSecret final private lazy val redirectUri = UserSystemConfig.orcidRedirectUri - // A user is waiting on the callback page while these run, so both sit far below the browser's - // patience: ORCID either answers promptly or this login has failed. private val CONNECT_TIMEOUT_MS = 5000 private val SOCKET_TIMEOUT_MS = 10000 private val mapper = new ObjectMapper() - /** - * The identity behind a redeemed authorization code. `orcidId` is the ORCID iD - * (`0000-0002-1825-0097`); `name` is absent when the record's owner keeps it private. - * - * Both arrived over the back channel, on a connection our client secret opened, which is what - * separates them from anything in the redirect URL: the browser cannot have chosen them. - */ private[auth] final case class OrcidIdentity(orcidId: String, name: Option[String]) private def textOf(node: JsonNode, field: String): Option[String] = Option(node.path(field).asText(null)).map(_.trim).filter(_.nonEmpty) - /** `a=1&b=2` with both halves percent-encoded — the client secret in particular may need it. */ - private def formEncode(fields: Seq[(String, String)]): String = - fields - .map { - case (name, value) => - s"${URLEncoder.encode(name, StandardCharsets.UTF_8)}=${URLEncoder.encode(value, StandardCharsets.UTF_8)}" - } - .mkString("&") + /** + * The names of the settings the ORCID flow cannot run without, among those given. Taken as + * parameters rather than read from [[UserSystemConfig]] because those are object vals resolved + * once per JVM, which leaves both the configured and unconfigured cases at the mercy of the + * environment a test happens to run in — the same reason `AuthResource.createAdminUser` takes + * its credentials as parameters. + */ + private[auth] def missingSettings( + clientId: String, + clientSecret: String, + redirectUri: String, + baseUrl: String + ): Seq[String] = + Seq( + "clientId" -> clientId, + "clientSecret" -> clientSecret, + "redirectUri" -> redirectUri, + "baseUrl" -> baseUrl + ).collect { case (name, value) if value == null || value.isBlank => name } /** * Read the identity out of a token-endpoint response body. @@ -87,22 +93,6 @@ object OrcidAuthResource { ) } - /** - * The address to offer as a prefill from ORCID's email response, preferring the one the record - * marks primary. Anything unparseable yields None, which costs a filled-in form field. - */ - private[auth] def prefillFrom(body: String): Option[String] = { - val entries = mapper.readTree(body).path("email") - if (!entries.isArray) None - else { - val all = entries.elements().asScala.toSeq - all - .find(_.path("primary").asBoolean(false)) - .orElse(all.headOption) - .flatMap(textOf(_, "email")) - .filter(EmailUtil.isValid) - } - } } /** @@ -119,14 +109,41 @@ object OrcidAuthResource { */ @Path("/auth/orcid") class OrcidAuthResource { + + /** + * What the login page needs to build its authorize redirect. + * + * A deployment missing any of the three settings the flow needs is reported unavailable rather + * than answered with blanks. The login page enables its ORCID button the moment this resolves, + * and each blank fails later and worse: an empty `client_id` lands the user on an ORCID error + * page, and an empty `redirect_uri` gets the exchange rejected after they have already + * consented. Failing here instead leaves the button disabled behind "ORCID sign-in is + * unavailable", which is what the page already does with a failed fetch + * (`texera-login.component.ts`). + * + * `redirectUri` and `baseUrl` are checked here even though only [[exchangeCode]] sends them, + * because both are easily left empty: the deployment templates ship them for an operator to + * fill in, and HOCON treats an env var set to "" as set, so it overrides the config default. An + * empty `baseUrl` would otherwise answer with the relative `authorizeUrl` "/oauth/authorize", + * which navigates the SPA to itself instead of ORCID. + */ @GET @Path("/config") @Produces(Array(MediaType.APPLICATION_JSON)) - def getConfig: Map[String, String] = + def getConfig: Map[String, String] = { + val missing = missingSettings(clientId, clientSecret, redirectUri, orcidBaseUrl) + if (missing.nonEmpty) { + logger.warn( + s"ORCID sign-in is enabled but ${missing.map("user-sys.orcid." + _).mkString(", ")} " + + "is not configured; reporting it unavailable." + ) + throw new ServiceUnavailableException("ORCID sign-in is not configured.") + } Map( "clientId" -> clientId, "authorizeUrl" -> s"$orcidBaseUrl/oauth/authorize" ) + } /** * Trade `code` for ORCID's token response, returning the raw body. @@ -135,104 +152,55 @@ class OrcidAuthResource { * the authorize call byte-for-byte, and honouring a caller-supplied one would let the browser * choose which registered redirect an exchange is attributed to. * - * One of the two seams that reach the network. Kept as a method rather than a constructor - * parameter for the same reason [[GoogleAuthResource.verifiedPayload]] is: Jersey instantiates - * this resource from `classOf[OrcidAuthResource]`, so tests override instead of injecting. + * The one seam that reaches the network. Kept as a method rather than a constructor parameter + * for the same reason [[GoogleAuthResource.verifiedPayload]] is: Jersey instantiates this + * resource from `classOf[OrcidAuthResource]`, so tests override instead of injecting. */ protected def exchangeCode(code: String): String = { - // Encoded by hand rather than with Unirest's `.field()`, which switches the request to - // multipart/form-data under conditions that are not obvious from the call site. ORCID accepts - // only application/x-www-form-urlencoded here, so the encoding is stated outright. - val form = formEncode( - Seq( - "client_id" -> clientId, - "client_secret" -> clientSecret, - "grant_type" -> "authorization_code", - "code" -> code, - "redirect_uri" -> redirectUri - ) - ) - val response = Unirest .post(s"$orcidBaseUrl/oauth/token") - .header("Content-Type", MediaType.APPLICATION_FORM_URLENCODED) .header("Accept", MediaType.APPLICATION_JSON) - .body(form) + .field("client_id", clientId) + .field("client_secret", clientSecret) + .field("grant_type", "authorization_code") + .field("code", code) + .field("redirect_uri", redirectUri) .connectTimeout(CONNECT_TIMEOUT_MS) .socketTimeout(SOCKET_TIMEOUT_MS) .asString() if (response.getStatus != 200) { - // Status only. The body of a failed exchange quotes the request back, and the body of a - // successful one carries a bearer token; neither belongs in a log. logger.warn(s"ORCID token exchange returned ${response.getStatus}") throw new NotAuthorizedException("Login credentials are incorrect.") } response.getBody } - /** - * The email this ORCID record publishes, if any — a prefill for the address prompt, never a key - * anything is matched on. ORCID returns only addresses the owner chose to make public, and an - * address being public is no evidence the owner controls it, so linking on this would be - * exactly the takeover [[ExternalProfile]] warns about. - * - * Best-effort: a failure here costs a prefilled form field, so it is logged and swallowed - * rather than failing a login that has already succeeded. - */ - protected def publishedEmail(orcidId: String, accessToken: String): Option[String] = - try { - val response = Unirest - .get(s"$orcidBaseUrl/v3.0/$orcidId/email") - .header("Accept", MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer $accessToken") - .connectTimeout(CONNECT_TIMEOUT_MS) - .socketTimeout(SOCKET_TIMEOUT_MS) - .asString() - - if (response.getStatus != 200) { - logger.info(s"ORCID published-email lookup returned ${response.getStatus}") - None - } else prefillFrom(response.getBody) - } catch { - case e: Exception => - logger.info(s"ORCID published-email lookup failed: ${e.getClass.getSimpleName}") - None - } - @POST @Consumes(Array(MediaType.TEXT_PLAIN)) @Produces(Array(MediaType.APPLICATION_JSON)) @Path("/login") - def login(code: String): OrcidLoginResponse = { + def login(code: String): TokenIssueResponse = { val trimmedCode = Option(code).map(_.trim).filter(_.nonEmpty).getOrElse { throw new NotAuthorizedException("Login credentials are incorrect.") } - val body = exchangeCode(trimmedCode) - val identity = identityOf(body) + val identity = identityOf(exchangeCode(trimmedCode)) val user = ExternalAuthProvisioner.loginOrProvision( ExternalProfile( ProviderTypeEnum.ORCID, identity.orcidId, - // The iD stands in for a private name: `"user".name` is NOT NULL, and an ORCID iD is at - // least a real handle rather than an invented placeholder. identity.name.getOrElse(identity.orcidId), email = None, avatar = None ) ) - // Only worth a second round trip on a login that will actually prompt for an address — a - // returning user who already supplied one is not asked again. - val suggestedEmail = Option(user.getEmail) match { - case Some(_) => None - case None => - textOf(mapper.readTree(body), "access_token") - .flatMap(token => publishedEmail(identity.orcidId, token)) - } - - OrcidLoginResponse(jwtToken(jwtClaims(user, Some(identity.orcidId))), suggestedEmail) + // No provider id in the claims. `jwtClaims`' second parameter is specifically the GOOGLE one — + // it writes a claim named `googleId` — and the frontend spends that claim as a Flarum account + // password (`flarum.service.ts`). An ORCID iD is public, so putting it there would set a + // guessable password on that account; the iD is in `auth_provider` for anything that needs it. + TokenIssueResponse(jwtToken(jwtClaims(user))) } } diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala index 645a371d74f..629e5d4b271 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResource.scala @@ -115,6 +115,16 @@ class AdminUserResource { if (existingUser != null && existingUser.getUid != user.getUid) { throw new WebApplicationException("Email already exists", Response.Status.CONFLICT) } + + // An identity-only login (e.g. ORCID) provisions an account with no email until its owner supplies + // one, and this table renders that as a blank cell. Safeguard as it easy to promote past INACTIVE by accident. + if (Option(user.getEmail).forall(_.trim.isEmpty) && user.getRole != UserRoleEnum.INACTIVE) { + throw new WebApplicationException( + "This account has no email address yet, so it cannot be activated. Its owner is asked for " + + "one the next time they sign in.", + Response.Status.BAD_REQUEST + ) + } val updatedUser = userDao.fetchOneByUid(user.getUid) val roleChanged = updatedUser.getRole != user.getRole updatedUser.setName(user.getName) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala index 17f4337ea83..8ddac4e7d9a 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/AuthResourceSpec.scala @@ -22,7 +22,7 @@ package org.apache.texera.web.resource.auth import org.apache.texera.auth.{JwtAuth, SessionUser} import org.apache.texera.common.config.UserSystemConfig import org.apache.texera.dao.MockTexeraDB -import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER} +import org.apache.texera.dao.jooq.generated.Tables.{AUTH_PROVIDER, USER, USER_LAST_ACTIVE_TIME} import org.apache.texera.dao.jooq.generated.enums.{ProviderTypeEnum, UserRoleEnum} import org.apache.texera.dao.jooq.generated.tables.daos.{AuthProviderDao, UserDao} import org.apache.texera.dao.jooq.generated.tables.pojos.{AuthProvider, User} @@ -36,6 +36,7 @@ import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} +import java.time.OffsetDateTime import java.util.UUID import javax.ws.rs.{NotAcceptableException, NotAuthorizedException, WebApplicationException} @@ -477,6 +478,25 @@ class AuthResourceSpec emailClaimOf(response.accessToken) shouldBe uemail("ghost") } + // `user_last_active_time.uid` references "user"(uid) with no ON DELETE CASCADE — the only FK to + // "user" that does not cascade — so discarding the caller's row fails unless that row goes first. + // Any authenticated request can have created it, so the adoption must not depend on its absence. + it should "adopt a placeholder even when the caller has an activity row" in { + val placeholder = seedPlaceholder(uname("tracked"), uemail("tracked")) + val caller = seedEmaillessUser("active") + getDSLContext + .insertInto(USER_LAST_ACTIVE_TIME) + .set(USER_LAST_ACTIVE_TIME.UID, caller.getUid) + .set(USER_LAST_ACTIVE_TIME.LAST_ACTIVE_TIME, OffsetDateTime.now()) + .execute() + + resource.setEmail(SetEmailRequest(uemail("tracked")), new SessionUser(caller)) + + userDao.fetchOneByUid(caller.getUid) shouldBe null + userDao.fetchOneByUid(placeholder.getUid).getIsPlaceholder shouldBe false + providerIdOf(placeholder.getUid, ProviderTypeEnum.ORCID) shouldBe "0000-0002-0000-active" + } + // Past INACTIVE the caller may own content, so its row cannot be discarded and the placeholder // has to be left for someone who can prove the address. it should "not discard a caller that is no longer INACTIVE to claim a placeholder" in { diff --git a/amber/src/test/scala/org/apache/texera/web/resource/auth/OrcidAuthResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/auth/OrcidAuthResourceSpec.scala index 102eb5a0b29..ed504a8d0d4 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/auth/OrcidAuthResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/auth/OrcidAuthResourceSpec.scala @@ -33,11 +33,10 @@ import javax.ws.rs.NotAuthorizedException /** * Integration spec for [[OrcidAuthResource]] against embedded Postgres. * - * The two network legs are what cannot run here, so the suite overrides them and drives the - * resource with bodies shaped like ORCID's. What that leaves under test is everything the - * exchange feeds: that an authenticated iD becomes an emailless INACTIVE account with an ORCID - * provider row, that the published address is offered as a suggestion without being written - * anywhere, and that a response authenticating nobody is a 401 rather than an account. + * The token exchange is what cannot run here, so the suite overrides that one seam and drives the + * resource with bodies shaped like ORCID's. What that leaves under test is everything the exchange + * feeds: that an authenticated iD becomes an emailless INACTIVE account with an ORCID provider row, + * and that a response authenticating nobody is a 401 rather than an account. */ class OrcidAuthResourceSpec extends AnyFlatSpec @@ -84,21 +83,14 @@ class OrcidAuthResourceSpec |"expires_in":631138518,"scope":"/authenticate",$nameMember"orcid":"$id"}""".stripMargin } - /** - * A resource whose network legs are canned: `body` stands in for the token exchange and - * `published` for the public-API email lookup. - */ - private class StubbedOrcidAuthResource(body: String, published: Option[String] = None) - extends OrcidAuthResource { + /** A resource whose one network leg is canned: `body` stands in for the token exchange. */ + private class StubbedOrcidAuthResource(body: String) extends OrcidAuthResource { var exchangedCode: Option[String] = None override protected def exchangeCode(code: String): String = { exchangedCode = Some(code) body } - - override protected def publishedEmail(orcidId: String, accessToken: String): Option[String] = - published } private def userBehind(orcidId: String): User = @@ -151,35 +143,16 @@ class OrcidAuthResourceSpec resource.exchangedCode shouldBe Some("auth-code") } - // ---- the suggested address ----------------------------------------------- - - // The suggestion is for the prompt to prefill and nothing else: writing it would be linking on - // an address ORCID merely publishes, which is the takeover ExternalProfile warns about. - it should "offer the published address as a suggestion without storing it" in { - val response = - new StubbedOrcidAuthResource(tokenBody(), published = Some("sofia@example.com")).login("c") - - response.suggestedEmail shouldBe Some("sofia@example.com") - userBehind(orcidId).getEmail shouldBe null - } - - it should "carry no suggestion when the record publishes no address" in { - new StubbedOrcidAuthResource(tokenBody(), published = None) - .login("c") - .suggestedEmail shouldBe None - } - - it should "stop suggesting an address once the account has one" in { + // An address the user supplied later has to survive: every subsequent ORCID login still asserts + // none, and refreshing must not blank what `AuthResource.setEmail` collected. + it should "leave a later-collected address alone when the identity returns" in { new StubbedOrcidAuthResource(tokenBody()).login("c") val user = userBehind(orcidId) user.setEmail("collected@example.com") userDao.update(user) - val response = - new StubbedOrcidAuthResource(tokenBody(), published = Some("published@example.com")) - .login("c") + new StubbedOrcidAuthResource(tokenBody()).login("c") - response.suggestedEmail shouldBe None userBehind(orcidId).getEmail shouldBe "collected@example.com" } @@ -200,30 +173,38 @@ class OrcidAuthResourceSpec resource.exchangedCode shouldBe None } - // ---- prefill parsing ----------------------------------------------------- - - behavior of "prefillFrom" - - it should "prefer the address the record marks primary" in { - val body = - """{"email":[{"email":"secondary@example.com","primary":false}, - |{"email":"primary@example.com","primary":true}]}""".stripMargin - - OrcidAuthResource.prefillFrom(body) shouldBe Some("primary@example.com") - } - - it should "fall back to the first address when none is marked primary" in { - val body = """{"email":[{"email":"only@example.com"}]}""" - - OrcidAuthResource.prefillFrom(body) shouldBe Some("only@example.com") - } - - it should "yield nothing for an empty or absent email array" in { - OrcidAuthResource.prefillFrom("""{"email":[]}""") shouldBe None - OrcidAuthResource.prefillFrom("""{"last-modified-date":null}""") shouldBe None - } - - it should "discard an address that is not a valid email" in { - OrcidAuthResource.prefillFrom("""{"email":[{"email":"not-an-address"}]}""") shouldBe None + // ---- configuration gating ------------------------------------------------ + + // What `getConfig` refuses on. Driven through the pure helper rather than the endpoint, because + // the endpoint reads `UserSystemConfig` object vals: a developer with USER_SYS_ORCID_* exported + // would see the opposite outcome from CI. + // + // Each blank matters at a different moment, and both are worse than failing here: an empty + // client id lands the user on an ORCID error page, and an empty redirect uri gets the exchange + // rejected after they have already consented. + behavior of "missingSettings" + + it should "accept a fully configured deployment" in { + OrcidAuthResource.missingSettings( + "APP-1", + "secret", + "http://127.0.0.1:4200/callback/orcid", + "https://sandbox.orcid.org" + ) shouldBe empty + } + + it should "name each setting that is empty, blank, or absent" in { + OrcidAuthResource.missingSettings("", "secret", "uri", "base") shouldBe Seq("clientId") + OrcidAuthResource.missingSettings("APP-1", " ", "uri", "base") shouldBe Seq("clientSecret") + OrcidAuthResource.missingSettings("APP-1", "secret", null, "base") shouldBe Seq("redirectUri") + // A blank baseUrl would make authorizeUrl the relative "/oauth/authorize", so the button would + // navigate the app to itself rather than to ORCID. + OrcidAuthResource.missingSettings("APP-1", "secret", "uri", "") shouldBe Seq("baseUrl") + OrcidAuthResource.missingSettings("", "", "", "") shouldBe Seq( + "clientId", + "clientSecret", + "redirectUri", + "baseUrl" + ) } } diff --git a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala index c839d29364c..bf7f278e125 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/dashboard/admin/user/AdminUserResourceSpec.scala @@ -288,6 +288,39 @@ class AdminUserResourceSpec a[WebApplicationException] should be thrownBy resource.updateUser(edit) } + // ─── Email safeguards ─────────────────────────────────────────────────── + + it should "refuse to activate an account that has no email address" in { + val emailless = makeUser(primaryUid, "orcid_only", UserRoleEnum.INACTIVE) + emailless.setEmail(null) + userDao.insert(emailless) + + val edit = new User + edit.setUid(primaryUid) + edit.setName("orcid_only") + edit.setEmail(null) + edit.setRole(UserRoleEnum.REGULAR) + + a[WebApplicationException] should be thrownBy resource.updateUser(edit) + userDao.fetchOneByUid(primaryUid).getRole shouldBe UserRoleEnum.INACTIVE + } + + it should "allow editing an emailless account that stays inactive" in { + val emailless = makeUser(primaryUid, "orcid_only", UserRoleEnum.INACTIVE) + emailless.setEmail(null) + userDao.insert(emailless) + + val edit = new User + edit.setUid(primaryUid) + edit.setName("renamed") + edit.setEmail(null) + edit.setRole(UserRoleEnum.INACTIVE) + edit.setComment("waiting on an address") + resource.updateUser(edit) + + userDao.fetchOneByUid(primaryUid).getName shouldBe "renamed" + } + // ─── getCreatedDatasets ─────────────────────────────────────────────────── "getCreatedDatasets" should "return an empty list for a user with no datasets" in { diff --git a/bin/k8s/values-development.yaml b/bin/k8s/values-development.yaml index aa64d6ecbc8..3e595a2bb72 100644 --- a/bin/k8s/values-development.yaml +++ b/bin/k8s/values-development.yaml @@ -330,6 +330,10 @@ texeraEnvVars: value: "true" - name: GUI_LOGIN_GOOGLE_LOGIN value: "true" + # Turn on together with the USER_SYS_ORCID_* credentials below. On with nothing configured, the + # button renders disabled and /auth/orcid/config reports the provider unavailable on every visit. + - name: GUI_LOGIN_ORCID_LOGIN + value: "false" - name: GUI_DATASET_SINGLE_FILE_UPLOAD_MAXIMUM_SIZE_MB value: "1024" - name: GUI_WORKFLOW_WORKSPACE_EXPORT_EXECUTION_RESULT_ENABLED @@ -350,6 +354,18 @@ texeraEnvVars: value: "" - name: USER_SYS_GOOGLE_SMTP_PASSWORD value: "" + # ORCID sign-in. The client id and secret come from an ORCID developer application; leave them + # empty to keep the provider switched off. baseUrl selects the deployment (sandbox vs + # production), and redirectUri must match this deployment's own /callback/orcid URL exactly and + # be registered on that ORCID application — ORCID does not accept `localhost`. + - name: USER_SYS_ORCID_CLIENT_ID + value: "" + - name: USER_SYS_ORCID_CLIENT_SECRET + value: "" + - name: USER_SYS_ORCID_BASE_URL + value: "https://sandbox.orcid.org" + - name: USER_SYS_ORCID_REDIRECT_URI + value: "" - name: USER_SYS_DOMAIN value: "" - name: AUTH_JWT_SECRET diff --git a/bin/k8s/values.yaml b/bin/k8s/values.yaml index 4651bae6947..4f2dd431e65 100644 --- a/bin/k8s/values.yaml +++ b/bin/k8s/values.yaml @@ -333,6 +333,10 @@ texeraEnvVars: value: "true" - name: GUI_LOGIN_GOOGLE_LOGIN value: "true" + # Turn on together with the USER_SYS_ORCID_* credentials below. On with nothing configured, the + # button renders disabled and /auth/orcid/config reports the provider unavailable on every visit. + - name: GUI_LOGIN_ORCID_LOGIN + value: "false" - name: GUI_DATASET_SINGLE_FILE_UPLOAD_MAXIMUM_SIZE_MB value: "1024" - name: GUI_WORKFLOW_WORKSPACE_EXPORT_EXECUTION_RESULT_ENABLED @@ -353,6 +357,18 @@ texeraEnvVars: value: "" - name: USER_SYS_GOOGLE_SMTP_PASSWORD value: "" + # ORCID sign-in. The client id and secret come from an ORCID developer application; leave them + # empty to keep the provider switched off. baseUrl selects the deployment (sandbox vs + # production), and redirectUri must match this deployment's own /callback/orcid URL exactly and + # be registered on that ORCID application — ORCID does not accept `localhost`. + - name: USER_SYS_ORCID_CLIENT_ID + value: "" + - name: USER_SYS_ORCID_CLIENT_SECRET + value: "" + - name: USER_SYS_ORCID_BASE_URL + value: "https://orcid.org" + - name: USER_SYS_ORCID_REDIRECT_URI + value: "" - name: USER_SYS_DOMAIN value: "" - name: AUTH_JWT_SECRET diff --git a/common/config/src/main/resources/gui.conf b/common/config/src/main/resources/gui.conf index e3e2a5f4f45..9715b70211b 100644 --- a/common/config/src/main/resources/gui.conf +++ b/common/config/src/main/resources/gui.conf @@ -34,8 +34,11 @@ gui { google-login = true google-login = ${?GUI_LOGIN_GOOGLE_LOGIN} - # whether orcid login is enabled - orcid-login = true + # whether orcid login is enabled. Off by default because it needs credentials that only an + # operator can supply (user-sys.orcid.clientId/clientSecret): with the button on and nothing + # configured, /auth/orcid/config reports the provider unavailable on every visit to the login + # page. Turn this on together with those settings. + orcid-login = false orcid-login = ${?GUI_LOGIN_ORCID_LOGIN} # Can be configured as { username: "texera", password: "password" } diff --git a/common/config/src/main/resources/user-system.conf b/common/config/src/main/resources/user-system.conf index ce9c8e1c00a..fb9c9880ebd 100644 --- a/common/config/src/main/resources/user-system.conf +++ b/common/config/src/main/resources/user-system.conf @@ -43,12 +43,19 @@ user-sys { clientSecret = "" clientSecret = ${?USER_SYS_ORCID_CLIENT_SECRET} + # The registry host, used for the authorize and token endpoints. baseUrl = "https://sandbox.orcid.org" baseUrl = ${?USER_SYS_ORCID_BASE_URL} # Must be byte-identical to the redirect_uri the login page sends to ORCID # (`${window.location.origin}/callback/orcid`) and to a redirect URI registered on the ORCID - # client, or the token exchange is rejected. Defaults to the `ng serve` origin. + # client, or the token exchange is rejected. + # + # 127.0.0.1 rather than localhost because ORCID does not accept `localhost` as a registered + # redirect URI. The Angular dev server binds `localhost` (::1) by default, so testing ORCID + # locally means starting it on the IPv4 loopback instead: + # cd frontend && npx ng serve --host 127.0.0.1 + # Deployments override this with USER_SYS_ORCID_REDIRECT_URI. redirectUri = "http://127.0.0.1:4200/callback/orcid" redirectUri = ${?USER_SYS_ORCID_REDIRECT_URI} } diff --git a/frontend/angular.json b/frontend/angular.json index 014f8e36d86..6fa78644c03 100644 --- a/frontend/angular.json +++ b/frontend/angular.json @@ -80,7 +80,8 @@ "builder": "@angular-builders/custom-webpack:dev-server", "options": { "buildTarget": "gui:build", - "proxyConfig": "proxy.config.json" + "proxyConfig": "proxy.config.json", + "host": "127.0.0.1" }, "configurations": { "production": { diff --git a/frontend/src/app/common/service/user/auth.service.spec.ts b/frontend/src/app/common/service/user/auth.service.spec.ts index d359143a538..ac85f94df94 100644 --- a/frontend/src/app/common/service/user/auth.service.spec.ts +++ b/frontend/src/app/common/service/user/auth.service.spec.ts @@ -21,6 +21,7 @@ import { HttpClientTestingModule, HttpTestingController } from "@angular/common/ import { TestBed } from "@angular/core/testing"; import { JwtHelperService } from "@auth0/angular-jwt"; import { NzModalService } from "ng-zorro-antd/modal"; +import { firstValueFrom } from "rxjs"; import { AppSettings } from "../../app-setting"; import { Role } from "../../type/user"; import { AuthService, TOKEN_KEY } from "./auth.service"; @@ -219,6 +220,162 @@ describe("AuthService", () => { }); }); + // An identity-only login (ORCID) authenticates an iD and asserts no address, so the token's + // `email` claim is null. Everything downstream is keyed on an address — the invite-only branch + // emails it to the admin, dataset paths and access grants are built from it — so the prompt has + // to come first. + describe("the missing-email prompt", () => { + const emaillessClaims = (role: Role = Role.REGULAR) => ({ ...claims, email: null, role }); + + beforeEach(() => { + modal.create.mockReturnValue({ + getContentComponent: () => ({ modalTitle: "t", getValues: () => ({ email: "typed@x.com" }) }), + updateConfig: vi.fn(), + }); + }); + + it("opens the prompt when the token carries no email", () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + + service.loginWithExistingToken(); + + expect(modal.create).toHaveBeenCalledTimes(1); + expect(modal.create.mock.calls[0][0].nzData).toMatchObject({ name: "Ursula" }); + }); + + it("does not open the prompt for an account that has an address", () => { + AuthService.setAccessToken("tok"); + + service.loginWithExistingToken(); + + expect(modal.create).not.toHaveBeenCalled(); + }); + + // loginWithExistingToken runs on every token refresh; a second dialog stacked on the one still + // waiting for an answer would be unanswerable. + it("does not stack a second prompt while one is open", () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + + service.loginWithExistingToken(); + service.loginWithExistingToken(); + + expect(modal.create).toHaveBeenCalledTimes(1); + }); + + // The regression this ordering exists for: the invite-only branch used to log out immediately, + // which stripped the token the prompt needs and sent the admin a request with a null address + // (rejected by /gmail/notify-unauthorized as an invalid email). + it("keeps the session and asks for an address before the invite-only request", () => { + AuthService.setAccessToken("tok"); + config.env.inviteOnly = true; + jwt.decodeToken.mockReturnValue(emaillessClaims(Role.INACTIVE)); + + const result = service.loginWithExistingToken(); + + expect(result).toBeUndefined(); + expect(modal.create).toHaveBeenCalledTimes(1); + // Still signed in as far as the token goes, so the prompt can PUT /auth/email... + expect(AuthService.getAccessToken()).toEqual("tok"); + // ...and the registration request has not been made yet — it waits for the next pass. + httpMock.expectNone(r => r.url === `${api}/user/joining-reason/required`); + }); + + it("still runs the invite-only request for an inactive user that has an address", () => { + AuthService.setAccessToken("tok"); + config.env.inviteOnly = true; + jwt.decodeToken.mockReturnValue({ ...claims, role: Role.INACTIVE }); + + service.loginWithExistingToken(); + + expect(modal.create).not.toHaveBeenCalled(); + httpMock.expectOne(r => r.url === `${api}/user/joining-reason/required`).flush(false); + }); + + it("stores the reissued token and announces it once the address is saved", async () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + const announced = firstValueFrom(service.sessionChanged()); + + service.loginWithExistingToken(); + const accepted = modal.create.mock.calls[0][0].nzOnOk(); + + const req = httpMock.expectOne(`${api}/${AuthService.SET_EMAIL_ENDPOINT}`); + expect(req.request.method).toEqual("PUT"); + expect(req.request.body).toEqual({ email: "typed@x.com" }); + req.flush({ accessToken: "fresh-token" }); + + expect(await accepted).toBe(true); + expect(AuthService.getAccessToken()).toEqual("fresh-token"); + await expect(announced).resolves.toBeUndefined(); + }); + + it("keeps the dialog open and reports why when the address is refused", async () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + + service.loginWithExistingToken(); + const accepted = modal.create.mock.calls[0][0].nzOnOk(); + + httpMock + .expectOne(`${api}/${AuthService.SET_EMAIL_ENDPOINT}`) + .flush( + { message: "That email address already belongs to an account." }, + { status: 409, statusText: "Conflict" } + ); + + expect(await accepted).toBe(false); + expect(notification.error).toHaveBeenCalledWith("That email address already belongs to an account."); + // The old token is untouched, so the user can try another address. + expect(AuthService.getAccessToken()).toEqual("tok"); + }); + + it("rejects a malformed address without calling the backend", async () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + modal.create.mockReturnValue({ + getContentComponent: () => ({ modalTitle: "t", getValues: () => ({ email: "not-an-address" }) }), + updateConfig: vi.fn(), + }); + + service.loginWithExistingToken(); + + expect(await modal.create.mock.calls[0][0].nzOnOk()).toBe(false); + expect(notification.error).toHaveBeenCalledWith("Email format is invalid."); + httpMock.expectNone(`${api}/${AuthService.SET_EMAIL_ENDPOINT}`); + }); + + // Cancelling has to announce itself too. The caller was already handed a User (outside + // invite-only, the app renders behind the dialog), so without the announcement UserService + // would keep showing an account whose token has just been thrown away. + it("signs out and announces the change when the prompt is cancelled", async () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + const announced = firstValueFrom(service.sessionChanged()); + + service.loginWithExistingToken(); + modal.create.mock.calls[0][0].nzOnCancel(); + + expect(AuthService.getAccessToken()).toBeNull(); + await expect(announced).resolves.toBeUndefined(); + }); + + // A second prompt has to be possible after a cancel, or a user who dismissed it once could + // never be asked again for the life of the tab. + it("can prompt again after a cancel", () => { + AuthService.setAccessToken("tok"); + jwt.decodeToken.mockReturnValue(emaillessClaims()); + + service.loginWithExistingToken(); + modal.create.mock.calls[0][0].nzOnCancel(); + AuthService.setAccessToken("tok"); + service.loginWithExistingToken(); + + expect(modal.create).toHaveBeenCalledTimes(2); + }); + }); + describe("registerAutoLogout", () => { afterEach(() => { // Restore real timers before the outer afterEach runs logout()/verify(). @@ -254,9 +411,6 @@ describe("AuthService", () => { }); describe("invite-only registration gating", () => { - // Drives loginWithExistingToken down the inactive/invite-only branch and - // answers the registration-required probe with `true`, which is what makes - // openRegistrationModal run. const openModalViaInactiveLogin = (): void => { AuthService.setAccessToken("tok"); config.env.inviteOnly = true; diff --git a/frontend/src/app/common/service/user/auth.service.ts b/frontend/src/app/common/service/user/auth.service.ts index 1b5c10c616c..3f8d435b34c 100644 --- a/frontend/src/app/common/service/user/auth.service.ts +++ b/frontend/src/app/common/service/user/auth.service.ts @@ -17,9 +17,9 @@ * under the License. */ -import { HttpClient } from "@angular/common/http"; +import { HttpClient, HttpErrorResponse } from "@angular/common/http"; import { Injectable } from "@angular/core"; -import { firstValueFrom, Observable, Subscription, timer } from "rxjs"; +import { firstValueFrom, Observable, Subject, Subscription, timer } from "rxjs"; import { AppSettings } from "../../app-setting"; import { Role, User } from "../../type/user"; import { ignoreElements } from "rxjs/operators"; @@ -29,6 +29,8 @@ import { GmailService } from "../gmail/gmail.service"; import { GuiConfigService } from "../gui-config.service"; import { NzModalService } from "ng-zorro-antd/modal"; import { RegistrationRequestModalComponent } from "./registration-request-modal/registration-request-modal.component"; +import { EmailRequestModalComponent } from "./email-request-modal/email-request-modal.component"; +import { validateEmailFormat } from "../../util/email"; export const TOKEN_KEY = "access_token"; @@ -50,6 +52,10 @@ export class AuthService { public static readonly SET_EMAIL_ENDPOINT = "auth/email"; private tokenExpirationSubscription?: Subscription; + private sessionChangedSubject = new Subject(); + // `loginWithExistingToken` runs on every token refresh, so this keeps a second dialog from + // stacking on the one still waiting for an answer. + private emailPromptOpen = false; constructor( private http: HttpClient, @@ -98,13 +104,12 @@ export class AuthService { /** * Trades the authorization code from `/callback/orcid` for a Texera token. * - * `suggestedEmail` comes back when the account has no address yet and the ORCID record publishes - * one: ORCID authenticates an iD without asserting an email, so the account is signed in but - * still needs one before the email-keyed parts of the product (dataset paths, access grants) - * work. It is a prefill for that prompt only — the backend has matched nothing on it. + * ORCID authenticates an iD without asserting an email, so the account behind this token may have + * none — `loginWithExistingToken` asks for one before the email-keyed parts of the product + * (dataset paths, access grants) are reachable. */ - public orcidAuth(code: string): Observable> { - return this.http.post>( + public orcidAuth(code: string): Observable> { + return this.http.post>( `${AppSettings.getApiEndpoint()}/${AuthService.ORCID_LOGIN_ENDPOINT}`, code, { @@ -116,6 +121,11 @@ export class AuthService { ); } + /** Emits when this service changed the stored token or cleared it itself (see `promptForEmail`). */ + public sessionChanged(): Observable { + return this.sessionChangedSubject.asObservable(); + } + /** Gives the signed-in account the address it lacks, returning the reissued token. */ public setEmail(email: string): Observable> { return this.http.put>( @@ -164,6 +174,20 @@ export class AuthService { const email = this.jwtHelperService.decodeToken(token).email; const name = this.jwtHelperService.decodeToken(token).sub; + // An identity-only login (ORCID) authenticates an iD and asserts no address, so ask for one + // before anything downstream needs it: the invite-only branch below sends the admin that + // address, and dataset paths and access grants are built from it elsewhere. + if (!email) { + this.promptForEmail(name); + if (this.config.env.inviteOnly && role === Role.INACTIVE) { + // Hold the session rather than logging out: the prompt needs this token to call + // PUT /auth/email, and answering it reissues one, which re-enters here with an address + // and falls through to the registration request below. Returning undefined still leaves + // the app signed out behind the modal, and cancelling it signs out for real. + return undefined; + } + } + if (this.config.env.inviteOnly && role === Role.INACTIVE) { this.checkRegistrationRequired(uid).subscribe(required => { if (required) { @@ -256,6 +280,97 @@ export class AuthService { }); } + /** + * Asks a signed-in user for the email address their account does not have, and stores it. + * + * Only an identity-only provider (ORCID) produces such an account — local registration and + * Google both assert an address — and email is what the rest of the product addresses a user + * by, so the dialog is not dismissable: cancelling signs out, matching how the invite-only + * registration request behaves. + * + * Either outcome announces itself through `sessionChanged`: a success replaces the token (its + * `email` claim was null), a cancel throws it away, and both need the current user re-derived. + */ + private promptForEmail(defaultName: string): void { + if (this.emailPromptOpen) { + return; + } + this.emailPromptOpen = true; + + const modalRef = this.modal.create({ + nzContent: EmailRequestModalComponent, + nzData: { name: defaultName }, + nzOkText: "Save", + nzCancelText: "Sign out", + nzMaskClosable: false, + nzClosable: false, + + nzOnOk: async () => { + const { email } = modalRef.getContentComponent().getValues(); + const validation = validateEmailFormat(email); + if (!validation.result) { + this.notificationService.error(validation.message); + return false; + } + + try { + const { accessToken } = await firstValueFrom(this.setEmail(email)); + AuthService.setAccessToken(accessToken); + } catch (e: unknown) { + // One of the refusals cannot be retried: the account already has an address. That happens + // when a second tab answered this same prompt first — localStorage is shared, so its + // reissued token is already here, and the right move is to accept it and close rather + // than trap the user in a dialog whose only other exit is signing out. + if (this.storedEmailClaim() != null) { + this.finishEmailPrompt(); + return true; + } + // Otherwise the address itself was rejected — most often because it belongs to an account + // that already holds a credential. Keep the dialog open with the reason so the user can + // try the address they actually own. + this.notificationService.error( + (e as HttpErrorResponse)?.error?.message ?? "That email address could not be saved." + ); + return false; + } + this.finishEmailPrompt(); + return true; + }, + + nzOnCancel: () => { + this.logout(); + // Announced for the same reason the success path is: the caller that asked for this login + // has already been handed a User (or nothing), so without this the app would keep showing + // a signed-in account whose token has just been thrown away. + this.finishEmailPrompt(); + }, + }); + + modalRef.updateConfig({ nzTitle: modalRef.getContentComponent().modalTitle }); + } + + /** + * Close out the email prompt: let another one open later, and tell `sessionChanged` subscribers to + * re-derive from whatever token is now stored. + */ + private finishEmailPrompt(): void { + this.emailPromptOpen = false; + this.sessionChangedSubject.next(); + } + + /** The `email` claim on the stored token, or null when there is no token or no claim. */ + private storedEmailClaim(): string | null { + const token = AuthService.getAccessToken(); + if (token == null) { + return null; + } + try { + return this.jwtHelperService.decodeToken(token)?.email ?? null; + } catch { + return null; + } + } + /** * Opens the registration modal (registration request modal) * @param uid diff --git a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts new file mode 100644 index 00000000000..296c9ed097f --- /dev/null +++ b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts @@ -0,0 +1,64 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { NZ_MODAL_DATA } from "ng-zorro-antd/modal"; +import { EmailRequestModalComponent } from "./email-request-modal.component"; +import { commonTestProviders } from "../../../testing/test-utils"; + +describe("EmailRequestModalComponent", () => { + async function createFixture( + data: { name: string } | undefined + ): Promise> { + await TestBed.configureTestingModule({ + imports: [EmailRequestModalComponent], + providers: [{ provide: NZ_MODAL_DATA, useValue: data }, ...commonTestProviders], + }).compileComponents(); + return TestBed.createComponent(EmailRequestModalComponent); + } + + it("should create and render the template", async () => { + const fixture = await createFixture({ name: "Sofia Garcia" }); + fixture.detectChanges(); + expect(fixture.componentInstance).toBeTruthy(); + }); + + it("shows the signed-in name and starts with an empty field", async () => { + const component = (await createFixture({ name: "Sofia" })).componentInstance; + expect(component.name).toBe("Sofia"); + expect(component.email).toBe(""); + }); + + it("defaults to empty strings when the modal data is undefined", async () => { + const component = (await createFixture(undefined)).componentInstance; + expect(component.name).toBe(""); + expect(component.email).toBe(""); + }); + + it("getValues trims the address", async () => { + const component = (await createFixture({ name: "Sofia" })).componentInstance; + component.email = " sofia@example.com "; + expect(component.getValues()).toEqual({ email: "sofia@example.com" }); + }); + + it("getValues returns an empty string for an untouched field", async () => { + const component = (await createFixture({ name: "Sofia" })).componentInstance; + expect(component.getValues()).toEqual({ email: "" }); + }); +}); diff --git a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts index dea2654242d..6174e4ede25 100644 --- a/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts +++ b/frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.ts @@ -29,9 +29,6 @@ import { FormsModule } from "@angular/forms"; * `email` unset — and email is what the rest of the product addresses a user by: dataset storage * paths are built from it and every access grant names one. So this is not a profile nicety; the * account cannot be shared with or own a dataset until it is answered. - * - * `suggestedEmail` prefills the field from what the ORCID record publishes, which is a convenience - * and not a verified fact — the user can replace it. */ @Component({ selector: "texera-email-request-modal", @@ -46,9 +43,8 @@ export class EmailRequestModalComponent { @ViewChild("modalTitle", { static: true }) modalTitle!: TemplateRef; - constructor(@Inject(NZ_MODAL_DATA) public data: { name: string; suggestedEmail?: string }) { + constructor(@Inject(NZ_MODAL_DATA) public data: { name: string }) { this.name = data?.name ?? ""; - this.email = data?.suggestedEmail ?? ""; } getValues() { diff --git a/frontend/src/app/common/service/user/stub-auth.service.ts b/frontend/src/app/common/service/user/stub-auth.service.ts index 707db46a4d2..f6842e18ab8 100644 --- a/frontend/src/app/common/service/user/stub-auth.service.ts +++ b/frontend/src/app/common/service/user/stub-auth.service.ts @@ -19,7 +19,7 @@ import { Injectable } from "@angular/core"; -import { Observable, of } from "rxjs"; +import { Observable, of, Subject } from "rxjs"; import { User } from "../../type/user"; import { PublicInterfaceOf } from "../../util/stub"; import { AuthService } from "./auth.service"; @@ -41,6 +41,8 @@ export const MOCK_INVALID_TOKEN = { */ @Injectable() export class StubAuthService implements PublicInterfaceOf { + private readonly reissued = new Subject(); + auth(username: string, password: string): Observable> { if (password === "password") { return of(MOCK_TOKEN); @@ -53,7 +55,7 @@ export class StubAuthService implements PublicInterfaceOf { return of(MOCK_TOKEN); } - orcidAuth(code: string): Observable> { + orcidAuth(code: string): Observable> { return of(MOCK_TOKEN); } @@ -61,6 +63,17 @@ export class StubAuthService implements PublicInterfaceOf { return of(MOCK_TOKEN); } + // The real service emits here when its email prompt replaces the token or signs out. Nothing in + // this stub opens that prompt, so `emitSessionChanged` stands in for it. + sessionChanged(): Observable { + return this.reissued.asObservable(); + } + + /** Test hook: pretend the email prompt resolved and the session changed underneath. */ + emitSessionChanged(): void { + this.reissued.next(); + } + loginWithExistingToken(): User | undefined { if (AuthService.getAccessToken() === MOCK_TOKEN.accessToken) { return MOCK_USER; diff --git a/frontend/src/app/common/service/user/stub-user.service.ts b/frontend/src/app/common/service/user/stub-user.service.ts index 2540a1250a5..5a428092ff4 100644 --- a/frontend/src/app/common/service/user/stub-user.service.ts +++ b/frontend/src/app/common/service/user/stub-user.service.ts @@ -61,10 +61,6 @@ export class StubUserService implements PublicInterfaceOf { throw new Error("Method not implemented."); } - setEmail(email: string): Observable { - throw new Error("Method not implemented."); - } - isLogin(): boolean { return this.user !== undefined; } diff --git a/frontend/src/app/common/service/user/user.service.spec.ts b/frontend/src/app/common/service/user/user.service.spec.ts index f7aafcfe9d4..85f4b887e88 100644 --- a/frontend/src/app/common/service/user/user.service.spec.ts +++ b/frontend/src/app/common/service/user/user.service.spec.ts @@ -23,33 +23,23 @@ import { fakeAsync, TestBed, tick } from "@angular/core/testing"; import { UserService } from "./user.service"; import { AuthService } from "./auth.service"; import { StubAuthService } from "./stub-auth.service"; +import { MOCK_USER } from "./stub-user.service"; import { skip } from "rxjs/operators"; import { firstValueFrom, Subject, throwError } from "rxjs"; import { commonTestProviders } from "../../testing/test-utils"; import { HttpClientTestingModule } from "@angular/common/http/testing"; import { GuiConfigService } from "../gui-config.service"; import { Role, User } from "../../type/user"; -import { NzModalService } from "ng-zorro-antd/modal"; describe("UserService", () => { let service: UserService; let config: GuiConfigService; - let modal: { create: ReturnType }; beforeEach(() => { AuthService.removeAccessToken(); - modal = { create: vi.fn().mockReturnValue({ getContentComponent: () => ({}), updateConfig: vi.fn() }) }; - TestBed.configureTestingModule({ imports: [HttpClientTestingModule], - providers: [ - UserService, - { provide: AuthService, useClass: StubAuthService }, - // UserService prompts for an email when the signed-in account has none; the mock records - // whether it would have opened without pulling NzModalModule into the suite. - { provide: NzModalService, useValue: modal }, - ...commonTestProviders, - ], + providers: [UserService, { provide: AuthService, useClass: StubAuthService }, ...commonTestProviders], }); service = TestBed.inject(UserService); @@ -206,38 +196,19 @@ describe("UserService", () => { expect(await nextEmission).toMatchObject({ uid: 1, name: "alice" }); }); - // ─── the missing-email prompt ───────────────────────────────────────────── - - // Only an identity-only provider (ORCID) produces an emailless account, and it cannot own a - // dataset with a resolvable path or be named in an access grant until one is supplied. - it("prompts for an email when the signed-in account has none", () => { - (service as any).changeUser({ ...baseUser, email: undefined }); - - expect(modal.create).toHaveBeenCalledTimes(1); - expect(modal.create.mock.calls[0][0].nzData).toMatchObject({ name: "alice" }); - }); - - it("prefills the prompt with the address ORCID published", async () => { - await firstValueFrom(service.orcidLogin("auth-code")); - (service as any).changeUser({ ...baseUser, email: undefined }); - - // StubAuthService returns no suggestion, so the field is left blank rather than invented. - expect(modal.create.mock.calls[0][0].nzData.suggestedEmail).toBeUndefined(); - }); - - it("does not prompt when the account already has an address", () => { - (service as any).changeUser(baseUser); - - expect(modal.create).not.toHaveBeenCalled(); - }); + // The email prompt lives in AuthService (it owns the token it replaces); what UserService owes + // it is a refreshed currentUser once that token lands. See auth.service.spec.ts for the prompt. + it("re-derives the current user when AuthService changes the session", async () => { + const auth = TestBed.inject(AuthService) as unknown as StubAuthService; + await firstValueFrom(service.login("test", "password")); + expect(service.isLogin()).toBe(true); - // changeUser runs on every token refresh, and a second dialog stacking on the one still waiting - // for an answer would be unanswerable. - it("does not stack a second prompt while one is open", () => { - (service as any).changeUser({ ...baseUser, email: undefined }); - (service as any).changeUser({ ...baseUser, email: undefined }); + const nextEmission = firstValueFrom(service.userChanged().pipe(skip(1))); + auth.emitSessionChanged(); - expect(modal.create).toHaveBeenCalledTimes(1); + // Re-derived, not merely re-emitted: the value comes back out of the (stubbed) token rather + // than from the copy UserService was holding. + expect(await nextEmission).toMatchObject({ uid: MOCK_USER.uid, name: MOCK_USER.name }); }); it("isAdmin reflects only the ADMIN role of the current user", () => { diff --git a/frontend/src/app/common/service/user/user.service.ts b/frontend/src/app/common/service/user/user.service.ts index 5e8ebe94e18..74c0317e0c7 100644 --- a/frontend/src/app/common/service/user/user.service.ts +++ b/frontend/src/app/common/service/user/user.service.ts @@ -18,17 +18,14 @@ */ import { Injectable } from "@angular/core"; -import { HttpClient, HttpErrorResponse } from "@angular/common/http"; +import { HttpClient } from "@angular/common/http"; import { AppSettings } from "../../app-setting"; -import { firstValueFrom, Observable, of, ReplaySubject } from "rxjs"; +import { Observable, of, ReplaySubject } from "rxjs"; import { Role, User } from "../../type/user"; import { AuthService } from "./auth.service"; import { GuiConfigService } from "../gui-config.service"; -import { catchError, map, shareReplay, switchMap, tap } from "rxjs/operators"; -import { UnimplementedException } from "@angular-devkit/schematics"; -import { NzModalService } from "ng-zorro-antd/modal"; -import { NotificationService } from "../notification/notification.service"; -import { EmailRequestModalComponent } from "./email-request-modal/email-request-modal.component"; +import { catchError, map, shareReplay, switchMap } from "rxjs/operators"; +import { validateEmailFormat } from "../../util/email"; /** * User Service manages User information. It relies on different @@ -42,18 +39,20 @@ export class UserService { private userChangeSubject: ReplaySubject = new ReplaySubject(1); private cache = new Map(); private readonly cacheDuration = 3600 * 1000; // cache duration: 1h - private suggestedEmail?: string; - private emailPromptOpen = false; constructor( private authService: AuthService, private config: GuiConfigService, - private http: HttpClient, - private modal: NzModalService, - private notificationService: NotificationService + private http: HttpClient ) { const user = this.authService.loginWithExistingToken(); this.changeUser(user); + + // AuthService changes the session on its own when its email prompt resolves — an identity-only + // login starts with no address, so it either replaces the token with one carrying the new + // address or signs out. Re-deriving here is what turns either outcome into the `currentUser` + // every subscriber reads. + this.authService.sessionChanged().subscribe(() => this.changeUser(this.authService.loginWithExistingToken())); } public getCurrentUser(): User | undefined { return this.currentUser; @@ -73,18 +72,7 @@ export class UserService { } public orcidLogin(code: string): Observable { - return this.authService.orcidAuth(code).pipe( - tap(({ suggestedEmail }) => (this.suggestedEmail = suggestedEmail)), - switchMap(({ accessToken }) => this.handleAccessToken(accessToken)) - ); - } - - /** - * Gives the signed-in account the address it lacks. The backend reissues the token so the - * `email` claim stops being null, and handling it here refreshes the current user. - */ - public setEmail(email: string): Observable { - return this.authService.setEmail(email).pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); + return this.authService.orcidAuth(code).pipe(switchMap(({ accessToken }) => this.handleAccessToken(accessToken))); } public isLogin(): boolean { @@ -120,71 +108,12 @@ export class UserService { const sat = Math.floor(60 + Math.random() * 20); // Saturation (60%-80%) const light = 50; // Lightness (50%) this.currentUser = { ...user, color: `hsl(${hue}, ${sat}%, ${light}%)` }; - this.promptForEmailIfMissing(); } else { this.currentUser = user; } this.userChangeSubject.next(this.currentUser); } - /** - * Asks for an email address when the signed-in account has none, and does nothing otherwise. - * - * Only an identity-only provider (ORCID) can produce such an account: local registration and - * Google both assert an address. Until one is supplied the account cannot own a dataset with a - * resolvable path or be named in an access grant, so the prompt is not dismissable — cancelling - * signs out, matching how the invite-only registration request behaves. - * - * Reached from `changeUser`, so it covers both the login that created the account and every - * later page load that restores its token. - */ - private promptForEmailIfMissing(): void { - const user = this.currentUser; - if (!user || (user.email ?? "").length > 0 || this.emailPromptOpen) { - return; - } - this.emailPromptOpen = true; - - const modalRef = this.modal.create({ - nzContent: EmailRequestModalComponent, - nzData: { name: user.name, suggestedEmail: this.suggestedEmail }, - nzOkText: "Save", - nzCancelText: "Sign out", - nzMaskClosable: false, - nzClosable: false, - - nzOnOk: async () => { - const { email } = modalRef.getContentComponent().getValues(); - const validation = UserService.validateEmail(email); - if (!validation.result) { - this.notificationService.error(validation.message); - return false; - } - - try { - await firstValueFrom(this.setEmail(email)); - } catch (e: unknown) { - // A 409 means the address belongs to an account that already has a credential, so it - // cannot be attached here. Keep the modal open with the message rather than signing out. - this.notificationService.error( - (e as HttpErrorResponse)?.error?.message ?? "That email address could not be saved." - ); - return false; - } - this.emailPromptOpen = false; - return true; - }, - - nzOnCancel: () => { - this.emailPromptOpen = false; - this.logout(); - }, - }); - - const comp = modalRef.getContentComponent(); - modalRef.updateConfig({ nzTitle: comp.modalTitle }); - } - // Returns Observable rather than void so callers (login / googleLogin / // register) can switchMap through the post-login config fetch. The /config/gui // and /config/user-system endpoints are @RolesAllowed, so we must wait for the @@ -221,17 +150,7 @@ export class UserService { * @param email */ static validateEmail(email: string): { result: boolean; message: string } { - const trimmed = (email ?? "").trim(); - if (trimmed.length === 0) { - return { result: false, message: "Email should not be empty." }; - } - // Pragmatic email regex: non-whitespace + @ + non-whitespace + . + non-whitespace. - // Matches what most users expect; we leave authoritative validation to the backend. - const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; - if (!emailRegex.test(trimmed)) { - return { result: false, message: "Email format is invalid." }; - } - return { result: true, message: "Email frontend validation success." }; + return validateEmailFormat(email); } /** diff --git a/frontend/src/app/common/util/email.ts b/frontend/src/app/common/util/email.ts new file mode 100644 index 00000000000..814d4ca5262 --- /dev/null +++ b/frontend/src/app/common/util/email.ts @@ -0,0 +1,41 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Syntactic check for an address the user typed, shared by every form that collects one: + * registration (`UserService.validateEmail`) and the prompt an identity-only login triggers + * (`AuthService`). It lives here rather than on either service because those two import each + * other's module, and one rule in one place is the point. + * + * Authoritative validation stays on the backend (`EmailUtil.isValid`); this only catches the + * typo before a round trip. + */ +export function validateEmailFormat(email: string): { result: boolean; message: string } { + const trimmed = (email ?? "").trim(); + if (trimmed.length === 0) { + return { result: false, message: "Email should not be empty." }; + } + // Pragmatic email regex: non-whitespace + @ + non-whitespace + . + non-whitespace. + // Matches what most users expect; we leave authoritative validation to the backend. + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(trimmed)) { + return { result: false, message: "Email format is invalid." }; + } + return { result: true, message: "Email frontend validation success." }; +} diff --git a/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts b/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts index 6a661b17600..fc0094d64c1 100644 --- a/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts +++ b/frontend/src/app/dashboard/service/user/flarum/flarum.service.ts @@ -32,14 +32,7 @@ export class FlarumService { register() { const user = this.userService.getCurrentUser(); - // Flarum identifies an account by email, so there is nothing to register without one. An - // account can lack one between an identity-only login (ORCID) and the prompt that collects it. - const email = user?.email; - if (!email) { - // Thrown rather than returned as a failing observable to match `auth()` below, whose - // non-null assertions throw synchronously for a caller with no user at all. - throw new Error("A Texera account needs an email address to use the forum."); - } + const email = this.requireEmail(); return this.http.post( "forum/api/users", { @@ -53,6 +46,23 @@ export class FlarumService { auth() { const user = this.userService.getCurrentUser(); - return this.http.post("forum/api/token", { identification: user!.email, password: user!.googleId, remember: "1" }); + const email = this.requireEmail(); + return this.http.post("forum/api/token", { identification: email, password: user!.googleId, remember: "1" }); + } + + /** + * The current user's email, or a throw. + * + * Flarum identifies an account by email on both calls above, so neither has anything to send + * without one — and an account can genuinely lack one, between an identity-only login (ORCID) + * and the prompt that collects an address. Thrown synchronously because that is what these two + * already did for a caller with no user at all, via their non-null assertions. + */ + private requireEmail(): string { + const email = this.userService.getCurrentUser()?.email; + if (!email) { + throw new Error("A Texera account needs an email address to use the forum."); + } + return email; } } diff --git a/frontend/src/app/hub/component/login/orcid-callback.component.spec.ts b/frontend/src/app/hub/component/login/orcid-callback.component.spec.ts new file mode 100644 index 00000000000..9abfe53d8c6 --- /dev/null +++ b/frontend/src/app/hub/component/login/orcid-callback.component.spec.ts @@ -0,0 +1,191 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { ActivatedRoute, convertToParamMap, Router } from "@angular/router"; +import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { HttpErrorResponse } from "@angular/common/http"; +import { of, throwError } from "rxjs"; +import { vi } from "vitest"; + +import { OrcidCallbackComponent } from "./orcid-callback.component"; +import { UserService } from "../../../common/service/user/user.service"; +import { NotificationService } from "../../../common/service/notification/notification.service"; +import { ORCID_STATE_KEY } from "../../../common/service/user/orcid-auth.service"; +import { commonTestProviders } from "../../../common/testing/test-utils"; +import { LOGIN, USER_WORKFLOW } from "../../../app-routing.constant"; + +/** + * The callback page has no interaction: everything it does happens in ngOnInit, and the only + * observable outcomes are which URL it navigates to and whether the code reached the exchange. + * The `state` cases are the point of most of this — that value is the flow's CSRF protection, so a + * missing or mismatched one must never reach `orcidLogin`. + */ +describe("OrcidCallbackComponent", () => { + let fixture: ComponentFixture; + let userServiceMock: { orcidLogin: ReturnType }; + let notificationServiceMock: { error: ReturnType }; + let routerMock: { navigateByUrl: ReturnType }; + + const STATE = "state-abc"; + + /** Builds the component with `queryParams` in the URL and `storedState` in sessionStorage. */ + const createComponent = async ( + queryParams: Record, + storedState: string | null = STATE, + orcidLogin = vi.fn().mockReturnValue(of(undefined)) + ) => { + TestBed.resetTestingModule(); + sessionStorage.clear(); + if (storedState !== null) { + sessionStorage.setItem(ORCID_STATE_KEY, storedState); + } + + userServiceMock = { orcidLogin }; + notificationServiceMock = { error: vi.fn() }; + routerMock = { navigateByUrl: vi.fn() }; + + await TestBed.configureTestingModule({ + imports: [OrcidCallbackComponent, HttpClientTestingModule], + providers: [ + { provide: UserService, useValue: userServiceMock }, + { provide: NotificationService, useValue: notificationServiceMock }, + { provide: Router, useValue: routerMock }, + { + provide: ActivatedRoute, + useValue: { snapshot: { queryParamMap: convertToParamMap(queryParams) } }, + }, + ...commonTestProviders, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(OrcidCallbackComponent); + fixture.detectChanges(); + }; + + afterEach(() => sessionStorage.clear()); + + // ─── the happy path ─────────────────────────────────────────────────────── + + it("exchanges the code and lands the user in the dashboard", async () => { + await createComponent({ code: "auth-code", state: STATE }); + + expect(userServiceMock.orcidLogin).toHaveBeenCalledWith("auth-code"); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(USER_WORKFLOW); + expect(notificationServiceMock.error).not.toHaveBeenCalled(); + }); + + // Good for exactly one round trip: a leftover key would let a later callback verify against a + // state nobody is waiting on. + it("clears the stored state once it has been read", async () => { + await createComponent({ code: "auth-code", state: STATE }); + + expect(sessionStorage.getItem(ORCID_STATE_KEY)).toBeNull(); + }); + + // ─── state verification ─────────────────────────────────────────────────── + + it("refuses a state that does not match the one it stored", async () => { + await createComponent({ code: "auth-code", state: "not-the-one" }); + + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + expect(notificationServiceMock.error).toHaveBeenCalled(); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + }); + + it("refuses a callback carrying no state at all", async () => { + await createComponent({ code: "auth-code" }); + + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + }); + + // Nothing stored means this browser did not start a sign-in: a bookmark, a stale tab, or a code + // planted by someone else. + it("refuses when it never stored a state to compare against", async () => { + await createComponent({ code: "auth-code", state: STATE }, null); + + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + }); + + // ─── what ORCID sends back instead of a code ────────────────────────────── + + it("reports the description when ORCID returns an error", async () => { + await createComponent({ error: "access_denied", error_description: "The user denied access" }); + + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + expect(notificationServiceMock.error).toHaveBeenCalledWith("The user denied access"); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + }); + + it("falls back to a generic message when ORCID's error carries no description", async () => { + await createComponent({ error: "access_denied" }); + + expect(notificationServiceMock.error).toHaveBeenCalledWith("ORCID sign-in was not completed"); + }); + + // An ORCID error takes precedence: there is nothing to verify a state against when no + // authorization happened. + it("reports an ORCID error even when the state does not match", async () => { + await createComponent({ error: "access_denied", state: "not-the-one" }); + + expect(notificationServiceMock.error).toHaveBeenCalledWith("ORCID sign-in was not completed"); + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + }); + + it("refuses a verified callback that carries no code", async () => { + await createComponent({ state: STATE }); + + expect(userServiceMock.orcidLogin).not.toHaveBeenCalled(); + expect(notificationServiceMock.error).toHaveBeenCalledWith("ORCID sign-in was not completed"); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + }); + + // ─── a failed exchange ──────────────────────────────────────────────────── + + // The backend's message is in `error.message`; HttpErrorResponse.message is Angular's generated + // "Http failure response for …" developer string, which must not reach the visitor. + it("sends the user back to the login page with the backend's message when the exchange fails", async () => { + const failing = vi.fn().mockReturnValue( + throwError( + () => + new HttpErrorResponse({ + status: 401, + statusText: "Unauthorized", + error: { message: "Login credentials are incorrect." }, + }) + ) + ); + await createComponent({ code: "auth-code", state: STATE }, STATE, failing); + + expect(notificationServiceMock.error).toHaveBeenCalledWith("Login credentials are incorrect."); + expect(routerMock.navigateByUrl).toHaveBeenCalledWith(LOGIN, { replaceUrl: true }); + expect(routerMock.navigateByUrl).not.toHaveBeenCalledWith(USER_WORKFLOW); + }); + + it("falls back to a generic message when the failure carries none", async () => { + const failing = vi + .fn() + .mockReturnValue(throwError(() => new HttpErrorResponse({ status: 500, statusText: "Server Error" }))); + await createComponent({ code: "auth-code", state: STATE }, STATE, failing); + + expect(notificationServiceMock.error).toHaveBeenCalledWith("ORCID sign-in failed"); + }); +}); diff --git a/frontend/src/app/hub/component/login/orcid-callback.component.ts b/frontend/src/app/hub/component/login/orcid-callback.component.ts index d5dcfc5e554..0e98fc97b01 100644 --- a/frontend/src/app/hub/component/login/orcid-callback.component.ts +++ b/frontend/src/app/hub/component/login/orcid-callback.component.ts @@ -16,20 +16,45 @@ * specific language governing permissions and limitations * under the License. */ +import { HttpErrorResponse } from "@angular/common/http"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { Component, OnInit } from "@angular/core"; import { ActivatedRoute, Router } from "@angular/router"; import { catchError } from "rxjs/operators"; import { EMPTY } from "rxjs"; +import { NzSpinComponent } from "ng-zorro-antd/spin"; import { UserService } from "../../../common/service/user/user.service"; import { NotificationService } from "../../../common/service/notification/notification.service"; +import { ORCID_STATE_KEY } from "../../../common/service/user/orcid-auth.service"; import { LOGIN, USER_WORKFLOW } from "../../../app-routing.constant"; +/** + * Where ORCID sends the browser back to after its consent screen, carrying the one-time `code` + * that only the backend can redeem (see `OrcidAuthResource`). Nothing here is interactive: it + * checks the round trip was one we started, hands the code over, and leaves. + */ @UntilDestroy() @Component({ selector: "texera-orcid-callback", - template: "...", - imports: [], + template: ` +
+ +

Signing you in with ORCID…

+
+ `, + styles: [ + ` + .orcid-callback { + display: flex; + flex-direction: column; + align-items: center; + justify-content: center; + gap: 16px; + height: 100vh; + } + `, + ], + imports: [NzSpinComponent], }) export class OrcidCallbackComponent implements OnInit { constructor( @@ -42,12 +67,23 @@ export class OrcidCallbackComponent implements OnInit { ngOnInit(): void { const params = this.route.snapshot.queryParamMap; + const expectedState = sessionStorage.getItem(ORCID_STATE_KEY); + + //remove key to prevent leakage that would authorize future sessions + sessionStorage.removeItem(ORCID_STATE_KEY); + const error = params.get("error"); if (error !== null) { this.failBackToLogin(params.get("error_description") ?? "ORCID sign-in was not completed"); return; } + const state = params.get("state"); + if (expectedState === null || state !== expectedState) { + this.failBackToLogin("ORCID sign-in could not be verified. Please try again."); + return; + } + const code = params.get("code"); if (code === null) { this.failBackToLogin("ORCID sign-in was not completed"); @@ -58,7 +94,8 @@ export class OrcidCallbackComponent implements OnInit { .orcidLogin(code) .pipe( catchError((e: unknown) => { - this.failBackToLogin((e as Error)?.message || "ORCID sign-in failed"); + const failure = e as HttpErrorResponse; + this.failBackToLogin(failure?.error?.message || "ORCID sign-in failed"); return EMPTY; }), untilDestroyed(this) diff --git a/frontend/src/app/hub/component/login/texera-login.component.spec.ts b/frontend/src/app/hub/component/login/texera-login.component.spec.ts index c17b595c700..f4b648fb33a 100644 --- a/frontend/src/app/hub/component/login/texera-login.component.spec.ts +++ b/frontend/src/app/hub/component/login/texera-login.component.spec.ts @@ -19,7 +19,7 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { ActivatedRoute, ActivatedRouteSnapshot, Router } from "@angular/router"; -import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { EMPTY, Subject, of, throwError } from "rxjs"; import { SocialAuthService, SocialUser } from "@abacritt/angularx-social-login"; import { vi } from "vitest"; @@ -31,6 +31,7 @@ import { GuiConfigService } from "../../../common/service/gui-config.service"; import { MockGuiConfigService } from "../../../common/service/gui-config.service.mock"; import { commonTestProviders } from "../../../common/testing/test-utils"; import { USER_WORKFLOW } from "../../../app-routing.constant"; +import { ORCID_STATE_KEY } from "../../../common/service/user/orcid-auth.service"; describe("TexeraLoginComponent", () => { let component: TexeraLoginComponent; @@ -306,4 +307,93 @@ describe("TexeraLoginComponent", () => { expect(routerMock.navigateByUrl).not.toHaveBeenCalled(); }); }); + + // ORCID is authorization-code OAuth, so this page's whole job is the redirect: everything after + // it happens on /callback/orcid and in the backend. What matters here is that the redirect is + // well formed and that the `state` the callback verifies actually gets stashed first. + describe("orcid sign-in", () => { + const ORCID_CONFIG = { clientId: "APP-123", authorizeUrl: "https://sandbox.orcid.org/oauth/authorize" }; + + /** Swaps window.location for the duration of `run`, per the pattern in app.component.spec.ts. */ + const withStubbedLocation = (run: (location: { origin: string; href: string }) => void) => { + const original = window.location; + const stub = { ...original, origin: "http://127.0.0.1:4200", href: "" } as unknown as { + origin: string; + href: string; + }; + Object.defineProperty(window, "location", { configurable: true, value: stub }); + try { + run(stub); + } finally { + Object.defineProperty(window, "location", { configurable: true, value: original }); + } + }; + + /** Answers the config fetch ngOnInit issues, which is what enables the button. */ + const flushOrcidConfig = ( + body: Record | string = ORCID_CONFIG, + status?: { status: number; statusText: string } + ) => { + const httpMock = TestBed.inject(HttpTestingController); + const req = httpMock.expectOne(r => r.url.endsWith("/auth/orcid/config")); + if (status) { + req.flush(body, status); + } else { + req.flush(body); + } + }; + + beforeEach(() => { + sessionStorage.clear(); + fixture.detectChanges(); + }); + + afterEach(() => sessionStorage.clear()); + + it("redirects to ORCID with the client id, callback and a fresh state", () => { + flushOrcidConfig(); + + withStubbedLocation(location => { + (component as any).orcidLogin(); + + const url = new URL(location.href); + expect(`${url.origin}${url.pathname}`).toBe(ORCID_CONFIG.authorizeUrl); + expect(url.searchParams.get("client_id")).toBe("APP-123"); + expect(url.searchParams.get("response_type")).toBe("code"); + expect(url.searchParams.get("scope")).toBe("/authenticate"); + expect(url.searchParams.get("redirect_uri")).toBe("http://127.0.0.1:4200/callback/orcid"); + // The callback compares this against what comes back; it has to be stored before leaving. + expect(url.searchParams.get("state")).toBe(sessionStorage.getItem(ORCID_STATE_KEY)); + expect(sessionStorage.getItem(ORCID_STATE_KEY)).toBeTruthy(); + }); + }); + + it("uses a different state on each attempt", () => { + flushOrcidConfig(); + + const states: Array = []; + withStubbedLocation(() => { + (component as any).orcidLogin(); + states.push(sessionStorage.getItem(ORCID_STATE_KEY)); + (component as any).orcidLogin(); + states.push(sessionStorage.getItem(ORCID_STATE_KEY)); + }); + + expect(states[0]).not.toBe(states[1]); + }); + + // A deployment with no ORCID credentials reports the provider unavailable, which leaves the + // button disabled — clicking it anyway must not send the user to a broken authorize URL. + it("reports unavailable and does not redirect when the config fetch failed", () => { + flushOrcidConfig("nope", { status: 503, statusText: "Service Unavailable" }); + + withStubbedLocation(location => { + (component as any).orcidLogin(); + + expect(notificationServiceMock.error).toHaveBeenCalledWith("ORCID sign-in is unavailable"); + expect(location.href).toBe(""); + expect(sessionStorage.getItem(ORCID_STATE_KEY)).toBeNull(); + }); + }); + }); }); diff --git a/frontend/src/app/hub/component/login/texera-login.component.ts b/frontend/src/app/hub/component/login/texera-login.component.ts index 7c80c50dadb..3b9188cf4e7 100644 --- a/frontend/src/app/hub/component/login/texera-login.component.ts +++ b/frontend/src/app/hub/component/login/texera-login.component.ts @@ -17,6 +17,7 @@ * under the License. */ +import { HttpErrorResponse } from "@angular/common/http"; import { Component, NgZone, OnInit } from "@angular/core"; import { AbstractControl, @@ -126,8 +127,10 @@ export class TexeraLoginComponent implements OnInit { this.orcidAuthService .getConfig() .pipe( - catchError(() => { - this.notificationService.error("ORCID sign-in is unavailable"); + catchError((err: unknown) => { + if ((err as HttpErrorResponse)?.status !== 503) { + this.notificationService.error("ORCID sign-in is unavailable"); + } return EMPTY; }), untilDestroyed(this) diff --git a/sql/updates/36.sql b/sql/updates/36.sql index 68f1e5fa460..45283f20c3a 100644 --- a/sql/updates/36.sql +++ b/sql/updates/36.sql @@ -22,9 +22,9 @@ -- ORCID is authorization-code OAuth rather than Google's id-token flow, but the identity it -- yields lands in the same place: one auth_provider row whose provider_id is the ORCID iD. -- --- `ADD VALUE IF NOT EXISTS` rather than a recreate: dropping and recreating the type would --- require dropping the column that uses it. The new label is only added here, never used, so --- this is safe inside the transaction on PG12+. +-- The type is schema-qualified because the two runners disagree about the search path: the +-- liquibase runner in sql/docker-compose.yml strips `SET search_path` out of these files before +-- applying them, while bin/local-dev.sh keeps it. \c texera_db @@ -32,6 +32,6 @@ SET search_path TO texera_db; BEGIN; -ALTER TYPE provider_type_enum ADD VALUE IF NOT EXISTS 'ORCID'; +ALTER TYPE texera_db.provider_type_enum ADD VALUE IF NOT EXISTS 'ORCID'; COMMIT; \ No newline at end of file From d52652d1bd06dc7eda8e2c62a8a95bd7d12a8554 Mon Sep 17 00:00:00 2001 From: Neilk1021 Date: Fri, 14 Aug 2026 11:57:08 -0700 Subject: [PATCH 6/6] fix(auth): Include new flags in config spec. --- .../apache/texera/service/resource/ConfigResourceSpec.scala | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala index 295de41c563..d95627363a6 100644 --- a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala +++ b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala @@ -132,6 +132,9 @@ class ConfigResourceSpec payload.keySet shouldBe Set( "localLogin", "googleLogin", + // The login page needs this before anyone is signed in, for the same reason as the other two + // provider flags: it decides whether the ORCID button is rendered at all. + "orcidLogin", "defaultLocalUser", "attributionEnabled", "deploymentVersionCheckEnabled", @@ -166,6 +169,7 @@ class ConfigResourceSpec payload.keySet should contain noneOf ( "localLogin", "googleLogin", + "orcidLogin", "defaultLocalUser", "attributionEnabled" )