Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial package and configuration files for FHIR info Gateway #304

Open
wants to merge 56 commits into
base: main
Choose a base branch
from

Conversation

drizzentic
Copy link
Collaborator

@drizzentic drizzentic commented May 20, 2024

Summary by CodeRabbit

  • New Features

    • Introduced Docker Compose configurations to support Keycloak for enhanced security and authentication.
    • Added configuration for the fhir-info-gateway, acting as an intermediary for FHIR access.
    • Implemented a comprehensive Keycloak configuration for secure user management.
    • Added a new service for automating Keycloak configuration updates.
  • Enhancements

    • Expanded system capabilities by updating profiles and service configurations with environment variables, network settings, and resource limits for better performance and reliability.
    • Added the fhir-info-gateway package to configuration profiles, improving integration across the system.
  • Documentation

    • Defined metadata for the FHIR info gateway, providing detailed package information and dependencies.

Copy link
Contributor

coderabbitai bot commented May 20, 2024

Walkthrough

This update enhances the FHIR Info Gateway by integrating the fhir-info-gateway package into the configuration, expanding Docker Compose setups for Keycloak, and detailing comprehensive Keycloak configurations. These changes improve service deployment, enhance interoperability, and facilitate secure access management within healthcare systems.

Changes

File Path Change Summary
config.yaml Included fhir-info-gateway in the packages list and expanded profiles to enhance system capabilities.
fhir-info-gateway/docker-compose-*.yml Added configurations for a smart-config service using jembi/keycloak-config, detailing environment variables.
fhir-info-gateway/docker-compose.dev.yml Specified version and port mapping from 8080 to 8880 for the development version of fhir-info-gateway.
fhir-info-gateway/docker-compose.yml Updated service configurations, including version changes, environment variables, network settings, and resource limits.
fhir-info-gateway/keycloak-config.json Introduced comprehensive Keycloak settings, including realms, client scopes, and user management configurations.
fhir-info-gateway/package-metadata.json Defined metadata such as name, description, version, dependencies, and environment variables for the gateway.
fhir-info-gateway/importer/keycloak-config.json Newly added file for Keycloak client configuration tailored for EMR users, including scopes and roles.
fhir-info-gateway/importer/update-keycloak-config.js Introduced script to automate Keycloak configuration management for roles, clients, and users.
fhir-info-gateway/importer/docker-compose.config.yml Added a service to automate Keycloak configuration updates using a Node.js environment.
fhir-info-gateway/swarm.sh Introduced script for managing the deployment of the fhir-info-gateway application using Docker Swarm.

Possibly related PRs

Suggested reviewers

  • Reviewers have not been specified.

Poem

In a realm where health data flows,
The FHIR gateway's brilliance grows.
With Keycloak's watch and ports anew,
Our services shine, reliable and true.
With each config and script we weave,
A tapestry of care we believe!
🌟👩‍⚕️🐇🚀


Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 5579cfb and abfd851.

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)
Files skipped from review as they are similar to previous changes (4)
  • fhir-info-gateway/importer/docker-compose-smart_keycloak.yml
  • fhir-info-gateway/importer/docker-compose.config.yml
  • fhir-info-gateway/importer/update-keycloak-config.js
  • fhir-info-gateway/swarm.sh

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai generate interesting stats about this repository and render them as a table.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@rcrichton
Copy link
Member

@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from a63ddec to 8e2c200 Compare May 20, 2024 14:54
@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from a821ec4 to 541a022 Compare May 29, 2024 16:10
@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from 541a022 to b0a333c Compare May 29, 2024 16:28
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ac4d67f and a4b770f.

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 readonly declarations ensure immutability of variables.


31-35: LGTM!

The source commands correctly import utility functions and logging capabilities.


37-51: LGTM!

The initialize_package function correctly initializes the package in DEV or PROD mode and handles errors during deployment.


54-56: LGTM!

The destroy_package function correctly destroys the Docker stack.


58-79: LGTM!

The main function correctly handles various actions and includes appropriate logging and error handling.

config.yaml (3)

30-30: LGTM!

The addition of the fhir-info-gateway package expands the system's capabilities.


52-52: LGTM!

The addition of the fhir-info-gateway package to the cdr-dw profile integrates it into this operational context.


70-70: LGTM!

The addition of the fhir-info-gateway package to the cdr profile integrates it into this operational context.

interoperability-layer-openhim/docker-compose.yml (4)

28-28: LGTM!

The authentication_enableJWTAuthentication variable correctly enables JWT authentication for the service.


29-29: LGTM!

The authentication_jwt_jwksUri variable correctly specifies the JWKS URI for JWT authentication.


30-30: LGTM!

The authentication_jwt_algorithms variable correctly specifies the algorithm for signing JWTs.


31-31: LGTM!

The authentication_jwt_issuer variable correctly specifies the issuer URL for JWTs.

fhir-info-gateway/swarm.sh Show resolved Hide resolved
fhir-info-gateway/swarm.sh Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a4b770f and e4e6edf.

Files selected for processing (1)
  • config.yaml (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • config.yaml

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e4e6edf and 138558a.

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.

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.

Copy link
Contributor

@brett-onions brett-onions left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rcrichton rcrichton left a comment

Choose a reason for hiding this comment

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

Looking good. I'm trying to test this but running inot an issue with the docker hub tag not existing.

fhir-info-gateway/package-metadata.json Outdated Show resolved Hide resolved
fhir-info-gateway/keycloak-config.json Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 138558a and aa68f9c.

Files selected for processing (2)
  • fhir-info-gateway/keycloak-config.json (1 hunks)
  • fhir-info-gateway/package-metadata.json (1 hunks)

fhir-info-gateway/package-metadata.json Show resolved Hide resolved
fhir-info-gateway/keycloak-config.json Outdated Show resolved Hide resolved
@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from aa68f9c to 2b94455 Compare August 19, 2024 13:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between aa68f9c and 2b94455.

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-config service uses the jembi/keycloak-config:v0.0.1 image. 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, and KEYCLOAK_REALM are 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 keycloak network is defined as external with the name keycloak_public. Verify that this network is available and correctly configured in your Docker environment.

@drizzentic
Copy link
Collaborator Author

@rcrichton I have updated the image with sample v2 client scopes. You should be able to test now.

@rcrichton
Copy link
Member

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 "[email protected]" 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": "[email protected]",
+    "email": "[email protected]",
fhir-info-gateway/importer/update-keycloak-config.js (1)

71-119: LGTM with a suggestion!

The getOrCreateClient function is implemented correctly and follows best practices:

  • It uses the axios library 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 the throw 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

Commits

Files that changed from the base of the PR and between 9f4f8ee and bb0b7ef.

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 clientScopes configuration 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 client configuration 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 the fhirUser group.

The fhirUser group 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 getAdminToken function is implemented correctly and follows best practices:

  • It uses the axios library 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 getRoleByName function is implemented correctly and follows best practices:

  • It uses the axios library 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 processKeycloakPayload function is implemented correctly and follows best practices:

  • It destructures the necessary properties from the payload.
  • The function validates the presence of the clientScopes property 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 getUniqueRolesArray function 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 getUniqueRolesArray function 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 clientScopes property from the payload.
  • It iterates over each client scope in the clientScopes object 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 main function 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 async to allow the use of await for asynchronous operations.
  • It retrieves the admin token by calling the getAdminToken function with the necessary arguments.
  • The function creates or updates the client by calling the getOrCreateClient function with the client configuration and other required arguments.
  • It retrieves the unique roles from the payload by calling the getUniqueRolesArray function.
  • The function iterates over each unique role and creates or updates it in Keycloak using the getRoleByName function and axios library.
  • It processes the Keycloak payload by calling the processKeycloakPayload function 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-catch block to catch and log any errors that occur during the execution.

Comment on lines +226 to +230
"resetPassword": {
"temporary": false,
"type": "password",
"value": "dev_password_only"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bb0b7ef and 5579cfb.

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-config service is properly defined with the necessary configuration.


15-18: LGTM!

The keycloak network 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-config service 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.js and keycloak-config.json are correct and align with the Keycloak configuration requirements.


16-19: Deployment configuration is appropriate.

The deployment configuration for the update-keycloak-config service 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-config service is correctly connected to the keycloak_public network, which allows it to communicate with the Keycloak server. Please ensure that the keycloak_public network 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_sources function looks good. It imports the necessary utility scripts and uses a shellcheck directive to disable a specific warning.


37-58: LGTM!

The initialize_package function 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_package function and the main function look good. The destroy_package function destroys the package using the appropriate utility function. The main function 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 main function 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.

Comment on lines 125 to 410
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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(
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 17, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is defined in L77

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

@drizzentic
Copy link
Collaborator Author

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.

Hi @rcrichton,
I have added a script to update keycloak with the defaults mentioned.

@drizzentic drizzentic marked this pull request as draft September 18, 2024 06:42
@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from 5579cfb to cb22626 Compare September 18, 2024 06:43
@drizzentic drizzentic force-pushed the CU-86byv0cyf_Add-initial-package-and-configuration-files-for-FHIR-info-Gateway branch from cb22626 to f965914 Compare September 18, 2024 06:44
@drizzentic drizzentic marked this pull request as ready for review September 18, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants