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

Add three SSP metadata role constraints #676

Conversation

DimitriZhurkin
Copy link

@DimitriZhurkin DimitriZhurkin commented Sep 10, 2024

Committer Notes

Add the following three constraints:

  1. role-defined-authorizing-official-poc
  2. role-defined-information-system-security-officer
  3. role-defined-system-poc-other

All Submissions:

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

david-waltermire and others added 23 commits February 6, 2024 11:36
…atibility breaking changes introduced in the 1.2.1 oscal content release. This approach uses a local catalog that contains these fixes.
Co-authored-by: Rene Tshiteya <rene-claude.tshiteya@gsa.gov>
…ersion

Local version of SP800-53rev5.1.1 that contains the correct labels
Adding the March 7 Data Bites presentation
Uploading April's Data Bites presentation
Uploading the most recent Data Bites presentation
Adding recent Data Bites presentations
* Fix OSCAL version
* Move submodule to v1.0.6
* Remove USWDS
* Fix validation errors in rev 4 templates
* Add files via upload

Adding recent Data Bites presentations

* Add files via upload
Fix invalid error (<prop name="privilege-level" value="privileged"/>)
Fix invalid error (<prop name="privilege-level" value="privileged"/>)
Add **validations** as one of the "This relates to..." options
Add the following constraints:
1. role-defined-authorizing-official-poc
2. role-defined-information-system-security-officer
3. role-defined-system-poc-other
@DimitriZhurkin DimitriZhurkin requested a review from a team as a code owner September 10, 2024 17:51
Comment on lines -232 to 238
let scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
const scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);

if (scenarioLines.length === 0) {
console.error(`No scenarios found for constraintId: ${constraintId}`);
execSync("npm run test:coverage",{stdio:'ignore'});
scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
if(scenarioLines.length===0){
execSync("npm run test:coverage",{stdio:'inherit'});
return false;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: can we explain why these changes were made and what the intended goal is? These changes seem separate of a constraint update, so I want to check we know what we're doing here and if it is necessary now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure where these changes came from. I have not modified the dev-constraint.cjs file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you know how to revert individual file changes from a commit, please do so. Otherwise, I read out the diff output to make a suggestion, just accept this change and I think you will get it back to the way it was. Let me know. If successful, there will be no change on this file afterward.

Suggested change
let scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
const scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
if (scenarioLines.length === 0) {
console.error(`No scenarios found for constraintId: ${constraintId}`);
execSync("npm run test:coverage",{stdio:'ignore'});
scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
if(scenarioLines.length===0){
execSync("npm run test:coverage",{stdio:'inherit'});
return false;
}
}
let scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
if (scenarioLines.length === 0) {
console.error(`No scenarios found for constraintId: ${constraintId}`);
execSync("npm run test:coverage",{stdio:'ignore'});
scenarioLines = getScenarioLineNumbers(featureFile, constraintId,testFiles);
if(scenarioLines.length===0){
return false;
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even see that file in my commit, and I couldn't find when it had been changed in history.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AJ,

I think Paul had made these changes some time ago, and when I pulled the repo yesterday, it overwrote dev-constraint.cjs on my local system.

Then, when I pushed my constraints today, the push also picked up the modified dev-constraint.cjs file.

It's a bit confusing now, which version of dev-constraint.cjs is correct. Might wanna wait for Paul's input on that puppy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it is a rebase issue (where you made a branch of your PR code from a copy of feature/external-constraints, but slightly old). So do not worry about it, I can look. I will resolve this when it is approved so I do not forget to check. Re the code I don't want to change functionality and revert the Javascript because it may bork the tests for you and others if not careful, so I totally agree.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes looks like an out dated version of the dev constraint script, that line is meant to handle the case where a new constraint is made but the full test runner hasn't been run and the feature file needs to be updated. but its best to rebase or leave as is on the feature/external-constraints, it shouldn't alter how the tests run at all since it is a different path

Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I too have questions about role-defined-system-poc-other.

Comment on lines +31 to +34
</expect>
<expect id="role-defined-system-poc-other" target="metadata" test="role[@id eq 'system-poc-other']" level="ERROR">
<message>SSP metadata must have the system other POC role.</message>
</expect>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks good and the others roles make sense, I looked up current docs and will need to do some history study in old Schematron and pre-website docs, I have never seen a requirement for this role before. I see you noted the same in the spreadsheet. Can we discuss this tomorrow morning?

Copy link
Author

@DimitriZhurkin DimitriZhurkin Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have doubts about this system-poc-other role.
I couldn't find it in the rev 5 documentation at all. Wonder if it has been deprecated.

Rene2mt and others added 3 commits September 10, 2024 22:31
* Create 0006-testing-harness-approach.md

* Create adr_template.md

* Update adr_template.md

Add instructions in ADR template

* Update ADR-006 with decision

* Update and finalize ADR 6 since it is defacto adopted

We can always make a new ADR, but I have been here a few weeks and our Cucumber testing infra is largely built out, so at this point it makes sense to mark it accepted and clear up the review queue for other PRs that are less established.

---------

Co-authored-by: A.J. Stein <alexander.stein@gsa.gov>
@aj-stein-gsa
Copy link
Contributor

I am going to close this out in favor of #686. Thanks for the update, Dimtri.

aj-stein-gsa added a commit that referenced this pull request Sep 17, 2024
* Update SSP metadata role constraints

* Adjust message text to be more approachable, per PR feedback

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
aj-stein-gsa added a commit that referenced this pull request Sep 24, 2024
* Update SSP metadata role constraints

* Adjust message text to be more approachable, per PR feedback

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
aj-stein-gsa added a commit that referenced this pull request Sep 25, 2024
* Update SSP metadata role constraints

* Adjust message text to be more approachable, per PR feedback

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
Rene2mt pushed a commit to Rene2mt/fedramp-automation that referenced this pull request Sep 27, 2024
* Update SSP metadata role constraints

* Adjust message text to be more approachable, per PR feedback

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
brian-ruf pushed a commit to brian-ruf/fedramp-automation that referenced this pull request Nov 8, 2024
* Update SSP metadata role constraints

* Adjust message text to be more approachable, per PR feedback

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
This was referenced Nov 13, 2024
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.

7 participants