From ba9d0b589788eb62be3a8a462a60e34b3e177428 Mon Sep 17 00:00:00 2001 From: mengw15 <125719918+mengw15@users.noreply.github.com> Date: Fri, 22 May 2026 23:31:45 -0700 Subject: [PATCH 1/4] fix: surface SMTP failures from gmail share endpoint to UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GmailResource.sendEmailRequest` discarded the `Either[String, Unit]` result from `sendEmail`. The method itself returned `Unit` and JAX-RS sent `204 No Content` regardless of outcome, so any SMTP failure (invalid credentials, network error, etc.) was logged server-side but the HTTP layer reported success. The frontend's `next:` callback fired and showed a green "Email sent successfully" toast even though no mail had been delivered. Pattern-match the `Either` and throw `WebApplicationException(error, BAD_GATEWAY)` on `Left`, matching the codebase convention in `AdminUserResource` / `UserResource`. JAX-RS converts the exception into an HTTP 502, which triggers the frontend's `error:` callback. The frontend `error:` branch in `gmail.service.ts` previously only `console.error`'d, so users still saw no feedback on failure. Add a `notificationService.error(...)` call mirroring the existing `notifyUnauthorizedLogin` error handler in the same file, so the user sees a red toast on failure. The user-facing message is generic ("Failed to send email. Please try again or contact admin.") rather than the raw SMTP error to avoid exposing protocol-level details to end users; the specific SMTP error stays in the backend log and the browser console for debugging. Add `gmail.service.spec.ts` (Vitest + `HttpClientTestingModule`) covering both the success-toast and error-toast contracts. No backend test added: `GmailResource` lacks a JAX-RS controller harness, and building one would require refactoring the companion-object `sendEmail` out as an injectable dependency — disproportionate to a five-line fix. Closes #5161. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../texera/web/resource/GmailResource.scala | 7 +- .../service/gmail/gmail.service.spec.ts | 66 +++++++++++++++++++ .../app/common/service/gmail/gmail.service.ts | 3 +- 3 files changed, 74 insertions(+), 2 deletions(-) create mode 100644 frontend/src/app/common/service/gmail/gmail.service.spec.ts diff --git a/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala index f06f1f92102..fdecde1bdeb 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala @@ -33,6 +33,7 @@ import javax.annotation.security.RolesAllowed import javax.mail.internet.{InternetAddress, MimeMessage} import javax.mail.{Message, PasswordAuthentication, Session, Transport} import javax.ws.rs._ +import javax.ws.rs.core.Response import scala.util.{Failure, Success, Try} case class EmailMessage( @@ -146,7 +147,11 @@ class GmailResource { @Path("/send") def sendEmailRequest(emailMessage: EmailMessage, @Auth user: SessionUser): Unit = { val recipientEmail = if (emailMessage.receiver.isEmpty) user.getEmail else emailMessage.receiver - sendEmail(emailMessage, recipientEmail) + sendEmail(emailMessage, recipientEmail) match { + case Right(_) => () + case Left(error) => + throw new WebApplicationException(error, Response.Status.BAD_GATEWAY) + } } @GET diff --git a/frontend/src/app/common/service/gmail/gmail.service.spec.ts b/frontend/src/app/common/service/gmail/gmail.service.spec.ts new file mode 100644 index 00000000000..fd4ce18ca72 --- /dev/null +++ b/frontend/src/app/common/service/gmail/gmail.service.spec.ts @@ -0,0 +1,66 @@ +/** + * 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 } from "@angular/core/testing"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; +import { GmailService } from "./gmail.service"; +import { NotificationService } from "../notification/notification.service"; + +describe("GmailService", () => { + let service: GmailService; + let httpTestingController: HttpTestingController; + let notificationSpy: { success: ReturnType; error: ReturnType }; + + beforeEach(() => { + notificationSpy = { success: vi.fn(), error: vi.fn() }; + TestBed.configureTestingModule({ + imports: [HttpClientTestingModule], + providers: [GmailService, { provide: NotificationService, useValue: notificationSpy }], + }); + service = TestBed.inject(GmailService); + httpTestingController = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpTestingController.verify(); + }); + + it("should show a success toast when the backend accepts the send request", () => { + service.sendEmail("subj", "body", "to@example.com"); + + const req = httpTestingController.expectOne(r => r.url.endsWith("/gmail/send") && r.method === "PUT"); + req.flush(null); + + expect(notificationSpy.success).toHaveBeenCalledWith("Email sent successfully"); + expect(notificationSpy.error).not.toHaveBeenCalled(); + }); + + it("should show an error toast when the backend returns an HTTP error (e.g. SMTP failure)", () => { + service.sendEmail("subj", "body", "to@example.com"); + + const req = httpTestingController.expectOne(r => r.url.endsWith("/gmail/send") && r.method === "PUT"); + req.flush("Failed to send email: 535-5.7.8 Username and Password not accepted", { + status: 502, + statusText: "Bad Gateway", + }); + + expect(notificationSpy.error).toHaveBeenCalledWith("Failed to send email. Please try again or contact admin."); + expect(notificationSpy.success).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/app/common/service/gmail/gmail.service.ts b/frontend/src/app/common/service/gmail/gmail.service.ts index 925e7e3a39d..3dfec89a064 100644 --- a/frontend/src/app/common/service/gmail/gmail.service.ts +++ b/frontend/src/app/common/service/gmail/gmail.service.ts @@ -42,7 +42,8 @@ export class GmailService { next: () => this.notificationService.success("Email sent successfully"), error: (error: unknown) => { if (error instanceof HttpErrorResponse) { - console.error(error.error); + this.notificationService.error("Failed to send email. Please try again or contact admin."); + console.error("Send email error:", error.error); } }, }); From 58f651d7090d19e5470675e134c21dbf8fd643a9 Mon Sep 17 00:00:00 2001 From: mengw15 <125719918+mengw15@users.noreply.github.com> Date: Sat, 23 May 2026 16:13:27 -0700 Subject: [PATCH 2/4] fix: return 400 BadRequest for invalid email format Address review feedback from #5164: `sendEmail` returns `Left("Invalid email format")` for client-side validation failures and `Left("Failed to send email: ...")` for SMTP / `Transport.send` failures. Mapping both to 502 made validation errors look like upstream/gateway failures. Match on the validation message explicitly and throw `BadRequestException` (400); reserve 502 for the SMTP/Transport path. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../scala/org/apache/texera/web/resource/GmailResource.scala | 2 ++ 1 file changed, 2 insertions(+) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala index fdecde1bdeb..ab91c9ad437 100644 --- a/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala +++ b/amber/src/main/scala/org/apache/texera/web/resource/GmailResource.scala @@ -149,6 +149,8 @@ class GmailResource { val recipientEmail = if (emailMessage.receiver.isEmpty) user.getEmail else emailMessage.receiver sendEmail(emailMessage, recipientEmail) match { case Right(_) => () + case Left("Invalid email format") => + throw new BadRequestException("Invalid email format") case Left(error) => throw new WebApplicationException(error, Response.Status.BAD_GATEWAY) } From 315dbf83a38855c371ca2821e5b6573864f81d13 Mon Sep 17 00:00:00 2001 From: mengw15 <125719918+mengw15@users.noreply.github.com> Date: Sat, 23 May 2026 16:40:22 -0700 Subject: [PATCH 3/4] =?UTF-8?q?test:=20add=20GmailResourceSpec=20covering?= =?UTF-8?q?=20invalid-email=20=E2=86=92=20400?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validates the BadRequest mapping added in the prior commit: `sendEmailRequest` invoked with a receiver that fails `isValidEmail` throws `BadRequestException` with HTTP status 400. Calls the resource method directly with a constructed `SessionUser`, matching the style used by `WorkflowResourceSpec` — no `ResourceExtension` or auth-provider scaffolding needed. The 502 SMTP-failure path is not covered here; it would require either real SMTP failure injection or a fake SMTP server, both out of scope for this fix. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../web/resource/GmailResourceSpec.scala | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala diff --git a/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala new file mode 100644 index 00000000000..ef771396a49 --- /dev/null +++ b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala @@ -0,0 +1,52 @@ +/* + * 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 + +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 + +import javax.ws.rs.BadRequestException + +class GmailResourceSpec extends AnyFlatSpec { + + private def newSessionUser(): SessionUser = { + val user = new User + user.setUid(Integer.valueOf(1)) + user.setName("test") + user.setRole(UserRoleEnum.REGULAR) + user.setEmail("test@example.com") + new SessionUser(user) + } + + it should "throw BadRequestException (HTTP 400) when the receiver fails email-format validation" in { + val resource = new GmailResource() + val msg = EmailMessage( + receiver = "not-a-valid-email", + subject = "subj", + content = "body" + ) + val ex = intercept[BadRequestException] { + resource.sendEmailRequest(msg, newSessionUser()) + } + assert(ex.getResponse.getStatus == 400) + } +} From c374d1856c5832355923544a1da8d04ac0e938cf Mon Sep 17 00:00:00 2001 From: mengw15 <125719918+mengw15@users.noreply.github.com> Date: Sat, 23 May 2026 17:03:23 -0700 Subject: [PATCH 4/4] test: cover 502 SMTP-failure branch in GmailResourceSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a deterministic test for the `Left(error) => 502` mapping in `sendEmailRequest`. In the test environment `UserSystemConfig.gmail` defaults to "", so `createMimeMessage`'s `new InternetAddress(senderGmail)` raises an `AddressException` immediately — with no network or SMTP contact — `sendEmail` catches it and returns `Left("Failed to send email: ...")`, and the resource throws `WebApplicationException` with HTTP 502. The spec runs in ~400ms. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../web/resource/GmailResourceSpec.scala | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala index ef771396a49..868b0e34cda 100644 --- a/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala +++ b/amber/src/test/scala/org/apache/texera/web/resource/GmailResourceSpec.scala @@ -24,7 +24,7 @@ import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum import org.apache.texera.dao.jooq.generated.tables.pojos.User import org.scalatest.flatspec.AnyFlatSpec -import javax.ws.rs.BadRequestException +import javax.ws.rs.{BadRequestException, WebApplicationException} class GmailResourceSpec extends AnyFlatSpec { @@ -49,4 +49,26 @@ class GmailResourceSpec extends AnyFlatSpec { } assert(ex.getResponse.getStatus == 400) } + + it should "throw WebApplicationException with HTTP 502 when sendEmail fails for a non-validation reason" in { + // In the test environment `UserSystemConfig.gmail` defaults to "", so + // `createMimeMessage`'s `new InternetAddress(senderGmail)` raises an + // `AddressException` deterministically — without any network or SMTP + // server contact — and `sendEmail` returns `Left("Failed to send email: + // ...")`. The resource then maps that `Left` to a 502 BadGateway. + val resource = new GmailResource() + val msg = EmailMessage( + receiver = "valid@example.com", + subject = "subj", + content = "body" + ) + val ex = intercept[WebApplicationException] { + resource.sendEmailRequest(msg, newSessionUser()) + } + assert( + !ex.isInstanceOf[BadRequestException], + s"expected non-validation failure, but got BadRequestException: ${ex.getMessage}" + ) + assert(ex.getResponse.getStatus == 502) + } }