From 43de649e7ad505bd7a92dbc7ac6488e1fc2ade7e Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 17:12:04 +0100 Subject: [PATCH 1/7] refactor(scorecard): add example entity Signed-off-by: Ihor Mykhno --- workspaces/scorecard/examples/all-scorecards.yaml | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/workspaces/scorecard/examples/all-scorecards.yaml b/workspaces/scorecard/examples/all-scorecards.yaml index 137e23b3da1..363d007eda5 100644 --- a/workspaces/scorecard/examples/all-scorecards.yaml +++ b/workspaces/scorecard/examples/all-scorecards.yaml @@ -12,3 +12,18 @@ spec: type: service owner: user:development/guest lifecycle: production +--- +# Component with both GitHub and Jira Scorecards with not specified owner +apiVersion: backstage.io/v1alpha1 +kind: Component +metadata: + name: all-scorecards-service-different-owner + annotations: + github.com/project-slug: redhat-developer/rhdh-plugins + backstage.io/source-location: url:https://github.com/redhat-developer/rhdh-plugins + jira/project-key: RSPT + jira/label: JupiterTeam +spec: + type: service + owner: rhdh-team + lifecycle: production From 04eeebde5821d21776519ed0648ffd87fd99c0e3 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 17:28:11 +0100 Subject: [PATCH 2/7] refactor(scorecard): metric and metrics aggregations endpoints Signed-off-by: Ihor Mykhno --- .../plugins/scorecard-backend/README.md | 5 +- .../scorecard-backend/docs/aggregation.md | 56 +++---------- .../src/service/router.test.ts | 82 +++++++++++-------- .../scorecard-backend/src/service/router.ts | 48 ++++------- .../validateCatalogMetricsSchema.test.ts | 32 ++++---- .../validateCatalogMetricsSchema.ts | 6 +- 6 files changed, 103 insertions(+), 126 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index 1291a81e9f4..54cb1300deb 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -106,10 +106,9 @@ The Scorecard plugin provides aggregation endpoints that return metrics for all - Entities directly owned by the user - Entities owned by groups the user is a direct member of (Only direct parent groups are considered) -### Available Endpoints +### Available Endpoint -- **`GET /metrics/catalog/aggregates`**: Returns aggregated metrics for all available metrics (optionally filtered by `metricIds` query parameter) -- **`GET /metrics/:metricId/catalog/aggregation`**: Returns aggregated metrics for a specific metric, with explicit access validation (returns `403` if the user doesn't have access to the metric) +- **`GET /metrics/:metricId/catalog/aggregations`**: Returns aggregated metrics for a specific metric across all entities owned by the authenticated user, with explicit access validation (returns `403` if the user doesn't have access to the metric) For comprehensive documentation on how entity aggregation works, API details, examples, and best practices, see [aggregation.md](./docs/aggregation.md). diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md index bc8da9aea60..a1983fcea50 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md @@ -1,10 +1,10 @@ # Entity Aggregation -The Scorecard plugin provides aggregation endpoints that return metrics aggregated across all entities owned by the authenticated user. This feature allows users to get a consolidated view of metrics across their entire portfolio of owned entities. +The Scorecard plugin provides an aggregation endpoint that returns metrics aggregated across all entities owned by the authenticated user. This feature allows users to get a consolidated view of metrics across their entire portfolio of owned entities. ## Overview -The aggregation endpoints (`/metrics/catalog/aggregates` and `/metrics/:metricId/catalog/aggregation`) aggregate metrics from multiple entities based on entity ownership. They collect metrics from: +The aggregation endpoint (`/metrics/:metricId/catalog/aggregations`) aggregates metrics from multiple entities based on entity ownership. It collects metrics from: - Entities directly owned by the user - Entities owned by groups the user is a direct member of @@ -28,39 +28,9 @@ In this case: - ✅ Entities owned by `group:default/developers` are included - ❌ Entities owned by `group:default/engineering` are **NOT** included -## API Endpoints +## API Endpoint -### `GET /metrics/catalog/aggregates` - -Returns aggregated metrics for all entities owned by the authenticated user. - -#### Query Parameters - -| Parameter | Type | Required | Description | -| ----------- | ------ | -------- | -------------------------------------------------------------------------------------------- | -| `metricIds` | string | No | Comma-separated list of metric IDs to filter. If not provided, returns all available metrics | - -#### Authentication - -Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. - -#### Permissions - -Requires `scorecard.metric.read` permission. Additionally, the user must have `catalog.entity.read` permission for each entity that will be included in the aggregation. - -#### Example Request - -```bash -# Get all aggregated metrics -curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates" \ - -H "Authorization: Bearer " - -# Get specific metrics -curl -X GET "{{url}}/api/scorecard/metrics/catalog/aggregates?metricIds=github.open_prs,jira.open_issues" \ - -H "Authorization: Bearer " -``` - -### `GET /metrics/:metricId/catalog/aggregation` +### `GET /metrics/:metricId/catalog/aggregations` Returns aggregated metrics for a specific metric across all entities owned by the authenticated user. This endpoint is useful when you need to check access to a specific metric and get its aggregation without requiring the `metricIds` query parameter. @@ -85,11 +55,11 @@ Requires `scorecard.metric.read` permission. Additionally: ```bash # Get aggregated metrics for a specific metric -curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregation" \ +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations" \ -H "Authorization: Bearer " ``` -#### Differences from `/metrics/catalog/aggregates` +#### Key Features - **Metric Access Validation**: This endpoint explicitly validates that the user has access to the specified metric and returns `403 Forbidden` if access is denied - **Single Metric Only**: Returns aggregation for only the specified metric (no need for `metricIds` query parameter) @@ -101,8 +71,8 @@ curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregation" If the authenticated user doesn't have an entity reference in the catalog: -- **Status Code**: `403 Forbidden` -- **Error**: `NotAllowedError: User entity reference not found` +- **Status Code**: `404 Not Found` +- **Error**: `NotFoundError: User entity reference not found` ### Permission Denied @@ -111,12 +81,12 @@ If the user doesn't have permission to read a specific entity: - **Status Code**: `403 Forbidden` - **Error**: Permission denied for the specific entity -### Metric Access Denied (for `/metrics/:metricId/catalog/aggregation`) +### Metric Access Denied (for `/metrics/:metricId/catalog/aggregations`) If the user doesn't have access to the specified metric: - **Status Code**: `403 Forbidden` -- **Error**: `NotAllowedError: Access to metric "" denied` +- **Error**: `NotAllowedError: To view the scorecard metrics, your administrator must grant you the required permission.` ### Invalid Query Parameters @@ -127,8 +97,8 @@ If invalid query parameters are provided: ## Best Practices -1. **Use Metric Filtering**: When you only need specific metrics, use the `metricIds` parameter to reduce response size and improve performance +1. **Handle Empty Results**: Always check for empty arrays when the user owns no entities -2. **Handle Empty Results**: Always check for empty arrays when the user owns no entities +2. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. If you need nested group aggregation, consider restructuring your groups or implementing custom logic -3. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. If you need nested group aggregation, consider restructuring your groups or implementing custom logic +3. **Metric Access**: This endpoint validates metric access upfront, so you'll get a clear `403 Forbidden` error if the user doesn't have permission to view the specified metric diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index d2c6b326b5f..8cffd5348ad 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -150,19 +150,8 @@ describe('createRouter', () => { metricProvidersRegistry.register(sonarProvider); }); - it('should return 403 Unauthorized when DENY permissions', async () => { - permissionsMock.authorizeConditional.mockResolvedValue([ - { result: AuthorizeResult.DENY }, - ]); - const response = await request(app).get('/metrics'); - - expect(response.statusCode).toBe(403); - expect(response.body.error.name).toEqual('NotAllowedError'); - }); - - it('should return all metrics', async () => { + it('should return all metrics when no metricIds parameter', async () => { const response = await request(app).get('/metrics'); - console.log('response', response.body); expect(response.status).toBe(200); expect(response.body).toHaveProperty('metrics'); @@ -174,8 +163,23 @@ describe('createRouter', () => { expect(metricIds).toContain('sonar.quality'); }); - it('should return metrics filtered by datasource', async () => { - const response = await request(app).get('/metrics?datasource=github'); + it('should return metrics filtered by metricIds - single metric', async () => { + const response = await request(app).get( + '/metrics?metricIds=github.open_prs', + ); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(1); + + const metricIds = response.body.metrics.map((m: Metric) => m.id); + expect(metricIds).toContain('github.open_prs'); + }); + + it('should return metrics filtered by metricIds - multiple metrics', async () => { + const response = await request(app).get( + '/metrics?metricIds=github.open_prs,github.open_issues', + ); expect(response.status).toBe(200); expect(response.body).toHaveProperty('metrics'); @@ -186,25 +190,39 @@ describe('createRouter', () => { expect(metricIds).toContain('github.open_issues'); }); - it('should filter authorized metrics when CONDITIONAL permission', async () => { - permissionsMock.authorizeConditional.mockResolvedValue([ - CONDITIONAL_POLICY_DECISION, - ]); - const response = await request(app).get('/metrics'); + it('should return metrics filtered by metricIds with whitespace', async () => { + const response = await request(app).get( + '/metrics?metricIds=github.open_prs, github.open_issues', + ); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(2); - expect(response.statusCode).toBe(200); const metricIds = response.body.metrics.map((m: Metric) => m.id); expect(metricIds).toContain('github.open_prs'); expect(metricIds).toContain('github.open_issues'); }); - it('should return 400 InputError when invalid datasource parameter - empty string', async () => { - const response = await request(app).get('/metrics?datasource='); + it('should return 400 InputError when invalid metricIds parameter - empty string', async () => { + const response = await request(app).get('/metrics?metricIds='); expect(response.status).toEqual(400); expect(response.body.error.name).toEqual('InputError'); expect(response.body.error.message).toContain('Invalid query parameters'); }); + + it('should return only existing metrics when metricIds contains non-existent IDs', async () => { + const response = await request(app).get( + '/metrics?metricIds=github.open_prs,non.existent.metric', + ); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(1); + + expect(response.body.metrics[0].id).toBe('github.open_prs'); + }); }); describe('GET /metrics/catalog/:kind/:namespace/:name', () => { @@ -374,7 +392,7 @@ describe('createRouter', () => { }); }); - describe('GET /metrics/:metricId/catalog/aggregation', () => { + describe('GET /metrics/:metricId/catalog/aggregations', () => { const mockAggregatedMetricResults: AggregatedMetricResult[] = [ { id: 'github.open_prs', @@ -467,7 +485,7 @@ describe('createRouter', () => { { result: AuthorizeResult.DENY }, ]); const result = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(result.statusCode).toBe(403); @@ -479,7 +497,7 @@ describe('createRouter', () => { principal: {}, } as any); const result = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(result.statusCode).toBe(404); @@ -494,7 +512,7 @@ describe('createRouter', () => { CONDITIONAL_POLICY_DECISION, ]); const result = await request(aggregationApp).get( - '/metrics/jira.open_issues/catalog/aggregation', + '/metrics/jira.open_issues/catalog/aggregations', ); expect(result.statusCode).toBe(403); @@ -506,7 +524,7 @@ describe('createRouter', () => { it('should return aggregated metrics for a specific metric', async () => { const response = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(response.status).toBe(200); @@ -522,7 +540,7 @@ describe('createRouter', () => { it('should get entities owned by user', async () => { const response = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(response.status).toBe(200); @@ -538,7 +556,7 @@ describe('createRouter', () => { it('should return empty array when user owns no entities', async () => { getEntitiesOwnedByUserSpy.mockResolvedValue([]); const response = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(response.status).toBe(200); @@ -550,7 +568,7 @@ describe('createRouter', () => { it('should check entity access for each entity owned by user', async () => { const response = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(checkEntityAccessSpy).toHaveBeenCalledTimes(2); @@ -575,7 +593,7 @@ describe('createRouter', () => { CONDITIONAL_POLICY_DECISION, ]); const response = await request(aggregationApp).get( - '/metrics/github.open_prs/catalog/aggregation', + '/metrics/github.open_prs/catalog/aggregations', ); expect(response.status).toBe(200); @@ -598,7 +616,7 @@ describe('createRouter', () => { it('should return 404 NotFoundError when metric is not found', async () => { const result = await request(aggregationApp).get( - '/metrics/non.existent.metric/catalog/aggregation', + '/metrics/non.existent.metric/catalog/aggregations', ); expect(result.statusCode).toBe(404); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 819e2a15014..7331a3d0e3c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -13,8 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { InputError, NotAllowedError, NotFoundError } from '@backstage/errors'; -import { z } from 'zod'; +import { NotAllowedError, NotFoundError } from '@backstage/errors'; import express, { Request } from 'express'; import Router from 'express-promise-router'; import type { CatalogMetricService } from './CatalogMetricService'; @@ -93,32 +92,17 @@ export async function createRouter({ }; router.get('/metrics', async (req, res) => { - const { conditions } = await authorizeConditional( - req, - scorecardMetricReadPermission, - ); - - const metricsSchema = z.object({ - datasource: z.string().min(1).optional(), - }); - - const parsed = metricsSchema.safeParse(req.query); - if (!parsed.success) { - throw new InputError(`Invalid query parameters: ${parsed.error.message}`); - } - - const { datasource } = parsed.data; - - let metrics; - if (datasource) { - metrics = metricProvidersRegistry.listMetricsByDatasource(datasource); - } else { - metrics = metricProvidersRegistry.listMetrics(); + const { metricIds } = validateCatalogMetricsSchema(req.query); + + if (metricIds) { + return res.json({ + metrics: metricProvidersRegistry.listMetrics( + parseCommaSeparatedString(metricIds), + ), + }); } - res.json({ - metrics: filterAuthorizedMetrics(metrics, conditions), - }); + return res.json({ metrics: metricProvidersRegistry.listMetrics() }); }); router.get('/metrics/catalog/:kind/:namespace/:name', async (req, res) => { @@ -128,19 +112,17 @@ export async function createRouter({ ); const { kind, namespace, name } = req.params; - const { metricIds } = req.query; - validateCatalogMetricsSchema(req.query); + const { metricIds } = validateCatalogMetricsSchema(req.query); const entityRef = stringifyEntityRef({ kind, namespace, name }); // Check if user has permission to read this specific catalog entity await checkEntityAccess(entityRef, req, permissions, httpAuth); - const metricIdArray = - typeof metricIds === 'string' - ? parseCommaSeparatedString(metricIds) - : undefined; + const metricIdArray = metricIds + ? parseCommaSeparatedString(metricIds) + : undefined; const results = await catalogMetricService.getLatestEntityMetrics( entityRef, @@ -150,7 +132,7 @@ export async function createRouter({ res.json(results); }); - router.get('/metrics/:metricId/catalog/aggregation', async (req, res) => { + router.get('/metrics/:metricId/catalog/aggregations', async (req, res) => { const { metricId } = req.params; const { conditions } = await authorizeConditional( diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts index ebc0f1da8f6..331fc49ad23 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.test.ts @@ -20,42 +20,46 @@ import { InputError } from '@backstage/errors'; describe('validateCatalogMetricsSchema', () => { describe('valid query parameters', () => { it('should validate empty object', () => { - expect(() => validateCatalogMetricsSchema({})).not.toThrow(); + expect(validateCatalogMetricsSchema({})).toEqual({}); }); it('should validate object with valid metricIds string', () => { - expect(() => - validateCatalogMetricsSchema({ metricIds: 'github.open_prs' }), - ).not.toThrow(); + expect( + validateCatalogMetricsSchema({ + metricIds: 'github.open_prs', + }), + ).toEqual({ metricIds: 'github.open_prs' }); }); it('should validate object with valid metricIds containing comma-separated values', () => { - expect(() => + expect( validateCatalogMetricsSchema({ metricIds: 'github.open_prs,github.open_issues', }), - ).not.toThrow(); + ).toEqual({ + metricIds: 'github.open_prs,github.open_issues', + }); }); it('should validate object with undefined metricIds', () => { - expect(() => - validateCatalogMetricsSchema({ metricIds: undefined }), - ).not.toThrow(); + expect(validateCatalogMetricsSchema({ metricIds: undefined })).toEqual( + {}, + ); }); it('should validate when query has additional properties along with valid metricIds', () => { - expect(() => + expect( validateCatalogMetricsSchema({ metricIds: 'github.open_prs', invalidProp: 'value', }), - ).not.toThrow(); + ).toEqual({ metricIds: 'github.open_prs' }); }); it('should validate when query has only additional properties', () => { - expect(() => - validateCatalogMetricsSchema({ invalidProp: 'value' }), - ).not.toThrow(); + expect(validateCatalogMetricsSchema({ invalidProp: 'value' })).toEqual( + {}, + ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts index 65aff9832cc..4c56d83bd47 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateCatalogMetricsSchema.ts @@ -17,7 +17,9 @@ import { z } from 'zod'; import { InputError } from '@backstage/errors'; -export function validateCatalogMetricsSchema(query: unknown): void { +export function validateCatalogMetricsSchema(query: unknown): { + metricIds?: string; +} { const catalogMetricsSchema = z.object({ metricIds: z.string().min(1).optional(), }); @@ -27,4 +29,6 @@ export function validateCatalogMetricsSchema(query: unknown): void { if (!parsed.success) { throw new InputError(`Invalid query parameters: ${parsed.error.message}`); } + + return parsed.data; } From 6dc50d53c9272ae9e76318f163e16eeba154d00c Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Fri, 9 Jan 2026 17:30:57 +0100 Subject: [PATCH 3/7] refactor(scorecard): remove unused `listMetricsByDatasource` method Signed-off-by: Ihor Mykhno --- .../mockMetricProvidersRegistry.ts | 1 - .../providers/MetricProvidersRegistry.test.ts | 58 +++++++------------ .../src/providers/MetricProvidersRegistry.ts | 19 ------ 3 files changed, 22 insertions(+), 56 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts index 50900883fb9..ffbaea7acae 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts @@ -26,7 +26,6 @@ export const mockMetricProvidersRegistry = { calculateMetrics: jest.fn(), listProviders: jest.fn().mockReturnValue([]), listMetrics: jest.fn().mockReturnValue([]), - listMetricsByDatasource: jest.fn().mockReturnValue([]), } as unknown as jest.Mocked; type BuildMockMetricProvidersRegistryParams = { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts index 50b361814a4..92e5b12b2bb 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts @@ -301,15 +301,19 @@ describe('MetricProvidersRegistry', () => { }); describe('listMetrics', () => { + beforeEach(() => { + registry.register(githubNumberProvider); + registry.register(jiraBooleanProvider); + }); + it('should return empty array when no providers registered', () => { + registry = new MetricProvidersRegistry(); + const metrics = registry.listMetrics(); expect(metrics).toEqual([]); }); it('should return all registered metrics', () => { - registry.register(githubNumberProvider); - registry.register(jiraBooleanProvider); - const metrics = registry.listMetrics(); expect(metrics).toHaveLength(2); @@ -318,50 +322,32 @@ describe('MetricProvidersRegistry', () => { }); it('should return filtered metrics', () => { - registry.register(githubNumberProvider); - registry.register(jiraBooleanProvider); - const metrics = registry.listMetrics(['jira.boolean_metric']); expect(metrics).toHaveLength(1); expect(metrics[0].id).toBe('jira.boolean_metric'); }); - }); - describe('listMetricsByDatasource', () => { - beforeEach(() => { - const githubProvider1 = new MockNumberProvider( - 'github.open_prs', - 'github', - 'GitHub Open PRs', - ); - const githubProvider2 = new MockNumberProvider( - 'github.open_issues', - 'github', - 'GitHub Open Issues', - ); - const sonarProvider = new MockBooleanProvider( - 'sonar.code-quality', - 'sonar', - 'Code Quality', - ); - - registry.register(githubProvider1); - registry.register(githubProvider2); - registry.register(sonarProvider); - }); + it('should return empty array when all provider IDs are non-existent', () => { + const metrics = registry.listMetrics([ + 'non.existent.metric1', + 'non.existent.metric2', + ]); - it('should return empty array for non_existent datasource', () => { - const metrics = registry.listMetricsByDatasource('non_existent'); expect(metrics).toEqual([]); }); - it('should return metrics for specific datasource', () => { - const githubMetrics = registry.listMetricsByDatasource('github'); + it('should return only existing metrics when mix of existing and non-existent IDs', () => { + const metrics = registry.listMetrics([ + 'github.number_metric', + 'non.existent.metric', + 'jira.boolean_metric', + 'another.non.existent', + ]); - expect(githubMetrics).toHaveLength(2); - expect(githubMetrics[0].id).toBe('github.open_prs'); - expect(githubMetrics[1].id).toBe('github.open_issues'); + expect(metrics).toHaveLength(2); + expect(metrics[0].id).toBe('github.number_metric'); + expect(metrics[1].id).toBe('jira.boolean_metric'); }); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts index 24d5dfeadba..72d20080cb0 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts @@ -27,7 +27,6 @@ import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecar */ export class MetricProvidersRegistry { private readonly metricProviders = new Map(); - private readonly datasourceIndex = new Map>(); register(metricProvider: MetricProvider): void { const providerId = metricProvider.getProviderId(); @@ -64,12 +63,6 @@ export class MetricProvidersRegistry { } this.metricProviders.set(providerId, metricProvider); - let datasourceProviders = this.datasourceIndex.get(providerDatasource); - if (!datasourceProviders) { - datasourceProviders = new Set(); - this.datasourceIndex.set(providerDatasource, datasourceProviders); - } - datasourceProviders.add(providerId); } getProvider(providerId: string): MetricProvider { @@ -124,16 +117,4 @@ export class MetricProvidersRegistry { provider.getMetric(), ); } - - listMetricsByDatasource(datasourceId: string): Metric[] { - const providerIdsOfDatasource = this.datasourceIndex.get(datasourceId); - if (!providerIdsOfDatasource) { - return []; - } - - return Array.from(providerIdsOfDatasource) - .map(providerId => this.metricProviders.get(providerId)) - .filter((provider): provider is MetricProvider => provider !== undefined) - .map(provider => provider.getMetric()); - } } From f0088e2240f522b25174b851e0e8f0b5e2c9c340 Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Mon, 12 Jan 2026 10:18:14 +0100 Subject: [PATCH 4/7] fix(scorecard): aggregation documentation Signed-off-by: Ihor Mykhno --- .../plugins/scorecard-backend/docs/aggregation.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md index a1983fcea50..d45e21811b3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md +++ b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md @@ -28,6 +28,10 @@ In this case: - ✅ Entities owned by `group:default/developers` are included - ❌ Entities owned by `group:default/engineering` are **NOT** included +**Enabling Transitive Ownership:** + +To include entities from all parent groups in the aggregation (not just direct parent groups), you can enable transitive parent groups. If you're using Red Hat Developer Hub (RHDH), you can enable transitive parent groups by following the [transitive parent group enablement documentation](https://docs.redhat.com/en/documentation/red_hat_developer_hub/1.5/html-single/authorization_in_red_hat_developer_hub/index#enabling-transitive-parent-groups). This will allow the aggregation to traverse nested group hierarchies and include entities from all parent groups in the hierarchy. + ## API Endpoint ### `GET /metrics/:metricId/catalog/aggregations` @@ -62,7 +66,6 @@ curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations" #### Key Features - **Metric Access Validation**: This endpoint explicitly validates that the user has access to the specified metric and returns `403 Forbidden` if access is denied -- **Single Metric Only**: Returns aggregation for only the specified metric (no need for `metricIds` query parameter) - **Empty Results Handling**: Returns an empty array `[]` when the user owns no entities, avoiding errors when filtering by a single metric ## Error Handling @@ -99,6 +102,7 @@ If invalid query parameters are provided: 1. **Handle Empty Results**: Always check for empty arrays when the user owns no entities -2. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. If you need nested group aggregation, consider restructuring your groups or implementing custom logic +2. **Group Structure**: Be aware of the direct parent group limitation when designing your group hierarchy. You currently receive scorecard results only for entities you own and those of your immediate parent group. To include results from _all_ parent + groups, you can either implement custom logic, restructure your groups, or (if using RHDH), enable transitive parent groups ([see transitive parent group enablement documentation](https://docs.redhat.com/en/documentation/red_hat_developer_hub/1.5/html-single/authorization_in_red_hat_developer_hub/index#enabling-transitive-parent-groups)). 3. **Metric Access**: This endpoint validates metric access upfront, so you'll get a clear `403 Forbidden` error if the user doesn't have permission to view the specified metric From 5533494605e85e6cb6960c591c17c5c3a37b847c Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Mon, 12 Jan 2026 11:07:57 +0100 Subject: [PATCH 5/7] fix(scorecard): the `/metrics` endpoint and update documentation Signed-off-by: Ihor Mykhno --- .../.changeset/orange-items-write.md | 7 ++ .../plugins/scorecard-backend/README.md | 97 ++++++++++++++- .../mockMetricProvidersRegistry.ts | 1 + .../providers/MetricProvidersRegistry.test.ts | 48 ++++++++ .../src/providers/MetricProvidersRegistry.ts | 21 ++++ .../src/service/router.test.ts | 53 ++++++++ .../scorecard-backend/src/service/router.ts | 9 +- .../validation/validateMetricsSchema.test.ts | 113 ++++++++++++++++++ .../src/validation/validateMetricsSchema.ts | 36 ++++++ 9 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 workspaces/scorecard/.changeset/orange-items-write.md create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.test.ts create mode 100644 workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.ts diff --git a/workspaces/scorecard/.changeset/orange-items-write.md b/workspaces/scorecard/.changeset/orange-items-write.md new file mode 100644 index 00000000000..806e3d12e51 --- /dev/null +++ b/workspaces/scorecard/.changeset/orange-items-write.md @@ -0,0 +1,7 @@ +--- +'@red-hat-developer-hub/backstage-plugin-scorecard-backend': minor +--- + +Added `metricIds` query parameter to the `/metrics` endpoint to filter metrics by metric IDs. + +Scorecard read permission are no longer needed to get available metrics for the `/metrics` endpoint. diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index 54cb1300deb..d794f4f227c 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -99,18 +99,103 @@ Thresholds are evaluated in order, and the first matching rule determines the ca For comprehensive threshold configuration guide, examples, and best practices, see [thresholds.md](./docs/thresholds.md). -## Entity Aggregation +## API Endpoints -The Scorecard plugin provides aggregation endpoints that return metrics for all entities owned by the authenticated user. This includes: +### `GET /metrics` + +Returns a list of available metrics. Supports filtering by metric IDs or datasource. + +#### Query Parameters + +| Parameter | Type | Required | Description | +| ------------ | ------ | -------- | -------------------------------------------------------------------------------------------- | +| `metricIds` | string | No | Comma-separated list of metric IDs to filter by (e.g., `github.open_prs,github.open_issues`) | +| `datasource` | string | No | Filter metrics by datasource ID (e.g., `github`, `jira`, `sonar`) | + +#### Behavior + +- If `metricIds` is provided, returns only the specified metrics (takes precedence over `datasource`) +- If `datasource` is provided (and `metricIds` is not), returns all metrics from that datasource +- If neither parameter is provided, returns all available metrics + +#### Example Requests + +```bash +# Get all metrics +curl -X GET "{{url}}/api/scorecard/metrics" \ + -H "Authorization: Bearer " + +# Get specific metrics by IDs +curl -X GET "{{url}}/api/scorecard/metrics?metricIds=github.open_prs,github.open_issues" \ + -H "Authorization: Bearer " + +# Get all metrics from a specific datasource +curl -X GET "{{url}}/api/scorecard/metrics?datasource=github" \ + -H "Authorization: Bearer " +``` + +### `GET /metrics/catalog/:kind/:namespace/:name` + +Returns the latest metric values for a specific catalog entity. + +#### Path Parameters + +| Parameter | Type | Required | Description | +| ----------- | ------ | -------- | ---------------------------------- | +| `kind` | string | Yes | Entity kind (e.g., `component`) | +| `namespace` | string | Yes | Entity namespace (e.g., `default`) | +| `name` | string | Yes | Entity name | + +#### Query Parameters + +| Parameter | Type | Required | Description | +| ----------- | ------ | -------- | -------------------------------------------------------------------------------------------- | +| `metricIds` | string | No | Comma-separated list of metric IDs to filter by (e.g., `github.open_prs,github.open_issues`) | + +#### Permissions + +Requires `scorecard.metric.read` permission and `catalog.entity.read` permission for the specific entity. + +#### Example Request + +```bash +curl -X GET "{{url}}/api/scorecard/metrics/catalog/component/default/my-service?metricIds=github.open_prs" \ + -H "Authorization: Bearer " +``` + +### `GET /metrics/:metricId/catalog/aggregations` + +Returns aggregated metrics for a specific metric across all entities owned by the authenticated user. This endpoint aggregates metrics from: - Entities directly owned by the user -- Entities owned by groups the user is a direct member of (Only direct parent groups are considered) +- Entities owned by groups the user is a direct member of (only direct parent groups are considered) + +#### Path Parameters -### Available Endpoint +| Parameter | Type | Required | Description | +| ---------- | ------ | -------- | --------------------------------- | +| `metricId` | string | Yes | The ID of the metric to aggregate | -- **`GET /metrics/:metricId/catalog/aggregations`**: Returns aggregated metrics for a specific metric across all entities owned by the authenticated user, with explicit access validation (returns `403` if the user doesn't have access to the metric) +#### Authentication + +Requires user authentication. The endpoint uses the authenticated user's entity reference to determine which entities to aggregate. + +#### Permissions + +Requires `scorecard.metric.read` permission. Additionally: + +- The user must have access to the specific metric (returns `403 Forbidden` if access is denied) +- The user must have `catalog.entity.read` permission for each entity that will be included in the aggregation + +#### Example Request + +```bash +# Get aggregated metrics for a specific metric +curl -X GET "{{url}}/api/scorecard/metrics/github.open_prs/catalog/aggregations" \ + -H "Authorization: Bearer " +``` -For comprehensive documentation on how entity aggregation works, API details, examples, and best practices, see [aggregation.md](./docs/aggregation.md). +For comprehensive documentation on how entity aggregation works, including details on transitive parent groups, error handling, and best practices, see [aggregation.md](./docs/aggregation.md). ## Configuration cleanup Job diff --git a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts index ffbaea7acae..50900883fb9 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/__fixtures__/mockMetricProvidersRegistry.ts @@ -26,6 +26,7 @@ export const mockMetricProvidersRegistry = { calculateMetrics: jest.fn(), listProviders: jest.fn().mockReturnValue([]), listMetrics: jest.fn().mockReturnValue([]), + listMetricsByDatasource: jest.fn().mockReturnValue([]), } as unknown as jest.Mocked; type BuildMockMetricProvidersRegistryParams = { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts index 92e5b12b2bb..cc58d10cb8a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts @@ -350,4 +350,52 @@ describe('MetricProvidersRegistry', () => { expect(metrics[1].id).toBe('jira.boolean_metric'); }); }); + + describe('listMetricsByDatasource', () => { + beforeEach(() => { + registry.register(githubNumberProvider); + registry.register(jiraBooleanProvider); + registry.register( + new MockNumberProvider( + 'github.open_issues', + 'github', + 'GitHub Open Issues', + ), + ); + }); + + it('should return empty array when no providers registered', () => { + registry = new MetricProvidersRegistry(); + + const metrics = registry.listMetricsByDatasource('github'); + expect(metrics).toEqual([]); + }); + + it('should return all metrics for a specific datasource', () => { + const metrics = registry.listMetricsByDatasource('github'); + + expect(metrics).toHaveLength(2); + expect(metrics[0].id).toBe('github.number_metric'); + expect(metrics[1].id).toBe('github.open_issues'); + }); + + it('should return metrics for jira datasource', () => { + const metrics = registry.listMetricsByDatasource('jira'); + + expect(metrics).toHaveLength(1); + expect(metrics[0].id).toBe('jira.boolean_metric'); + }); + + it('should return empty array when datasource does not exist', () => { + const metrics = registry.listMetricsByDatasource('nonexistent'); + + expect(metrics).toEqual([]); + }); + + it('should return empty array when datasource is empty string', () => { + const metrics = registry.listMetricsByDatasource(''); + + expect(metrics).toEqual([]); + }); + }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts index 72d20080cb0..be6204d2246 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts @@ -27,6 +27,7 @@ import { MetricProvider } from '@red-hat-developer-hub/backstage-plugin-scorecar */ export class MetricProvidersRegistry { private readonly metricProviders = new Map(); + private readonly datasourceIndex = new Map>(); register(metricProvider: MetricProvider): void { const providerId = metricProvider.getProviderId(); @@ -63,6 +64,13 @@ export class MetricProvidersRegistry { } this.metricProviders.set(providerId, metricProvider); + + let datasourceProviders = this.datasourceIndex.get(providerDatasource); + if (!datasourceProviders) { + datasourceProviders = new Set(); + this.datasourceIndex.set(providerDatasource, datasourceProviders); + } + datasourceProviders.add(providerId); } getProvider(providerId: string): MetricProvider { @@ -117,4 +125,17 @@ export class MetricProvidersRegistry { provider.getMetric(), ); } + + listMetricsByDatasource(datasourceId: string): Metric[] { + const providerIdsOfDatasource = this.datasourceIndex.get(datasourceId); + + if (!providerIdsOfDatasource) { + return []; + } + + return Array.from(providerIdsOfDatasource) + .map(providerId => this.metricProviders.get(providerId)) + .filter((provider): provider is MetricProvider => provider !== undefined) + .map(provider => provider.getMetric()); + } } diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index 8cffd5348ad..de23d105b36 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -223,6 +223,59 @@ describe('createRouter', () => { expect(response.body.metrics[0].id).toBe('github.open_prs'); }); + + it('should return metrics filtered by datasource', async () => { + const response = await request(app).get('/metrics?datasource=github'); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(2); + + const metricIds = response.body.metrics.map((m: Metric) => m.id); + expect(metricIds).toContain('github.open_prs'); + expect(metricIds).toContain('github.open_issues'); + }); + + it('should return metrics filtered by datasource - sonar', async () => { + const response = await request(app).get('/metrics?datasource=sonar'); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(1); + + const metricIds = response.body.metrics.map((m: Metric) => m.id); + expect(metricIds).toContain('sonar.quality'); + }); + + it('should return empty array when datasource does not exist', async () => { + const response = await request(app).get( + '/metrics?datasource=nonexistent', + ); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(0); + }); + + it('should return 400 InputError when invalid datasource parameter - empty string', async () => { + const response = await request(app).get('/metrics?datasource='); + + expect(response.status).toEqual(400); + expect(response.body.error.name).toEqual('InputError'); + expect(response.body.error.message).toContain('Invalid query parameters'); + }); + + it('should prioritize metricIds over datasource when both are provided', async () => { + const response = await request(app).get( + '/metrics?metricIds=sonar.quality&datasource=github', + ); + + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(1); + + expect(response.body.metrics[0].id).toBe('sonar.quality'); + }); }); describe('GET /metrics/catalog/:kind/:namespace/:name', () => { diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index 7331a3d0e3c..c04f3b44488 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -38,6 +38,7 @@ import { stringifyEntityRef } from '@backstage/catalog-model'; import { validateCatalogMetricsSchema } from '../validation/validateCatalogMetricsSchema'; import { getEntitiesOwnedByUser } from '../utils/getEntitiesOwnedByUser'; import { parseCommaSeparatedString } from '../utils/parseCommaSeparatedString'; +import { validateMetricsSchema } from '../validation/validateMetricsSchema'; export type ScorecardRouterOptions = { metricProvidersRegistry: MetricProvidersRegistry; @@ -92,7 +93,7 @@ export async function createRouter({ }; router.get('/metrics', async (req, res) => { - const { metricIds } = validateCatalogMetricsSchema(req.query); + const { metricIds, datasource } = validateMetricsSchema(req.query); if (metricIds) { return res.json({ @@ -102,6 +103,12 @@ export async function createRouter({ }); } + if (datasource) { + return res.json({ + metrics: metricProvidersRegistry.listMetricsByDatasource(datasource), + }); + } + return res.json({ metrics: metricProvidersRegistry.listMetrics() }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.test.ts new file mode 100644 index 00000000000..460c3771c73 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.test.ts @@ -0,0 +1,113 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { validateMetricsSchema } from './validateMetricsSchema'; +import { InputError } from '@backstage/errors'; + +describe('validateMetricsSchema', () => { + describe('valid query parameters', () => { + it('should validate empty object', () => { + expect(validateMetricsSchema({})).toEqual({}); + }); + + it('should validate object with valid metricIds string', () => { + expect( + validateMetricsSchema({ + metricIds: 'github.open_prs', + }), + ).toEqual({ metricIds: 'github.open_prs' }); + }); + + it('should validate object with valid metricIds containing comma-separated values', () => { + expect( + validateMetricsSchema({ + metricIds: 'github.open_prs,github.open_issues', + }), + ).toEqual({ + metricIds: 'github.open_prs,github.open_issues', + }); + }); + + it('should validate object with valid datasource string', () => { + expect( + validateMetricsSchema({ + datasource: 'github', + }), + ).toEqual({ datasource: 'github' }); + }); + + it('should validate object with undefined metricIds', () => { + expect(validateMetricsSchema({ metricIds: undefined })).toEqual({}); + }); + + it('should validate object with undefined datasource', () => { + expect(validateMetricsSchema({ datasource: undefined })).toEqual({}); + }); + + it('should validate when query has additional properties along with valid parameters', () => { + expect( + validateMetricsSchema({ + metricIds: 'github.open_prs', + datasource: 'github', + invalidProp: 'value', + }), + ).toEqual({ + metricIds: 'github.open_prs', + datasource: 'github', + }); + }); + + it('should validate when query has only additional properties', () => { + expect(validateMetricsSchema({ invalidProp: 'value' })).toEqual({}); + }); + }); + + describe('invalid query parameters', () => { + it.each([ + { metricIds: '', description: 'empty string' }, + { metricIds: null, description: 'null' }, + { metricIds: 123, description: 'number' }, + { metricIds: true, description: 'boolean' }, + { metricIds: ['github.open_prs'], description: 'array' }, + { metricIds: { id: 'test' }, description: 'object' }, + ])( + 'should throw InputError when metricIds is $description', + ({ metricIds }) => { + expect(() => validateMetricsSchema({ metricIds })).toThrow(InputError); + expect(() => validateMetricsSchema({ metricIds })).toThrow( + 'Invalid query parameters', + ); + }, + ); + + it.each([ + { datasource: '', description: 'empty string' }, + { datasource: null, description: 'null' }, + { datasource: 123, description: 'number' }, + { datasource: true, description: 'boolean' }, + { datasource: ['github'], description: 'array' }, + { datasource: { id: 'test' }, description: 'object' }, + ])( + 'should throw InputError when datasource is $description', + ({ datasource }) => { + expect(() => validateMetricsSchema({ datasource })).toThrow(InputError); + expect(() => validateMetricsSchema({ datasource })).toThrow( + 'Invalid query parameters', + ); + }, + ); + }); +}); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.ts b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.ts new file mode 100644 index 00000000000..cb0bcc69023 --- /dev/null +++ b/workspaces/scorecard/plugins/scorecard-backend/src/validation/validateMetricsSchema.ts @@ -0,0 +1,36 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { z } from 'zod'; +import { InputError } from '@backstage/errors'; + +export function validateMetricsSchema(query: unknown): { + metricIds?: string; + datasource?: string; +} { + const catalogMetricsSchema = z.object({ + metricIds: z.string().min(1).optional(), + datasource: z.string().min(1).optional(), + }); + + const parsed = catalogMetricsSchema.safeParse(query); + + if (!parsed.success) { + throw new InputError(`Invalid query parameters: ${parsed.error.message}`); + } + + return parsed.data; +} From 75a0360b3961efc78c0b08f6e62103eabdb6efaa Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Mon, 12 Jan 2026 12:53:24 +0100 Subject: [PATCH 6/7] fix(scorecard): remove created RBAC role after all e2e tests Signed-off-by: Ihor Mykhno --- .../packages/app/e2e-tests/scorecard.test.ts | 10 +++++ .../app/e2e-tests/utils/rbacDelete.ts | 38 +++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 workspaces/scorecard/packages/app/e2e-tests/utils/rbacDelete.ts diff --git a/workspaces/scorecard/packages/app/e2e-tests/scorecard.test.ts b/workspaces/scorecard/packages/app/e2e-tests/scorecard.test.ts index 3b5d66f0658..6c9add1d2d7 100644 --- a/workspaces/scorecard/packages/app/e2e-tests/scorecard.test.ts +++ b/workspaces/scorecard/packages/app/e2e-tests/scorecard.test.ts @@ -31,6 +31,7 @@ import { getTranslations, } from './utils/translationUtils'; import { runAccessibilityTests } from './utils/accessibility'; +import { deleteRBAC } from './utils/rbacDelete'; test.describe.serial('Pre-RBAC Access Tests', () => { let translations: ScorecardMessages; @@ -90,6 +91,15 @@ test.describe.serial('Scorecard Plugin Tests', () => { scorecardPage = new ScorecardPage(page, translations); }); + test.afterAll(async ({ browser }) => { + const context = await browser.newContext(); + const page = await context.newPage(); + + await deleteRBAC(page); + + await context.close(); + }); + test('Validate scorecard tabs for GitHub PRs and Jira tickets', async ({ page, }, testInfo) => { diff --git a/workspaces/scorecard/packages/app/e2e-tests/utils/rbacDelete.ts b/workspaces/scorecard/packages/app/e2e-tests/utils/rbacDelete.ts new file mode 100644 index 00000000000..2e5ac95d295 --- /dev/null +++ b/workspaces/scorecard/packages/app/e2e-tests/utils/rbacDelete.ts @@ -0,0 +1,38 @@ +/* + * Copyright Red Hat, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { Page, expect } from '@playwright/test'; + +/** + * Deletes RBAC configuration after tests. + */ +export async function deleteRBAC(page: Page) { + await page.goto('/rbac'); + const enterButton = page.getByRole('button', { name: 'Enter' }); + await expect(enterButton).toBeVisible(); + await enterButton.click(); + + await page.getByTestId('delete-role-role:default/rhdh-testing').click(); + + await page + .getByRole('textbox', { name: 'Role name' }) + .fill('role:default/rhdh-testing'); + + await page.getByRole('button', { name: 'Delete' }).click(); + + await expect( + page.getByTestId('delete-role-role:default/rhdh-testing'), + ).toBeHidden(); +} From 19a48a13a7761213b93c253c1b73070769b0d91d Mon Sep 17 00:00:00 2001 From: Ihor Mykhno Date: Mon, 12 Jan 2026 14:49:15 +0100 Subject: [PATCH 7/7] fix(scorecard): throw 400 if both `metricIds` and `datasource` are specified Signed-off-by: Ihor Mykhno --- .../scorecard/plugins/scorecard-backend/README.md | 3 ++- .../scorecard-backend/src/service/router.test.ts | 12 ++++++------ .../plugins/scorecard-backend/src/service/router.ts | 6 +++++- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index d794f4f227c..e3b6b3b6763 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -114,9 +114,10 @@ Returns a list of available metrics. Supports filtering by metric IDs or datasou #### Behavior -- If `metricIds` is provided, returns only the specified metrics (takes precedence over `datasource`) +- If `metricIds` is provided, returns only the specified metrics - If `datasource` is provided (and `metricIds` is not), returns all metrics from that datasource - If neither parameter is provided, returns all available metrics +- **Note**: Providing both `metricIds` and `datasource` will result in a `400 Bad Request` error #### Example Requests diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts index de23d105b36..40f5aaef539 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -265,16 +265,16 @@ describe('createRouter', () => { expect(response.body.error.message).toContain('Invalid query parameters'); }); - it('should prioritize metricIds over datasource when both are provided', async () => { + it('should return 400 InputError when both metricIds and datasource are provided', async () => { const response = await request(app).get( '/metrics?metricIds=sonar.quality&datasource=github', ); - expect(response.status).toBe(200); - expect(response.body).toHaveProperty('metrics'); - expect(response.body.metrics).toHaveLength(1); - - expect(response.body.metrics[0].id).toBe('sonar.quality'); + expect(response.status).toBe(400); + expect(response.body.error.name).toBe('InputError'); + expect(response.body.error.message).toBe( + 'Cannot filter by both metricIds and datasource', + ); }); }); diff --git a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts index c04f3b44488..50afaa02a6a 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.ts @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { NotAllowedError, NotFoundError } from '@backstage/errors'; +import { InputError, NotAllowedError, NotFoundError } from '@backstage/errors'; import express, { Request } from 'express'; import Router from 'express-promise-router'; import type { CatalogMetricService } from './CatalogMetricService'; @@ -95,6 +95,10 @@ export async function createRouter({ router.get('/metrics', async (req, res) => { const { metricIds, datasource } = validateMetricsSchema(req.query); + if (metricIds && datasource) { + throw new InputError('Cannot filter by both metricIds and datasource'); + } + if (metricIds) { return res.json({ metrics: metricProvidersRegistry.listMetrics(