Skip to content

Conversation

@rikukissa
Copy link
Member

Description

Name of the integration client can still change. The purpose is to have an integration client type that has sufficient privileges for creating declarations, searching records and requesting certificate printouts.

Checklist

  • I have linked the correct Github issue under "Development"
  • I have tested the changes locally, and written appropriate tests
  • I have tested beyond the happy path (e.g. edge cases, failure paths)
  • I have updated the changelog with this change (if applicable)
  • I have updated the GitHub issue status accordingly

@github-actions
Copy link

Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:

  • Changelog is read by country implementors who might not always be familiar with all technical details of OpenCRVS. Keep language high-level, user friendly and avoid technical references to internals.
  • Answer "What's new?", "Why was the change made?" and "Why should I care?" for each change.
  • If it's a breaking change, include a migration guide answering "What do I need to do to upgrade?".

@github-actions
Copy link

github-actions bot commented Jan 22, 2026

ℹ️ Coverage metrics explained:
Statements — Executed code statements (basic logic lines)
Branches — Tested decision paths (if/else, switch, ternaries)
Functions — Functions invoked during tests
Lines — Source lines executed

@github-actions
Copy link

github-actions bot commented Jan 22, 2026

📊 commons test coverage

Statements: 67.24%
Branches:   32.84%
Functions:  50.2%
Lines:      66.64%
Updated at: Wed, 28 Jan 2026 11:22:43 GMT

@github-actions
Copy link

github-actions bot commented Jan 22, 2026

📊 events test coverage

Statements: 86.03%
Branches:   83.65%
Functions:  92.94%
Lines:      86.03%
Updated at: Wed, 28 Jan 2026 11:27:08 GMT

// - record.registered.print-certified-copies[event=birth|tennis-club-membership]
const rawConfigurableScopeRegex =
/^([a-zA-Z][a-zA-Z0-9.-]*(?:\.[a-zA-Z0-9.-]+)*)\[((?:\w+=[\w.-]+(?:\|[\w.-]+)*)(?:,[\w]+=[\w.-]+(?:\|[\w.-]+)*)*)\]$/
/^([a-zA-Z][a-zA-Z0-9.-]*(?:\.[a-zA-Z0-9.-]+)*)(?:\[((?:\w+=[\w.-]+(?:\|[\w.-]+)*)(?:,[\w]+=[\w.-]+(?:\|[\w.-]+)*)*)\])?$/
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the options section of the scope [event=birth] optional

@ocrvs-bot
Copy link
Contributor

Your environment is deployed to https://self-service-portal-apis.e2e-k8s.opencrvs.dev

@rikukissa rikukissa added the 🔒 Keep e2e Don't delete E2E environment after testing label Jan 28, 2026
@rikukissa rikukissa requested a review from Copilot January 28, 2026 09:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new CITIZEN_PORTAL integration client type that enables programmatic access to core operations including creating declarations, reading records, and sending notifications.

Changes:

  • Added CITIZEN_PORTAL as a new system integration type with scopes for record read, create, and notify operations
  • Modified scope parsing to allow configurable scopes without event type specifications (brackets optional)
  • Updated authorization middleware to bypass location-based access checks for all system users
  • Updated API signature for event.get endpoint from UUID parameter to EventIdParam object

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
packages/user-mgnt/src/utils/system.ts Added CITIZEN_PORTAL to system types enum
packages/user-mgnt/src/model/system.ts Added CITIZEN_PORTAL to system schema enum values
packages/user-mgnt/src/features/system/scopes.ts Defined default scopes for CITIZEN_PORTAL (record.read, record.create, record.notify)
packages/user-mgnt/src/features/system/handler.ts Added CITIZEN_PORTAL to list of systems that receive client credentials
packages/gateway/src/graphql/schema.graphql Added CITIZEN_PORTAL to SystemType enum
packages/gateway/src/features/systems/schema.graphql Added CITIZEN_PORTAL to SystemType enum
packages/events/src/router/middleware/authorization/index.ts Modified userCanReadEventV2 to bypass authorization checks for system users and changed input type to EventIdParam
packages/events/src/router/event/index.ts Changed event.get endpoint to use EventIdParam input type and added OpenAPI metadata
packages/events/src/router/event/*.test.ts Updated test calls to use new EventIdParam object format
packages/commons/src/users/User.ts Added CITIZEN_PORTAL to SystemRole enum
packages/commons/src/scopes.ts Modified regex and RecordScope schema to make event specifications optional for configurable scopes
packages/client/src/views/SysAdmin/Config/Systems/Systems.tsx Added CITIZEN_PORTAL option to system type selector
packages/client/src/v2-events/hooks/useUserDetails.ts Added CITIZEN_PORTAL role label mapping
packages/client/src/v2-events/features/workqueues/Workqueue.interaction.stories.tsx Updated mock to use new EventIdParam format
packages/client/src/v2-events/features/events/useEvents/procedures/get.ts Updated all event.get calls to use EventIdParam format
packages/client/src/v2-events/features/events/useEvents/procedures/actions/utils.ts Updated optimistic update to use EventIdParam format
packages/client/src/v2-events/features/events/useEvents/outbox.ts Updated query key generation to use EventIdParam format
packages/client/src/v2-events/features/events/useEvents/api.ts Updated event data operations to use EventIdParam format
packages/client/src/v2-events/features/drafts/useDrafts.ts Updated draft prefetching to use EventIdParam format
packages/client/src/i18n/messages/views/integrations.ts Added internationalization message for Citizen Portal label

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

WEBHOOK: [SCOPES.WEBHOOK],
REINDEX: [SCOPES.RECORD_REINDEX]
REINDEX: [SCOPES.RECORD_REINDEX],
CITIZEN_PORTAL: ['record.read', 'record.create', 'record.notify']
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The PR description states that CITIZEN_PORTAL should have "sufficient privileges for creating declarations, searching records and requesting certificate printouts". However, the scopes defined only include 'record.read', 'record.create', and 'record.notify'. The scope for requesting certificate printouts ('record.registered.print-certified-copies') is missing. Either add this scope if it's required, or update the PR description to reflect the actual intended capabilities.

Copilot uses AI. Check for mistakes.
throw new EventNotFoundError(input)
if (!hasAccess) {
throw new EventNotFoundError(input.eventId)
}
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The authorization logic has a potential issue: if the user is not a system user (line 350 fails), and acceptedScopes is not empty (line 364), the code checks if humanUser.success at line 412 but continues to the next() call even if humanUser.success is false. This means if ctx.user is neither a valid SystemContext nor a valid UserContext (which should not happen but could theoretically occur), access would be granted without proper authorization checks. Consider adding an else clause to throw an error if humanUser.success is false, or restructuring the logic to ensure one of the two user types is always valid.

Suggested change
}
}
} else {
// At this point the request is not from a system user and the user context
// does not match the expected UserContext schema. Treat this as an invalid
// authorization state rather than allowing access by falling through.
throw new TRPCError({
code: 'INTERNAL_SERVER_ERROR',
message:
'Invalid user context: expected a valid system or user context for authorization'
})

Copilot uses AI. Check for mistakes.
Comment on lines +352 to +360
if (isSystemUser) {
return next({
ctx: {
...ctx,
eventId: input.eventId,
eventType: event.type
}
})
}
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The authorization middleware now bypasses all authorization checks for system users, allowing any system integration client (including CITIZEN_PORTAL) to read any event regardless of scopes. This is a significant security concern. System users should still be subject to scope-based authorization checks. The scopes should be validated to ensure the system has the appropriate 'record.read' scope for the event type being accessed.

Copilot uses AI. Check for mistakes.
// - record.registered.print-certified-copies[event=birth|tennis-club-membership]
const rawConfigurableScopeRegex =
/^([a-zA-Z][a-zA-Z0-9.-]*(?:\.[a-zA-Z0-9.-]+)*)\[((?:\w+=[\w.-]+(?:\|[\w.-]+)*)(?:,[\w]+=[\w.-]+(?:\|[\w.-]+)*)*)\]$/
/^([a-zA-Z][a-zA-Z0-9.-]*(?:\.[a-zA-Z0-9.-]+)*)(?:\[((?:\w+=[\w.-]+(?:\|[\w.-]+)*)(?:,[\w]+=[\w.-]+(?:\|[\w.-]+)*)*)\])?$/
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The regex change makes the options bracket optional for configurable scopes, which allows scopes like 'record.read' and 'record.notify' without event type specifications. However, the stringifyScope function at line 488 always adds brackets even for empty options, resulting in 'record.read[]'. This inconsistency could cause issues when comparing or matching scopes. Consider either: 1) ensuring stringifyScope handles scopes without options correctly by omitting brackets when options are empty, or 2) requiring event specifications for these scopes and keeping the brackets mandatory.

Copilot uses AI. Check for mistakes.
Comment on lines +299 to +307
options: z
.object({
event: z
.array(z.string())
.describe('Event type, e.g. birth, death')
.optional()
})
.optional()
.default({})
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The RecordScope schema now allows optional event specifications (lines 301-304 and 305-307), which enables scopes like 'record.read' and 'record.create' without event types. This means CITIZEN_PORTAL could potentially access all event types, not just those it should be authorized for. Consider whether this is the intended behavior. If scopes without event specifications should grant access to all event types, this should be explicitly documented and validated. Otherwise, event specifications should remain required for these scopes.

Suggested change
options: z
.object({
event: z
.array(z.string())
.describe('Event type, e.g. birth, death')
.optional()
})
.optional()
.default({})
options: z.object({
event: z
.array(z.string())
.nonempty()
.describe('Event type, e.g. birth, death')
})

Copilot uses AI. Check for mistakes.
WEBHOOK: [SCOPES.WEBHOOK],
REINDEX: [SCOPES.RECORD_REINDEX]
REINDEX: [SCOPES.RECORD_REINDEX],
CITIZEN_PORTAL: ['record.read', 'record.create', 'record.notify']
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The scopes for CITIZEN_PORTAL are defined as plain strings without event type specifications: 'record.read', 'record.create', 'record.notify'. This grants unrestricted access to all event types. For a citizen portal integration, access should typically be scoped to specific event types (e.g., 'record.read[event=birth|death]'). Consider whether this broad access is intentional and secure. If citizens should only access certain types of records, the scopes should be more restrictive.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔒 Keep e2e Don't delete E2E environment after testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants