-
Notifications
You must be signed in to change notification settings - Fork 2
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
create cli-pull-ot command to sync assessments from OneTrust to disk #375
Conversation
This pull request has been linked to and will mark 1 task as "Pending Deploy" when merged:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abrantesarthur can you make sure to update the version in package.json and also add README updates for this command? 🙏
for sure! |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, I think I get what's going on!
/** Overall risk score after considering existing controls. */ | ||
residualRiskScore: number; | ||
/** Result of the assessment. NOTE: This field will be deprecated soon. Please reference the 'resultName' field instead. */ | ||
result: 'Approved' | 'AutoClosed' | 'Rejected'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linked documentation says these are the appropriate value for result, but the example response provided there looks like it has result and resultId the same, rather than result and resultName. Not sure if that makes a difference to us.
"result": "b1b0a831-b7d7-410e-b3e5-4a3e5e141851",
"resultId": "b1b0a831-b7d7-410e-b3e5-4a3e5e141851",
"resultName": "Pending",
(I didn't check the rest of this file closely, I was just curious how you'd gotten the info about which values a field might be limited to and that detail happened to pop out to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, checking the actual response that I got, I see that sometimes "result" = "resultId", other times "result" = "resultName", other times "result" = "Approved" but "resultName" = "Approved - Remediation required". In general, since their docs say that "result" will be deprecated, I will just trust "resultId" + "resultName" for purposes of importing into Transcend. Thanks for the heads up!
README.md
Outdated
@@ -571,6 +573,43 @@ tr-pull --auth=./transcend-api-keys.json --resources=consentManager --file=./tra | |||
|
|||
Note: This command will overwrite the existing transcend.yml file that you have locally. | |||
|
|||
### tr-pull-ot | |||
|
|||
Pulls resources from a OneTrust instance. For now, it only supports retrieving OneTrust Assessments. It sends a request to the (Get List of Assessments)[https://developer.onetrust.com/onetrust/reference/getallassessmentbasicdetailsusingget] endpoint to fetch a list of all Assessments in your account. Then, it queries the (Get Assessment)[https://developer.onetrust.com/onetrust/reference/exportassessmentusingget] and (Get Risk)[https://developer.onetrust.com/onetrust/reference/getriskusingget] endpoints to enrich these assessments with more details such as respondents, approvers, assessment questions and responses, and assessment risks. Finally, it syncs the enriched resources to disk in the specified file and format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the link syntax here is reversed--I think we want [link text](link address)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good stuff!
|
||
```sh | ||
# Writes out file to ./oneTrustAssessments.json | ||
tr-pull-ot --auth=$ONE_TRUST_OAUTH_TOKEN --hostname=trial.onetrust.com --file=./oneTrustAssessments.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you do with the file after its pulled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now we do nothing, but I am going to create another command that accepts the file path as input and then pushes it to Transcend via the importOneTrustAssessmentForms mutation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, maybe use the same command but have a --dryRun
parameter or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or have it dump into the transcend.yml shape and use tr-push to push up (would need to add assessment support to that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could work as well! So if dryRun = true we push to transcend. Otherwise, we save it as a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was thinking opposite, default behavior is to pull from OT and push to transcend. probaly would make sense to rename to tr-sync-ot
then if you specify --dryRun=true
this would only pull from OT and write to file instead of syncing to transcend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense! I'll do that.
parseCliPullOtArguments(); | ||
|
||
try { | ||
if (resource === OneTrustPullResource.Assessments) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: best to move these into helper functions so that they can be imported as code if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in a follow up PR!
`api/assessment/v2/assessments/${assessmentId}/export?ExcludeSkippedQuestions=false`, | ||
); | ||
|
||
return JSON.parse(body) as OneTrustGetAssessmentResponse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should use an io-ts codec to validate here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created the codecs here https://github.com/transcend-io/privacy-types/pull/205/files and battle-tested them agains the OneTrust API responses. Caught lots of stuff! Thanks for the suggestion.
logger.info('Getting list of all assessments from OneTrust...'); | ||
while (currentPage < totalPages) { | ||
// eslint-disable-next-line no-await-in-loop | ||
const { body } = await oneTrust.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit sjould use io-ts codec validation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in a follow up PR as well!
yarn ts-node ./src/cli-pull-ot.ts --hostname=app-eu.onetrust.com --auth=$ONE_TRUST_OAUTH_TOKEN --file=./oneTrustAssessment.json
, whereONE_TRUST_OAUTH_TOKEN
should be the authentication token (you can find it in the parent ticket's description).Related Issues