Copilot commented on code in PR #5251: URL: https://github.com/apache/texera/pull/5251#discussion_r3448058118
########## frontend/src/app/dashboard/service/user/google-drive/drive.service.ts: ########## @@ -0,0 +1,202 @@ +/** + * 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, Subject, from, firstValueFrom } from "rxjs"; +import { AppSettings } from "../../../../common/app-setting"; + +export interface DriveTokenResponse { + status: string; + accessToken?: string; +} + +export interface DriveFolder { + id: string; + name: string; +} + +// gapi is loaded via the script tag in index.html +declare var gapi: any; +declare var google: any; + +@Injectable({ + providedIn: "root", +}) +export class DriveService { + private readonly BASE = `${AppSettings.getApiEndpoint()}/auth/google/drive`; + private readonly CONFIG_URL = `${AppSettings.getApiEndpoint()}/auth/google/config`; + + private connected$ = new Subject<void>(); + private pickerLoaded = false; + + constructor( + private http: HttpClient, + private ngZone: NgZone + ) {} + + connect(reauth = false): void { + this.http.get(`${this.BASE}/connect?reauth=${reauth}`, { responseType: "text" }).subscribe(url => { + const popup = window.open(url, "gdrive-connect", "width=500,height=600"); + + const onMessage = (event: MessageEvent) => { + if (event.data === "gdrive-connected") { + window.removeEventListener("message", onMessage); + popup?.close(); + this.ngZone.run(() => this.connected$.next()); + } + }; Review Comment: The `message` handler will accept `gdrive-connected` from any window/origin. This allows unrelated pages (or other browser contexts) to spoof a “connected” event and trigger `connected$`. Consider validating both `event.origin` and `event.source` (the popup window) before accepting the message. ########## amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala: ########## @@ -160,6 +164,7 @@ class TexeraWebApplication environment.jersey.register(classOf[UserQuotaResource]) environment.jersey.register(classOf[AdminSettingsResource]) environment.jersey.register(classOf[AIAssistantResource]) + environment.jersey.register(classOf[GoogleDriveAuthResource]) Review Comment: `GoogleDriveAuthResource` is already exposed as a sub-resource via `GoogleAuthResource` (`@Path("/auth/google")` + `@Path("/drive")`). Registering `GoogleDriveAuthResource` directly here is redundant, and since the class has no class-level `@Path`, it’s not a valid root resource on its own (can be confusing for future maintenance). ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.ts: ########## @@ -0,0 +1,202 @@ +/** + * 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, Subject, from, firstValueFrom } from "rxjs"; +import { AppSettings } from "../../../../common/app-setting"; + +export interface DriveTokenResponse { + status: string; + accessToken?: string; +} + +export interface DriveFolder { + id: string; + name: string; +} + +// gapi is loaded via the script tag in index.html +declare var gapi: any; +declare var google: any; + +@Injectable({ + providedIn: "root", +}) +export class DriveService { + private readonly BASE = `${AppSettings.getApiEndpoint()}/auth/google/drive`; + private readonly CONFIG_URL = `${AppSettings.getApiEndpoint()}/auth/google/config`; + + private connected$ = new Subject<void>(); + private pickerLoaded = false; + + constructor( + private http: HttpClient, + private ngZone: NgZone + ) {} + + connect(reauth = false): void { + this.http.get(`${this.BASE}/connect?reauth=${reauth}`, { responseType: "text" }).subscribe(url => { + const popup = window.open(url, "gdrive-connect", "width=500,height=600"); + + const onMessage = (event: MessageEvent) => { + if (event.data === "gdrive-connected") { + window.removeEventListener("message", onMessage); + popup?.close(); + this.ngZone.run(() => this.connected$.next()); + } + }; + + window.addEventListener("message", onMessage); + }); + } + + onConnected(): Observable<void> { + return this.connected$.asObservable(); + } + + getToken(): Observable<DriveTokenResponse> { + return this.http.get<DriveTokenResponse>(`${this.BASE}/token`); + } + + exportToDrive(blob: Blob, fileName: string): Observable<void> { + const result$ = new Subject<void>(); + + Promise.all([this.loadPicker(), this.getAccessToken()]).then(([, accessToken]) => { + if (!accessToken) { + result$.error(new Error("Not connected to Google Drive")); + return; + } + + this.http.get<{ clientId: string; apiKey: string }>(this.CONFIG_URL).subscribe(config => { + const folderView = new google.picker.DocsView(google.picker.ViewId.FOLDERS) + .setIncludeFolders(true) + .setSelectFolderEnabled(true) + .setMimeTypes("application/vnd.google-apps.folder"); + + const picker = new google.picker.PickerBuilder() + .addView(folderView) + .setOAuthToken(accessToken) + .setDeveloperKey(config.apiKey) + .setTitle("Choose a folder to export to") + .setCallback((data: any) => { + if (data.action === google.picker.Action.PICKED) { + const folderId = data.docs[0].id; + this.ngZone.run(() => { + this.uploadToDrive(blob, fileName, folderId, accessToken).subscribe({ + next: () => { + result$.next(); + result$.complete(); + }, + error: (err: unknown) => result$.error(err), + }); + }); + } else if (data.action === google.picker.Action.CANCEL) { + this.ngZone.run(() => result$.complete()); + } + }) + .build(); + + picker.setVisible(true); + }); Review Comment: `exportToDrive()` subscribes to `CONFIG_URL` without an error handler. If the config request fails, `result$` never completes/errors, leaving callers hanging. ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.ts: ########## @@ -0,0 +1,202 @@ +/** + * 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, Subject, from, firstValueFrom } from "rxjs"; +import { AppSettings } from "../../../../common/app-setting"; + +export interface DriveTokenResponse { + status: string; + accessToken?: string; +} + +export interface DriveFolder { + id: string; + name: string; +} + +// gapi is loaded via the script tag in index.html +declare var gapi: any; +declare var google: any; + +@Injectable({ + providedIn: "root", +}) +export class DriveService { + private readonly BASE = `${AppSettings.getApiEndpoint()}/auth/google/drive`; + private readonly CONFIG_URL = `${AppSettings.getApiEndpoint()}/auth/google/config`; + + private connected$ = new Subject<void>(); + private pickerLoaded = false; + + constructor( + private http: HttpClient, + private ngZone: NgZone + ) {} + + connect(reauth = false): void { + this.http.get(`${this.BASE}/connect?reauth=${reauth}`, { responseType: "text" }).subscribe(url => { + const popup = window.open(url, "gdrive-connect", "width=500,height=600"); + + const onMessage = (event: MessageEvent) => { + if (event.data === "gdrive-connected") { + window.removeEventListener("message", onMessage); + popup?.close(); + this.ngZone.run(() => this.connected$.next()); + } + }; + + window.addEventListener("message", onMessage); + }); + } + + onConnected(): Observable<void> { + return this.connected$.asObservable(); + } + + getToken(): Observable<DriveTokenResponse> { + return this.http.get<DriveTokenResponse>(`${this.BASE}/token`); + } + + exportToDrive(blob: Blob, fileName: string): Observable<void> { + const result$ = new Subject<void>(); + + Promise.all([this.loadPicker(), this.getAccessToken()]).then(([, accessToken]) => { + if (!accessToken) { + result$.error(new Error("Not connected to Google Drive")); + return; + } + + this.http.get<{ clientId: string; apiKey: string }>(this.CONFIG_URL).subscribe(config => { + const folderView = new google.picker.DocsView(google.picker.ViewId.FOLDERS) + .setIncludeFolders(true) + .setSelectFolderEnabled(true) + .setMimeTypes("application/vnd.google-apps.folder"); + + const picker = new google.picker.PickerBuilder() + .addView(folderView) + .setOAuthToken(accessToken) + .setDeveloperKey(config.apiKey) + .setTitle("Choose a folder to export to") + .setCallback((data: any) => { + if (data.action === google.picker.Action.PICKED) { + const folderId = data.docs[0].id; + this.ngZone.run(() => { + this.uploadToDrive(blob, fileName, folderId, accessToken).subscribe({ + next: () => { + result$.next(); + result$.complete(); + }, + error: (err: unknown) => result$.error(err), + }); + }); + } else if (data.action === google.picker.Action.CANCEL) { + this.ngZone.run(() => result$.complete()); + } + }) + .build(); + + picker.setVisible(true); + }); + }); + + return result$.asObservable(); + } + + openFolderPicker(): Observable<DriveFolder> { + const result$ = new Subject<DriveFolder>(); + + Promise.all([this.loadPicker(), this.getAccessToken()]).then(([, accessToken]) => { + if (!accessToken) { + result$.error(new Error("Not connected to Google Drive")); + return; + } + + this.http.get<{ clientId: string; apiKey: string }>(this.CONFIG_URL).subscribe(config => { + const folderView = new google.picker.DocsView(google.picker.ViewId.FOLDERS) + .setIncludeFolders(true) + .setSelectFolderEnabled(true) + .setMimeTypes("application/vnd.google-apps.folder"); + + const picker = new google.picker.PickerBuilder() + .addView(folderView) + .setOAuthToken(accessToken) + .setDeveloperKey(config.apiKey) + .setTitle("Choose a folder to export to") + .setCallback((data: any) => { + if (data.action === google.picker.Action.PICKED) { + const doc = data.docs[0]; + this.ngZone.run(() => { + result$.next({ id: doc.id, name: doc.name }); + result$.complete(); + }); + } else if (data.action === google.picker.Action.CANCEL) { + this.ngZone.run(() => result$.complete()); + } + }) + .build(); + + picker.setVisible(true); + }); Review Comment: `openFolderPicker()` subscribes to `CONFIG_URL` without an error handler. If the config request fails, `result$` never completes/errors, which can stall the UI waiting on this observable. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResource.scala: ########## @@ -0,0 +1,209 @@ +/* + * 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.fasterxml.jackson.databind.ObjectMapper +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.auth.{SessionUser, TokenEncryptionService} +import org.apache.texera.web.model.http.response.DriveTokenIssueResponse +import org.apache.texera.web.resource.auth.GoogleDriveAuthResource._ +import org.apache.texera.dao.jooq.generated.tables.daos.UserOauthTokenDao +import org.apache.texera.dao.jooq.generated.tables.pojos.UserOauthToken +import org.apache.texera.dao.SqlServer +import org.apache.texera.config.UserSystemConfig +import com.google.api.client.googleapis.auth.oauth2.{ + GoogleAuthorizationCodeRequestUrl, + GoogleAuthorizationCodeTokenRequest, + GoogleRefreshTokenRequest, + GoogleTokenResponse +} +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 STATUS_OK = "ok" + private val STATUS_NO_REFRESH_TOKEN = "no_refresh_token" + private val STATUS_INVALID_GRANT = "invalid_grant" + private val PROVIDER_GOOGLE_DRIVE = "google_drive" + + private val STATE_TTL_MS = 10 * 60 * 1000L + + private val mapper = new ObjectMapper() + + // state token → (uid, expiresAtMs) + private val pendingStates = new ConcurrentHashMap[String, (Int, Long)]() + + private def oauthTokenDao = + new UserOauthTokenDao( + SqlServer + .getInstance() + .createDSLContext() + .configuration + ) +} + +@Consumes(Array(MediaType.APPLICATION_JSON)) +@Produces(Array(MediaType.APPLICATION_JSON)) +class GoogleDriveAuthResource extends LazyLogging { + 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("/token") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getDriveAccessToken(@Auth sessionUser: SessionUser): Response = { + val uid = sessionUser.getUid + val record = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + .orElse(null) + + if (record == null) { + return Response.ok(DriveTokenIssueResponse(STATUS_NO_REFRESH_TOKEN, None)).build() + } + + try { + val blob = mapper.readTree(TokenEncryptionService.decrypt(record.getAuthBlob)) + val refreshToken = blob.get("refreshToken").asText() + + val tokenResponse = new GoogleRefreshTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + refreshToken, + clientId, + clientSecret + ).execute() + + Response.ok(DriveTokenIssueResponse(STATUS_OK, Some(tokenResponse.getAccessToken))).build() + } catch { + case e: TokenResponseException => + if (e.getDetails != null && e.getDetails.getError == STATUS_INVALID_GRANT) { + Response.ok(DriveTokenIssueResponse(STATUS_INVALID_GRANT, None)).build() + } else { + logger.error("Failed to refresh access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + case e: Exception => + logger.error("Unexpected error refreshing access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + } + + @GET + @Path("/callback") + @Produces(Array(MediaType.TEXT_HTML, MediaType.APPLICATION_JSON)) + def getCallback( + @QueryParam("code") @DefaultValue("") code: String, + @QueryParam("state") @DefaultValue("") state: String + ): Response = { + if (code.isEmpty || state.isEmpty) { + return Response.status(Response.Status.BAD_REQUEST).build() + } + try { + val entry = pendingStates.remove(state) + if (entry == null || System.currentTimeMillis() > entry._2) { + return Response + .status(Response.Status.UNAUTHORIZED) + .entity("OAuth state token is invalid or expired") + .build() + } + + val uid = entry._1 + + val tokenResponse: GoogleTokenResponse = new GoogleAuthorizationCodeTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + clientId, + clientSecret, + code, + redirectUri + ).execute() + + val blobMap = new java.util.HashMap[String, String]() + blobMap.put("refreshToken", tokenResponse.getRefreshToken) + blobMap.put("scopes", tokenResponse.getScope) + val blobJson = mapper.writeValueAsString(blobMap) + val encryptedBlob = TokenEncryptionService.encrypt(blobJson) + + val existing = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + + if (existing.isPresent) { + existing.get().setAuthBlob(encryptedBlob) + oauthTokenDao.update(existing.get()) + } else { + val record = new UserOauthToken() + record.setUid(uid) + record.setProvider(PROVIDER_GOOGLE_DRIVE) + record.setAuthBlob(encryptedBlob) + oauthTokenDao.insert(record) + } + + val html = + """<html><body><script> + |window.opener.postMessage('gdrive-connected', window.location.origin); + |window.close(); + |</script></body></html>""".stripMargin + Response.ok(html).build() + } catch { + case e: TokenResponseException => + logger.error("Google token exchange failed in callback", e) + Response.status(Response.Status.BAD_GATEWAY).build() + case e: Exception => + logger.error("Unexpected error in OAuth callback", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + } + + @GET + @Path("/connect") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getOAuth( + @Auth sessionUser: SessionUser, + @QueryParam("reauth") @DefaultValue("false") reauth: Boolean + ): 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";) + ) + .setState(stateToken) + .setAccessType("offline") + .set("prompt", if (reauth) "consent" else null) + .set("include_granted_scopes", true) + .build() + + Response.ok(url).build() Review Comment: `GET /auth/google/drive/connect` currently returns a raw URL string under the resource's default `application/json` produces. Angular is requesting `responseType: 'text'`; returning JSON here can add quotes / incorrect content-type and break `window.open(url, ...)`. Return the URL as `text/plain` explicitly. ########## common/auth/src/main/scala/org/apache/texera/auth/TokenEncryptionService.scala: ########## @@ -0,0 +1,50 @@ +/* + * 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.auth + +import org.apache.texera.config.AuthConfig +import org.jose4j.jwe.{ + ContentEncryptionAlgorithmIdentifiers, + JsonWebEncryption, + KeyManagementAlgorithmIdentifiers +} +import org.jose4j.keys.AesKey + +import java.nio.charset.StandardCharsets + +object TokenEncryptionService { + private val key = new AesKey(AuthConfig.encryptionSecretKey.getBytes(StandardCharsets.UTF_8)) Review Comment: `AES_256_GCM` with `DIRECT` requires a 256-bit (32 byte) key. Right now the key is derived from the UTF-8 bytes of the configured string without any validation, so a misconfigured secret (wrong length) will fail at runtime and can be hard to diagnose. Consider validating the key length up front and failing fast with a clear message. ########## frontend/src/app/app-routing.module.ts: ########## @@ -143,6 +144,10 @@ routes.push({ path: "discussion", component: FlarumComponent, }, + { + path: "google-drive", + component: GoogleDriveConnectComponent, + }, Review Comment: PR description says the OAuth callback landing page is at `/gdrive-connect`, but the router change adds a dashboard route at `dashboard/user/google-drive` pointing to `GoogleDriveConnectComponent` (which is also implemented as a full UI page rather than a minimal OAuth callback handler). This is a significant mismatch between the described flow and the implemented routing/component behavior. ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.spec.ts: ########## @@ -0,0 +1,161 @@ +/** + * 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 { firstValueFrom } from "rxjs"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; + +describe("DriveService", () => { + let service: DriveService; + let httpMock: HttpTestingController; + let ngZone: NgZone; + + const BASE = `${AppSettings.getApiEndpoint()}/auth/google/drive`; + + beforeEach(() => { + // Stub gapi so loadPicker() resolves without the real script + vi.stubGlobal("gapi", { + load: (_feature: string, cb: () => void) => cb(), + }); + + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [DriveService, ...commonTestProviders], + }); + + service = TestBed.inject(DriveService); + httpMock = TestBed.inject(HttpTestingController); + ngZone = TestBed.inject(NgZone); + }); + + afterEach(() => { + httpMock.verify(); + vi.unstubAllGlobals(); + }); + + describe("getToken", () => { + it("calls the token endpoint and returns the response", async () => { + const promise = firstValueFrom(service.getToken()); + + const req = httpMock.expectOne(`${BASE}/token`); + expect(req.request.method).toBe("GET"); + req.flush({ status: "ok", accessToken: "abc123" }); + + const result = await promise; + expect(result.status).toBe("ok"); + expect(result.accessToken).toBe("abc123"); + }); + + it("returns no_refresh_token status when user has not connected Drive", async () => { + const promise = firstValueFrom(service.getToken()); + + httpMock.expectOne(`${BASE}/token`).flush({ status: "no_refresh_token" }); + + const result = await promise; + expect(result.status).toBe("no_refresh_token"); + expect(result.accessToken).toBeUndefined(); + }); + }); + + describe("connect", () => { + it("fetches the connect URL and opens a popup", () => { + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + + service.connect(); + + const req = httpMock.expectOne(`${BASE}/connect?reauth=false`); + 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("passes reauth=true when reauth flag is set", () => { + vi.spyOn(window, "open").mockReturnValue(null); + + service.connect(true); + + httpMock.expectOne(`${BASE}/connect?reauth=true`).flush("https://accounts.google.com/...";); + }); + + it("emits on onConnected() when the popup posts gdrive-connected", fakeAsync(() => { + const mockPopup = { close: vi.fn() } as unknown as Window; + vi.spyOn(window, "open").mockReturnValue(mockPopup); + + let connected = false; + service.onConnected().subscribe(() => { + connected = true; + }); + + service.connect(); + httpMock.expectOne(`${BASE}/connect?reauth=false`).flush("https://accounts.google.com/...";); + + ngZone.run(() => { + window.dispatchEvent(new MessageEvent("message", { data: "gdrive-connected" })); + }); + tick(); + + expect(connected).toBe(true); + expect(mockPopup.close).toHaveBeenCalled(); + })); + + it("does not emit on onConnected() for unrelated messages", fakeAsync(() => { + vi.spyOn(window, "open").mockReturnValue(null); + + let connected = false; + service.onConnected().subscribe(() => { + connected = true; + }); + + service.connect(); + httpMock.expectOne(`${BASE}/connect?reauth=false`).flush("https://accounts.google.com/...";); + + window.dispatchEvent(new MessageEvent("message", { data: "some-other-message" })); + tick(); + + expect(connected).toBe(false); + })); + }); + + describe("exportToDrive", () => { + it("errors the observable when no access token is available", fakeAsync(async () => { + // Token endpoint returns no_refresh_token so getAccessToken returns null + const result$ = service.exportToDrive(new Blob(["test"]), "test.json"); + + // getToken is called inside getAccessToken + httpMock.expectOne(`${BASE}/token`).flush({ status: "no_refresh_token" }); + + tick(); + + let errorMessage = ""; + result$.subscribe({ error: (e: unknown) => (errorMessage = (e as Error).message) }); + tick(); + + expect(errorMessage).toBe("Not connected to Google Drive"); + })); + }); Review Comment: The PR description claims unit tests cover that `exportToDrive()` "calls the Drive multipart upload API and resolves", but the added spec only asserts the no-access-token error path. There is currently no test that stubs the picker selection and verifies the upload `fetch(...)` is invoked / resolves. ########## frontend/src/app/dashboard/service/user/google-drive/drive.service.spec.ts: ########## @@ -0,0 +1,161 @@ +/** + * 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 { firstValueFrom } from "rxjs"; +import { commonTestProviders } from "../../../../common/testing/test-utils"; + +describe("DriveService", () => { + let service: DriveService; + let httpMock: HttpTestingController; + let ngZone: NgZone; + + const BASE = `${AppSettings.getApiEndpoint()}/auth/google/drive`; + + beforeEach(() => { + // Stub gapi so loadPicker() resolves without the real script + vi.stubGlobal("gapi", { + load: (_feature: string, cb: () => void) => cb(), + }); + + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [DriveService, ...commonTestProviders], + }); + + service = TestBed.inject(DriveService); + httpMock = TestBed.inject(HttpTestingController); + ngZone = TestBed.inject(NgZone); + }); + + afterEach(() => { + httpMock.verify(); + vi.unstubAllGlobals(); + }); + + describe("getToken", () => { + it("calls the token endpoint and returns the response", async () => { + const promise = firstValueFrom(service.getToken()); + + const req = httpMock.expectOne(`${BASE}/token`); + expect(req.request.method).toBe("GET"); + req.flush({ status: "ok", accessToken: "abc123" }); + + const result = await promise; + expect(result.status).toBe("ok"); + expect(result.accessToken).toBe("abc123"); + }); + + it("returns no_refresh_token status when user has not connected Drive", async () => { + const promise = firstValueFrom(service.getToken()); + + httpMock.expectOne(`${BASE}/token`).flush({ status: "no_refresh_token" }); + + const result = await promise; + expect(result.status).toBe("no_refresh_token"); + expect(result.accessToken).toBeUndefined(); + }); + }); + + describe("connect", () => { + it("fetches the connect URL and opens a popup", () => { + const openSpy = vi.spyOn(window, "open").mockReturnValue(null); + + service.connect(); + + const req = httpMock.expectOne(`${BASE}/connect?reauth=false`); + 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("passes reauth=true when reauth flag is set", () => { + vi.spyOn(window, "open").mockReturnValue(null); + + service.connect(true); + + httpMock.expectOne(`${BASE}/connect?reauth=true`).flush("https://accounts.google.com/...";); + }); + + it("emits on onConnected() when the popup posts gdrive-connected", fakeAsync(() => { + const mockPopup = { close: vi.fn() } as unknown as Window; + vi.spyOn(window, "open").mockReturnValue(mockPopup); + + let connected = false; + service.onConnected().subscribe(() => { + connected = true; + }); + + service.connect(); + httpMock.expectOne(`${BASE}/connect?reauth=false`).flush("https://accounts.google.com/...";); + + ngZone.run(() => { + window.dispatchEvent(new MessageEvent("message", { data: "gdrive-connected" })); + }); + tick(); + + expect(connected).toBe(true); + expect(mockPopup.close).toHaveBeenCalled(); + })); + + it("does not emit on onConnected() for unrelated messages", fakeAsync(() => { + vi.spyOn(window, "open").mockReturnValue(null); + + let connected = false; + service.onConnected().subscribe(() => { + connected = true; + }); + + service.connect(); + httpMock.expectOne(`${BASE}/connect?reauth=false`).flush("https://accounts.google.com/...";); + + window.dispatchEvent(new MessageEvent("message", { data: "some-other-message" })); + tick(); + + expect(connected).toBe(false); + })); + }); + + describe("exportToDrive", () => { + it("errors the observable when no access token is available", fakeAsync(async () => { Review Comment: This test mixes `fakeAsync` with an `async` test function (`fakeAsync(async () => { ... })`). Angular’s `fakeAsync` zone is not compatible with `async`/`await` and can lead to flaky behavior. Since the body doesn’t use `await`, drop the `async` keyword. ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResource.scala: ########## @@ -0,0 +1,209 @@ +/* + * 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.fasterxml.jackson.databind.ObjectMapper +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.auth.{SessionUser, TokenEncryptionService} +import org.apache.texera.web.model.http.response.DriveTokenIssueResponse +import org.apache.texera.web.resource.auth.GoogleDriveAuthResource._ +import org.apache.texera.dao.jooq.generated.tables.daos.UserOauthTokenDao +import org.apache.texera.dao.jooq.generated.tables.pojos.UserOauthToken +import org.apache.texera.dao.SqlServer +import org.apache.texera.config.UserSystemConfig +import com.google.api.client.googleapis.auth.oauth2.{ + GoogleAuthorizationCodeRequestUrl, + GoogleAuthorizationCodeTokenRequest, + GoogleRefreshTokenRequest, + GoogleTokenResponse +} +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 STATUS_OK = "ok" + private val STATUS_NO_REFRESH_TOKEN = "no_refresh_token" + private val STATUS_INVALID_GRANT = "invalid_grant" + private val PROVIDER_GOOGLE_DRIVE = "google_drive" + + private val STATE_TTL_MS = 10 * 60 * 1000L + + private val mapper = new ObjectMapper() + + // state token → (uid, expiresAtMs) + private val pendingStates = new ConcurrentHashMap[String, (Int, Long)]() + + private def oauthTokenDao = + new UserOauthTokenDao( + SqlServer + .getInstance() + .createDSLContext() + .configuration + ) +} + +@Consumes(Array(MediaType.APPLICATION_JSON)) +@Produces(Array(MediaType.APPLICATION_JSON)) +class GoogleDriveAuthResource extends LazyLogging { + 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("/token") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getDriveAccessToken(@Auth sessionUser: SessionUser): Response = { + val uid = sessionUser.getUid + val record = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + .orElse(null) + + if (record == null) { + return Response.ok(DriveTokenIssueResponse(STATUS_NO_REFRESH_TOKEN, None)).build() + } + + try { + val blob = mapper.readTree(TokenEncryptionService.decrypt(record.getAuthBlob)) + val refreshToken = blob.get("refreshToken").asText() + + val tokenResponse = new GoogleRefreshTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + refreshToken, + clientId, + clientSecret + ).execute() + + Response.ok(DriveTokenIssueResponse(STATUS_OK, Some(tokenResponse.getAccessToken))).build() + } catch { + case e: TokenResponseException => + if (e.getDetails != null && e.getDetails.getError == STATUS_INVALID_GRANT) { + Response.ok(DriveTokenIssueResponse(STATUS_INVALID_GRANT, None)).build() + } else { + logger.error("Failed to refresh access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + case e: Exception => + logger.error("Unexpected error refreshing access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + } + + @GET + @Path("/callback") + @Produces(Array(MediaType.TEXT_HTML, MediaType.APPLICATION_JSON)) + def getCallback( + @QueryParam("code") @DefaultValue("") code: String, + @QueryParam("state") @DefaultValue("") state: String + ): Response = { + if (code.isEmpty || state.isEmpty) { + return Response.status(Response.Status.BAD_REQUEST).build() + } + try { + val entry = pendingStates.remove(state) + if (entry == null || System.currentTimeMillis() > entry._2) { + return Response + .status(Response.Status.UNAUTHORIZED) + .entity("OAuth state token is invalid or expired") + .build() + } + + val uid = entry._1 + + val tokenResponse: GoogleTokenResponse = new GoogleAuthorizationCodeTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + clientId, + clientSecret, + code, + redirectUri + ).execute() + + val blobMap = new java.util.HashMap[String, String]() + blobMap.put("refreshToken", tokenResponse.getRefreshToken) + blobMap.put("scopes", tokenResponse.getScope) + val blobJson = mapper.writeValueAsString(blobMap) + val encryptedBlob = TokenEncryptionService.encrypt(blobJson) + + val existing = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + + if (existing.isPresent) { + existing.get().setAuthBlob(encryptedBlob) + oauthTokenDao.update(existing.get()) + } else { + val record = new UserOauthToken() + record.setUid(uid) + record.setProvider(PROVIDER_GOOGLE_DRIVE) + record.setAuthBlob(encryptedBlob) + oauthTokenDao.insert(record) + } + + val html = + """<html><body><script> + |window.opener.postMessage('gdrive-connected', window.location.origin); + |window.close(); + |</script></body></html>""".stripMargin + Response.ok(html).build() + } catch { + case e: TokenResponseException => + logger.error("Google token exchange failed in callback", e) + Response.status(Response.Status.BAD_GATEWAY).build() + case e: Exception => + logger.error("Unexpected error in OAuth callback", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + } + + @GET + @Path("/connect") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getOAuth( + @Auth sessionUser: SessionUser, + @QueryParam("reauth") @DefaultValue("false") reauth: Boolean + ): Response = { + val stateToken = java.util.UUID.randomUUID().toString + pendingStates.put(stateToken, (sessionUser.getUid, System.currentTimeMillis() + STATE_TTL_MS)) + Review Comment: `pendingStates` is only pruned when the callback is hit; abandoned OAuth attempts (user closes popup, network error, etc.) will leave entries in this map until process restart. Over time this can grow unbounded (and is user-triggerable via repeated `/connect` calls). ########## amber/src/main/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResource.scala: ########## @@ -0,0 +1,209 @@ +/* + * 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.fasterxml.jackson.databind.ObjectMapper +import com.typesafe.scalalogging.LazyLogging +import org.apache.texera.auth.{SessionUser, TokenEncryptionService} +import org.apache.texera.web.model.http.response.DriveTokenIssueResponse +import org.apache.texera.web.resource.auth.GoogleDriveAuthResource._ +import org.apache.texera.dao.jooq.generated.tables.daos.UserOauthTokenDao +import org.apache.texera.dao.jooq.generated.tables.pojos.UserOauthToken +import org.apache.texera.dao.SqlServer +import org.apache.texera.config.UserSystemConfig +import com.google.api.client.googleapis.auth.oauth2.{ + GoogleAuthorizationCodeRequestUrl, + GoogleAuthorizationCodeTokenRequest, + GoogleRefreshTokenRequest, + GoogleTokenResponse +} +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 STATUS_OK = "ok" + private val STATUS_NO_REFRESH_TOKEN = "no_refresh_token" + private val STATUS_INVALID_GRANT = "invalid_grant" + private val PROVIDER_GOOGLE_DRIVE = "google_drive" + + private val STATE_TTL_MS = 10 * 60 * 1000L + + private val mapper = new ObjectMapper() + + // state token → (uid, expiresAtMs) + private val pendingStates = new ConcurrentHashMap[String, (Int, Long)]() + + private def oauthTokenDao = + new UserOauthTokenDao( + SqlServer + .getInstance() + .createDSLContext() + .configuration + ) +} + +@Consumes(Array(MediaType.APPLICATION_JSON)) +@Produces(Array(MediaType.APPLICATION_JSON)) +class GoogleDriveAuthResource extends LazyLogging { + 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("/token") + @RolesAllowed(Array("REGULAR", "ADMIN")) + def getDriveAccessToken(@Auth sessionUser: SessionUser): Response = { + val uid = sessionUser.getUid + val record = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + .orElse(null) + + if (record == null) { + return Response.ok(DriveTokenIssueResponse(STATUS_NO_REFRESH_TOKEN, None)).build() + } + + try { + val blob = mapper.readTree(TokenEncryptionService.decrypt(record.getAuthBlob)) + val refreshToken = blob.get("refreshToken").asText() + + val tokenResponse = new GoogleRefreshTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + refreshToken, + clientId, + clientSecret + ).execute() + + Response.ok(DriveTokenIssueResponse(STATUS_OK, Some(tokenResponse.getAccessToken))).build() + } catch { + case e: TokenResponseException => + if (e.getDetails != null && e.getDetails.getError == STATUS_INVALID_GRANT) { + Response.ok(DriveTokenIssueResponse(STATUS_INVALID_GRANT, None)).build() + } else { + logger.error("Failed to refresh access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + case e: Exception => + logger.error("Unexpected error refreshing access token", e) + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build() + } + } + + @GET + @Path("/callback") + @Produces(Array(MediaType.TEXT_HTML, MediaType.APPLICATION_JSON)) + def getCallback( + @QueryParam("code") @DefaultValue("") code: String, + @QueryParam("state") @DefaultValue("") state: String + ): Response = { + if (code.isEmpty || state.isEmpty) { + return Response.status(Response.Status.BAD_REQUEST).build() + } + try { + val entry = pendingStates.remove(state) + if (entry == null || System.currentTimeMillis() > entry._2) { + return Response + .status(Response.Status.UNAUTHORIZED) + .entity("OAuth state token is invalid or expired") + .build() + } + + val uid = entry._1 + + val tokenResponse: GoogleTokenResponse = new GoogleAuthorizationCodeTokenRequest( + new NetHttpTransport(), + GsonFactory.getDefaultInstance, + clientId, + clientSecret, + code, + redirectUri + ).execute() + + val blobMap = new java.util.HashMap[String, String]() + blobMap.put("refreshToken", tokenResponse.getRefreshToken) + blobMap.put("scopes", tokenResponse.getScope) + val blobJson = mapper.writeValueAsString(blobMap) + val encryptedBlob = TokenEncryptionService.encrypt(blobJson) + + val existing = oauthTokenDao.fetchByUid(uid).stream() + .filter(r => r.getProvider == PROVIDER_GOOGLE_DRIVE) + .findFirst() + + if (existing.isPresent) { + existing.get().setAuthBlob(encryptedBlob) + oauthTokenDao.update(existing.get()) + } else { + val record = new UserOauthToken() + record.setUid(uid) + record.setProvider(PROVIDER_GOOGLE_DRIVE) + record.setAuthBlob(encryptedBlob) + oauthTokenDao.insert(record) + } Review Comment: The OAuth callback overwrites the stored credential blob with `tokenResponse.getRefreshToken`, but Google often omits `refresh_token` on subsequent auth code exchanges unless the user is forced through `prompt=consent`. In that case this code will persist a null/empty refresh token and permanently break future `/token` refreshes for that user. Guard against missing refresh tokens: keep the existing DB row unchanged when `refresh_token` is absent (and only error if this is the first-ever connect). -- 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]
