Yicong-Huang commented on code in PR #7664:
URL: https://github.com/apache/texera/pull/7664#discussion_r3910934236
##########
frontend/src/app/hub/component/login/texera-login.component.ts:
##########
@@ -319,4 +344,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`,
Review Comment:
The redirect URI has two independent owners. This line derives it from the
browser's origin; the token exchange sends the server's configured
`UserSystemConfig.orcidRedirectUri`. ORCID requires both legs to carry a
byte-identical value.
`/auth/orcid/config` exists to stop exactly this — its own comment says both
values come from the server so the two can never disagree — and it serves
`clientId` and `authorizeUrl`, but not `redirectUri`. So the one value whose
mismatch is user-visible is the one the mechanism omits.
The blank-config 503 does not cover it either: the HOCON default is
non-blank, so that guard never fires for this key outside k8s, which ships
`""`. A deployment that sets only client id and secret gets an enabled button
and a post-consent failure surfaced as "Login credentials are incorrect."
Serving `redirectUri` from `getConfig` and using it here makes the server
the single owner — a two-line change.
##########
frontend/src/app/hub/component/login/texera-login.component.ts:
##########
@@ -319,4 +344,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.
Review Comment:
Worth confirming the scope was intended: the `angular.json` dev-server host
pin is repo-wide, changing the default for every developer to serve one
optional, off-by-default provider. On a machine whose resolver prefers `::1`, a
habitual `http://localhost:4200` stops working.
The motivation is sound — ORCID refuses `localhost` as a registered redirect
URI — and the description documents the pin. The narrower fix is the `ng serve
--host 127.0.0.1` flag the docs already mention.
##########
frontend/src/app/hub/component/login/orcid-callback.component.ts:
##########
@@ -0,0 +1,142 @@
+/**
+ * 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 { 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, skip, take } 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: `
+ <div class="orcid-callback">
+ <nz-spin nzSimple></nz-spin>
+ <p>Signing you in with ORCID…</p>
+ </div>
+ `,
+ 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(
+ private route: ActivatedRoute,
+ private router: Router,
+ private userService: UserService,
+ private notificationService: NotificationService
+ ) {}
+
+ 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");
Review Comment:
`state` is read and cleared above, then this branch acts on the provider's
`error` response and returns — so the comparison below never runs on the error
path.
RFC 6749 §4.1.2.1 has the authorization server return `state` on the error
response for exactly this correlation, so the value needed for the check is
present and simply unused.
The reachable consequence: a link to
`/callback/orcid?error=…&error_description=…` discards whatever ORCID state
that browsing context was holding, and renders the attacker's text as Texera's
own error. ng-zorro's message service renders via `[innerHTML]` and Angular's
sanitizer keeps `<a href>`, so that error can carry a live link on the login
page.
Hoisting the `state` comparison above this branch fixes it. The spec pins
the current order as intended, so that case needs retargeting too — it should
assert an uncorrelated error is refused, not reported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
