Skip to content

Issue/3608 alt 2#3646

Open
chensuihui wants to merge 39 commits into
magda-io:nextfrom
chensuihui:issue/3608-alt-2
Open

Issue/3608 alt 2#3646
chensuihui wants to merge 39 commits into
magda-io:nextfrom
chensuihui:issue/3608-alt-2

Conversation

@chensuihui

Copy link
Copy Markdown
Contributor

What this PR does

Fixes #3608

This PR introduces an alternative authorization-aware semantic search strategy based on access-scope caching, implemented as a conditional optimization path (not a replacement for the existing two-phase approach).

Summary of changes

  • Added a new Registry API endpoint: GET /v0/records/accessibleIds
    • Computes all record IDs accessible under the current (tenant, authDecision) context
    • Stores the result in Redis
    • Returns a Redis cache key instead of raw IDs
  • Introduced Redis-backed cache keying/versioning model:
    • Data key: records:{tenantId}:{authDecisionHash}:{eventId}
    • Index key: records:idx:{tenantId}:{authDecisionHash}
  • Added read-path optimization behavior:
    • Reuse cached key when present
    • Build and store snapshot on cache miss
  • Added/updated tests for accessibleIds behavior and related auth flow integration.

Why this approach

The existing two-phase query model remains correct, but can be inefficient for users with narrow access scopes (frequent fallback / repeated filtering).

This PR shifts authorization handling from post-retrieval filtering to pre-query scoping when beneficial, by reusing cached accessible ID sets for repeated authorization contexts.

Design choices and trade-offs

  • Kept existing two-phase strategy as baseline
    • This PR does not replace current logic globally.
    • The cache-based path is intended as a targeted optimization.
  • Versioned cache with eventId
    • Ensures cache snapshots can be invalidated/refreshed as data changes.
  • Full refresh first (no incremental diff in this PR)
    • Chosen for correctness and implementation simplicity.
    • Incremental streamEventsSince updates are intentionally deferred due to complexity around auth-membership changes.

Operational notes

  • Effectiveness is best for small/medium access scopes and repeated queries under stable auth contexts.
  • For large accessible ID sets, performance can degrade (Redis size + OpenSearch terms filter cost), so this is designed to coexist with fallback behavior.

Checklist

  • There are unit tests to verify my changes are correct or unit tests aren't applicable
  • I've updated CHANGES.md with what I changed.

Copilot AI review requested due to automatic review settings April 19, 2026 15:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements an alternative authorization-aware semantic search path for issue #3608 by introducing Redis-backed access-scope caching and integrating new Registry endpoints into the semantic-search service to scope OpenSearch queries to accessible record IDs.

Changes:

  • Added Registry endpoints for access filtering (POST /records/filterByAccess) and building/caching accessible IDs (GET /records/accessibleIds) with Redis support.
  • Extended semantic-search-api to support a cache-key-based search (/searchAlt) and to enforce record access in search + retrieve flows.
  • Added Redis clients/configuration across Scala/TypeScript services and Helm charts to deploy/connect to shared Redis.

Reviewed changes

