Add initial package and configuration files for FHIR info Gateway #304
Conversation
WalkthroughThis update enhances the FHIR Info Gateway by integrating the Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review as they are similar to previous changes (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
a63ddec
to
8e2c200
Compare
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
a821ec4
to
541a022
Compare
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
541a022
to
b0a333c
Compare
THe mpi mediator has been modified and the returned response has a different structure
This also adds bootstrapper node placement. Has to be on the leader where the commands are run as we run a command in the bootstrapper docker container
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- config.yaml (3 hunks)
- fhir-info-gateway/swarm.sh (1 hunks)
- interoperability-layer-openhim/docker-compose.yml (1 hunks)
Additional comments not posted (12)
fhir-info-gateway/swarm.sh (5)
24-28: LGTM!The
readonlydeclarations ensure immutability of variables.
31-35: LGTM!The
sourcecommands correctly import utility functions and logging capabilities.
37-51: LGTM!The
initialize_packagefunction correctly initializes the package in DEV or PROD mode and handles errors during deployment.
54-56: LGTM!The
destroy_packagefunction correctly destroys the Docker stack.
58-79: LGTM!The
mainfunction correctly handles various actions and includes appropriate logging and error handling.config.yaml (3)
30-30: LGTM!The addition of the
fhir-info-gatewaypackage expands the system's capabilities.
52-52: LGTM!The addition of the
fhir-info-gatewaypackage to thecdr-dwprofile integrates it into this operational context.
70-70: LGTM!The addition of the
fhir-info-gatewaypackage to thecdrprofile integrates it into this operational context.interoperability-layer-openhim/docker-compose.yml (4)
28-28: LGTM!The
authentication_enableJWTAuthenticationvariable correctly enables JWT authentication for the service.
29-29: LGTM!The
authentication_jwt_jwksUrivariable correctly specifies the JWKS URI for JWT authentication.
30-30: LGTM!The
authentication_jwt_algorithmsvariable correctly specifies the algorithm for signing JWTs.
31-31: LGTM!The
authentication_jwt_issuervariable correctly specifies the issuer URL for JWTs.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ration-files-for-FHIR-info-Gateway
…ration-files-for-FHIR-info-Gateway
…ration-files-for-FHIR-info-Gateway
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- interoperability-layer-openhim/docker-compose.yml (1 hunks)
Additional comments not posted (4)
interoperability-layer-openhim/docker-compose.yml (4)
28-28: Verify JWT authentication configuration.Ensure that enabling JWT authentication aligns with your security strategy and that all necessary configurations, such as token validation and error handling, are correctly implemented.
31-31: Verify JWT issuer configuration.Ensure that the issuer URL is correct and matches the issuer claim in the JWTs, as this is crucial for validating the token's origin.
Verification successful
JWT Issuer Configuration Verified
The issuer URL
${KC_FRONTEND_URL}/realms/${KC_REALM_NAME}is consistently set to "http://localhost:9088/realms/platform-realm" across the codebase, matching the expected issuer claim for JWTs. No issues found with the configuration.
KC_FRONTEND_URLis set to "http://localhost:9088".KC_REALM_NAMEis set to "platform-realm".Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the issuer URL configuration for JWTs. # Test: Search for issuer configurations in the codebase. Expect: Correct issuer URL matching the JWTs. rg --type yaml 'authentication_jwt_issuer'Length of output: 169
Script:
#!/bin/bash # Search for the definition or usage of KC_FRONTEND_URL in the codebase. rg 'KC_FRONTEND_URL' # Search for the definition or usage of KC_REALM_NAME in the codebase. rg 'KC_REALM_NAME'Length of output: 5740
30-30: Verify JWT algorithm usage.Ensure that the RS256 algorithm is used consistently for signing JWTs and aligns with your security requirements.
29-29: Verify JWKS URI accessibility.Ensure that the JWKS URI is correct and accessible, as it is essential for validating JWTs. This URI should point to a valid JWKS endpoint.
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
aa68f9c
to
2b94455
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- fhir-info-gateway/docker-compose-smart_keycloak.yml (1 hunks)
- fhir-info-gateway/keycloak-config.json (1 hunks)
- fhir-info-gateway/package-metadata.json (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- fhir-info-gateway/keycloak-config.json
- fhir-info-gateway/package-metadata.json
Additional comments not posted (5)
fhir-info-gateway/docker-compose-smart_keycloak.yml (5)
1-1: Specify a more recent Docker Compose version if needed.The file uses version "3.9". Ensure this version is appropriate for your use case and consider using a more recent version if new features are needed.
4-5: Ensure the image version is appropriate.The
smart-configservice uses thejembi/keycloak-config:v0.0.1image. Verify that this version is stable and meets your requirements.
8-12: Verify environment variable values.Ensure that the environment variables
KEYCLOAK_BASE_URL,KEYCLOAK_USER,KEYCLOAK_PASSWORD, andKEYCLOAK_REALMare correctly set in your environment to avoid runtime errors.
13-13: Check the configuration file path.The command specifies a configuration file path
config/backend-services-config.json. Ensure this path is correct and the file exists in the expected location.
14-17: Ensure external network availability.The
keycloaknetwork is defined as external with the namekeycloak_public. Verify that this network is available and correctly configured in your Docker environment.
|
@rcrichton I have updated the image with sample v2 client scopes. You should be able to test now. |
|
I think we also need to provide some default users, groups and roles in keycloak so it's easier to get this working. We could have a read only role, etc with some default users assigned to those. Also, we expect these 'users' to actually be entire EMR system so i think they need to be created as clients in KC with service accounts enabled. See my arch notes on the clickup task for how to do this. I think we should have all this preconfigured to make it easier on the end user. |
…ration-files-for-FHIR-info-Gateway
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
fhir-info-gateway/importer/keycloak-config.json (1)
217-225: Consider using a generic or placeholder email address for the default user.The default user configuration looks good overall. However, the email address "fhir@jembi.org" might not be a valid or appropriate email for a default user. Consider using a more generic or placeholder email address to avoid potential confusion or misuse.
Apply this diff to update the email address:
- "email": "fhir@jembi.org", + "email": "fhir-user@example.com",fhir-info-gateway/importer/update-keycloak-config.js (1)
71-119: LGTM with a suggestion!The
getOrCreateClientfunction is implemented correctly and follows best practices:
- It uses the
axioslibrary to make the HTTP requests.- The function constructs the clients endpoint URL and sets the necessary headers, including the admin token for authentication.
- It checks if the client exists by searching for it in the response data of the GET request.
- If the client exists, it updates the client using a PUT request with the provided client configuration.
- If the client doesn't exist, it creates a new client using a POST request with the provided client configuration.
- The function logs the result of the operation (updated or created client).
- Error handling is implemented to catch and log any errors that occur during the requests.
Suggestion:
Consider uncommenting thethrow error;statement on line 117 to propagate the error and handle it at a higher level instead of silently suppressing it.Uncomment the following line to throw the error:
- //throw error; + throw error;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- fhir-info-gateway/importer/keycloak-config.json (1 hunks)
- fhir-info-gateway/importer/update-keycloak-config.js (1 hunks)
Additional comments not posted (8)
fhir-info-gateway/importer/keycloak-config.json (3)
2-192: LGTM!The
clientScopesconfiguration is well-structured and covers the necessary FHIR resources. The roles and access levels are appropriately mapped to each scope, and the audience mapper is correctly configured.
195-212: LGTM!The
clientconfiguration for the EMR user is correctly set up as a confidential client with service accounts enabled. The necessary OAuth2 and OpenID Connect settings are appropriately configured.
213-215: Clarify the purpose and usage of thefhirUsergroup.The
fhirUsergroup is defined but empty. Please provide more information on how this group is intended to be used or populated. Consider adding a description or mapping the group to specific roles or scopes if applicable.fhir-info-gateway/importer/update-keycloak-config.js (5)
15-46: LGTM!The
getAdminTokenfunction is implemented correctly and follows best practices:
- It uses the
axioslibrary to make the HTTP request.- The function constructs the token endpoint URL and sets the necessary headers and request body parameters.
- It extracts the access token from the response data and returns it.
- Error handling is implemented to catch and log any errors that occur during the request.
48-69: LGTM!The
getRoleByNamefunction is implemented correctly and follows best practices:
- It uses the
axioslibrary to make the HTTP request.- The function constructs the roles endpoint URL and sets the necessary headers, including the admin token for authentication.
- It searches for the role by name in the response data and returns the role ID if found, otherwise it returns null.
- Error handling is implemented to catch and log any errors that occur during the request.
121-404: LGTM!The
processKeycloakPayloadfunction is implemented correctly and follows best practices:
- It destructures the necessary properties from the payload.
- The function validates the presence of the
clientScopesproperty in the payload.- It iterates over each client scope in the payload and performs the necessary operations:
- Creates or updates the role associated with the client scope.
- Creates or updates the client scope itself.
- Maps the created role to the client scope.
- The function creates or updates the service-account user and adds it to the specified group.
- It retrieves the unique roles from the payload using the
getUniqueRolesArrayfunction and maps them to the group.- The function logs the result of each operation for informational purposes.
- Error handling is implemented to catch and log any errors, and the errors are thrown for further handling.
406-419: LGTM!The
getUniqueRolesArrayfunction is implemented correctly and follows best practices:
- It creates a new Set to store the unique roles, ensuring that duplicate roles are eliminated.
- The function extracts the
clientScopesproperty from the payload.- It iterates over each client scope in the
clientScopesobject and adds the stringified role to the Set if it exists.- After iterating over all client scopes, the function converts the Set to an array using
Array.from().- It then maps over each stringified role in the array and parses it back to an object using
JSON.parse().- The function returns the resulting array of unique roles.
422-474: LGTM!The
mainfunction is implemented correctly and follows best practices:
- It serves as the entry point of the script and orchestrates the execution flow.
- The function is marked as
asyncto allow the use ofawaitfor asynchronous operations.- It retrieves the admin token by calling the
getAdminTokenfunction with the necessary arguments.- The function creates or updates the client by calling the
getOrCreateClientfunction with the client configuration and other required arguments.- It retrieves the unique roles from the payload by calling the
getUniqueRolesArrayfunction.- The function iterates over each unique role and creates or updates it in Keycloak using the
getRoleByNamefunction andaxioslibrary.- It processes the Keycloak payload by calling the
processKeycloakPayloadfunction with the payload and other required arguments.- The function logs a success message if the processing is successful.
- Error handling is implemented using a
try-catchblock to catch and log any errors that occur during the execution.
| "resetPassword": { | ||
| "temporary": false, | ||
| "type": "password", | ||
| "value": "dev_password_only" | ||
| } |
There was a problem hiding this comment.
Strongly advise against using the current password in production.
The resetPassword configuration sets a non-temporary password with the value "dev_password_only". This password is weak and not suitable for production environments. Using such a password poses a significant security risk, as attackers could easily guess or brute-force it, gaining unauthorized access to the system.
Strongly recommend generating a secure, random password for production use. Consider using a password manager or a secure password generation tool to create a strong password with a minimum of 12 characters, including a mix of uppercase and lowercase letters, numbers, and special characters.
Apply this diff to remove the current password:
- "value": "dev_password_only"
+ "value": "<GENERATE_SECURE_PASSWORD>"Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- fhir-info-gateway/importer/docker-compose-smart_keycloak.yml (1 hunks)
- fhir-info-gateway/importer/docker-compose.config.yml (1 hunks)
- fhir-info-gateway/importer/update-keycloak-config.js (1 hunks)
- fhir-info-gateway/swarm.sh (1 hunks)
Additional context used
Biome
fhir-info-gateway/importer/update-keycloak-config.js
[error] 103-103: Can't assign clientResponse because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
Additional comments not posted (17)
fhir-info-gateway/importer/docker-compose-smart_keycloak.yml (3)
1-13: LGTM!The Docker Compose version is set correctly, and the
smart-configservice is properly defined with the necessary configuration.
15-18: LGTM!The
keycloaknetwork is properly defined as an external network, which is appropriate for a shared network in a microservices architecture.
1-18: Great work!The Docker Compose file is well-structured, follows best practices, and includes all necessary configurations for deploying the Keycloak service. The use of environment variables for configuration is a good practice for containerized applications.
fhir-info-gateway/importer/docker-compose.config.yml (4)
3-10: LGTM!The
update-keycloak-configservice definition looks good:
- The choice of the Node.js image version is appropriate for the task.
- The environment variables are sourced from the Docker environment, which is a good practice.
- The command installs the required dependencies and runs the configuration update script.
11-15: Configuration files are mapped correctly.The configuration files are mapped to the correct locations in the service's container. Please ensure that the contents of
update-keycloak-config.jsandkeycloak-config.jsonare correct and align with the Keycloak configuration requirements.
16-19: Deployment configuration is appropriate.The deployment configuration for the
update-keycloak-configservice is appropriate:
- Having one replica is sufficient for this type of service.
- The restart policy ensures that the service does not automatically restart, which is appropriate for a one-time configuration update task.
20-21: Network configuration is correct.The
update-keycloak-configservice is correctly connected to thekeycloak_publicnetwork, which allows it to communicate with the Keycloak server. Please ensure that thekeycloak_publicnetwork is properly defined in the relevant Docker Compose file.Also applies to: 33-36
fhir-info-gateway/swarm.sh (4)
1-8: LGTM!The shebang line and variable declarations look good. The variables are initialized with appropriate default values.
31-35: LGTM!The
import_sourcesfunction looks good. It imports the necessary utility scripts and uses a shellcheck directive to disable a specific warning.
37-58: LGTM!The
initialize_packagefunction looks good. It handles the deployment of the package based on the mode and action. It also deploys a config importer if necessary and removes a specific service after deployment. The error handling and logging are in place.
60-85: LGTM!The
destroy_packagefunction and themainfunction look good. Thedestroy_packagefunction destroys the package using the appropriate utility function. Themainfunction is well-structured and handles different actions by calling the appropriate functions. The logging messages are informative and cover all the cases. The error handling for invalid actions is in place.fhir-info-gateway/importer/update-keycloak-config.js (6)
19-50: LGTM!The function correctly retrieves the admin token from Keycloak and handles errors appropriately.
52-73: LGTM!The function correctly retrieves the role by name from Keycloak and handles errors appropriately.
75-123: LGTM!The function correctly retrieves or creates the client in Keycloak and handles errors appropriately. Just make sure to address the constant variable assignment issue.
Tools
Biome
[error] 103-103: Can't assign clientResponse because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
125-410: LGTM!The function correctly processes the Keycloak payload, handling the creation and mapping of roles, client scopes, and the service account user. The logic appears to be sound, but the function could benefit from some refactoring to improve its structure.
411-424: LGTM!The function correctly retrieves the unique roles from the payload using a Set to ensure uniqueness. The logic is clear and concise.
426-485: LGTM!The
mainfunction correctly orchestrates the Keycloak configuration process by retrieving the admin token, managing the client, processing unique roles, and handling the payload processing. The error handling is appropriate, and the function provides a clear entry point for the script.
| console.log(`Updated user: ${defaultUser.username}`); | ||
| } else { | ||
| // User does not exist, create a new one | ||
| userResponse = await axios.post( | ||
| `${keycloakBaseUrl}/admin/realms/${realm}/users`, | ||
| defaultUser, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${adminToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| } | ||
| ); | ||
| console.log(`Created user: ${defaultUser.username}`); | ||
| } | ||
|
|
||
| const createdUser = userResponse.data; | ||
| console.log("here", user); | ||
| // Reset the password | ||
| const newPass = await axios.put( | ||
| `${keycloakBaseUrl}/admin/realms/${realm}/users/${ | ||
| userResponse.id ? userResponse.id : user.id | ||
| }/reset-password`, | ||
| resetPassword, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${adminToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| } | ||
| ); | ||
| console.log(`Reset password for user ${createdUser}`, newPass.data); | ||
| // Step 5: Add service-account user to the group | ||
| await axios.put( | ||
| `${keycloakBaseUrl}/admin/realms/${realm}/users/${ | ||
| createdUser.id ? createdUser.id : user.id | ||
| }/groups/${groupId}`, | ||
| {}, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${adminToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| } | ||
| ); | ||
| console.log(`Added ${createdUser} to group ${createdgroupResponse}`); | ||
| const uniqueRolesArray = await getUniqueRolesArray(payload); | ||
| for (const role of uniqueRolesArray) { | ||
| const roleID = await getRoleByName( | ||
| role.name, | ||
| keycloakBaseUrl, | ||
| realm, | ||
| adminToken | ||
| ); | ||
| console.log(roleID); | ||
| const roleMapping = await axios.post( | ||
| `${keycloakBaseUrl}/admin/realms/${realm}/groups/${groupId}/role-mappings/realm`, | ||
| [ | ||
| { | ||
| id: roleID, | ||
| clientRole: false, | ||
| composite: false, | ||
| containerId: realm, | ||
| name: role.name, | ||
| description: role.description, | ||
| }, | ||
| ], | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${adminToken}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| } | ||
| ); | ||
| console.log(`Added role mapping to group ${roleMapping}`, role); | ||
| } | ||
| } catch (error) { | ||
| console.error( | ||
| "Error creating or updating user:", | ||
| error.response ? error.response.data : error.message | ||
| ); | ||
| throw error; | ||
| } | ||
|
|
||
| // Step 6: Add role mapping to the group | ||
| // Extract unique roles | ||
|
|
||
| // const rolesToBeMapped = []; | ||
| // uniqueRolesArray.forEach(async (role) => { | ||
| // const rolesToBeMappedPayload = await getRoleByName( | ||
| // role.name, | ||
| // keycloakBaseUrl, | ||
| // realm, | ||
| // adminToken | ||
| // ); | ||
| // console.log(rolesToBeMappedPayload); | ||
| // rolesToBeMapped.push(rolesToBeMappedPayload); | ||
| // }); | ||
| // console.log("sdsdsd", rolesToBeMapped); | ||
| } |
There was a problem hiding this comment.
Consider refactoring the function to improve readability and maintainability.
The processKeycloakPayload function is quite large and complex, handling multiple responsibilities such as processing client scopes, managing roles, updating the service account user, and mapping roles to groups.
To improve readability and maintainability, consider breaking down the function into smaller, focused functions that handle specific tasks. This will make the code easier to understand, test, and maintain.
| console.log(`Updated client: ${client.clientId}`); | ||
| } else { | ||
| // Client does not exist, create a new one | ||
| clientResponse = await axios.post( |
There was a problem hiding this comment.
Fix the assignment to a constant variable.
The static analysis tool correctly identified an issue where the clientResponse variable, declared with const, is being reassigned a new value.
To fix this, change the variable declaration to let:
- clientResponse = await axios.post(
+ let clientResponse = await axios.post(Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 103-103: Can't assign clientResponse because it's a constant
This is where the variable is defined as constant
Unsafe fix: Replace const with let if you assign it to a new value.
(lint/correctness/noConstAssign)
There was a problem hiding this comment.
it is defined in L77
There was a problem hiding this comment.
@drizzentic, thank you for pointing that out. I see that clientResponse is defined at line 77. My previous comment was incorrect, and I apologize for the confusion.
Learnings added
Learnt from: drizzentic
PR: jembi/platform#304
File: fhir-info-gateway/importer/update-keycloak-config.js:103-103
Timestamp: 2024-09-18T06:50:31.835Z
Learning: When reviewing variable assignments, ensure to verify existing declarations to avoid incorrect suggestions about constant reassignments.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Hi @rcrichton, |
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
5579cfb
to
cb22626
Compare
drizzentic
force-pushed
the
CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway
branch
from
cb22626
to
f965914
Compare
…ration-files-for-FHIR-info-Gateway
Summary by CodeRabbit
New Features
fhir-info-gateway, acting as an intermediary for FHIR access.Enhancements
fhir-info-gatewaypackage to configuration profiles, improving integration across the system.Documentation