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

New STIX 2.1 SCO extension name requirement: must end with "-ext" #370

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

chisholm
Copy link
Contributor

@chisholm chisholm commented Mar 11, 2020

Fixes #364

I made more changes than you may think was strictly necessary, but I thought some aspects of the custom SCO extension code needed some improvement.

  • Most requirements on SCO extensions were checked in _custom_extension_builder, which is the implementation for the CustomExtension decorators (both 2.0 and 2.1). The former called _register_observable_extension to enable the extension. So you could bypass most STIX2 requirements by bypassing the decorator and builder function and registering an extension directly. I decided that was undesirable, so I moved all of the requirement checking into the register function.
  • Some of the aforementioned checking was for some reason embedded inside the dynamically created extension subclass (_CustomExtension), which didn't make any sense and cluttered up the class. So a side effect of the above is that the class is a lot cleaner and clearer now.
  • The custom extension decorator's properties had previously been documented as requiring a list of tuples. That seemed unnecessarily restrictive, so I changed the check to a more sensible one (I thought), and changed the exception message for a violation. Really, all you ever needed was something you can turn into a dict via OrderedDict(...), which includes any mapping, as well as iterables of length-2-iterables (e.g. tuples of tuples, lists of lists, etc). One might argue that allowing unordered mappings means property order might be unpredictable. But if that's important, people don't have to use those types (and we never do that). I think the added flexibility is fine.
  • Changing the exception message broke several unit tests which were checking message contents. I removed the message checks. The important thing is that the exception is being thrown.
  • Since the SCO extension name syntax rules are now different between STIX 2.0 and 2.1, I had to ensure that both rulesets were checked as appropriate. The existing code had a regex; I added a special second regex for 2.1+ SCO's which hopefully enforces the correct requirements (it's basically the same as the old one but with "-ext" at the end). The extension registration function now has to check the STIX version and choose the right regex to use.
  • The version default changed in the registration function. I needed a version so I could choose the right SCO extension name regex. None doesn't fit the bill, but the stix2.DEFAULT_VERSION does, and is immutable. The code already behaved that way, but didn't use the obvious implementation, so I simplified it and made my life easier.
  • Many 2.1 unit tests used SCO extension names which became invalid, so I had to make up valid ones. I tried to make them similar to the old ones.

@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

Merging #370 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #370   +/-   ##
=======================================
  Coverage   98.18%   98.18%           
=======================================
  Files         124      124           
  Lines       14230    14238    +8     
=======================================
+ Hits        13972    13980    +8     
  Misses        258      258           
Impacted Files Coverage Δ
stix2/core.py 100.00% <100.00%> (ø)
stix2/custom.py 97.33% <100.00%> (-0.11%) ⬇️
stix2/test/v20/test_custom.py 100.00% <100.00%> (ø)
stix2/test/v21/test_custom.py 100.00% <100.00%> (ø)
stix2/utils.py 97.17% <100.00%> (+0.01%) ⬆️
stix2/v21/sdo.py 94.44% <0.00%> (-0.10%) ⬇️
stix2/v21/common.py 100.00% <0.00%> (ø)
stix2/v21/observables.py 96.93% <0.00%> (ø)
stix2/test/v21/test_malware.py 100.00% <0.00%> (ø)
stix2/test/v21/test_indicator.py 100.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 380926c...15316e7. Read the comment docs.

@chisholm
Copy link
Contributor Author

Hmmm... and now that I go back and look at that, I see there were several customization functions which basically worked the same way (object, observable, marking, ...) and I only changed one of them. So now it lacks symmetry. Maybe I should change all the rest too? :)

@clenk
Copy link
Contributor

clenk commented Mar 12, 2020

This is definitely cleaner; I like it! Custom extension names still have a SHOULD start with "x-" though - could we add that back into the v21 tests so they follow best practices?

best practice and follow a spec "should" rule.
@clenk clenk merged commit 6842abb into oasis-open:master Mar 12, 2020
@clenk
Copy link
Contributor

clenk commented Mar 12, 2020

Thanks @chisholm!

@emmanvg emmanvg added this to the 1.4.0 milestone Mar 12, 2020
@chisholm chisholm deleted the observable_extension_names_ext branch April 3, 2020 22:09
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.

Enforce requirement that SCO extension names must end with "-ext"
4 participants