-
Notifications
You must be signed in to change notification settings - Fork 919
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
Prevent input name collision with db name for refs (#6056) #6080
Conversation
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.
None of the input names modified here are exposed in the graphql API
The refs can only be created through stixRefRelationShipAdd
, using relationship_type
key and it does not need to change there.
@@ -32,16 +32,24 @@ steps: | |||
commands: | |||
- apk add build-base git libffi-dev cargo | |||
- pip3 install --upgrade setuptools | |||
- cd opencti-platform | |||
- OPENCTI_COMMIT=$(git rev-parse --short HEAD) | |||
- echo [INFO] using opencti@$DRONE_COMMIT_BRANCH:$OPENCTI_COMMIT |
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.
just an improvement on the way, to finally have a clear feedback on what is the precise version used.
sample: String | ||
analysisSample: String |
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.
This is the only breaking change to the API.
All other renaming are on attribute never exposed in the API for now.
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.
Why not objectSample?
if (dst === INPUT_FROM && isStixRefRelationship(type)) { | ||
if (dst === 'from' && isStixRefRelationship(type)) { |
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.
this one was tricky.
"from" is a key used in 2 different context: it's the "from" of a relationship, and the "from" in an email message.
INPUT_FROM
was defined for use with email message only, but it was also imported and used in places related to relationship.
Now that we renamed for email to emailFrom
it's not working anymore for relationships. But it was never meant to work like that in the first place.
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.
Now that email is now on INPUT_EMAIL_FROM, are you sure is still needed to not used the constant?
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.
Most of the time the constant INPUT_NAME was not used, its' the string "from".
It was an input for email, but as for relationship it's not in any schema def, it's not an input.
We think it's an old confusion (email 'from' being used in place where we actually meant "from" of a relationship).
We removed the constant INPUT_NAME. It's not used in any schema def.
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.
This part was causing the test "should all elements correctly deleted" in middleware-test to fail, because the "from" was not resolved for one of the entity (it was a stix ref relationship). and yes we think "INPUT_FROM" was used by mistake because it was "from" until we changed it to "emailFrom", since it was an input specific to email message. And it was the only place where it was used as a constant, in all other places in middleware we use the string 'from' directly.
export const INPUT_TO = 'to'; | ||
export const INPUT_CC = 'cc'; | ||
export const INPUT_BCC = 'bcc'; | ||
export const INPUT_SENDER = 'emailSender'; |
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 thought we talk about pushing the objectXXX convention
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 convention objectXXX produces the change contains
-> objectContains
but objectContains
was an old filter key still renamed on the fly to objects
, so we can't use objectContains
one thing leading to another, we thought it would be better to prefix with the entity they're used for as per the schema description. It's the case for a lot of other keys.
cdc32a6
to
6368e31
Compare
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.
Need Julien eye on that
f091b07
to
2f44eff
Compare
Related python client PR open for malwareAnalysis : OpenCTI-Platform/client-python#563 |
2f44eff
to
60b029a
Compare
Several refs attributes have the same input name as their db name, resulting in bugs during stream event publishing.
If the attribute is multiple, the event published in stream contains
null
in patch.If it's a single value, the stream event won't be sent and an error will be thrown (
store update event error
)Proposed changes
MalwareAnalysisAddInput.sample
to renameMalwareAnalysisAddInput.analysisSample
Related issues
Checklist
Further comments
e2e tests would be added in #6073 to cover network traffic creation + ref, I won't add it here (or improve on it here once merge and rebase)
To test this PR, test the following: