feat: refactor agent endpoint for token #1375
Conversation
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Sahil Kamble <[email protected]>
Signed-off-by: Sahil Kamble <[email protected]>
Signed-off-by: Sahil Kamble <[email protected]>
Signed-off-by: Sahil Kamble <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API_Gateway
participant AgentService
participant CommonService
participant AgentProvisioning
participant ShellScript
User->>API_Gateway: Request agent provisioning (with apiKey)
API_Gateway->>AgentService: createTenant / walletProvision (payload includes apiKey)
AgentService->>AgentProvisioning: Calls provisioning script with apiKey
AgentProvisioning->>ShellScript: Executes start_agent.sh/start_agent_ecs.sh with apiKey
ShellScript->>ShellScript: Writes config with apiKey, starts agent
ShellScript->>AgentProvisioning: Returns agent info (including apiKey)
AgentProvisioning->>AgentService: Returns provisioning result
AgentService->>CommonService: getBaseAgentToken / getTenantWalletToken as needed
CommonService->>AgentService: Returns token
AgentService->>API_Gateway: Returns agent details (with apiKey/token)
API_Gateway->>User: Responds with agent info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| walletPassword, | ||
| walletStorageHost, | ||
| walletStoragePassword, | ||
| walletStoragePort, |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix this problem, we should avoid constructing a shell command string and passing it to exec. Instead, we should use execFile (or execFileSync/spawn), which allows us to specify the command and its arguments as an array, so that each argument is passed as-is to the underlying process without shell interpretation. This prevents shell injection and issues with spaces or special characters in arguments.
Detailed steps:
- Parse the script path and all arguments as separate elements.
- Build an array of arguments, ensuring each value is passed as a separate argument.
- Replace
exec(walletProvision, ...)withexecFile(scriptPath, args, ...). - Import
execFilefromchild_process. - Update the code to extract the script path and arguments from the original string.
- No change to the rest of the logic.
Files/regions to change:
- In
apps/agent-provisioning/src/agent-provisioning.service.ts, lines 43–45: refactor the command construction and execution. - Add import for
execFileif not already present.
| @@ -6,3 +6,3 @@ | ||
| import * as fs from 'fs'; | ||
| import { exec } from 'child_process'; | ||
| import { execFile } from 'child_process'; | ||
| dotenv.config(); | ||
| @@ -42,5 +42,30 @@ | ||
| // The wallet provision command is used to invoke a shell script | ||
| const walletProvision = `${process.cwd() + process.env.AFJ_AGENT_SPIN_UP} ${orgId} "${externalIp}" "${walletName}" "${walletPassword}" ${seed} ${webhookEndpoint} ${walletStorageHost} ${walletStoragePort} ${walletStorageUser} ${walletStoragePassword} ${containerName} ${protocol} ${tenant} ${credoImage} "${indyLedger}" ${inboundEndpoint} ${process.env.SCHEMA_FILE_SERVER_URL} ${apiKey} ${process.env.AGENT_HOST} ${process.env.AWS_ACCOUNT_ID} ${process.env.S3_BUCKET_ARN} ${process.env.CLUSTER_NAME} ${process.env.TESKDEFINITION_FAMILY}`; | ||
| const scriptPath = process.cwd() + process.env.AFJ_AGENT_SPIN_UP; | ||
| const args = [ | ||
| orgId, | ||
| externalIp, | ||
| walletName, | ||
| walletPassword, | ||
| seed, | ||
| webhookEndpoint, | ||
| walletStorageHost, | ||
| walletStoragePort, | ||
| walletStorageUser, | ||
| walletStoragePassword, | ||
| containerName, | ||
| protocol, | ||
| tenant, | ||
| credoImage, | ||
| indyLedger, | ||
| inboundEndpoint, | ||
| process.env.SCHEMA_FILE_SERVER_URL, | ||
| apiKey, | ||
| process.env.AGENT_HOST, | ||
| process.env.AWS_ACCOUNT_ID, | ||
| process.env.S3_BUCKET_ARN, | ||
| process.env.CLUSTER_NAME, | ||
| process.env.TESKDEFINITION_FAMILY | ||
| ]; | ||
| const spinUpResponse: object = new Promise(async (resolve) => { | ||
| await exec(walletProvision, async (err, stdout, stderr) => { | ||
| await execFile(scriptPath, args, async (err, stdout, stderr) => { | ||
| this.logger.log(`shell script output: ${stdout}`); |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (4)
apps/agent-provisioning/src/agent-provisioning.service.ts (1)
1-1: Consider structured logging instead of console statements.While the ESLint disable is acceptable for a provisioning service that needs console output, consider using the existing Logger instance for consistency with the rest of the application.
apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh (2)
40-40: Consider potential service name conflicts.The change from a dynamic random string to a fixed pattern
${CONTAINER_NAME}-service-testcould lead to conflicts if multiple services are created with the same container name. Ensure that container names are unique in your deployment environment.
197-204: Consider making the AWS region configurable.The CloudWatch logging configuration is correctly structured, but the region is hardcoded to
ap-south-1. Consider using an environment variable like$AWS_REGIONor$AWS_PUBLIC_REGIONfor better flexibility across different deployment regions.- "awslogs-region": "ap-south-1", + "awslogs-region": "$AWS_PUBLIC_REGION",apps/agent-service/src/agent-service.service.ts (1)
1908-1930: Well-implemented tenant token retrieval method.The method has proper parameter validation and error handling. Note the TODO comment on line 1924 about adding the tenant wallet token to the database - this should be tracked for future implementation.
The comment on line 1924 suggests storing the tenant wallet token in the database. Would you like me to create an issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.eslintrc.js(1 hunks)apps/agent-provisioning/AFJ/scripts/start_agent.sh(4 hunks)apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh(5 hunks)apps/agent-provisioning/src/agent-provisioning.service.ts(2 hunks)apps/agent-service/src/agent-service.service.ts(13 hunks)apps/api-gateway/src/agent-service/agent-service.controller.ts(0 hunks)apps/connection/src/connection.service.ts(10 hunks)apps/issuance/src/issuance.service.ts(5 hunks)apps/verification/src/verification.service.ts(7 hunks)libs/common/src/common.constant.ts(5 hunks)libs/common/src/common.service.ts(2 hunks)libs/common/src/common.utils.ts(3 hunks)libs/common/src/response-messages/index.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/api-gateway/src/agent-service/agent-service.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: in the credebl/platform repository, the team prefers to create utilities for validating required env...
Learnt from: tipusinghaw
PR: credebl/platform#1297
File: apps/api-gateway/src/tracer.ts:24-28
Timestamp: 2025-06-24T11:15:29.468Z
Learning: In the credebl/platform repository, the team prefers to create utilities for validating required environment variables rather than implementing inline validation. This approach promotes reusability across different parts of the application.
Applied to files:
apps/agent-provisioning/src/agent-provisioning.service.ts
🧬 Code Graph Analysis (3)
libs/common/src/common.service.ts (1)
libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-505)
apps/issuance/src/issuance.service.ts (1)
libs/common/src/common.utils.ts (1)
getAgentUrl(70-113)
libs/common/src/common.utils.ts (1)
libs/common/src/response-messages/index.ts (1)
ResponseMessages(1-505)
🪛 GitHub Check: CodeQL
apps/agent-provisioning/src/agent-provisioning.service.ts
[warning] 31-31: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
🪛 Shellcheck (0.10.0)
apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh
[warning] 283-283: This is interpreted as a command name ending with '.'. Double check syntax.
(SC2288)
🪛 ESLint
libs/common/src/common.utils.ts
[error] 107-107: Unexpected console statement.
(no-console)
[error] 110-110: Unexpected console statement.
(no-console)
🔇 Additional comments (38)
.eslintrc.js (1)
104-104: Good practice: newline at EOF retainedThe trailing newline keeps editors, formatters, and some POSIX tools happy. No further action required.
libs/common/src/response-messages/index.ts (2)
243-243: LGTM! Well-structured error messages for enhanced agent functionality.The new error messages appropriately support the enhanced agent configuration and token management features. They follow the existing naming conventions and provide clear descriptions for the failure scenarios.
Also applies to: 275-277
499-504: LGTM! Appropriate addition of common error messages.The new
common.errorsection provides necessary error messages for URL and endpoint validation. The messages are clear and follow the established pattern in the file.apps/agent-provisioning/src/agent-provisioning.service.ts (2)
13-13: LGTM! Clean constructor formatting.The simplified constructor format improves readability.
22-44: LGTM! Proper integration of API key parameter.The addition of the
apiKeyparameter and its integration into the shell script execution is implemented correctly and aligns with the enhanced token management approach.The static analysis warning about shell command construction is noted, but this follows the existing pattern in the codebase for agent provisioning.
apps/agent-provisioning/AFJ/scripts/start_agent.sh (3)
22-22: LGTM! Proper API key parameter integration.The addition of the
AGENT_API_KEYparameter as the 18th argument is correctly implemented with appropriate debugging output.Also applies to: 119-123
130-130: LGTM! Proper JSON configuration updates.The quoted filename in the heredoc prevents shell expansion issues, and the
apiKeyfield is correctly added to the agent configuration JSON with proper formatting.Also applies to: 162-164
247-247: LGTM! Updated log parsing for new agent output format.The change from "API Token:" to "API Key:" in the log parsing correctly reflects the updated agent output format.
apps/verification/src/verification.service.ts (3)
30-30: LGTM! Proper import updates for shared utility usage.The removal of unused enum imports and addition of the
getAgentUrlutility import correctly supports the refactoring to use shared URL construction logic.Also applies to: 52-52
228-232: LGTM! Effective refactoring to shared URL utility.The replacement of the internal URL construction with the shared
getAgentUrlutility simplifies the code and centralizes URL handling logic. The use of constants for URL flags improves maintainability.
284-284: LGTM! Consistent refactoring across all URL construction points.All calls to the shared
getAgentUrlutility are consistently implemented with appropriate constant flags. This refactoring successfully centralizes URL construction logic while maintaining the same functionality.Also applies to: 405-405, 483-486, 765-765
libs/common/src/common.service.ts (3)
329-332: LGTM! Minor formatting improvement.The indentation adjustment improves code readability without changing functionality.
351-366: LGTM! Well-implemented base agent token retrieval.The
getBaseAgentTokenmethod is properly structured with:
- Appropriate HTTP POST request to the agent endpoint
- Proper authorization header handling
- Clear error handling with descriptive error messages
- Consistent return pattern
368-382: LGTM! Consistent tenant wallet token retrieval implementation.The
getTenantWalletTokenmethod follows the same solid pattern asgetBaseAgentTokenwith appropriate adaptations for tenant-specific token retrieval:
- Proper tenantId parameter integration in URL
- Consistent error handling approach
- Clear method signature and return type
apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh (2)
156-156: Verify the container image reference change.The change from
${AFJ_IMAGE_URL}to${AFJ_VERSION}suggests the image registry/repository is now implied. Ensure that the container runtime can resolve the image correctly with just the version identifier.
273-332: Verify region usage and log stream namingPlease ensure the following before merging:
Region consistency
Replace any hard-coded region (e.g.ap-southeast-1) with$AWS_PUBLIC_REGIONso all AWS CLI calls target the same region.token=$(aws logs get-log-events \ --log-group-name "$log_group" \ --log-stream-name "$log_stream" \ - --region ap-southeast-1 \ + --region "$AWS_PUBLIC_REGION" \ | grep -o '*** API Key: [^ ]*' \ | cut -d ' ' -f 3 )Log stream format validation
Verify in your AWS account that your CloudWatch log streams follow the assumed naming convention (ecs/<containerName>/<taskId>) under the/ecs/<taskDefinitionFamily>log group. For example:aws logs describe-log-groups \ --log-group-name-prefix "/ecs/${TASK_DEFINITION_FAMILY}" \ --region "$AWS_PUBLIC_REGION"And then check streams:
aws logs describe-log-streams \ --log-group-name "/ecs/${TASK_DEFINITION_FAMILY}" \ --log-stream-name-prefix "ecs/${CONTAINER_NAME}/" \ --region "$AWS_PUBLIC_REGION"Token-pattern robustness
The grep pattern ('*** API Key: [^ ]*') may break if logs change format. Consider usingjqon structured logs or a more flexible regex.apps/issuance/src/issuance.service.ts (5)
65-65: LGTM! Import statement correctly updated.The addition of
getAgentUrlto the imports from the common utils module is consistent with the refactoring to use shared utility functions.
187-187: LGTM! URL construction properly refactored.The replacement of the internal
getAgentUrlmethod with the shared utility function is correctly implemented. The parameters align with the utility's interface.
372-372: LGTM! Consistent URL construction refactoring.The refactoring to use the shared
getAgentUrlutility withCommonConstants.CREATE_OFFER_OUT_OF_BANDis correctly implemented.
685-685: LGTM! Parameterized URL construction correctly implemented.The refactoring correctly uses the shared utility with the
credentialRecordIdparameter, which should replace the placeholder in the URL suffix as intended.
827-827: LGTM! Final URL construction refactoring completed.The last usage of the shared
getAgentUrlutility is correctly implemented, completing the refactoring to centralize URL construction logic.libs/common/src/common.utils.ts (3)
8-8: LGTM! Parameter formatting improved.The single-line parameter formatting improves code readability.
50-52: LGTM! Code formatting improved.The formatting improvements enhance code readability and consistency.
61-68: LGTM! Style consistency improved.The addition of parentheses to arrow function parameters and trailing semicolon improves code style consistency.
apps/connection/src/connection.service.ts (4)
35-35: LGTM! Good refactoring to centralize URL construction.The import of the shared
getAgentUrlutility promotes code reuse and consistency across services.
671-671: Correct usage of the centralized URL utility.The migration to use the shared
getAgentUrlfunction with theCONNECTION_INVITATIONflag is implemented correctly.
781-819: Good formatting improvements for better readability.The reformatting of the
deleteConnectionRecordsmethod improves code clarity without changing functionality. The status counts object and Promise.all block are now more readable.
743-743: Clean formatting of method chain.The single-line formatting of the natsCall chain is appropriate for this simple operation.
libs/common/src/common.constant.ts (3)
26-32: URL path refactoring looks good.The addition of
/didcomm/prefix to connection and OOB URLs follows a clear routing structure. The new token-related URLs (URL_AGENT_TOKENandURL_SHARED_WALLET_TOKEN) support the enhanced token management functionality.
72-77: Consistent URL path updates for credential operations.The
/didcomm/prefix has been correctly applied to credential issuance URLs, maintaining consistency with the new routing structure.
400-414: Well-structured agent URL action flags.The new constants provide a clean way to identify different agent operations for URL construction. The naming convention is clear and self-documenting.
apps/agent-service/src/agent-service.service.ts (7)
81-81: Good consolidation of URL construction logic.The import of the shared
getAgentUrlutility aligns with the refactoring across other services.
424-424: Appropriate addition of API key to provisioning payload.Adding the API key from environment variables to the wallet provision payload supports the enhanced token management.
528-538: Well-implemented token retrieval with proper validation.The code correctly retrieves the base agent token, validates it, and encrypts it before storage. The error handling provides a clear message if the token is not obtained.
876-891: Excellent implementation of dynamic token retrieval for shared agents.The code properly handles the case where shared agents have no stored API key by fetching the tenant wallet token from the platform admin. This ensures shared agents can always obtain valid tokens.
1551-1551: Correct usage of centralized URL construction for signing operations.The code properly uses the shared
getAgentUrlutility with appropriate action flags for sign and verify operations.Also applies to: 1611-1611
1775-1776: Clean refactoring of variable destructuring.The updated destructuring assignment improves code readability.
623-624: No references todid/verkeyfound – ‘NA’ placeholders are safeWe searched the repository for any reads or validations of the
.didor.verkeyfields (including inorg_agents) and found no occurrences. There are no downstream consumers relying on those values, so using"NA"here will not break existing functionality.
| #echo "task_id=$task_id" | ||
|
|
||
| # to fetch log group of container | ||
| ............................................................. |
There was a problem hiding this comment.
Fix the placeholder text that could be misinterpreted as a command.
The line with dots is flagged by shellcheck as potentially being interpreted as a command. Replace with a proper comment.
-# to fetch log group of container
-.............................................................
+# to fetch log group of containerCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 283-283: This is interpreted as a command name ending with '.'. Double check syntax.
(SC2288)
🤖 Prompt for AI Agents
In apps/agent-provisioning/AFJ/scripts/start_agent_ecs.sh at line 283, replace
the line containing only dots with a proper shell comment to avoid it being
misinterpreted as a command by shellcheck. Use a hash symbol (#) followed by a
descriptive comment or leave it as an empty comment line.
There was a problem hiding this comment.
@KambleSahil3 Can you please have a look at this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
apps/agent-service/src/repositories/agent-service.repository.ts
Outdated
Show resolved
Hide resolved
apps/agent-service/src/repositories/agent-service.repository.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: Tipu_Singh <[email protected]>
Signed-off-by: Tipu_Singh <[email protected]>
| echo "Random String: $random_string" | ||
|
|
||
| SERVICE_NAME="${AGENCY}-${CONTAINER_NAME}-service-${random_string}" | ||
| SERVICE_NAME="${CONTAINER_NAME}-service-test" |
There was a problem hiding this comment.
Do we want to revert this?
| { | ||
| "name": "$CONTAINER_NAME", | ||
| "image": "${AFJ_IMAGE_URL}", | ||
| "image": "${AFJ_VERSION}", |
There was a problem hiding this comment.
We might need some ENV changes in this case for ecs?
There was a problem hiding this comment.
@KambleSahil3 Please have a look
| #echo "task_id=$task_id" | ||
|
|
||
| # to fetch log group of container | ||
| ............................................................. |
There was a problem hiding this comment.
@KambleSahil3 Can you please have a look at this
| async sendProofRequest(requestProof: IProofRequestData): Promise<string | string[]> { | ||
| try { | ||
| const comment = requestProof.comment ? requestProof.comment : ''; | ||
| const getAgentDetails = await this.verificationRepository.getAgentEndPoint(requestProof.orgId); |
There was a problem hiding this comment.
Can we also take agent details in common?
If not used afterwards
|
|
||
| const issuanceMethodLabel = 'create-offer'; | ||
| const url = await this.getAgentUrl(issuanceMethodLabel, orgAgentType, agentEndPoint, agentDetails?.tenantId); | ||
| const url = await getAgentUrl(agentEndPoint, CommonConstants.CREATE_OFFER); |
There was a problem hiding this comment.
Similarly to mentioned below, can we have fetching of agentEndPoint in a common function for all occurrences?
Signed-off-by: Tipu_Singh <[email protected]>
|
* feat: added logic for updated routes for agent Signed-off-by: Tipu_Singh <[email protected]> * refactor: uncommented required code Signed-off-by: Tipu_Singh <[email protected]> * feat:added logic to get token from cloudwatch Signed-off-by: Sahil Kamble <[email protected]> * feat: update credo image in taskdef Signed-off-by: Sahil Kamble <[email protected]> * feat: change service name for testing Signed-off-by: Sahil Kamble <[email protected]> * chore: WIP Signed-off-by: Sahil Kamble <[email protected]> * debug: added logs for agent details Signed-off-by: Tipu_Singh <[email protected]> * refactor:removed console logs Signed-off-by: Tipu_Singh <[email protected]> * refactor:removed commented code Signed-off-by: Tipu_Singh <[email protected]> * feat: added token for old wallet Signed-off-by: Tipu_Singh <[email protected]> * refactor: cloud wallet urls and token changes Signed-off-by: Tipu_Singh <[email protected]> * refactor: removed logs from shell scripts Signed-off-by: Tipu_Singh <[email protected]> * refactor: resolve comment Signed-off-by: Tipu_Singh <[email protected]> * refactor: resolve comments for getOrgAgentApiKey Signed-off-by: Tipu_Singh <[email protected]> * refactor: agent service comments Signed-off-by: Tipu_Singh <[email protected]> --------- Signed-off-by: Tipu_Singh <[email protected]> Signed-off-by: Sahil Kamble <[email protected]> Co-authored-by: Sahil Kamble <[email protected]>
3 participants
What
Summary by CodeRabbit
New Features
Refactor
Chores