833 oscal version constraint

[kyhu65867]

Committer Notes

{Please provide a description of what this PR accomplishes. Be sure to reference any issues addressed. If the PR is a work-in-progress submitted for early review, please submit the PR as a draft PR using the "Draft pull request" dropdown.}

All Submissions:

• Have you selected the correct base branch per Contributing guidance?
• Have you set "Allow edits and access to secrets by maintainers"?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?
• Have you squashed any non-relevant commits and commit messages? [instructions]
• If applicable, have all FedRAMP Documents Related to OSCAL Adoption affected by the changes in this issue have been updated.?

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

[aj-stein-gsa]

@kyhu65867 I beefed it up and think it works before I mark it anymore complicated. Let me know what you think, especially the tests. I may have to abstain from review, but hope our colleagues roast me and show you that no one is sacred during review. 😄

[aj-stein-gsa]

I add myself not just because I contributed but I want honest feedback and review from others since I did my fair share on this one, I should have to defend it with Kylie.

[Gabeblis]

I think this is a good start. My biggest issue with this is the FedRAMP versions. Right now we have only 1 version with its minimum required OSCAL version hardcoded. Are there different versions? If not, we could simplify this constraint a lot. Simply changing the FedRAMP version at all breaks this constraint.

[Gabeblis]

We already have a constraint that checks and requires the existence of the fedramp-version prop. I am not sure adding a default value in the case of the missing prop is what we want to do here.

[aj-stein-gsa]

If not you will get null pointer exceptions when you dereference other elements, so I was curious how others would consider appropriate for the balancing act. I appreciate the feedback.

[Gabeblis]

The first thing that comes to mind is creating a let expression that checks for the existence of the prop. If it doesn't exist, assign some null value. Add that check as part of the other let expressions: if (true) then do something... else null value. Then in the constraint test just check that the value isn't null and all the other conditions are still truthy

[aj-stein-gsa]

OK, I can keep that in mind.

[Gabeblis]

One more thing about the current implementation is that in the case that no FedRAMP version is included, it incorrectly results in a pass because of the way it defaults to a certain value.

[aj-stein-gsa]

Oh I moved it back to in progress because I was not done addressing your other feedback, sir. I did not forget!

[Gabeblis]

Apologies. It just hit me that I forgot to mention that yesterday is all 🤕

[Gabeblis]

One more thing about the current implementation is that in the case that no FedRAMP version is included, it incorrectly results in a pass because of the way it defaults to a certain value.