diff --git a/workspaces/scorecard/.changeset/orange-items-write.md b/workspaces/scorecard/.changeset/orange-items-write.md new file mode 100644 index 0000000000..806e3d12e5 --- /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/examples/all-scorecards.yaml b/workspaces/scorecard/examples/all-scorecards.yaml index 137e23b3da..363d007eda 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 diff --git a/workspaces/scorecard/plugins/scorecard-backend/README.md b/workspaces/scorecard/plugins/scorecard-backend/README.md index 1291a81e9f..d794f4f227 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/README.md +++ b/workspaces/scorecard/plugins/scorecard-backend/README.md @@ -99,19 +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 Endpoints +| Parameter | Type | Required | Description | +| ---------- | ------ | -------- | --------------------------------- | +| `metricId` | string | Yes | The ID of the metric to aggregate | -- **`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) +#### 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/docs/aggregation.md b/workspaces/scorecard/plugins/scorecard-backend/docs/aggregation.md index bc8da9aea6..d45e21811b 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,13 @@ In this case: - ✅ Entities owned by `group:default/developers` are included - ❌ Entities owned by `group:default/engineering` are **NOT** included -## API Endpoints +**Enabling Transitive Ownership:** -### `GET /metrics/catalog/aggregates` +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. -Returns aggregated metrics for all entities owned by the authenticated user. +## API Endpoint -#### 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,14 +59,13 @@ 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) - **Empty Results Handling**: Returns an empty array `[]` when the user owns no entities, avoiding errors when filtering by a single metric ## Error Handling @@ -101,8 +74,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 +84,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 +100,9 @@ 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. 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. **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/providers/MetricProvidersRegistry.test.ts b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.test.ts index 50b361814a..cc58d10cb8 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,80 @@ 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'); }); + + it('should return empty array when all provider IDs are non-existent', () => { + const metrics = registry.listMetrics([ + 'non.existent.metric1', + 'non.existent.metric2', + ]); + + expect(metrics).toEqual([]); + }); + + 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(metrics).toHaveLength(2); + expect(metrics[0].id).toBe('github.number_metric'); + expect(metrics[1].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(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(); - registry.register(githubProvider1); - registry.register(githubProvider2); - registry.register(sonarProvider); + const metrics = registry.listMetricsByDatasource('github'); + expect(metrics).toEqual([]); }); - it('should return empty array for non_existent datasource', () => { - const metrics = registry.listMetricsByDatasource('non_existent'); + 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 metrics for specific datasource', () => { - const githubMetrics = registry.listMetricsByDatasource('github'); + it('should return empty array when datasource is empty string', () => { + const metrics = registry.listMetricsByDatasource(''); - expect(githubMetrics).toHaveLength(2); - expect(githubMetrics[0].id).toBe('github.open_prs'); - expect(githubMetrics[1].id).toBe('github.open_issues'); + 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 24d5dfeadb..be6204d224 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/providers/MetricProvidersRegistry.ts @@ -64,6 +64,7 @@ export class MetricProvidersRegistry { } this.metricProviders.set(providerId, metricProvider); + let datasourceProviders = this.datasourceIndex.get(providerDatasource); if (!datasourceProviders) { datasourceProviders = new Set(); @@ -127,6 +128,7 @@ export class MetricProvidersRegistry { listMetricsByDatasource(datasourceId: string): Metric[] { const providerIdsOfDatasource = this.datasourceIndex.get(datasourceId); + if (!providerIdsOfDatasource) { return []; } 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 d2c6b326b5..de23d105b3 100644 --- a/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts +++ b/workspaces/scorecard/plugins/scorecard-backend/src/service/router.test.ts @@ -150,32 +150,50 @@ describe('createRouter', () => { metricProvidersRegistry.register(sonarProvider); }); - it('should return 403 Unauthorized when DENY permissions', async () => { - permissionsMock.authorizeConditional.mockResolvedValue([ - { result: AuthorizeResult.DENY }, - ]); + it('should return all metrics when no metricIds parameter', async () => { const response = await request(app).get('/metrics'); - expect(response.statusCode).toBe(403); - expect(response.body.error.name).toEqual('NotAllowedError'); + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('metrics'); + expect(response.body.metrics).toHaveLength(3); + + const metricIds = response.body.metrics.map((m: Metric) => m.id); + expect(metricIds).toContain('github.open_prs'); + expect(metricIds).toContain('github.open_issues'); + expect(metricIds).toContain('sonar.quality'); }); - it('should return all metrics', async () => { - const response = await request(app).get('/metrics'); - console.log('response', response.body); + 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(3); + 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'); + 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'); - 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 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'); @@ -186,18 +204,59 @@ 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 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'); + }); + + 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); - 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 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='); @@ -205,6 +264,18 @@ describe('createRouter', () => { 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', () => { @@ -374,7 +445,7 @@ describe('createRouter', () => { }); }); - describe('GET /metrics/:metricId/catalog/aggregation', () => { + describe('GET /metrics/:metricId/catalog/aggregations', () => { const mockAggregatedMetricResults: AggregatedMetricResult[] = [ { id: 'github.open_prs', @@ -467,7 +538,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 +550,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 +565,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 +577,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 +593,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 +609,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 +621,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 +646,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 +669,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 819e2a1501..c04f3b4448 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'; @@ -39,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; @@ -93,32 +93,23 @@ 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 { metricIds, datasource } = validateMetricsSchema(req.query); + + if (metricIds) { + return res.json({ + metrics: metricProvidersRegistry.listMetrics( + parseCommaSeparatedString(metricIds), + ), + }); } - const { datasource } = parsed.data; - - let metrics; if (datasource) { - metrics = metricProvidersRegistry.listMetricsByDatasource(datasource); - } else { - metrics = metricProvidersRegistry.listMetrics(); + return res.json({ + metrics: metricProvidersRegistry.listMetricsByDatasource(datasource), + }); } - res.json({ - metrics: filterAuthorizedMetrics(metrics, conditions), - }); + return res.json({ metrics: metricProvidersRegistry.listMetrics() }); }); router.get('/metrics/catalog/:kind/:namespace/:name', async (req, res) => { @@ -128,19 +119,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 +139,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 ebc0f1da8f..331fc49ad2 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 65aff9832c..4c56d83bd4 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; } 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 0000000000..460c3771c7 --- /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 0000000000..cb0bcc6902 --- /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; +}