Copilot commented on code in PR #5721: URL: https://github.com/apache/texera/pull/5721#discussion_r3448057214
########## frontend/src/app/dashboard/service/user/google-drive/drive.service.spec.ts: ########## @@ -0,0 +1,137 @@ +/** + * 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 { TestBed, fakeAsync, tick } from "@angular/core/testing"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +import { NgZone } from "@angular/core"; +import { DriveService } from "./drive.service"; +import { AppSettings } from "../../../../common/app-setting"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; + +describe("DriveService", () => { + let service: DriveService; + let httpMock: HttpTestingController; + let ngZone: NgZone; + + const CONNECT_URL = `${AppSettings.getApiEndpoint()}/auth/google/drive/connect`; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [DriveService, ...commonTestProviders], + }); + + service = TestBed.inject(DriveService); + httpMock = TestBed.inject(HttpTestingController); + ngZone = TestBed.inject(NgZone); + }); + + afterEach(() => { + httpMock.verify(); + }); + + describe("connect", () => { + it("fetches the connect URL and opens a popup", () => { + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + + service.connect().subscribe(); + + const req = httpMock.expectOne(CONNECT_URL); + expect(req.request.method).toBe("GET"); + req.flush("https://accounts.google.com/o/oauth2/auth?...";); + + expect(openSpy).toHaveBeenCalledWith( + "https://accounts.google.com/o/oauth2/auth?...";, + "gdrive-connect", + "width=500,height=600" + ); + }); + + it("completes the observable when the popup posts gdrive-connected", fakeAsync(() => { + const mockPopup = { close: vi.fn() } as unknown as Window; + vi.spyOn(window, "open").mockReturnValue(mockPopup); + + let completed = false; + service.connect().subscribe({ complete: () => (completed = true) }); + + httpMock.expectOne(CONNECT_URL).flush("https://accounts.google.com/...";); + + ngZone.run(() => { + window.dispatchEvent( + new MessageEvent("message", { + data: "gdrive-connected", + origin: window.location.origin, + }) + ); + }); + tick(); + + expect(completed).toBe(true); + expect(mockPopup.close).toHaveBeenCalled(); + })); + + it("errors the observable when the popup posts gdrive-error", fakeAsync(() => { + vi.spyOn(window, "open").mockReturnValue(null); + + let errorMessage = ""; + service.connect().subscribe({ error: (e: unknown) => (errorMessage = (e as Error).message) }); + + httpMock.expectOne(CONNECT_URL).flush("https://accounts.google.com/...";); + Review Comment: In this test, `window.open` is mocked to return `null`, but the flow being tested assumes the popup exists (it later relies on the popup to post an error message). If `window.open` returns `null` (popup blocked), `connect()` should error immediately rather than attaching a message handler. ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.ts: ########## @@ -0,0 +1,65 @@ +/** + * 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, NgZone } from "@angular/core"; +import { HttpClient } from "@angular/common/http"; +import { Observable } from "rxjs"; +import { AppSettings } from "../../../../common/app-setting"; + +@Injectable({ + providedIn: "root", +}) +export class DriveService { + private readonly CONNECT_URL = `${AppSettings.getApiEndpoint()}/auth/google/drive/connect`; + + constructor( + private http: HttpClient, + private ngZone: NgZone + ) {} + + connect(): Observable<void> { + return new Observable(observer => { + this.http.get(this.CONNECT_URL, { responseType: "text" }).subscribe({ + next: url => { + const popup = window.open(url, "gdrive-connect", "width=500,height=600"); + + const onMessage = (event: MessageEvent) => { + if (event.origin !== window.location.origin) return; + if (event.data === "gdrive-connected") { + window.removeEventListener("message", onMessage); + popup?.close(); + this.ngZone.run(() => { + observer.next(); + observer.complete(); + }); + } else if (event.data === "gdrive-error") { + window.removeEventListener("message", onMessage); + this.ngZone.run(() => { + observer.error(new Error("Google Drive connection failed")); + }); + } + }; + + window.addEventListener("message", onMessage); Review Comment: `connect()` proceeds even if the popup is blocked (`window.open` returns null), and it accepts any same-origin `postMessage` regardless of which window sent it. This allows spoofed "connected" messages and can also leave a dangling `message` listener if the subscriber is unsubscribed (e.g., component destroyed) before receiving a message. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResource.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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 io.dropwizard.auth.Auth +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.auth.SessionUser +import org.apache.texera.web.resource.auth.GoogleDriveAuthResource._ +import org.apache.texera.common.config.UserSystemConfig +import com.google.api.client.googleapis.auth.oauth2.{ + GoogleAuthorizationCodeRequestUrl, + GoogleAuthorizationCodeTokenRequest +} +import com.google.api.client.auth.oauth2.TokenResponseException +import com.google.api.client.http.javanet.NetHttpTransport +import com.google.api.client.json.gson.GsonFactory + +import java.util.concurrent.ConcurrentHashMap +import javax.annotation.security.RolesAllowed +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import javax.ws.rs.core.Response + +object GoogleDriveAuthResource { + private val STATE_TTL_MS = 10 * 60 * 1000L + + // state token → (uid, expiresAtMs) + private val pendingStates = new ConcurrentHashMap[String, (Int, Long)]() +} Review Comment: `pendingStates` is an unbounded in-memory `ConcurrentHashMap`. Since entries are only removed on callback, repeated `/connect` calls without completing OAuth (e.g., user closes popup) will grow the map indefinitely. Also, the stored `uid` is currently unused, which adds confusion and suggests missing validation/logic. ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.spec.ts: ########## @@ -0,0 +1,137 @@ +/** + * 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 { TestBed, fakeAsync, tick } from "@angular/core/testing"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +import { NgZone } from "@angular/core"; +import { DriveService } from "./drive.service"; +import { AppSettings } from "../../../../common/app-setting"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; + +describe("DriveService", () => { + let service: DriveService; + let httpMock: HttpTestingController; + let ngZone: NgZone; + + const CONNECT_URL = `${AppSettings.getApiEndpoint()}/auth/google/drive/connect`; + + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [DriveService, ...commonTestProviders], + }); + + service = TestBed.inject(DriveService); + httpMock = TestBed.inject(HttpTestingController); + ngZone = TestBed.inject(NgZone); + }); + + afterEach(() => { + httpMock.verify(); + }); + + describe("connect", () => { + it("fetches the connect URL and opens a popup", () => { + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + + service.connect().subscribe(); + + const req = httpMock.expectOne(CONNECT_URL); + expect(req.request.method).toBe("GET"); + req.flush("https://accounts.google.com/o/oauth2/auth?...";); + + expect(openSpy).toHaveBeenCalledWith( + "https://accounts.google.com/o/oauth2/auth?...";, + "gdrive-connect", + "width=500,height=600" + ); + }); + + it("completes the observable when the popup posts gdrive-connected", fakeAsync(() => { + const mockPopup = { close: vi.fn() } as unknown as Window; + vi.spyOn(window, "open").mockReturnValue(mockPopup); + + let completed = false; + service.connect().subscribe({ complete: () => (completed = true) }); + + httpMock.expectOne(CONNECT_URL).flush("https://accounts.google.com/...";); + + ngZone.run(() => { + window.dispatchEvent( + new MessageEvent("message", { + data: "gdrive-connected", + origin: window.location.origin, + }) + ); + }); + tick(); + + expect(completed).toBe(true); + expect(mockPopup.close).toHaveBeenCalled(); + })); + + it("errors the observable when the popup posts gdrive-error", fakeAsync(() => { + vi.spyOn(window, "open").mockReturnValue(null); + + let errorMessage = ""; + service.connect().subscribe({ error: (e: unknown) => (errorMessage = (e as Error).message) }); + + httpMock.expectOne(CONNECT_URL).flush("https://accounts.google.com/...";); + + ngZone.run(() => { + window.dispatchEvent( + new MessageEvent("message", { + data: "gdrive-error", + origin: window.location.origin, + }) + ); + }); + tick(); + + expect(errorMessage).toBe("Google Drive connection failed"); + })); + + it("ignores messages from other origins", fakeAsync(() => { + vi.spyOn(window, "open").mockReturnValue(null); + + let completed = false; + let errored = false; + service.connect().subscribe({ + complete: () => (completed = true), + error: () => (errored = true), + }); + + httpMock.expectOne(CONNECT_URL).flush("https://accounts.google.com/...";); + + window.dispatchEvent( + new MessageEvent("message", { + data: "gdrive-connected", + origin: "https://evil.example.com";, + }) + ); + tick(); + + expect(completed).toBe(false); + expect(errored).toBe(false); + + // clean up the hanging observable + httpMock.verify(); + })); Review Comment: This test subscribes to `connect()` but never unsubscribes, so the `window` "message" listener remains registered after the test finishes (and can leak across tests). The `httpMock.verify()` call here also doesn't clean up the hanging subscription (and `afterEach` already verifies). ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResource.scala: ########## @@ -0,0 +1,133 @@ +/* + * 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 io.dropwizard.auth.Auth +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.auth.SessionUser +import org.apache.texera.web.resource.auth.GoogleDriveAuthResource._ +import org.apache.texera.common.config.UserSystemConfig +import com.google.api.client.googleapis.auth.oauth2.{ + GoogleAuthorizationCodeRequestUrl, + GoogleAuthorizationCodeTokenRequest +} +import com.google.api.client.auth.oauth2.TokenResponseException +import com.google.api.client.http.javanet.NetHttpTransport +import com.google.api.client.json.gson.GsonFactory + +import java.util.concurrent.ConcurrentHashMap +import javax.annotation.security.RolesAllowed +import javax.ws.rs._ +import javax.ws.rs.core.MediaType +import javax.ws.rs.core.Response + +object GoogleDriveAuthResource { + private val STATE_TTL_MS = 10 * 60 * 1000L + + // state token → (uid, expiresAtMs) + private val pendingStates = new ConcurrentHashMap[String, (Int, Long)]() +} + +@Path("/auth/google/drive") +@Consumes(Array(MediaType.APPLICATION_JSON)) +@Produces(Array(MediaType.APPLICATION_JSON)) +class GoogleDriveAuthResource extends LazyLogging { + + private def errorHtml(message: String): String = + s"""<html><body> + |<p style="font-family:sans-serif;padding:20px">$message</p> + |<script> + |window.opener?.postMessage('gdrive-error', window.location.origin); + |setTimeout(function(){ window.close(); }, 10000); + |</script> + |</body></html>""".stripMargin + + final private lazy val clientId = UserSystemConfig.googleClientId + final private lazy val clientSecret = UserSystemConfig.googleClientSecret + final private lazy val redirectUri = UserSystemConfig.appDomain + .map(domain => s"https://$domain/api/auth/google/drive/callback";) + .getOrElse("http://localhost:4200/api/auth/google/drive/callback";) + + @GET + @Path("/connect") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getOAuth(@Auth sessionUser: SessionUser): Response = { + val stateToken = java.util.UUID.randomUUID().toString + pendingStates.put(stateToken, (sessionUser.getUid, System.currentTimeMillis() + STATE_TTL_MS)) + + val url = new GoogleAuthorizationCodeRequestUrl( + clientId, + redirectUri, + java.util.Arrays.asList("https://www.googleapis.com/auth/drive.file";) + ) + .setState(stateToken) + .build() + + Response.ok(url).build() + } Review Comment: `GET /connect` currently inherits `@Produces(application/json)` and returns a bare URL string. Depending on the JAX-RS JSON provider, this may be serialized as a JSON string (quoted/escaped) or served with an incorrect content type, breaking the frontend’s expectation of a raw URL. -- 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]
