xuang7 commented on code in PR #5721: URL: https://github.com/apache/texera/pull/5721#discussion_r3455949849
########## 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)) Review Comment: uid is stored in the state but unused in Part 1. If it is reserved for a later part, we could add a comment. ########## amber/src/test/scala/org/apache/texera/web/resource/auth/GoogleDriveAuthResourceSpec.scala: ########## @@ -0,0 +1,75 @@ +/* + * 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 org.apache.texera.auth.SessionUser +import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum +import org.apache.texera.dao.jooq.generated.tables.pojos.User +import org.scalatest.flatspec.AnyFlatSpec + +class GoogleDriveAuthResourceSpec extends AnyFlatSpec { + + private def newSessionUser(): SessionUser = { + val user = new User + user.setUid(Integer.valueOf(1)) + user.setName("test") + user.setRole(UserRoleEnum.REGULAR) + user.setEmail("[email protected]") + new SessionUser(user) + } + + it should "return error HTML when code is missing" in { Review Comment: Here can be "GoogleDriveAuthResource" should "...". ########## 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() + } + + @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) { Review Comment: `pendingStates` is process-global and only removes entries after a matching callback. Abandoned flows may leave stale states in the map, so we can add state expiration. ########## 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> { Review Comment: This Observable returns no teardown, so on unsubscribe the message listener leaks. Consider returning a cleanup function that removes the listener, closes the popup, and unsubscribes the inner http.get. ########## 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"); Review Comment: window.open returns null when the popup is blocked, but we proceed and wait for a message that never arrives. ########## frontend/src/app/dashboard/component/user/list-item/list-item.component.html: ########## @@ -217,12 +218,29 @@ nz-button nzType="text" *ngIf="entry.type === 'workflow' || entry.type === 'dataset'" - title="Download" - (click)="onClickDownload()"> + nz-dropdown + [(nzVisible)]="exportMenuVisible" + nzOverlayClassName="export-dropdown-menu" + [nzDropdownMenu]="exportMenu" + title="Export"> <i nz-icon nzType="cloud-download"></i> </button> + <nz-dropdown-menu #exportMenu="nzDropdownMenu"> + <ul nz-menu> + <li + nz-menu-item + (click)="onClickDownload()"> + Download + </li> + <li + nz-menu-item + (click)="onClickExportToDrive(); $event.stopPropagation()"> Review Comment: Only "Export to Drive" calls $event.stopPropagation(), If it's to stop the click bubbling to the card, "Download" needs the same treatment. -- 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]
