This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6116-39db110e74b4bdcf0e4b0df961c25973e6bbb20c in repository https://gitbox.apache.org/repos/asf/texera.git
commit 3e62b8c06430bdee00250ee0c149c6c0b6d99e92 Author: Yicong Huang <[email protected]> AuthorDate: Sat Jul 18 09:31:24 2026 -0700 feat(config-service): move site_settings API from texera-web to config-service (#6116) ### What changes were proposed in this PR? Moves the `site_settings` API from texera-web's `AdminSettingsResource` into config-service's `ConfigResource` — config-service already seeds the table and has the JWT stack registered. New surface: - `GET /config/settings/public` — anonymous, all user-visible keys (branding, sidebar tabs, upload limits) in one payload, replacing the request-per-key pattern. Anonymous by design: these values render on the logged-out shell, and the SPA login never recreates the dashboard shell, so gating them would blank public pages and leave freshly logged-in users unbranded until a refresh. - `GET /config/settings` — `ADMIN`, all rows in one payload; the admin settings page now loads with one request instead of 20. - `GET/PUT /config/settings/{key}`, `POST /config/settings/reset/{key}` — `ADMIN`. `PUT` returns 400 for a null/empty body or a null value, and for a key with no `default.conf` entry, so writes stay within the known-default namespace (mirroring `reset`). The public whitelist is derived from the `gui`/`dataset` sections of `default.conf` (new `DefaultsConfig.keysUnderSections`, pinned by a spec) instead of a hand-maintained list; keys outside those sections stay management-only. `DefaultsConfig` also fails fast when two settings collapse to the same `site_settings` row key, and operator-tunable keys move to a new `operator` section (e.g. `csv_parser_max_columns`) that is deliberately excluded from the anonymous whitelist. These endpoints complement the HOCON-backed `/config/*` reads (deploy-time, frozen per JVM) with no key overlap. All `site_settings` access — resource queries, startup seeder, `dao.SiteSettings` — now uses the generated jOOQ `SITE_SETTINGS` table instead of string-based lookups. Writes go through new `dao.SiteSettings.upsert`/`insertIfAbsent` helpers so the resource and the seeder share one column/audit-stamp shape, and `ConfigService` declares its direct `DAO` dependency. Frontend: `AdminSettingsService` moves to `/api/config/settings` (existing routing covers it; `/settings/public` is excluded from JWT injection). `getPublicSetting` reads from one shared cached request typed `string | null` and invalidates the cache on save/reset; the per-key `getSetting` is gone. Numeric settings parse through `parseIntOrDefault` (a stored `0` is preserved), the admin page blocks a save until a load has succeeded, and the dashboard keeps no copy of the tab defaults — every tab stays hidden until `/config/settings/public` loads, so the backend is the single source of the tab defaults. ### Any related issues, documentation, discussions? Closes #6115 ### How was this PR tested? - `sbt ConfigService/test` — `ConfigResourceSpec` covers the HTTP auth gates and the endpoint bodies against embedded Postgres, including the whitelist-derivation pin, the null-value / null-body / unknown-key 400s, upsert-on-conflict, and reset; `DefaultsConfigSpec` covers `keysUnderSections`. - Frontend: `admin-settings.service.spec.ts` (shared cache, error retry, invalidation on save/reset); component specs cover the admin bulk load (defaults on missing/unparsable values, a stored `0`, error toast, save-blocked-until-loaded) and the dashboard branding/tab fallbacks; `ng test`, `tsc --noEmit`, prettier, and eslint clean. - Manual end-to-end on the local dev stack: anonymous `GET /settings/public` serves the whitelisted map, `GET /settings` and `GET /settings/{key}` reject anonymous callers with 401, old `/api/admin/settings/{key}` 404s. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (claude-fable-5, claude-opus-4-8) --------- Co-authored-by: Claude Fable 5 <[email protected]> --- .../apache/texera/web/TexeraWebApplication.scala | 2 - .../admin/settings/AdminSettingsResource.scala | 108 ----- build.sbt | 3 +- common/config/src/main/resources/default.conf | 9 + .../texera/common/config/DefaultsConfig.scala | 41 +- .../texera/common/config/DefaultsConfigSpec.scala | 36 ++ .../scala/org/apache/texera/dao/SiteSettings.scala | 53 ++- .../org/apache/texera/service/ConfigService.scala | 18 +- .../texera/service/resource/ConfigResource.scala | 132 +++++- .../service/resource/ConfigResourceAuthSpec.scala | 230 ---------- .../service/resource/ConfigResourceSpec.scala | 481 +++++++++++++++++++++ frontend/src/app/app.module.ts | 2 +- frontend/src/app/common/util/format.util.ts | 10 + .../settings/admin-settings.component.spec.ts | 76 +++- .../admin/settings/admin-settings.component.ts | 105 +++-- .../component/dashboard.component.spec.ts | 64 ++- .../app/dashboard/component/dashboard.component.ts | 49 ++- .../files-uploader.component.spec.ts | 18 +- .../files-uploader/files-uploader.component.ts | 9 +- .../dataset-detail.component.spec.ts | 20 +- .../dataset-detail.component.ts | 25 +- .../admin/settings/admin-settings.service.spec.ts | 104 +++-- .../admin/settings/admin-settings.service.ts | 51 ++- 23 files changed, 1145 insertions(+), 501 deletions(-) diff --git a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala index 20ce01008b..73e473ba7a 100644 --- a/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala +++ b/amber/src/main/scala/org/apache/texera/web/TexeraWebApplication.scala @@ -36,7 +36,6 @@ import org.apache.texera.web.resource._ import org.apache.texera.web.resource.auth.{AuthResource, GoogleAuthResource} import org.apache.texera.web.resource.dashboard.DashboardResource import org.apache.texera.web.resource.dashboard.admin.execution.AdminExecutionResource -import org.apache.texera.web.resource.dashboard.admin.settings.AdminSettingsResource import org.apache.texera.web.resource.dashboard.admin.user.AdminUserResource import org.apache.texera.web.resource.dashboard.hub.HubResource import org.apache.texera.web.resource.dashboard.user.UserResource @@ -159,7 +158,6 @@ class TexeraWebApplication environment.jersey.register(classOf[GmailResource]) environment.jersey.register(classOf[AdminExecutionResource]) environment.jersey.register(classOf[UserQuotaResource]) - environment.jersey.register(classOf[AdminSettingsResource]) environment.jersey.register(classOf[AIAssistantResource]) environment.jersey.register(classOf[HuggingFaceModelResource]) diff --git a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/settings/AdminSettingsResource.scala b/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/settings/AdminSettingsResource.scala deleted file mode 100644 index a1880f3c3c..0000000000 --- a/amber/src/main/scala/org/apache/texera/web/resource/dashboard/admin/settings/AdminSettingsResource.scala +++ /dev/null @@ -1,108 +0,0 @@ -/* - * 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.dashboard.admin.settings - -import com.fasterxml.jackson.annotation.JsonProperty -import io.dropwizard.auth.Auth -import org.apache.texera.auth.SessionUser -import org.apache.texera.common.config.DefaultsConfig -import org.apache.texera.dao.SqlServer -import org.jooq.impl.DSL - -import javax.annotation.security.RolesAllowed -import javax.ws.rs._ -import javax.ws.rs.core.{MediaType, Response} - -case class AdminSettingsPojo( - @JsonProperty("key") settingKey: String, - @JsonProperty("value") settingValue: String -) - -@Path("/admin/settings") -@Produces(Array(MediaType.APPLICATION_JSON)) -class AdminSettingsResource { - - private def ctx = SqlServer.getInstance().createDSLContext() - private val siteSettings = DSL.table("site_settings") - private val key = DSL.field("key", classOf[String]) - private val value = DSL.field("value", classOf[String]) - private val updatedBy = DSL.field("updated_by", classOf[String]) - - @GET - @Path("{key}") - def getSetting(@PathParam("key") keyParam: String): AdminSettingsPojo = { - ctx - .select(key, value) - .from(siteSettings) - .where(key.eq(keyParam)) - .fetchOneInto(classOf[AdminSettingsPojo]) - } - - @PUT - @Path("{key}") - @RolesAllowed(Array("ADMIN")) - @Consumes(Array(MediaType.APPLICATION_JSON)) - def updateSetting( - @Auth currentUser: SessionUser, - @PathParam("key") keyParam: String, - setting: AdminSettingsPojo - ): Response = { - if (setting.settingValue != null && keyParam.nonEmpty) { - upsertSetting(keyParam, setting.settingValue, currentUser.getName) - } - Response.ok().build() - } - - /** - * Resets the specified configuration key to its default value defined in default.conf. - */ - @POST - @Path("/reset/{key}") - @RolesAllowed(Array("ADMIN")) - def resetSetting( - @Auth currentUser: SessionUser, - @PathParam("key") keyParam: String - ): Response = { - DefaultsConfig.allDefaults.get(keyParam) match { - case Some(defaultValue) => - upsertSetting(keyParam, defaultValue, currentUser.getName) - Response.ok().build() - case None => - Response - .status(Response.Status.NOT_FOUND) - .entity(s"No default for key '$keyParam'") - .build() - } - } - - private def upsertSetting(keyParam: String, valueParam: String, userName: String): Unit = { - ctx - .insertInto(siteSettings) - .set(key, keyParam) - .set(value, valueParam) - .set(updatedBy, userName) - .onConflict(key) - .doUpdate() - .set(value, valueParam) - .set(DSL.field("updated_by", classOf[String]), userName) - .set(DSL.field("updated_at", classOf[java.sql.Timestamp]), DSL.currentTimestamp()) - .execute() - } -} diff --git a/build.sbt b/build.sbt index 47aea23e8b..479bfe4631 100644 --- a/build.sbt +++ b/build.sbt @@ -126,7 +126,8 @@ lazy val Auth = (project in file("common/auth")) .dependsOn(DAO, Config) .dependsOn(DAO % "test->test") // reuse MockTexeraDB embedded Postgres in tests lazy val ConfigService = (project in file("config-service")) - .dependsOn(Auth, Config, Resource) + .dependsOn(Auth, Config, DAO, Resource) + .dependsOn(DAO % "test->test") // reuse MockTexeraDB embedded Postgres in tests .settings(commonModuleSettings) .settings( dependencyOverrides ++= Seq( diff --git a/common/config/src/main/resources/default.conf b/common/config/src/main/resources/default.conf index 62e533e694..6b83f50f6b 100644 --- a/common/config/src/main/resources/default.conf +++ b/common/config/src/main/resources/default.conf @@ -94,3 +94,12 @@ dataset { multipart_upload_chunk_size_mib = 50 multipart_upload_chunk_size_mib = ${?DATASET_MULTIPART_UPLOAD_CHUNK_SIZE_MIB} } + +# Operator-level defaults. These are management-only site settings (edited from +# the admin page, read by the engine) and are deliberately NOT under gui/dataset, +# so they stay out of the anonymous /config/settings/public whitelist. +operator { + # Upper bound on the number of columns the CSV scan source will parse. + csv_parser_max_columns = 512 + csv_parser_max_columns = ${?OPERATOR_CSV_PARSER_MAX_COLUMNS} +} diff --git a/common/config/src/main/scala/org/apache/texera/common/config/DefaultsConfig.scala b/common/config/src/main/scala/org/apache/texera/common/config/DefaultsConfig.scala index 965dcb8c0c..3a00e8da5d 100644 --- a/common/config/src/main/scala/org/apache/texera/common/config/DefaultsConfig.scala +++ b/common/config/src/main/scala/org/apache/texera/common/config/DefaultsConfig.scala @@ -27,20 +27,53 @@ object DefaultsConfig { val reinit: Boolean = conf.getBoolean("config-service.always-reset-configurations-to-default-values") + // site_settings rows are keyed by the last path segment of a HOCON key, so + // this is the single place that turns a config path into a row key. + private def shortKey(fullKey: String): String = fullKey.split("\\.").last + val allDefaults: Map[String, String] = { - conf + val entries = conf .entrySet() .asScala + .toSeq .map { entry => - val shortKey = entry.getKey.split("\\.").last val value = entry.getValue.valueType() match { case ConfigValueType.STRING | ConfigValueType.NUMBER | ConfigValueType.BOOLEAN => entry.getValue.unwrapped().toString case _ => entry.getValue.render(ConfigRenderOptions.concise()) } - shortKey -> value + shortKey(entry.getKey) -> value } - .toMap + + // Since the row key is only the last path segment, two entries under + // different sections that share a last segment would silently collapse into + // one site_settings row (and one whitelist entry). Fail fast at load time + // rather than serving a wrong default or an unaddressable key. + val collisions = entries.groupBy(_._1).collect { case (k, vs) if vs.size > 1 => k } + require( + collisions.isEmpty, + s"default.conf declares settings whose last path segment collides: " + + s"${collisions.toSeq.sorted.mkString(", ")}. site_settings keys are the last path " + + "segment, so give these a unique leaf name." + ) + + entries.toMap } + + /** + * Short keys (last path segment, matching the site_settings row keys) of every + * default declared under the given top-level sections of default.conf. Lets + * callers derive key groups (e.g. the user-visible gui/dataset settings) from + * the file that already defines them, instead of maintaining a parallel list. + */ + def keysUnderSections(sections: Set[String]): Set[String] = + conf + .entrySet() + .asScala + .collect { + case entry if sections.contains(entry.getKey.takeWhile(_ != '.')) => + shortKey(entry.getKey) + } + .toSet } diff --git a/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala b/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala index 3da26e1ff8..e7bf3fb6ab 100644 --- a/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala +++ b/common/config/src/test/scala/org/apache/texera/common/config/DefaultsConfigSpec.scala @@ -46,7 +46,43 @@ class DefaultsConfigSpec extends AnyFlatSpec with Matchers { defaults.get("single_file_upload_max_size_mib") shouldBe Some("20") ) ifUnset("GUI_TABS_HUB_ENABLED")(defaults.get("hub_enabled") shouldBe Some("true")) + // management-only keys are flattened too (used by reset + the startup seeder) + ifUnset("OPERATOR_CSV_PARSER_MAX_COLUMNS")( + defaults.get("csv_parser_max_columns") shouldBe Some("512") + ) // every value is rendered as a String defaults.values.foreach(_ shouldBe a[String]) } + + it should "keep management-only keys out of the public gui/dataset whitelist" in { + // csv_parser_max_columns is seeded and resettable (present in allDefaults) + // but lives under `operator`, so it must never reach the anonymous + // /config/settings/public payload. + DefaultsConfig.allDefaults.keySet should contain("csv_parser_max_columns") + DefaultsConfig.keysUnderSections( + Set("gui", "dataset") + ) should not contain "csv_parser_max_columns" + } + + "DefaultsConfig.keysUnderSections" should "collect the short keys of the requested sections only" in { + val guiKeys = DefaultsConfig.keysUnderSections(Set("gui")) + guiKeys should contain allOf ("logo", "mini_logo", "favicon", "hub_enabled") + // keys from other sections are excluded + guiKeys should not contain "single_file_upload_max_size_mib" + guiKeys should not contain "always-reset-configurations-to-default-values" + + val datasetKeys = DefaultsConfig.keysUnderSections(Set("dataset")) + datasetKeys should contain("single_file_upload_max_size_mib") + datasetKeys should not contain "logo" + } + + it should "union multiple sections and be empty for an unknown section" in { + val union = DefaultsConfig.keysUnderSections(Set("gui", "dataset")) + union should contain allOf ("logo", "single_file_upload_max_size_mib") + // every returned key exists in allDefaults under the same short name + union.subsetOf(DefaultsConfig.allDefaults.keySet) shouldBe true + + DefaultsConfig.keysUnderSections(Set("no-such-section")) shouldBe empty + DefaultsConfig.keysUnderSections(Set.empty) shouldBe empty + } } diff --git a/common/dao/src/main/scala/org/apache/texera/dao/SiteSettings.scala b/common/dao/src/main/scala/org/apache/texera/dao/SiteSettings.scala index a1d70ea841..d818f0f056 100644 --- a/common/dao/src/main/scala/org/apache/texera/dao/SiteSettings.scala +++ b/common/dao/src/main/scala/org/apache/texera/dao/SiteSettings.scala @@ -18,19 +18,25 @@ package org.apache.texera.dao +import org.apache.texera.dao.jooq.generated.Tables.SITE_SETTINGS +import org.jooq.DSLContext import org.jooq.impl.DSL import scala.util.Try /** - * Read-side accessor for the `site_settings` key/value table that admin pages - * write through. Centralises the "look up by key, parse, fall back on any - * failure" pattern that previously lived inline in ConfigResource, - * CSVScanSourceOpExec, and DatasetResource. + * Accessor for the `site_settings` key/value table that admin pages write + * through. Centralises the "look up by key, parse, fall back on any failure" + * read pattern (previously inline in ConfigResource, CSVScanSourceOpExec, and + * DatasetResource) and the write shape used by the admin API and the + * config-service startup seeder, so the column set / audit stamping live in + * one place. * - * Failures swallowed by the outer Try include: SqlServer not initialised - * (e.g. on workers in distributed mode), no row for the key, and value that - * can't be parsed. In all of these cases the caller's default takes over. + * Failures swallowed by the outer Try on the read path include: SqlServer not + * initialised (e.g. on workers in distributed mode), no row for the key, and + * value that can't be parsed. In all of these cases the caller's default takes + * over. The write helpers take an explicit [[DSLContext]] so callers can run + * them inside their own transaction and surface failures. */ object SiteSettings { @@ -40,6 +46,33 @@ object SiteSettings { def getLong(key: String, default: => Long): Long = readAndParse(key, default)(_.toLong) + /** Insert or overwrite the row for `key`, stamping who/when. */ + def upsert(ctx: DSLContext, key: String, value: String, updatedBy: String): Unit = + ctx + .insertInto(SITE_SETTINGS) + .set(SITE_SETTINGS.KEY, key) + .set(SITE_SETTINGS.VALUE, value) + .set(SITE_SETTINGS.UPDATED_BY, updatedBy) + .onConflict(SITE_SETTINGS.KEY) + .doUpdate() + .set(SITE_SETTINGS.VALUE, value) + .set(SITE_SETTINGS.UPDATED_BY, updatedBy) + .set(SITE_SETTINGS.UPDATED_AT, DSL.currentTimestamp()) + .execute() + + /** Seed the row for `key` only if it does not already exist (never overwrites + * an admin-edited value). Used by the startup default seeder. + */ + def insertIfAbsent(ctx: DSLContext, key: String, value: String, updatedBy: String): Unit = + ctx + .insertInto(SITE_SETTINGS) + .set(SITE_SETTINGS.KEY, key) + .set(SITE_SETTINGS.VALUE, value) + .set(SITE_SETTINGS.UPDATED_BY, updatedBy) + .set(SITE_SETTINGS.UPDATED_AT, DSL.currentTimestamp()) + .onDuplicateKeyIgnore() + .execute() + private[dao] def parseOrDefault[T](raw: Option[String], default: T)(parse: String => T): T = raw.flatMap(s => Try(parse(s.trim)).toOption).getOrElse(default) @@ -48,9 +81,9 @@ object SiteSettings { val raw = SqlServer .getInstance() .createDSLContext() - .select(DSL.field("value", classOf[String])) - .from(DSL.table(DSL.name("texera_db", "site_settings"))) - .where(DSL.field("key", classOf[String]).eq(key)) + .select(SITE_SETTINGS.VALUE) + .from(SITE_SETTINGS) + .where(SITE_SETTINGS.KEY.eq(key)) .fetchOneInto(classOf[String]) parseOrDefault(Option(raw), default)(parse) }.getOrElse(default) diff --git a/config-service/src/main/scala/org/apache/texera/service/ConfigService.scala b/config-service/src/main/scala/org/apache/texera/service/ConfigService.scala index b45c6ce62b..a7e9b61d99 100644 --- a/config-service/src/main/scala/org/apache/texera/service/ConfigService.scala +++ b/config-service/src/main/scala/org/apache/texera/service/ConfigService.scala @@ -26,10 +26,10 @@ import io.dropwizard.core.Application import io.dropwizard.core.setup.{Bootstrap, Environment} import org.apache.texera.auth.{AuthFeatures, RequestLoggingFilter, RoleAnnotationEnforcer} import org.apache.texera.common.config.{DefaultsConfig, StorageConfig} -import org.apache.texera.dao.SqlServer +import org.apache.texera.dao.{SiteSettings, SqlServer} +import org.apache.texera.dao.jooq.generated.Tables.SITE_SETTINGS import org.apache.texera.service.resource.{ConfigResource, HealthCheckResource} import org.eclipse.jetty.server.session.SessionHandler -import org.jooq.impl.DSL import java.nio.file.Path @@ -73,22 +73,12 @@ class ConfigService extends Application[ConfigServiceConfiguration] with LazyLog SqlServer.withTransaction(ctx) { tx => if (DefaultsConfig.reinit) { - tx.deleteFrom(DSL.table("site_settings")).execute() + tx.deleteFrom(SITE_SETTINGS).execute() } DefaultsConfig.allDefaults.foreach { case (key, value) => - tx - .insertInto(DSL.table("site_settings")) - .columns( - DSL.field("key"), - DSL.field("value"), - DSL.field("updated_by"), - DSL.field("updated_at") - ) - .values(key, value, "texera", DSL.currentTimestamp()) - .onDuplicateKeyIgnore() - .execute() + SiteSettings.insertIfAbsent(tx, key, value, "texera") } } } catch { diff --git a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala index 72f346349a..440d7ca7e0 100644 --- a/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala +++ b/config-service/src/main/scala/org/apache/texera/service/resource/ConfigResource.scala @@ -19,21 +19,40 @@ package org.apache.texera.service.resource +import com.fasterxml.jackson.annotation.JsonProperty +import io.dropwizard.auth.Auth import jakarta.annotation.security.{PermitAll, RolesAllowed} -import jakarta.ws.rs.core.MediaType -import jakarta.ws.rs.{GET, Path, Produces} +import jakarta.ws.rs.core.{MediaType, Response} +import jakarta.ws.rs.{Consumes, GET, POST, PUT, Path, PathParam, Produces} +import org.apache.texera.auth.SessionUser import org.apache.texera.common.config.{ ApplicationConfig, AuthConfig, ComputingUnitConfig, + DefaultsConfig, GuiConfig, UserSystemConfig } +import org.apache.texera.dao.{SiteSettings, SqlServer} +import org.apache.texera.dao.jooq.generated.Tables.SITE_SETTINGS +import org.jooq.Condition +import org.jooq.impl.DSL + +import scala.jdk.CollectionConverters._ + +// Wire DTO for /config/settings: the JSON contract is exactly {key, value}; +// the generated jOOQ pojo would also expose updated_by/updated_at. +case class ConfigSettingPojo( + @JsonProperty("key") settingKey: String, + @JsonProperty("value") settingValue: String +) @Path("/config") @Produces(Array(MediaType.APPLICATION_JSON)) class ConfigResource { + private def ctx = SqlServer.getInstance().createDSLContext() + // Anonymous endpoint loaded by the frontend's APP_INITIALIZER before any user has // logged in. Only fields that the login page (or the logged-out branches of the // dashboard shell) actually need belong here — anything else lives on /gui or @@ -99,4 +118,113 @@ class ConfigResource { // flags from the user-system.conf "inviteOnly" -> UserSystemConfig.inviteOnly ) + + // The site_settings keys that non-admin pages consume: dashboard branding, + // sidebar tab toggles, and dataset upload limits — exactly the gui.* and + // dataset.* sections of default.conf, which is also where the seeding + // pipeline gets them. Keys declared outside those sections (e.g. + // csv_parser_max_columns) are management-only. Deriving the set from the + // file keeps "which section does this default live in" the single place + // where visibility is decided. + private val publicSettingKeys: Set[String] = + DefaultsConfig.keysUnderSections(Set("gui", "dataset")) + + // SECURITY: every key returned here is served anonymously (see + // /settings/public below), so `publicSettingKeys` is the anonymous-exposure + // surface. It is derived from the gui/dataset sections of default.conf and + // pinned by ConfigResourceSpec/DefaultsConfigSpec — adding a key under those + // sections (or moving one in) changes what unauthenticated callers can read + // and MUST be reviewed there. Never place a secret under gui/dataset. + + private def fetchSettings(condition: Condition): Map[String, String] = + ctx + .select(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE) + .from(SITE_SETTINGS) + .where(condition) + .fetchMap(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE) + .asScala + .toMap + + // Read side for the public keys in one payload, so the dashboard doesn't + // fire a request per key. Anonymous by design: these values render on the + // logged-out shell (custom logo/favicon, Hub/About sidebar entries), so + // gating them behind a login would blank the public landing pages. + @GET + @PermitAll + @Path("/settings/public") + def getPublicSettings: Map[String, String] = + fetchSettings(SITE_SETTINGS.KEY.in(publicSettingKeys.asJava)) + + // Management read over the site_settings table this service seeds at + // startup: every row, including the ones not exposed through + // /settings/public, in one payload for the admin settings page. + @GET + @RolesAllowed(Array("ADMIN")) + @Path("/settings") + def getAllSettings: Map[String, String] = + fetchSettings(DSL.noCondition()) + + // Single-key management read, kept for API completeness alongside the bulk + // read above. + @GET + @RolesAllowed(Array("ADMIN")) + @Path("/settings/{key}") + def getSetting(@PathParam("key") keyParam: String): ConfigSettingPojo = { + ctx + .select(SITE_SETTINGS.KEY, SITE_SETTINGS.VALUE) + .from(SITE_SETTINGS) + .where(SITE_SETTINGS.KEY.eq(keyParam)) + .fetchOneInto(classOf[ConfigSettingPojo]) + } + + @PUT + @RolesAllowed(Array("ADMIN")) + @Path("/settings/{key}") + @Consumes(Array(MediaType.APPLICATION_JSON)) + def updateSetting( + @Auth currentUser: SessionUser, + @PathParam("key") keyParam: String, + setting: ConfigSettingPojo + ): Response = { + if (setting == null || setting.settingValue == null) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity("Setting value must not be null") + .build() + } + // Only keys backed by a default.conf entry are writable, mirroring + // resetSetting. This keeps site_settings within the known-default + // namespace: an arbitrary key would be un-resettable and would pollute + // getAllSettings forever. + if (!DefaultsConfig.allDefaults.contains(keyParam)) { + return Response + .status(Response.Status.BAD_REQUEST) + .entity(s"Unknown setting key '$keyParam'") + .build() + } + SiteSettings.upsert(ctx, keyParam, setting.settingValue, currentUser.getName) + Response.ok().build() + } + + /** + * Resets the specified configuration key to its default value defined in default.conf. + */ + @POST + @RolesAllowed(Array("ADMIN")) + @Path("/settings/reset/{key}") + def resetSetting( + @Auth currentUser: SessionUser, + @PathParam("key") keyParam: String + ): Response = { + DefaultsConfig.allDefaults.get(keyParam) match { + case Some(defaultValue) => + SiteSettings.upsert(ctx, keyParam, defaultValue, currentUser.getName) + Response.ok().build() + case None => + Response + .status(Response.Status.NOT_FOUND) + .entity(s"No default for key '$keyParam'") + .build() + } + } } diff --git a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceAuthSpec.scala b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceAuthSpec.scala deleted file mode 100644 index e395f5f89d..0000000000 --- a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceAuthSpec.scala +++ /dev/null @@ -1,230 +0,0 @@ -/* - * 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.service.resource - -import com.fasterxml.jackson.databind.ObjectMapper -import com.fasterxml.jackson.module.scala.DefaultScalaModule -import io.dropwizard.jackson.Jackson -import io.dropwizard.testing.junit5.ResourceExtension -import jakarta.annotation.security.RolesAllowed -import jakarta.ws.rs.core.MediaType -import jakarta.ws.rs.{GET, Path, Produces} -import org.apache.texera.auth.{JwtAuth, JwtAuthFilter, UnauthorizedExceptionMapper} -import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum -import org.apache.texera.dao.jooq.generated.tables.pojos.User -import org.glassfish.jersey.server.filter.RolesAllowedDynamicFeature -import org.scalatest.BeforeAndAfterAll -import org.scalatest.flatspec.AnyFlatSpec -import org.scalatest.matchers.should.Matchers - -// Wires ConfigResource through the same Jersey auth pipeline production uses -// (JwtAuthFilter + RolesAllowedDynamicFeature) and fires HTTP requests with and -// without an Authorization header. /config/pre-login is the only @PermitAll -// endpoint and must answer unauthenticated callers (bootstrap regression guard, -// same shape as the break that caused PR #5049 to be reverted in #5173). -// /config/gui and /config/user-system are @RolesAllowed; they must reject -// anonymous traffic with a 401 (now from JwtAuthFilter's eager check, not -// from a downstream RolesAllowedRequestFilter 403) and accept callers with a -// valid Bearer token. -class ConfigResourceAuthSpec extends AnyFlatSpec with Matchers with BeforeAndAfterAll { - - // Mirror production's mapper: ConfigService bootstraps Dropwizard's default mapper - // (Jackson.newObjectMapper) and registers DefaultScalaModule on top. Same call here. - private val testMapper: ObjectMapper = - Jackson.newObjectMapper().registerModule(DefaultScalaModule) - - private val resources: ResourceExtension = ResourceExtension - .builder() - .setMapper(testMapper) - .addProvider(classOf[JwtAuthFilter]) - .addProvider(classOf[UnauthorizedExceptionMapper]) - .addProvider(classOf[RolesAllowedDynamicFeature]) - .addResource(new ConfigResource) - .addResource(new ConfigResourceAuthSpec.ProtectedProbe) - .build() - - override protected def beforeAll(): Unit = resources.before() - override protected def afterAll(): Unit = resources.after() - - private def regularToken(): String = { - val u = new User() - u.setUid(2) - u.setName("test-regular") - u.setEmail("[email protected]") - u.setGoogleId(null) - u.setRole(UserRoleEnum.REGULAR) - JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) - } - - private def adminToken(): String = { - val u = new User() - u.setUid(1) - u.setName("test-admin") - u.setEmail("[email protected]") - u.setGoogleId(null) - u.setRole(UserRoleEnum.ADMIN) - JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) - } - - "GET /config/pre-login" should "return 200 without an Authorization header" in { - val response = resources.target("/config/pre-login").request(MediaType.APPLICATION_JSON).get() - response.getStatus shouldBe 200 - } - - it should "expose exactly the fields the login UI needs and nothing else" in { - // Locking down the payload keeps anonymous callers from reading workspace flags, - // feature toggles, or session timers. If a new field is needed before login, it - // must be added here explicitly; the assertion forces that decision into review. - val payload = resources - .target("/config/pre-login") - .request(MediaType.APPLICATION_JSON) - .get(classOf[Map[String, Any]]) - payload.keySet shouldBe Set( - "localLogin", - "googleLogin", - "defaultLocalUser", - "attributionEnabled", - "deploymentVersionCheckEnabled", - "inviteOnly" - ) - } - - "GET /config/gui" should "return 401 with a Bearer challenge without an Authorization header" in { - val response = resources.target("/config/gui").request(MediaType.APPLICATION_JSON).get() - response.getStatus shouldBe 401 - response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge - } - - it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { - val response = resources - .target("/config/gui") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get() - response.getStatus shouldBe 200 - } - - it should "not leak any pre-login field through the authenticated payload" in { - // The split is only meaningful if /gui drops the fields that /pre-login owns. - // Without this, a future refactor could re-add them under the @RolesAllowed - // endpoint, doubling the surface and creating two sources of truth. - val payload = resources - .target("/config/gui") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get(classOf[Map[String, Any]]) - payload.keySet should contain noneOf ( - "localLogin", - "googleLogin", - "defaultLocalUser", - "attributionEnabled" - ) - } - - "GET /config/user-system" should "return 401 with a Bearer challenge without an Authorization header" in { - val response = - resources.target("/config/user-system").request(MediaType.APPLICATION_JSON).get() - response.getStatus shouldBe 401 - response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge - } - - it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { - val response = resources - .target("/config/user-system") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get() - response.getStatus shouldBe 200 - } - - "GET /config/amber" should "return 401 with a Bearer challenge without an Authorization header" in { - val response = - resources.target("/config/amber").request(MediaType.APPLICATION_JSON).get() - response.getStatus shouldBe 401 - response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge - } - - it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { - val response = resources - .target("/config/amber") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get() - response.getStatus shouldBe 200 - } - - it should "expose the engine config separated from the gui payload" in { - // The endpoint exists to keep engine configs out of /config/gui (see PR #5545). - // Pin that defaultDataTransferBatchSize is served here and not folded back into gui. - val amberPayload = resources - .target("/config/amber") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get(classOf[Map[String, Any]]) - amberPayload.keySet should contain("defaultDataTransferBatchSize") - - val guiPayload = resources - .target("/config/gui") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${regularToken()}") - .get(classOf[Map[String, Any]]) - guiPayload.keySet should not contain "defaultDataTransferBatchSize" - } - - "GET an @RolesAllowed probe endpoint" should "return 401 without an Authorization header" in { - // Sanity: JwtAuthFilter is now eager — missing Authorization is rejected - // by the filter itself with a 401 + Bearer challenge, before - // RolesAllowedDynamicFeature ever sees the request. Pre-eager behavior - // here was a 403 from the role filter; the test pins the new contract. - val response = - resources.target("/auth-probe").request(MediaType.APPLICATION_JSON).get() - response.getStatus shouldBe 401 - response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge - } - - it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { - // Positive-direction sibling to the previous test. Without this, a filter- - // priority bug that lets RolesAllowedRequestFilter run *before* JwtAuthFilter - // is invisible to the spec: the no-auth case still 403s, and the only path - // that actually exercises auth → authz ordering is "valid JWT → 200". Manual - // integration testing of PR #5199 found this: a real admin JWT was getting - // 403 on every @RolesAllowed endpoint until JwtAuthFilter was pinned to - // Priorities.AUTHENTICATION. - val response = resources - .target("/auth-probe") - .request(MediaType.APPLICATION_JSON) - .header("Authorization", s"Bearer ${adminToken()}") - .get() - response.getStatus shouldBe 200 - } -} - -object ConfigResourceAuthSpec { - // A deliberately @RolesAllowed companion to ConfigResource, so the same setup also - // proves the feature actually rejects when it should — a 200 on the @PermitAll - // endpoint would otherwise be consistent with the feature being silently no-op'd. - @Path("/auth-probe") - @Produces(Array(MediaType.APPLICATION_JSON)) - class ProtectedProbe { - @GET - @RolesAllowed(Array("REGULAR", "ADMIN")) - def probe: String = "should never reach this" - } -} diff --git a/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala new file mode 100644 index 0000000000..f27f9463d8 --- /dev/null +++ b/config-service/src/test/scala/org/apache/texera/service/resource/ConfigResourceSpec.scala @@ -0,0 +1,481 @@ +/* + * 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.service.resource + +import com.fasterxml.jackson.databind.ObjectMapper +import com.fasterxml.jackson.module.scala.DefaultScalaModule +import io.dropwizard.auth.AuthValueFactoryProvider +import io.dropwizard.jackson.Jackson +import io.dropwizard.testing.junit5.ResourceExtension +import jakarta.annotation.security.RolesAllowed +import jakarta.ws.rs.client.Entity +import jakarta.ws.rs.core.MediaType +import jakarta.ws.rs.{GET, Path, Produces} +import org.apache.texera.auth.{JwtAuth, JwtAuthFilter, SessionUser, UnauthorizedExceptionMapper} +import org.apache.texera.common.config.DefaultsConfig +import org.apache.texera.dao.MockTexeraDB +import org.apache.texera.dao.jooq.generated.Tables.SITE_SETTINGS +import org.apache.texera.dao.jooq.generated.enums.UserRoleEnum +import org.apache.texera.dao.jooq.generated.tables.pojos.User +import org.glassfish.jersey.server.filter.RolesAllowedDynamicFeature +import org.scalatest.BeforeAndAfterAll +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +// Covers ConfigResource from two directions against one embedded database. +// +// HTTP auth gates: wires the resource through the same Jersey auth pipeline +// production uses (JwtAuthFilter + RolesAllowedDynamicFeature) and fires +// requests with and without an Authorization header. /config/pre-login and +// /config/settings/public are the @PermitAll endpoints and must answer +// unauthenticated callers (bootstrap regression guard, same shape as the break +// that caused PR #5049 to be reverted in #5173); the remaining endpoints are +// @RolesAllowed and must reject anonymous traffic with a 401 from +// JwtAuthFilter's eager check. +// +// Endpoint bodies: calls the resource methods directly for the positive +// read/write paths — read-miss, insert, upsert-on-conflict, null-value 400, +// public-whitelist filtering, reset-to-default. +class ConfigResourceSpec + extends AnyFlatSpec + with Matchers + with BeforeAndAfterAll + with MockTexeraDB { + + // Mirror production's mapper: ConfigService bootstraps Dropwizard's default mapper + // (Jackson.newObjectMapper) and registers DefaultScalaModule on top. Same call here. + private val testMapper: ObjectMapper = + Jackson.newObjectMapper().registerModule(DefaultScalaModule) + + private val resources: ResourceExtension = ResourceExtension + .builder() + .setMapper(testMapper) + .addProvider(classOf[JwtAuthFilter]) + .addProvider(classOf[UnauthorizedExceptionMapper]) + .addProvider(classOf[RolesAllowedDynamicFeature]) + // Production (AuthFeatures.register) binds this so @Auth SessionUser + // parameters resolve; without it the /config/settings write endpoints + // fail resource-model validation at startup. + .addProvider(new AuthValueFactoryProvider.Binder(classOf[SessionUser])) + .addResource(new ConfigResource) + .addResource(new ConfigResourceSpec.ProtectedProbe) + .build() + + // Direct-call handle for the endpoint-body tests below. + private val resource = new ConfigResource + + override protected def beforeAll(): Unit = { + initializeDBAndReplaceDSLContext() + resources.before() + } + + override protected def afterAll(): Unit = { + resources.after() + shutdownDB() + } + + private def adminSession(name: String = "test-admin"): SessionUser = { + val u = new User() + u.setUid(1) + u.setName(name) + new SessionUser(u) + } + + private def regularToken(): String = { + val u = new User() + u.setUid(2) + u.setName("test-regular") + u.setEmail("[email protected]") + u.setGoogleId(null) + u.setRole(UserRoleEnum.REGULAR) + JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) + } + + private def adminToken(): String = { + val u = new User() + u.setUid(1) + u.setName("test-admin") + u.setEmail("[email protected]") + u.setGoogleId(null) + u.setRole(UserRoleEnum.ADMIN) + JwtAuth.jwtToken(JwtAuth.jwtClaims(u, expireInDays = 1)) + } + + "GET /config/pre-login" should "return 200 without an Authorization header" in { + val response = resources.target("/config/pre-login").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 200 + } + + it should "expose exactly the fields the login UI needs and nothing else" in { + // Locking down the payload keeps anonymous callers from reading workspace flags, + // feature toggles, or session timers. If a new field is needed before login, it + // must be added here explicitly; the assertion forces that decision into review. + val payload = resources + .target("/config/pre-login") + .request(MediaType.APPLICATION_JSON) + .get(classOf[Map[String, Any]]) + payload.keySet shouldBe Set( + "localLogin", + "googleLogin", + "defaultLocalUser", + "attributionEnabled", + "deploymentVersionCheckEnabled", + "inviteOnly" + ) + } + + "GET /config/gui" should "return 401 with a Bearer challenge without an Authorization header" in { + val response = resources.target("/config/gui").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { + val response = resources + .target("/config/gui") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get() + response.getStatus shouldBe 200 + } + + it should "not leak any pre-login field through the authenticated payload" in { + // The split is only meaningful if /gui drops the fields that /pre-login owns. + // Without this, a future refactor could re-add them under the @RolesAllowed + // endpoint, doubling the surface and creating two sources of truth. + val payload = resources + .target("/config/gui") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get(classOf[Map[String, Any]]) + payload.keySet should contain noneOf ( + "localLogin", + "googleLogin", + "defaultLocalUser", + "attributionEnabled" + ) + } + + "GET /config/user-system" should "return 401 with a Bearer challenge without an Authorization header" in { + val response = + resources.target("/config/user-system").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { + val response = resources + .target("/config/user-system") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get() + response.getStatus shouldBe 200 + } + + "GET /config/amber" should "return 401 with a Bearer challenge without an Authorization header" in { + val response = + resources.target("/config/amber").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { + val response = resources + .target("/config/amber") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get() + response.getStatus shouldBe 200 + } + + it should "expose the engine config separated from the gui payload" in { + // The endpoint exists to keep engine configs out of /config/gui (see PR #5545). + // Pin that defaultDataTransferBatchSize is served here and not folded back into gui. + val amberPayload = resources + .target("/config/amber") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get(classOf[Map[String, Any]]) + amberPayload.keySet should contain("defaultDataTransferBatchSize") + + val guiPayload = resources + .target("/config/gui") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get(classOf[Map[String, Any]]) + guiPayload.keySet should not contain "defaultDataTransferBatchSize" + } + + // /config/settings is the site_settings API: /settings/public serves the + // user-visible keys (gui/dataset sections of default.conf) to anonymous + // callers — the values render on the logged-out shell (custom logo, Hub/About + // sidebar entries) — while the bulk read, single-key read, and all mutation + // are ADMIN-only. These tests pin the auth gates; the endpoint bodies are + // covered by the direct-call tests further down. + "GET /config/settings/public" should "return 200 without an Authorization header (anonymous branding read)" in { + val response = + resources.target("/config/settings/public").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 200 + } + + "GET /config/settings" should "return 401 with a Bearer challenge without an Authorization header" in { + val response = + resources.target("/config/settings").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 403 for a REGULAR user (bulk management read is ADMIN-only)" in { + val response = resources + .target("/config/settings") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get() + response.getStatus shouldBe 403 + } + + "GET /config/settings/{key}" should "return 401 with a Bearer challenge without an Authorization header" in { + val response = + resources.target("/config/settings/logo").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 403 for a REGULAR user (management read is ADMIN-only)" in { + val response = resources + .target("/config/settings/logo") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .get() + response.getStatus shouldBe 403 + } + + "PUT /config/settings/{key}" should "return 401 without an Authorization header" in { + val response = resources + .target("/config/settings/logo") + .request(MediaType.APPLICATION_JSON) + .put(Entity.json("""{"key":"logo","value":"x"}""")) + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 403 for a REGULAR user" in { + val response = resources + .target("/config/settings/logo") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .put(Entity.json("""{"key":"logo","value":"x"}""")) + response.getStatus shouldBe 403 + } + + "POST /config/settings/reset/{key}" should "return 403 for a REGULAR user" in { + val response = resources + .target("/config/settings/reset/logo") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${regularToken()}") + .post(Entity.json("{}")) + response.getStatus shouldBe 403 + } + + it should "pass the role gate for an ADMIN (404 for a key with no default)" in { + val response = resources + .target("/config/settings/reset/no-such-key") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${adminToken()}") + .post(Entity.json("{}")) + response.getStatus shouldBe 404 + } + + "GET an @RolesAllowed probe endpoint" should "return 401 without an Authorization header" in { + // Sanity: JwtAuthFilter is now eager — missing Authorization is rejected + // by the filter itself with a 401 + Bearer challenge, before + // RolesAllowedDynamicFeature ever sees the request. Pre-eager behavior + // here was a 403 from the role filter; the test pins the new contract. + val response = + resources.target("/auth-probe").request(MediaType.APPLICATION_JSON).get() + response.getStatus shouldBe 401 + response.getHeaderString("WWW-Authenticate") shouldBe JwtAuthFilter.BearerChallenge + } + + it should "return 200 with a valid Bearer token whose role matches @RolesAllowed" in { + // Positive-direction sibling to the previous test. Without this, a filter- + // priority bug that lets RolesAllowedRequestFilter run *before* JwtAuthFilter + // is invisible to the spec: the no-auth case still 403s, and the only path + // that actually exercises auth → authz ordering is "valid JWT → 200". Manual + // integration testing of PR #5199 found this: a real admin JWT was getting + // 403 on every @RolesAllowed endpoint until JwtAuthFilter was pinned to + // Priorities.AUTHENTICATION. + val response = resources + .target("/auth-probe") + .request(MediaType.APPLICATION_JSON) + .header("Authorization", s"Bearer ${adminToken()}") + .get() + response.getStatus shouldBe 200 + } + + // ----- endpoint bodies, called directly against the embedded database ----- + + "getSetting" should "return null for a key that has no row" in { + resource.getSetting("no-such-key") shouldBe null + } + + "updateSetting" should "insert a new row and record who wrote it" in { + val response = + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "custom.png")) + response.getStatus shouldBe 200 + + val stored = resource.getSetting("logo") + stored.settingKey shouldBe "logo" + stored.settingValue shouldBe "custom.png" + + getDSLContext + .select(SITE_SETTINGS.UPDATED_BY) + .from(SITE_SETTINGS) + .where(SITE_SETTINGS.KEY.eq("logo")) + .fetchOne(SITE_SETTINGS.UPDATED_BY) shouldBe "test-admin" + } + + it should "update the existing row on a repeated PUT (upsert conflict path)" in { + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "v1.png")) + resource.updateSetting( + adminSession("second-admin"), + "logo", + ConfigSettingPojo("logo", "v2.png") + ) + + resource.getSetting("logo").settingValue shouldBe "v2.png" + getDSLContext.fetchCount(SITE_SETTINGS, SITE_SETTINGS.KEY.eq("logo")) shouldBe 1 + getDSLContext + .select(SITE_SETTINGS.UPDATED_BY) + .from(SITE_SETTINGS) + .where(SITE_SETTINGS.KEY.eq("logo")) + .fetchOne(SITE_SETTINGS.UPDATED_BY) shouldBe "second-admin" + } + + it should "reject a null value with 400 and leave the stored row untouched" in { + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "kept.png")) + val response = resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", null)) + response.getStatus shouldBe 400 + resource.getSetting("logo").settingValue shouldBe "kept.png" + } + + it should "reject a null body with 400 rather than a 500 (empty/malformed request)" in { + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "kept.png")) + val response = resource.updateSetting(adminSession(), "logo", null) + response.getStatus shouldBe 400 + resource.getSetting("logo").settingValue shouldBe "kept.png" + } + + it should "reject a key with no default.conf entry with 400 and write nothing" in { + val response = + resource.updateSetting(adminSession(), "no-such-key", ConfigSettingPojo("no-such-key", "x")) + response.getStatus shouldBe 400 + resource.getSetting("no-such-key") shouldBe null + } + + "getPublicSettings" should "serve whitelisted keys and hide management-only ones" in { + resource.updateSetting(adminSession(), "favicon", ConfigSettingPojo("favicon", "fav.ico")) + resource.updateSetting( + adminSession(), + "csv_parser_max_columns", + ConfigSettingPojo("csv_parser_max_columns", "4096") + ) + + val publicSettings = resource.getPublicSettings + publicSettings("favicon") shouldBe "fav.ico" + publicSettings should not contain key("csv_parser_max_columns") + } + + // The public whitelist is derived from the gui/dataset sections of + // default.conf. This pins the derived set, so moving a key between sections + // (or adding one) forces the visibility decision into review here. + it should "expose exactly the gui and dataset section keys of default.conf" in { + DefaultsConfig.keysUnderSections(Set("gui", "dataset")) shouldBe Set( + "logo", + "mini_logo", + "favicon", + "hub_enabled", + "home_enabled", + "workflow_enabled", + "dataset_enabled", + "your_work_enabled", + "projects_enabled", + "workflows_enabled", + "datasets_enabled", + "compute_enabled", + "quota_enabled", + "forum_enabled", + "about_enabled", + "single_file_upload_max_size_mib", + "multipart_upload_chunk_size_mib", + "max_number_of_concurrent_uploading_file", + "max_number_of_concurrent_uploading_file_chunks" + ) + } + + "getAllSettings" should "serve every stored row, including management-only keys" in { + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "all.png")) + resource.updateSetting( + adminSession(), + "csv_parser_max_columns", + ConfigSettingPojo("csv_parser_max_columns", "2048") + ) + + val allSettings = resource.getAllSettings + allSettings("logo") shouldBe "all.png" + allSettings("csv_parser_max_columns") shouldBe "2048" + } + + "resetSetting" should "restore the default.conf value for a known key" in { + resource.updateSetting(adminSession(), "logo", ConfigSettingPojo("logo", "overridden.png")) + + val response = resource.resetSetting(adminSession(), "logo") + response.getStatus shouldBe 200 + resource.getSetting("logo").settingValue shouldBe DefaultsConfig.allDefaults("logo") + } + + it should "return 404 for a key that has no default" in { + resource.resetSetting(adminSession(), "no-such-key").getStatus shouldBe 404 + } + + it should "restore a management-only key (csv_parser_max_columns) that is seeded from default.conf" in { + resource.updateSetting( + adminSession(), + "csv_parser_max_columns", + ConfigSettingPojo("csv_parser_max_columns", "4096") + ) + resource.resetSetting(adminSession(), "csv_parser_max_columns").getStatus shouldBe 200 + resource.getSetting("csv_parser_max_columns").settingValue shouldBe + DefaultsConfig.allDefaults("csv_parser_max_columns") + } +} + +object ConfigResourceSpec { + // A deliberately @RolesAllowed companion to ConfigResource, so the same setup also + // proves the feature actually rejects when it should — a 200 on the @PermitAll + // endpoint would otherwise be consistent with the feature being silently no-op'd. + @Path("/auth-probe") + @Produces(Array(MediaType.APPLICATION_JSON)) + class ProtectedProbe { + @GET + @RolesAllowed(Array("REGULAR", "ADMIN")) + def probe: String = "should never reach this" + } +} diff --git a/frontend/src/app/app.module.ts b/frontend/src/app/app.module.ts index 8f5023b50a..3180ae187f 100644 --- a/frontend/src/app/app.module.ts +++ b/frontend/src/app/app.module.ts @@ -211,7 +211,7 @@ registerLocaleData(en); tokenGetter: AuthService.getAccessToken, skipWhenExpired: true, throwNoTokenError: false, - disallowedRoutes: ["forum/api/users", "api/config/pre-login"], + disallowedRoutes: ["forum/api/users", "api/config/pre-login", "api/config/settings/public"], }, }), BrowserAnimationsModule, diff --git a/frontend/src/app/common/util/format.util.ts b/frontend/src/app/common/util/format.util.ts index 9046db5fa3..8393559aaa 100644 --- a/frontend/src/app/common/util/format.util.ts +++ b/frontend/src/app/common/util/format.util.ts @@ -90,3 +90,13 @@ export const formatCount = (count: number): string => { } return count.toString(); }; + +/** + * Parse an integer setting value, falling back when the raw value is missing or + * unparsable. Unlike `parseInt(raw) || fallback`, a legitimately stored 0 is + * preserved (0 is falsy, so the `||` idiom would silently drop it). + */ +export const parseIntOrDefault = (raw: string | null | undefined, fallback: number): number => { + const parsed = parseInt(raw ?? "", 10); + return Number.isNaN(parsed) ? fallback : parsed; +}; diff --git a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts index 7217fe6f9a..69daa21716 100644 --- a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts +++ b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.spec.ts @@ -19,12 +19,16 @@ import { ComponentFixture, TestBed } from "@angular/core/testing"; import { AdminSettingsComponent } from "./admin-settings.component"; -import { HttpClientTestingModule } from "@angular/common/http/testing"; +import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { NzCardModule } from "ng-zorro-antd/card"; +import { NzMessageService } from "ng-zorro-antd/message"; describe("AdminSettingsComponent", () => { let component: AdminSettingsComponent; let fixture: ComponentFixture<AdminSettingsComponent>; + let httpTestingController: HttpTestingController; + + const SETTINGS_URL = "/api/config/settings"; beforeEach(async () => { await TestBed.configureTestingModule({ @@ -33,6 +37,7 @@ describe("AdminSettingsComponent", () => { }); beforeEach(() => { + httpTestingController = TestBed.inject(HttpTestingController); fixture = TestBed.createComponent(AdminSettingsComponent); component = fixture.componentInstance; fixture.detectChanges(); @@ -49,4 +54,73 @@ describe("AdminSettingsComponent", () => { expect(el.textContent?.trim()).toBe("MiB"); }); }); + + it("loads every form field from one bulk settings request", () => { + const req = httpTestingController.expectOne(SETTINGS_URL); + expect(req.request.method).toBe("GET"); + req.flush({ + logo: "logo.png", + mini_logo: "mini.png", + favicon: "fav.ico", + hub_enabled: "true", + home_enabled: "false", + max_number_of_concurrent_uploading_file: "5", + single_file_upload_max_size_mib: "128", + max_number_of_concurrent_uploading_file_chunks: "7", + multipart_upload_chunk_size_mib: "64", + csv_parser_max_columns: "4096", + }); + + expect(component.logoData).toBe("logo.png"); + expect(component.miniLogoData).toBe("mini.png"); + expect(component.faviconData).toBe("fav.ico"); + expect(component.sidebarTabs.hub_enabled).toBe(true); + expect(component.sidebarTabs.home_enabled).toBe(false); + expect(component.maxConcurrentFiles).toBe(5); + expect(component.maxFileSizeMiB).toBe(128); + expect(component.maxConcurrentChunks).toBe(7); + expect(component.chunkSizeMiB).toBe(64); + expect(component.csvMaxColumns).toBe(4096); + }); + + it("keeps the initializer defaults for missing or unparsable values", () => { + httpTestingController.expectOne(SETTINGS_URL).flush({ + single_file_upload_max_size_mib: "not-a-number", + }); + + expect(component.logoData).toBeNull(); + expect(component.maxFileSizeMiB).toBe(20); + expect(component.maxConcurrentFiles).toBe(3); + expect(component.csvMaxColumns).toBe(512); + }); + + it("surfaces a load failure through the message service", () => { + const message = TestBed.inject(NzMessageService); + const errorSpy = vi.spyOn(message, "error").mockReturnValue({} as ReturnType<NzMessageService["error"]>); + + httpTestingController.expectOne(SETTINGS_URL).flush("boom", { status: 500, statusText: "Server Error" }); + + expect(errorSpy).toHaveBeenCalledWith("Failed to load settings."); + }); + + it("preserves a legitimately stored 0 instead of falling back to the default", () => { + httpTestingController.expectOne(SETTINGS_URL).flush({ + max_number_of_concurrent_uploading_file: "0", + csv_parser_max_columns: "0", + }); + + expect(component.maxConcurrentFiles).toBe(0); + expect(component.csvMaxColumns).toBe(0); + }); + + it("blocks a tab save when the bulk load failed (no destructive all-off write)", () => { + const message = TestBed.inject(NzMessageService); + const errorSpy = vi.spyOn(message, "error").mockReturnValue({} as ReturnType<NzMessageService["error"]>); + httpTestingController.expectOne(SETTINGS_URL).flush("boom", { status: 500, statusText: "Server Error" }); + + component.saveTabs(); + + httpTestingController.expectNone((req: { method: string }) => req.method === "PUT"); + expect(errorSpy).toHaveBeenCalledWith("Settings have not loaded; refresh before saving."); + }); }); diff --git a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts index 691ae79f6f..3714ac0de8 100644 --- a/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts +++ b/frontend/src/app/dashboard/component/admin/settings/admin-settings.component.ts @@ -23,6 +23,7 @@ import { NzMessageService } from "ng-zorro-antd/message"; import { NotificationService } from "../../../../common/service/notification/notification.service"; import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { SidebarTabs } from "../../../../common/type/gui-config"; +import { parseIntOrDefault } from "../../../../common/util/format.util"; import { forkJoin } from "rxjs"; import { NzCardComponent } from "ng-zorro-antd/card"; import { NzSpaceCompactItemDirective } from "ng-zorro-antd/space"; @@ -93,61 +94,49 @@ export class AdminSettingsComponent implements OnInit { private readonly RELOAD_DELAY = 1000; + // Guards the save buttons: a failed bulk load leaves every field at its + // initializer, so saving would persist those placeholders (e.g. disabling + // every sidebar tab). Only allow saves once a load has actually succeeded. + private settingsLoaded = false; + constructor( private adminSettingsService: AdminSettingsService, private message: NzMessageService, private notificationService: NotificationService ) {} ngOnInit(): void { - this.loadBranding(); - this.loadTabs(); - this.loadDatasetSettings(); - this.loadCsvSettings(); + this.loadSettings(); } - private loadBranding(): void { - this.adminSettingsService - .getSetting("logo") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.logoData = value || null)); - - this.adminSettingsService - .getSetting("mini_logo") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.miniLogoData = value || null)); - - this.adminSettingsService - .getSetting("favicon") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.faviconData = value || null)); - } - - private loadTabs(): void { - (Object.keys(this.sidebarTabs) as (keyof SidebarTabs)[]).forEach(tab => { - this.adminSettingsService - .getSetting(tab) - .pipe(untilDestroyed(this)) - .subscribe(value => (this.sidebarTabs[tab] = value === "true")); - }); - } - - private loadDatasetSettings(): void { - this.adminSettingsService - .getSetting("max_number_of_concurrent_uploading_file") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.maxConcurrentFiles = parseInt(value))); - this.adminSettingsService - .getSetting("single_file_upload_max_size_mib") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.maxFileSizeMiB = parseInt(value))); + // One bulk read instead of a request per key; missing or unparsable values + // keep the field initializers above as their defaults. + private loadSettings(): void { this.adminSettingsService - .getSetting("max_number_of_concurrent_uploading_file_chunks") + .getAllSettings() .pipe(untilDestroyed(this)) - .subscribe(value => (this.maxConcurrentChunks = parseInt(value))); - this.adminSettingsService - .getSetting("multipart_upload_chunk_size_mib") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.chunkSizeMiB = parseInt(value))); + .subscribe({ + next: settings => { + this.logoData = settings["logo"] || null; + this.miniLogoData = settings["mini_logo"] || null; + this.faviconData = settings["favicon"] || null; + (Object.keys(this.sidebarTabs) as (keyof SidebarTabs)[]).forEach( + tab => (this.sidebarTabs[tab] = settings[tab] === "true") + ); + this.maxConcurrentFiles = parseIntOrDefault( + settings["max_number_of_concurrent_uploading_file"], + this.maxConcurrentFiles + ); + this.maxFileSizeMiB = parseIntOrDefault(settings["single_file_upload_max_size_mib"], this.maxFileSizeMiB); + this.maxConcurrentChunks = parseIntOrDefault( + settings["max_number_of_concurrent_uploading_file_chunks"], + this.maxConcurrentChunks + ); + this.chunkSizeMiB = parseIntOrDefault(settings["multipart_upload_chunk_size_mib"], this.chunkSizeMiB); + this.csvMaxColumns = parseIntOrDefault(settings["csv_parser_max_columns"], this.csvMaxColumns); + this.settingsLoaded = true; + }, + error: () => this.message.error("Failed to load settings."), + }); } onFileChange(type: "logo" | "mini_logo" | "favicon", event: Event): void { @@ -205,6 +194,10 @@ export class AdminSettingsComponent implements OnInit { } saveTabs(): void { + if (!this.settingsLoaded) { + this.message.error("Settings have not loaded; refresh before saving."); + return; + } const saveRequests = (Object.keys(this.sidebarTabs) as (keyof SidebarTabs)[]).map(tab => this.adminSettingsService.updateSetting(tab, this.sidebarTabs[tab].toString()) ); @@ -242,6 +235,10 @@ export class AdminSettingsComponent implements OnInit { } saveDatasetSettings(): void { + if (!this.settingsLoaded) { + this.message.error("Settings have not loaded; refresh before saving."); + return; + } if ( this.maxFileSizeMiB < 1 || this.maxConcurrentFiles < 1 || @@ -293,14 +290,11 @@ export class AdminSettingsComponent implements OnInit { setTimeout(() => window.location.reload(), this.RELOAD_DELAY); } - private loadCsvSettings(): void { - this.adminSettingsService - .getSetting("csv_parser_max_columns") - .pipe(untilDestroyed(this)) - .subscribe(value => (this.csvMaxColumns = parseInt(value) || 512)); - } - saveCsvSettings(): void { + if (!this.settingsLoaded) { + this.message.error("Settings have not loaded; refresh before saving."); + return; + } const saveRequests = [ this.adminSettingsService.updateSetting("csv_parser_max_columns", this.csvMaxColumns.toString()), ]; @@ -314,7 +308,12 @@ export class AdminSettingsComponent implements OnInit { } resetCsvSettings(): void { - this.adminSettingsService.resetSetting("csv_parser_max_columns").pipe(untilDestroyed(this)).subscribe({}); + this.adminSettingsService + .resetSetting("csv_parser_max_columns") + .pipe(untilDestroyed(this)) + .subscribe({ + error: () => this.notificationService.error("Could not reset result panel settings."), + }); this.notificationService.info("Resetting result panel settings..."); setTimeout(() => window.location.reload(), this.RELOAD_DELAY); diff --git a/frontend/src/app/dashboard/component/dashboard.component.spec.ts b/frontend/src/app/dashboard/component/dashboard.component.spec.ts index 2e7d7c4b1c..19b6a298ad 100644 --- a/frontend/src/app/dashboard/component/dashboard.component.spec.ts +++ b/frontend/src/app/dashboard/component/dashboard.component.spec.ts @@ -130,7 +130,7 @@ describe("DashboardComponent", () => { }; adminSettingsServiceMock = { - getSetting: vi.fn().mockReturnValue(EMPTY), + getPublicSetting: vi.fn().mockReturnValue(EMPTY), }; activatedRouteMock = { @@ -337,4 +337,66 @@ describe("DashboardComponent", () => { expect(quota!.componentInstance.nzMatchRouter).toBe(true); }); }); + + describe("public settings consumption", () => { + it("applies logo, mini logo, and favicon from the public settings", () => { + const favicon = document.createElement("link"); + favicon.setAttribute("rel", "icon"); + document.head.appendChild(favicon); + const values: Record<string, string> = { + logo: "custom-logo.png", + mini_logo: "custom-mini.png", + favicon: "custom-fav.ico", + }; + (adminSettingsServiceMock.getPublicSetting as Mock).mockImplementation((key: string) => of(values[key] ?? null)); + + component.loadLogos(); + + expect(component.logo).toBe("custom-logo.png"); + expect(component.miniLogo).toBe("custom-mini.png"); + expect(favicon.href).toContain("custom-fav.ico"); + favicon.remove(); + }); + + it("keeps the default branding when the settings are missing or the fetch fails", () => { + component.logo = ""; + component.miniLogo = ""; + (adminSettingsServiceMock.getPublicSetting as Mock).mockReturnValue(of(null)); + component.loadLogos(); + expect(component.logo).toBe(""); + expect(component.miniLogo).toBe(""); + + (adminSettingsServiceMock.getPublicSetting as Mock).mockReturnValue(throwError(() => new Error("401"))); + expect(() => component.loadLogos()).not.toThrow(); + expect(component.logo).toBe(""); + }); + + it("toggles sidebar tabs from the public settings and treats missing keys as disabled", () => { + (adminSettingsServiceMock.getPublicSetting as Mock).mockImplementation((key: string) => + of(key === "hub_enabled" ? "true" : key === "about_enabled" ? "false" : null) + ); + + component.loadTabs(); + + expect(component.sidebarTabs.hub_enabled).toBe(true); + expect(component.sidebarTabs.about_enabled).toBe(false); + expect(component.sidebarTabs.forum_enabled).toBe(false); + }); + + it("leaves tabs unchanged when the settings fetch fails", () => { + component.sidebarTabs.hub_enabled = true; + (adminSettingsServiceMock.getPublicSetting as Mock).mockReturnValue(throwError(() => new Error("500"))); + + expect(() => component.loadTabs()).not.toThrow(); + expect(component.sidebarTabs.hub_enabled).toBe(true); + }); + + it("hides every tab until the settings load (no hardcoded default copy)", () => { + // The default mock returns EMPTY, so loadTabs never overrides. The + // frontend keeps no copy of the default.conf tab defaults: every tab + // starts false so an unfetched or failed load shows no tabs, and the + // backend /config/settings/public is the single source of the real values. + expect(Object.values(component.sidebarTabs).every(enabled => enabled === false)).toBe(true); + }); + }); }); diff --git a/frontend/src/app/dashboard/component/dashboard.component.ts b/frontend/src/app/dashboard/component/dashboard.component.ts index 88dc04bb69..598959edd8 100644 --- a/frontend/src/app/dashboard/component/dashboard.component.ts +++ b/frontend/src/app/dashboard/component/dashboard.component.ts @@ -92,6 +92,10 @@ export class DashboardComponent implements OnInit { showLinks: boolean = false; logo: string = ""; miniLogo: string = ""; + // Every tab starts hidden; loadTabs turns on the ones /config/settings/public + // reports as enabled. The frontend keeps no copy of the default.conf gui.tabs + // defaults, so an unfetched or failed load shows no tabs (each *ngIf sees + // false) rather than a guessed set — the backend stays the single source. sidebarTabs: SidebarTabs = { hub_enabled: false, home_enabled: false, @@ -174,36 +178,57 @@ export class DashboardComponent implements OnInit { this.loadTabs(); } + // A missing key or a failed settings fetch keeps the branding/tab defaults; + // the error callbacks stop a single failed shared request from surfacing as + // one unhandled RxJS error per subscription. loadLogos(): void { this.adminSettingsService - .getSetting("logo") + .getPublicSetting("logo") .pipe(untilDestroyed(this)) - .subscribe(dataUri => { - this.logo = dataUri; + .subscribe({ + next: dataUri => { + if (dataUri) { + this.logo = dataUri; + } + }, + error: () => {}, }); this.adminSettingsService - .getSetting("mini_logo") + .getPublicSetting("mini_logo") .pipe(untilDestroyed(this)) - .subscribe(dataUri => { - this.miniLogo = dataUri; + .subscribe({ + next: dataUri => { + if (dataUri) { + this.miniLogo = dataUri; + } + }, + error: () => {}, }); this.adminSettingsService - .getSetting("favicon") + .getPublicSetting("favicon") .pipe(untilDestroyed(this)) - .subscribe(dataUri => { - document.querySelectorAll("link[rel*='icon']").forEach(el => ((el as HTMLLinkElement).href = dataUri)); + .subscribe({ + next: dataUri => { + if (dataUri) { + document.querySelectorAll("link[rel*='icon']").forEach(el => ((el as HTMLLinkElement).href = dataUri)); + } + }, + error: () => {}, }); } loadTabs(): void { (Object.keys(this.sidebarTabs) as (keyof SidebarTabs)[]).forEach(tab => { this.adminSettingsService - .getSetting(tab) + .getPublicSetting(tab) .pipe(untilDestroyed(this)) - .subscribe(value => { - this.sidebarTabs[tab] = value === "true"; + .subscribe({ + next: value => { + this.sidebarTabs[tab] = value === "true"; + }, + error: () => {}, }); }); } diff --git a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.spec.ts b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.spec.ts index 098aea82cb..ecbd203f68 100644 --- a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.spec.ts @@ -72,7 +72,7 @@ describe("FilesUploaderComponent", () => { }), } as unknown as NzModalService; const adminSettingsService = { - getSetting: vi.fn().mockReturnValue(of("20")), + getPublicSetting: vi.fn().mockReturnValue(of("20")), } as unknown as AdminSettingsService; datasetService = { listMultipartUploads: vi.fn().mockReturnValue(of(["failed.csv"])), @@ -90,6 +90,22 @@ describe("FilesUploaderComponent", () => { component.did = 7; }); + it("keeps the default upload size limit when the public setting is missing, and parses it when present", () => { + const build = (value: string | null) => + new FilesUploaderComponent( + { error: vi.fn() } as unknown as NotificationService, + { getPublicSetting: vi.fn().mockReturnValue(of(value)) } as unknown as AdminSettingsService, + datasetService as unknown as DatasetService, + { create: vi.fn() } as unknown as NzModalService + ); + + expect(build(null).singleFileUploadMaxSizeMiB).toBe(20); + expect(build("128").singleFileUploadMaxSizeMiB).toBe(128); + // an unparsable value keeps the default, but a stored 0 is honoured + expect(build("nope").singleFileUploadMaxSizeMiB).toBe(20); + expect(build("0").singleFileUploadMaxSizeMiB).toBe(0); + }); + it("asks to resume failed multipart files and skip completed matching files in one retry batch", async () => { const emitted = new Promise<FileUploadItem[]>(resolve => component.uploadedFiles.subscribe(resolve)); diff --git a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts index 0a2f1c6c79..5673e55878 100644 --- a/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts +++ b/frontend/src/app/dashboard/component/user/files-uploader/files-uploader.component.ts @@ -28,6 +28,7 @@ import { AdminSettingsService } from "../../../service/admin/settings/admin-sett import { UntilDestroy, untilDestroyed } from "@ngneat/until-destroy"; import { DatasetService } from "../../../service/user/dataset/dataset.service"; import { formatSize } from "../../../../common/util/size-formatter.util"; +import { parseIntOrDefault } from "../../../../common/util/format.util"; import { ConflictingFileModalContentComponent, ConflictingFileModalData, @@ -82,10 +83,14 @@ export class FilesUploaderComponent { private datasetService: DatasetService, private modal: NzModalService ) { + // A missing key or failed fetch keeps the initializer default above. this.adminSettingsService - .getSetting("single_file_upload_max_size_mib") + .getPublicSetting("single_file_upload_max_size_mib") .pipe(untilDestroyed(this)) - .subscribe(value => (this.singleFileUploadMaxSizeMiB = parseInt(value))); + .subscribe({ + next: value => (this.singleFileUploadMaxSizeMiB = parseIntOrDefault(value, this.singleFileUploadMaxSizeMiB)), + error: () => {}, + }); } private markForceRestart(item: FileUploadItem): void { diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts index 3e3e027309..f5ebba48ac 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.spec.ts @@ -105,7 +105,7 @@ describe("DatasetDetailComponent upload queue", () => { isLiked: vi.fn(() => of([{ isLiked: false }])), }, }, - { provide: AdminSettingsService, useValue: { getSetting: vi.fn(() => of("3")) } }, + { provide: AdminSettingsService, useValue: { getPublicSetting: vi.fn(() => of("3")) } }, { provide: MarkdownService, useValue: { parse: vi.fn(() => "") } }, ...commonTestProviders, ], @@ -446,7 +446,7 @@ describe("DatasetDetailComponent behavior", () => { postLike: vi.fn(() => of(true)), postUnlike: vi.fn(() => of(true)), }; - adminSettingsServiceStub = { getSetting: vi.fn(() => of("50")) }; + adminSettingsServiceStub = { getPublicSetting: vi.fn(() => of("50")) }; }); describe("ngOnInit", () => { @@ -465,7 +465,7 @@ describe("DatasetDetailComponent behavior", () => { expect(component.likeCount).toBe(7); expect(component.viewCount).toBe(42); expect(hubServiceStub.isLiked).not.toHaveBeenCalled(); - expect(adminSettingsServiceStub.getSetting).not.toHaveBeenCalled(); + expect(adminSettingsServiceStub.getPublicSetting).not.toHaveBeenCalled(); }); it("fetches liked status and upload settings for a logged-in user", () => { @@ -477,7 +477,19 @@ describe("DatasetDetailComponent behavior", () => { expect(hubServiceStub.isLiked).toHaveBeenCalled(); expect(component.isLiked).toBe(true); - expect(adminSettingsServiceStub.getSetting).toHaveBeenCalled(); + expect(adminSettingsServiceStub.getPublicSetting).toHaveBeenCalled(); + }); + + it("keeps the default upload settings when the public settings are missing", () => { + adminSettingsServiceStub.getPublicSetting.mockReturnValue(of(null)); + + createComponent({ did: 5 }); + login(); + fixture.detectChanges(); + + expect(component.chunkSizeMiB).toBe(50); + expect(component.maxConcurrentChunks).toBe(10); + expect(component.maxConcurrentFiles).toBe(3); }); it("makes no hub calls when the route carries no did", () => { diff --git a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts index c3b5c9a80f..bdef5e0168 100644 --- a/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts +++ b/frontend/src/app/dashboard/component/user/user-dataset/user-dataset-explorer/dataset-detail.component.ts @@ -41,7 +41,7 @@ import { NzModalService } from "ng-zorro-antd/modal"; import { AdminSettingsService } from "../../../../service/admin/settings/admin-settings.service"; import { HttpErrorResponse, HttpStatusCode } from "@angular/common/http"; import { Subscription } from "rxjs"; -import { formatCount, formatSpeed, formatTime } from "src/app/common/util/format.util"; +import { formatCount, formatSpeed, formatTime, parseIntOrDefault } from "src/app/common/util/format.util"; import { format } from "date-fns"; import { NgIf, NgClass, NgFor } from "@angular/common"; import { NzCardComponent, NzCardMetaComponent } from "ng-zorro-antd/card"; @@ -503,20 +503,29 @@ export class DatasetDetailComponent implements OnInit { return fileName; } + // A missing key or failed fetch keeps the field defaults; NaN here would + // silently stall the upload queue (`activeUploads < NaN` is always false). private loadUploadSettings(): void { this.adminSettingsService - .getSetting("multipart_upload_chunk_size_mib") + .getPublicSetting("multipart_upload_chunk_size_mib") .pipe(untilDestroyed(this)) - .subscribe(value => (this.chunkSizeMiB = parseInt(value))); + .subscribe({ + next: value => (this.chunkSizeMiB = parseIntOrDefault(value, this.chunkSizeMiB)), + error: () => {}, + }); this.adminSettingsService - .getSetting("max_number_of_concurrent_uploading_file_chunks") + .getPublicSetting("max_number_of_concurrent_uploading_file_chunks") .pipe(untilDestroyed(this)) - .subscribe(value => (this.maxConcurrentChunks = parseInt(value))); + .subscribe({ + next: value => (this.maxConcurrentChunks = parseIntOrDefault(value, this.maxConcurrentChunks)), + error: () => {}, + }); this.adminSettingsService - .getSetting("max_number_of_concurrent_uploading_file") + .getPublicSetting("max_number_of_concurrent_uploading_file") .pipe(untilDestroyed(this)) - .subscribe(value => { - this.maxConcurrentFiles = parseInt(value); + .subscribe({ + next: value => (this.maxConcurrentFiles = parseIntOrDefault(value, this.maxConcurrentFiles)), + error: () => {}, }); } diff --git a/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts b/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts index f42654c400..a2255b10f2 100644 --- a/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts +++ b/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.spec.ts @@ -17,76 +17,104 @@ * under the License. */ +import { TestBed } from "@angular/core/testing"; import { HttpClientTestingModule, HttpTestingController } from "@angular/common/http/testing"; import { AdminSettingsService } from "./admin-settings.service"; -import { TestBed } from "@angular/core/testing"; describe("AdminSettingsService", () => { - let httpMock: HttpTestingController; let service: AdminSettingsService; + let httpTestingController: HttpTestingController; - const BASE_URL = "/api/admin/settings"; - const EXAMPLE_SETTING = "multipart_upload_chunk_size_mib"; - const EXAMPLE_SETTING_VALUE = "4"; + const PUBLIC_URL = "/api/config/settings/public"; beforeEach(() => { TestBed.configureTestingModule({ imports: [HttpClientTestingModule], providers: [AdminSettingsService], }); - httpMock = TestBed.inject(HttpTestingController); + + httpTestingController = TestBed.inject(HttpTestingController); service = TestBed.inject(AdminSettingsService); }); afterEach(() => { - httpMock.verify(); + httpTestingController.verify(); }); - it("an empty getSetting body should map to null", () => { - let result: string | null | undefined; - service.getSetting(EXAMPLE_SETTING).subscribe(res => { - result = res; - }); + it("shares one public-settings request across per-key subscribers", () => { + let logo: string | null | undefined; + let hubEnabled: string | null | undefined; - const req = httpMock.expectOne(`${BASE_URL}/${EXAMPLE_SETTING}`); + service.getPublicSetting("logo").subscribe(value => (logo = value)); + service.getPublicSetting("hub_enabled").subscribe(value => (hubEnabled = value)); - req.flush(null); + const req = httpTestingController.expectOne(PUBLIC_URL); + req.flush({ logo: "custom.png", hub_enabled: "true" }); - expect(req.request.method).toBe("GET"); - expect(result).toBeNull(); + expect(logo).toEqual("custom.png"); + expect(hubEnabled).toEqual("true"); }); - it("a getSetting body should map to its value", () => { - let result: string | undefined; - service.getSetting(EXAMPLE_SETTING).subscribe(res => { - result = res; - }); + it("emits null for a key absent from the public payload", () => { + let value: string | null | undefined; + service.getPublicSetting("no-such-key").subscribe(v => (value = v)); - const req = httpMock.expectOne(`${BASE_URL}/${EXAMPLE_SETTING}`); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "custom.png" }); + + expect(value).toBeNull(); + }); + + it("drops the cached observable on error so the next read retries", () => { + let firstError = false; + service.getPublicSetting("logo").subscribe({ error: () => (firstError = true) }); + httpTestingController.expectOne(PUBLIC_URL).flush("boom", { status: 500, statusText: "Server Error" }); + expect(firstError).toBeTruthy(); + + let logo: string | null | undefined; + service.getPublicSetting("logo").subscribe(value => (logo = value)); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "recovered.png" }); + expect(logo).toEqual("recovered.png"); + }); - req.flush({ key: EXAMPLE_SETTING, value: EXAMPLE_SETTING_VALUE }); - expect(req.request.method).toBe("GET"); - expect(result).toBe(EXAMPLE_SETTING_VALUE); + it("invalidates the public-settings cache after a save", () => { + service.getPublicSetting("logo").subscribe(); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "old.png" }); + + service.updateSetting("logo", "new.png").subscribe(); + const put = httpTestingController.expectOne("/api/config/settings/logo"); + expect(put.request.method).toEqual("PUT"); + expect(put.request.body).toEqual({ value: "new.png" }); + expect(put.request.withCredentials).toBe(true); + put.flush(null); + + let logo: string | null | undefined; + service.getPublicSetting("logo").subscribe(value => (logo = value)); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "new.png" }); + expect(logo).toEqual("new.png"); }); - it("updateSetting issues a PUT request with value and credentials", () => { - service.updateSetting(EXAMPLE_SETTING, EXAMPLE_SETTING_VALUE).subscribe(); + it("invalidates the public-settings cache after a reset", () => { + service.getPublicSetting("logo").subscribe(); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "old.png" }); - const req = httpMock.expectOne(`${BASE_URL}/${EXAMPLE_SETTING}`); + service.resetSetting("logo").subscribe(); + const post = httpTestingController.expectOne("/api/config/settings/reset/logo"); + expect(post.request.method).toEqual("POST"); + expect(post.request.body).toEqual({}); + post.flush(null); - req.flush(null); - expect(req.request.method).toBe("PUT"); - expect(req.request.body).toEqual({ value: EXAMPLE_SETTING_VALUE }); - expect(req.request.withCredentials).toBe(true); + service.getPublicSetting("logo").subscribe(); + httpTestingController.expectOne(PUBLIC_URL).flush({ logo: "default.png" }); }); - it("resetSetting issues a POST request with an empty body", () => { - service.resetSetting(EXAMPLE_SETTING).subscribe(); + it("reads every stored setting through the bulk management endpoint", () => { + let settings: Record<string, string> | undefined; + service.getAllSettings().subscribe(value => (settings = value)); - const req = httpMock.expectOne(`${BASE_URL}/reset/${EXAMPLE_SETTING}`); + const req = httpTestingController.expectOne("/api/config/settings"); + expect(req.request.method).toEqual("GET"); + req.flush({ logo: "a.png", csv_parser_max_columns: "4096" }); - req.flush(null); - expect(req.request.method).toBe("POST"); - expect(req.request.body).toEqual({}); + expect(settings).toEqual({ logo: "a.png", csv_parser_max_columns: "4096" }); }); }); diff --git a/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.ts b/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.ts index 9a9b40d3da..d71777929f 100644 --- a/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.ts +++ b/frontend/src/app/dashboard/service/admin/settings/admin-settings.service.ts @@ -19,8 +19,8 @@ import { Injectable } from "@angular/core"; import { HttpClient } from "@angular/common/http"; -import { Observable } from "rxjs"; -import { map } from "rxjs/operators"; +import { Observable, throwError } from "rxjs"; +import { catchError, map, shareReplay, tap } from "rxjs/operators"; /** * Service for managing site-wide settings (key-value pairs) via REST API. @@ -31,20 +31,53 @@ import { map } from "rxjs/operators"; providedIn: "root", }) export class AdminSettingsService { - private readonly BASE_URL = "/api/admin/settings"; + private readonly BASE_URL = "/api/config/settings"; + + // One request for all user-visible settings, shared by every consumer. + // Invalidated by the write methods below, so a save through this service + // never leaves the cache stale. + private publicSettings$?: Observable<Record<string, string>>; + constructor(private http: HttpClient) {} - getSetting(key: string): Observable<string> { - return this.http - .get<{ key: string; value: string }>(`${this.BASE_URL}/${key}`) - .pipe(map(resp => resp?.value ?? null)); + /** + * Reads one of the user-visible settings (branding, sidebar tabs, upload + * limits) through the aggregated anonymous endpoint. Emits null when the + * key is absent from the payload, so callers must apply their own default. + */ + getPublicSetting(key: string): Observable<string | null> { + if (!this.publicSettings$) { + this.publicSettings$ = this.http.get<Record<string, string>>(`${this.BASE_URL}/public`).pipe( + // shareReplay would otherwise cache a failed fetch and replay the + // error to every consumer forever; drop the cached observable so the + // next getPublicSetting call retries the request. + catchError((err: unknown) => { + this.publicSettings$ = undefined; + return throwError(() => err); + }), + shareReplay(1) + ); + } + return this.publicSettings$.pipe(map(settings => settings[key] ?? null)); + } + + /** + * Reads every stored setting (including management-only keys) in one + * payload. ADMIN-only; used by the admin settings page. + */ + getAllSettings(): Observable<Record<string, string>> { + return this.http.get<Record<string, string>>(this.BASE_URL); } updateSetting(key: string, value: string): Observable<void> { - return this.http.put<void>(`${this.BASE_URL}/${key}`, { value }, { withCredentials: true }); + return this.http + .put<void>(`${this.BASE_URL}/${key}`, { value }, { withCredentials: true }) + .pipe(tap(() => (this.publicSettings$ = undefined))); } resetSetting(key: string): Observable<void> { - return this.http.post<void>(`${this.BASE_URL}/reset/${key}`, {}); + return this.http + .post<void>(`${this.BASE_URL}/reset/${key}`, {}) + .pipe(tap(() => (this.publicSettings$ = undefined))); } }
