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

Default controls to unsatisfied #79

Closed
wants to merge 1 commit into from
Closed

Default controls to unsatisfied #79

wants to merge 1 commit into from

Conversation

joepurdy
Copy link

This would fix #77

By default the template policies are mapped to TSC 2017 criteria using satisfies front matter. Because the logic in comply is to mark controls as satisfied based on this front matter a newly initialized set of templates show all controls as satisfied.

This is confusing to new users as it makes tracking the work to implement the compliance program less obvious and there's no mechanism to manually mark controls/policies as satisfied.

This PR adds a new boolean field to the Document and Procedure structs for Live to determine if a document/procedure is live and in-place in an organizations control environment. I've also updated the ControlsSatisfied() func to only mark controls as satisfied if the document/procedure has this new field set to true.

To help guide users on the workflow I've also updated the default README to include a section about satisfying controls that describes setting live: true once the policy is implemented. To make it even more clear I updated the template files that are already mapped to criteria as having live: false added to the front matter to ensure new users have an example of the right front matter.

@joepurdy
Copy link
Author

@jmccarthy 👋

Sorry to bug you direct so close to the holidays. Feel free to ignore and I'll follow-up in the new year. I like to wait a week or so before mentioning project maintainers about PRs. I couldn't find much in the way of contributing guidelines for Comply aside from https://comply.strongdm.com/contribute/ which doesn't go into too much detail on what your desired process is for new PRs.

Let me know if you have any Qs about the PR or the related issue. Happy to discuss further and make adjustments. Also if adding an additional field to the structs doesn't seem reasonable there's alternate ideas I have in mind.

The primary concern I have with this implementation is that every Comply site running in the wild would lack live: true in their front matter and when/if this PR landed in a future release it would be a breaking change since the zero value will be false.

In other words, by not adding live: true to existing policies folks would have their controls start showing unsatisfied.

That said, breaking changes happen and life goes on. If that's not a risk that seems reasonable the alternative would be to default the template policies and narratives to leave the satisfies front matter commented out much in the way I suggested here: #77 (comment)

That effectively accomplishes the same outcome while only impacting newly generated content.

Looking forward to your thoughts!

@masonhensley
Copy link
Contributor

@joepurdy (not a maintainer) but I've made a contribution in the past. The maintainers are super nice!

I think they hang out in this slack group that you can join if you need to have a convo with them about breaking changes ... link from readme.md

@joepurdy
Copy link
Author

joepurdy commented Feb 5, 2020

Thanks @masonhensley! I've been hanging around the Slack workspace for some time. Just didn't want to be obnoxious with a "Hey! Look at my PR!" message for something low priority and more on the quality of life side of things.

I'll raise it later today as something to review and discuss. After sitting with it I think this approach isn't great. The breaking change aspect is no good and changing the default templates seems better given that only impacts newly initialized comply projects.

@joepurdy
Copy link
Author

Closing this PR as unmerged. I believe #85 is a better approach as it is backwards compatible. Adding new frontmatter would definitely cause existing Comply backed projects to start reporting no controls satisfied when the maintainers of those projects fail to pickup the config change.

As the issue in #77 is really an issue only new users run into it makes more sense to fix it in a manner that only affects newly generated projects.

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.

Marking items un-satisfied
2 participants