Copilot reviewed 33 out of 34 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
scripts/generate-registry-typescript-win.js Adds a Windows-friendly script to generate Registry TS client from swagger.
magda-typescript-common/src/registry/RegistryClient.ts Adds client methods for filterByAccess and accessibleIds cache key retrieval.
magda-typescript-common/src/redis/RedisClient.ts Introduces a minimal Redis client wrapper for Node services.
magda-typescript-common/src/generated/registry/api.ts Updates generated Registry API client with new endpoints.
magda-typescript-common/src/SearchApiClient.ts Adds a Search API client used for phase-2 fallback scoping.
magda-typescript-common/package.json Adds redis dependency for the new TS Redis client.
magda-semantic-search-api/src/test/service/semanticSearchService.spec.ts Expands tests for access filtering, phase-2 fallback, and searchAlt.
magda-semantic-search-api/src/test/service/queryBuilder.spec.ts Adds tests for new query-builder branches and recordId terms filtering.
magda-semantic-search-api/src/test/searchRoute.spec.ts Adds route tests for /searchAlt and header propagation for auth/tenant.
magda-semantic-search-api/src/service/queryBuilder.ts Adds optional recordId terms filter support in query building.
magda-semantic-search-api/src/service/SemanticSearchService.ts Implements two-phase auth filtering and the Redis-cached searchAlt path; enforces access in retrieve.
magda-semantic-search-api/src/model.ts Extends search/retrieve params to include jwt and tenantId.
magda-semantic-search-api/src/index.ts Wires Registry/Search/Redis clients and CLI args into semantic-search-api startup.
magda-semantic-search-api/src/api/createApiRouter.ts Passes X-Magda-Session / X-Magda-Tenant-Id headers into service params; adds /searchAlt.
magda-scala-common/src/main/scala/au/csiro/data61/magda/client/RedisClient.scala Adds a minimal blocking Scala Redis client wrapper (Jedis).
magda-scala-common/src/main/resources/common.conf Adds default Redis configuration section.
magda-scala-common/build.sbt Adds Jedis dependency to scala-common.
magda-registry-api/src/test/scala/au/csiro/data61/magda/registry/RecordsServiceSpec.scala Updates service construction to inject Redis client.
magda-registry-api/src/test/scala/au/csiro/data61/magda/registry/RecordServiceAuthSpec.scala Adds auth/integration tests for filterByAccess and accessibleIds.
magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordsServiceRO.scala Implements the new endpoints and Redis snapshot/index keying model.
magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordsService.scala Plumbs Redis client into RecordsService/RO constructor chain.
magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Api.scala Instantiates Redis client and injects it into routes.
magda-registry-api/build.sbt Adds Jedis dependency for registry-api.
deploy/helm/magda-core/values.yaml Adds global Redis configuration and a chart tag for registry-redis.
deploy/helm/magda-core/Chart.yaml Adds registry-redis as a magda-core dependency.
deploy/helm/internal-charts/semantic-search-api/values.yaml Adds redis connection values for semantic-search-api.
deploy/helm/internal-charts/semantic-search-api/templates/deployment.yaml Passes Redis args/env into semantic-search-api deployment (incl. optional password).
deploy/helm/internal-charts/registry-redis/values.yaml Defines Bitnami Redis dependency configuration for the shared cache.
deploy/helm/internal-charts/registry-redis/templates/service.yaml Adds a stable registry-redis Service name for in-cluster Redis.
deploy/helm/internal-charts/registry-redis/README.md Documents the shared Redis chart usage and configuration.
deploy/helm/internal-charts/registry-redis/Chart.yaml Introduces the registry-redis internal chart and its Bitnami dependency.
deploy/helm/internal-charts/registry-api/values.yaml Adds redis config to registry-api appConfig defaults.
deploy/helm/internal-charts/registry-api/templates/_helpers.tpl Wires optional Redis password secret + global redis host/port into registry-api config/env.
CHANGES.md Adds a v6.0.0 changelog entry for #3608.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +291 to +320
async filterRecordsByAccess(
records: string[],
jwtToken?: string,
tenantId?: number
): Promise<string[] | ServerError> {
const resolvedTenantId = tenantId ?? this.tenantId;
const resolvedJwt = jwtToken ?? this.jwt;

const operation = () => () =>
this.recordsApi.filterByAccess(
records,
resolvedTenantId,
resolvedJwt
);

return <any>retry(
operation(),
this.secondsBetweenRetries,
this.maxRetries,
(e, retriesLeft) =>
console.log(
formatServiceError(
"Failed to filter records by access.",
e,
retriesLeft
)
)
)
.then((result) => ({ records: result.body }))
.catch(toServerError("filterRecordsByAccess"));

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filterRecordsByAccess is declared to return Promise<string[] | ServerError>, but the success path currently resolves to an object ({ records: result.body }). This will break callers that expect a string[] (e.g. SemanticSearchService._toAllowedRecordIds), causing runtime errors. Return result.body directly (and keep the return type consistent).

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chensuihui this is valid

@chensuihui chensuihui May 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image I think this copilot comment is outdated. I have fixed this problem via [Merge branch 'issue/3608-alt-1' into issue/3608-alt-2](commit: e403c30).

Comment on lines +59 to +65
const jwt = req.header("X-Magda-Session");
const tenantId = req.header("X-Magda-Tenant-Id");
const params: SearchParams = {
...(rawParams as SearchParams),
jwt: jwt || undefined,
tenantId: tenantId === undefined ? 0 : Number(tenantId)
};

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenantId is parsed via Number(tenantId) without validating the header value. If the header is present but non-numeric, this becomes NaN and is passed through to downstream clients/services. Consider validating and returning 400 for invalid tenant ids, or falling back to 0 when Number.isNaN(...) (apply the same fix in handleSearchAlt and handleRetrieve).

Copilot uses AI. Check for mistakes.

@chensuihui chensuihui May 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image fixed. commit: e403c30

Comment on lines +206 to +216
private _toAllowedRecordIds(registryResponse: unknown): Set<string> {
if (!Array.isArray(registryResponse)) {
throw new Error(
"Invalid filterByAccess response: expected string[]"
);
}
return new Set(
registryResponse.filter(
(id): id is string => typeof id === "string" && id.length > 0
)
);

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_toAllowedRecordIds throws whenever filterRecordsByAccess returns a non-array (e.g. a ServerError object on network failure). This will turn transient registry issues into 500s for /search and /retrieve. Consider handling error responses explicitly (e.g. treat as deny-all and return [], or log + fall back to the existing two-phase path) rather than throwing.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that transient registry failures should be handled explicitly, but I do not think converting a non-array response into an empty allow-list is the right behaviour here.

The access-filter endpoint uses an empty array to represent “no accessible records”. A non-array response is not a valid authorisation result; it indicates either an upstream registry failure or a broken response contract. Treating that as deny-all would hide infrastructure errors and make debugging difficult, because users would receive an apparently valid empty result instead of an error.

I will keep the validation here, but I can improve the error handling around the registry client call so registry failures are reported more explicitly rather than being mixed with normal authorisation denial.

Comment on lines +152 to +159
// Step 2: Fetch record ids from Redis using the data key
let accessibleRecordIds: string[] = [];
try {
const cachedJson = await this.redisClient.get(dataKey);
if (cachedJson) {
accessibleRecordIds = JSON.parse(cachedJson);
}
} catch (e) {

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

searchAlt parses the Redis value with JSON.parse but doesn't validate that the result is actually a string[]. If Redis contains malformed JSON or a different JSON shape, this will build an invalid OpenSearch terms filter. Validate that the parsed value is an array of non-empty strings (and consider de-duping / bounding the size) before passing it to executeVectorSearch.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the Redis value should not be trusted as a valid string[] just because JSON.parse succeeds.

I have updated the code to add runtime validation for the cached value before using it in the OpenSearch terms filter. The parsed Redis value is now checked to ensure it is an array, and the array contents are filtered so that only non-empty strings are passed through. I also de-duplicate the IDs before calling executeVectorSearch.

If the cached JSON is malformed or the parsed value has an unexpected shape, searchAlt now falls back to the existing search path instead of building an invalid OpenSearch query.

Comment on lines +1323 to +1358
optionalHeaderValueByName("X-Magda-Session") { sessionTenantId =>
withAuthDecision(
authApiClient,
AuthDecisionReqConfig("object/record/read")
) { authDecision =>
completeBlockingTask {
DB readOnly { implicit dbSession =>
dbSession.queryTimeout(this.defaultQueryTimeout)

val latestEventId = getLatestTenantEventId(tenantId)
val authDecisionHash = sha256Hex(authDecision.toJson.compactPrint)
val tenantKeyPart = sessionTenantId
.map(_.trim)
.filter(_.nonEmpty)
.getOrElse(tenantIdToString(tenantId))

val dataKey =
s"records:${tenantKeyPart}:${authDecisionHash}:${latestEventId}"
val indexKey =
s"records:idx:${tenantKeyPart}:${authDecisionHash}"

redisClient.get(dataKey) match {
case Some(_) =>
dataKey
case None =>
val ids = collectAccessibleRecordIds(tenantId, authDecision)
redisClient.set(dataKey, ids.toJson.compactPrint)

// Index key stores all historical data keys for the same tenant+auth hash.
// Future incremental flow can locate the newest cached key, call
// streamEventsSince(latestEventId), patch only changed record IDs,
// and write a new records:{tenant}:{authHash}:{newEventId} snapshot.
redisClient.sAdd(indexKey, Seq(dataKey))

dataKey
}

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessibleIds uses the X-Magda-Session header value as part of tenantKeyPart, which effectively embeds the session/JWT into Redis key names. This is sensitive data exposure and will also create unbounded per-session cache keys. The key should be derived from stable, non-sensitive context only (e.g. tenant id + authDecision hash + event id), so tenantKeyPart should not incorporate X-Magda-Session.

Suggested change
optionalHeaderValueByName("X-Magda-Session") { sessionTenantId =>
withAuthDecision(
authApiClient,
AuthDecisionReqConfig("object/record/read")
) { authDecision =>
completeBlockingTask {
DB readOnly { implicit dbSession =>
dbSession.queryTimeout(this.defaultQueryTimeout)
val latestEventId = getLatestTenantEventId(tenantId)
val authDecisionHash = sha256Hex(authDecision.toJson.compactPrint)
val tenantKeyPart = sessionTenantId
.map(_.trim)
.filter(_.nonEmpty)
.getOrElse(tenantIdToString(tenantId))
val dataKey =
s"records:${tenantKeyPart}:${authDecisionHash}:${latestEventId}"
val indexKey =
s"records:idx:${tenantKeyPart}:${authDecisionHash}"
redisClient.get(dataKey) match {
case Some(_) =>
dataKey
case None =>
val ids = collectAccessibleRecordIds(tenantId, authDecision)
redisClient.set(dataKey, ids.toJson.compactPrint)
// Index key stores all historical data keys for the same tenant+auth hash.
// Future incremental flow can locate the newest cached key, call
// streamEventsSince(latestEventId), patch only changed record IDs,
// and write a new records:{tenant}:{authHash}:{newEventId} snapshot.
redisClient.sAdd(indexKey, Seq(dataKey))
dataKey
}
withAuthDecision(
authApiClient,
AuthDecisionReqConfig("object/record/read")
) { authDecision =>
completeBlockingTask {
DB readOnly { implicit dbSession =>
dbSession.queryTimeout(this.defaultQueryTimeout)
val latestEventId = getLatestTenantEventId(tenantId)
val authDecisionHash = sha256Hex(authDecision.toJson.compactPrint)
val tenantKeyPart = tenantIdToString(tenantId)
val dataKey =
s"records:${tenantKeyPart}:${authDecisionHash}:${latestEventId}"
val indexKey =
s"records:idx:${tenantKeyPart}:${authDecisionHash}"
redisClient.get(dataKey) match {
case Some(_) =>
dataKey
case None =>
val ids = collectAccessibleRecordIds(tenantId, authDecision)
redisClient.set(dataKey, ids.toJson.compactPrint)
// Index key stores all historical data keys for the same tenant+auth hash.
// Future incremental flow can locate the newest cached key, call
// streamEventsSince(latestEventId), patch only changed record IDs,
// and write a new records:{tenant}:{authHash}:{newEventId} snapshot.
redisClient.sAdd(indexKey, Seq(dataKey))
dataKey

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chensuihui yeah this comment is valid - we need to treat JWT token as secret and not to use raw JWT token value as key.
It seems the key (that includes the JWT token) could also be printed in logs in SemanticSearchService.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this. I previously used X-Magda-Session incorrectly as part of the Redis key, but that was not intended.

I have updated the cache key generation so it no longer incorporates the raw session/JWT value. The Redis key is now derived only from stable, non-sensitive context: tenant id, auth decision hash, and latest event id.

X-Magda-Session is still documented because it is required by the existing withAuthDecision flow to derive the current user's auth decision, but I have updated the API documentation to clarify that it is not used as part of the Redis key.

Comment on lines +1344 to +1356
redisClient.get(dataKey) match {
case Some(_) =>
dataKey
case None =>
val ids = collectAccessibleRecordIds(tenantId, authDecision)
redisClient.set(dataKey, ids.toJson.compactPrint)

// Index key stores all historical data keys for the same tenant+auth hash.
// Future incremental flow can locate the newest cached key, call
// streamEventsSince(latestEventId), patch only changed record IDs,
// and write a new records:{tenant}:{authHash}:{newEventId} snapshot.
redisClient.sAdd(indexKey, Seq(dataKey))

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessibleIds writes dataKey snapshots and appends to indexKey but never sets an expiry or performs cleanup. As events advance this will accumulate historical snapshots indefinitely in Redis. Consider setting TTLs on both the snapshot key and the index set (or trimming old snapshot keys) to prevent unbounded Redis growth.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that these cache keys should not live indefinitely.

I have updated the implementation to use a configurable TTL for the accessible ID cache. The snapshot key is now written with setEx, and the index set is also assigned an expiry so historical snapshot references do not accumulate indefinitely in Redis. The TTL is configurable rather than hard-coded, so it can be tuned based on deployment and evaluation needs.

Comment on lines +384 to +386
_source: {
itemType: "storegeObject",
recordId: "record1",

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in test fixture: itemType value is storegeObject, which doesn't match the expected storageObject enum/value used elsewhere. Using the correct string here helps the test better reflect real OpenSearch documents.

Copilot uses AI. Check for mistakes.
# -- Redis database number
db: 0
# -- Redis connection timeout
timeout: 1h

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appConfig.redis.timeout is set to 1h, but in the new Scala RedisClient this value is used as the Jedis socket/connect timeout (passed to new Jedis(host, port, timeoutMs)). A 1-hour timeout can cause requests to hang for a very long time during Redis/network issues. Consider using a much smaller timeout (e.g. seconds) and relying on retries/fallbacks instead.

Suggested change
timeout: 1h
timeout: 5s

Copilot uses AI. Check for mistakes.
Comment on lines +106 to +110
requestConfig.headers = this.setHeader(
requestConfig.headers,
"X-Magda-Session",
jwtToken
);

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SearchApiClient.searchDatasets always sets X-Magda-Session even when jwtToken is undefined. With cross-fetch, an undefined header value can be serialized as the literal string "undefined" (or cause a header validation error), which can lead to unexpected auth behaviour. Only set X-Magda-Session when a non-empty token is provided, otherwise omit the header.

Suggested change
requestConfig.headers = this.setHeader(
requestConfig.headers,
"X-Magda-Session",
jwtToken
);
if (jwtToken) {
requestConfig.headers = this.setHeader(
requestConfig.headers,
"X-Magda-Session",
jwtToken
);
}

Copilot uses AI. Check for mistakes.
Comment on lines +401 to +403
_source: {
itemType: "storegeObject",
recordId: "record2",

Copilot AI Apr 19, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in test fixture: itemType value is storegeObject (should be storageObject). This appears to be a copy of the earlier typo and should be corrected so fixtures match real data.

Copilot uses AI. Check for mistakes.

@t83714 t83714 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess benchmark & experiment design (against baseline) is more important for this PR than making it production-grade. Once the functionality looks good, you probably can add some scripts to help facilitate your benchmarking and experiments.

By the way, besides my comments, many of the copilot auto-review comments are worth looking into as well. Better have a look at all copilot auto-review comments closely.

@@ -1,10 +1,10 @@
global: {}
image:
image:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big issue - just a note: would always be good to minimise unnecessary changes to the files to reduce review workload 😄
If we do want to adjust the format / whitespace largely, could use a seperate PR.

dependencies:
- name: redis
version: "17.11.3"
repository: "https://charts.bitnami.com/bitnami"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitnami has moved to OCI registry for helm charts since 2024 - see: bitnami/charts#30110

kubeVersion: ">= 1.21.0"
annotations:
magdaModuleType: "core"
dependencies:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we adds the external dependency, we should include a Chart.lock file for "registry-redis" chart

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@@ -0,0 +1,14 @@
apiVersion: v2
name: registry-redis

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registry-redis name is a little too registry-specific if it is explicitly shared by semantic search

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image magda-redis as new name

@@ -0,0 +1,21 @@
# This service is used to make Redis accessible via a consistent DNS name
# when deployed as part of the Magda cluster
apiVersion: v1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this manual service?
Bitnami Redis already creates services with chart-specific names/selectors.
Better to use Bitnami's own service directly - if you want to control the name of the Bitnami's own service - I guess the fullnameOverride config or similar should do.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Comment on lines +1323 to +1358
optionalHeaderValueByName("X-Magda-Session") { sessionTenantId =>
withAuthDecision(
authApiClient,
AuthDecisionReqConfig("object/record/read")
) { authDecision =>
completeBlockingTask {
DB readOnly { implicit dbSession =>
dbSession.queryTimeout(this.defaultQueryTimeout)

val latestEventId = getLatestTenantEventId(tenantId)
val authDecisionHash = sha256Hex(authDecision.toJson.compactPrint)
val tenantKeyPart = sessionTenantId
.map(_.trim)
.filter(_.nonEmpty)
.getOrElse(tenantIdToString(tenantId))

val dataKey =
s"records:${tenantKeyPart}:${authDecisionHash}:${latestEventId}"
val indexKey =
s"records:idx:${tenantKeyPart}:${authDecisionHash}"

redisClient.get(dataKey) match {
case Some(_) =>
dataKey
case None =>
val ids = collectAccessibleRecordIds(tenantId, authDecision)
redisClient.set(dataKey, ids.toJson.compactPrint)

// Index key stores all historical data keys for the same tenant+auth hash.
// Future incremental flow can locate the newest cached key, call
// streamEventsSince(latestEventId), patch only changed record IDs,
// and write a new records:{tenant}:{authHash}:{newEventId} snapshot.
redisClient.sAdd(indexKey, Seq(dataKey))

dataKey
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chensuihui yeah this comment is valid - we need to treat JWT token as secret and not to use raw JWT token value as key.
It seems the key (that includes the JWT token) could also be printed in logs in SemanticSearchService.ts

)
);

const redisClient = new RedisClient({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems we don't use "REDIS_PASSWORD" at all when create client?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove REDIS_PASSWORD

Comment on lines +291 to +320
async filterRecordsByAccess(
records: string[],
jwtToken?: string,
tenantId?: number
): Promise<string[] | ServerError> {
const resolvedTenantId = tenantId ?? this.tenantId;
const resolvedJwt = jwtToken ?? this.jwt;

const operation = () => () =>
this.recordsApi.filterByAccess(
records,
resolvedTenantId,
resolvedJwt
);

return <any>retry(
operation(),
this.secondsBetweenRetries,
this.maxRetries,
(e, retriesLeft) =>
console.log(
formatServiceError(
"Failed to filter records by access.",
e,
retriesLeft
)
)
)
.then((result) => ({ records: result.body }))
.catch(toServerError("filterRecordsByAccess"));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chensuihui this is valid

redis:
auth:
# -- Enable Redis authentication
enabled: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need "REDIS_PASSWORD" if we set enabled: false here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove REDIS_PASSWORD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants