[GPCAPIM-310] Add user ID and user role ID headers in proxy#157
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR updates the API proxy configuration to propagate CIS2 identity information, and also refactors gateway-api error/request logging.
Changes:
- Add
target-identityentries forcis2-uuidandcis2-uridin the proxygen APIM template. - Remove
AbstractCDGError.log()(print-based traceback logging) and centralize error logging in the Flask app. - Add request-received logging and structured error logging via
app.loggeringateway_api/app.py.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
proxygen/x-nhsd-apim.template.yaml |
Configures proxy target identity forwarding for CIS2 UUID and URID. |
gateway-api/src/gateway_api/common/error.py |
Removes print/traceback-based logging method from the base error type. |
gateway-api/src/gateway_api/app.py |
Introduces Flask logger usage and logs requests/errors around the structured record endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def log_request_received(request: Request) -> None: | ||
| log_details = { | ||
| "description": "Received request", | ||
| "method": request.method, |
There was a problem hiding this comment.
The PR title is focused on adding CIS2 user/user-role identity headers in the proxy config, but this change set also introduces request/error logging behavior changes in gateway_api/app.py. Please either update the PR description to explain the additional scope and rationale, or split the logging refactor into a separate PR to keep changes reviewable and traceable.
…dd-user-details-to-request-in-proxy
… decide on role that the user is fulfilling at point of calling API
Vox-Ben
left a comment
Vox-Ben
left a comment
There was a problem hiding this comment.
- If I understand this right, we're currently accepting the User ID and Role ID parameters but not actually doing anything with them? Is that what we wanted?
- What's the logging for? If it's just there for debug is it better to take it out again for now until we've worked out what we're doing with creating a logging system properly?
- If we're not doing that because it's needed, should it have unit tests?
Correct. GPCAPIM-309 GPCAPIM-311 pick this header up.
Debugging. But given how lightweight it is, and there will definitely be work coming to redo the logging, I'm happy to leave it there.
For the logging? Given it is not needed, but helpful for development debugging, and there is no conditional logic or functionality. I'm inclined not to write unit tests for it. |
👍 |
|
|
Deployment Complete
|
davidhamill1-nhs
deleted the
feature/GPCAPIM-310-add-user-details-to-request-in-proxy
branch
Description
Upon receiving a request in the proxy, Apigee, add the user details, user ID and user role ID, to the request before passing it on to the backend.
Context
In order to correctly audit requests, Gateway API and the JWT sent to the provider, requires accurate and safe identifiers for the logged-in users. Proxygen/API Platform allow for this through User ID and User Role ID headers added within the proxy layer, taken from the auth flow within the platform.
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.