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

Hurl scripts refactor #1522

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from
Draft

Hurl scripts refactor #1522

wants to merge 36 commits into from

Conversation

basiliskus
Copy link
Contributor

@basiliskus basiliskus commented Nov 1, 2024

Add a PR title

Describe what changed in this PR at a high level.

Issue

#1566

Checklist

  • I have added tests to cover my changes
  • I have added logging where useful (with appropriate log level)
  • I have added JavaDocs where required
  • I have updated the documentation accordingly

Note: You may remove items that are not applicable

…me of the envars. Also working of having absolute paths so the scripts could run from anywhere
…ov/trusted-intermediary into story/1488/rs-setup-scripts-updates
- Use new setup-rs.sh script
- Add alternate ways to build and run RS
- Fixed RS docs URL
- Clean up and simplify
Copy link

github-actions bot commented Nov 1, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling
The script should handle potential errors when parsing the sender string format more robustly.

JWT Generation
The JWT generation function has been modified to use sender details directly, which may not align with the intended use of JWT claims.

@@ -41,53 +39,54 @@ parse_arguments() {
hurl_file_path="$CDCTI_HOME/scripts/hurl/rs/$1.hurl"
shift # Remove endpoint name from args

while getopts ':f:r:t:e:c:s:k:i:v' opt; do
while getopts ':f:r:e:k:i:s:t:v' opt; do
Copy link

Choose a reason for hiding this comment

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

Consider adding a default case in the option parsing switch statement to handle unexpected options more gracefully. This can improve the script's robustness and provide clearer error messages to the user. [important]

Copy link

github-actions bot commented Nov 1, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Add error handling after parsing the sender string to ensure data integrity

Add error handling for the parse_sender_string function to manage cases where the
sender string does not meet the expected format, preventing script continuation with
invalid data.

scripts/rs.sh [56]

-parse_sender_string "$sender" sender_org sender_name
+if ! parse_sender_string "$sender" sender_org sender_name; then
+    fail "Parsing sender string failed"
+fi
Suggestion importance[1-10]: 8

Why: This suggestion is highly relevant and correct. It addresses a potential issue where the script could continue executing with invalid data if the sender string does not meet the expected format. Adding error handling after parsing the sender string is a crucial improvement for maintaining data integrity and script robustness.

8
Verify the existence of the private_key file before generating a JWT token

Check if the private_key file exists before attempting to generate a JWT token to
prevent failures during the token generation process.

scripts/rs.sh [81]

+[ ! -f "$private_key" ] && fail "Private key file not found: $private_key"
 jwt_token=$(generate_jwt "$sender_org.$sender_name" "$host" "$private_key") || fail "Failed to generate JWT token"
Suggestion importance[1-10]: 7

Why: This suggestion is valid and improves the script by ensuring that the private_key file exists before attempting to generate a JWT token. This check prevents potential runtime errors during the token generation process, enhancing the script's reliability and error handling.

7
Possible bug
Add a check for the existence of the private key file before generating a JWT token

Ensure that the private key file check is performed before attempting to generate a
JWT token to avoid runtime errors if the key file is missing.

scripts/ti.sh [78]

+[ ! -f "$private_key" ] && fail "Sender private key file not found: $private_key"
 jwt_token=$(generate_jwt "$sender" "$host" "$private_key") || fail "Failed to generate JWT token"
Suggestion importance[1-10]: 8

Why: This suggestion correctly identifies a potential runtime error if the private key file is missing when generating a JWT token. Adding a check before this operation enhances the robustness of the script.

8
Ensure the file path specified in fpath exists before executing the hurl command

Add a check to ensure that the fpath variable is set and points to a valid file
before attempting to use it in the hurl command, to prevent errors related to file
paths.

scripts/ti.sh [80-87]

+[ ! -f "$fpath" ] && fail "File path specified does not exist: $fpath"
 hurl \
     --variable "fpath=$fpath" \
     --file-root "$root" \
     --variable "url=$url" \
     --variable "sender=$sender" \
     --variable "jwt=$jwt_token" \
     ${submission_id:-} \
     ${verbose:-}
Suggestion importance[1-10]: 8

Why: This suggestion is highly relevant as it prevents the hurl command from failing due to a non-existent file path, thus improving the script's error handling and user experience.

8
Validate that the sender variable is not empty before generating a JWT token

Add a validation to ensure that the sender variable is not empty before using it to
generate a JWT token, as an empty sender could lead to invalid JWT generation.

scripts/ti.sh [78]

+[ -z "$sender" ] && fail "Sender ID is required to generate JWT token"
 jwt_token=$(generate_jwt "$sender" "$host" "$private_key") || fail "Failed to generate JWT token"
Suggestion importance[1-10]: 7

Why: Validating the sender variable before generating a JWT token is crucial to ensure the token's validity. This suggestion effectively prevents potential issues with JWT generation.

7
Validate that the root variable points to a valid directory before using it in the hurl command

Ensure that the root variable is validated to be a directory before using it as a
file root in the hurl command to avoid runtime errors.

scripts/ti.sh [80-87]

+[ ! -d "$root" ] && fail "Root path specified is not a directory: $root"
 hurl \
     --variable "fpath=$fpath" \
     --file-root "$root" \
     --variable "url=$url" \
     --variable "sender=$sender" \
     --variable "jwt=$jwt_token" \
     ${submission_id:-} \
     ${verbose:-}
Suggestion importance[1-10]: 7

Why: Ensuring that the root variable points to a valid directory before using it in the hurl command is a good practice to avoid runtime errors, making this a valuable suggestion.

7
Possible issue
Verify and ensure the correct definition of the 'scope' variable

Verify that the new 'scope' variable '{{sender}}' is correctly defined and populated
across all environments to ensure the script functions as expected.

scripts/hurl/ti/auth.hurl [4]

-scope: {{sender}}
+scope: {{correctly-defined-sender-variable}}
Suggestion importance[1-10]: 1

Why: The suggestion is valid but only asks to verify the correct definition of the 'scope' variable without providing a specific improvement or identifying an issue in the PR code.

1
Security
Confirm that the 'scope' parameter change maintains correct access controls

Confirm that the change from '{{client}}' to '{{sender}}' in the 'scope' parameter
does not affect the intended permissions or access controls.

scripts/hurl/ti/metadata.hurl [4]

-scope: {{sender}}
+scope: {{verified-sender-or-client}}
Suggestion importance[1-10]: 1

Why: This suggestion is valid as it raises a potential security concern regarding the change of the 'scope' parameter, but it does not provide a concrete solution or identify a specific issue in the PR code.

1

Base automatically changed from story/1488/rs-setup-scripts-updates to main November 1, 2024 20:47
Copy link

sonarcloud bot commented Nov 1, 2024

@tjohnson7021
Copy link
Contributor

I am still tweaking my setup for the RS side of these scripts, but the TI-oriented scripts are working for me as of now.

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.

2 participants