Review own documents should not be possible

[tommylehmann]

According to documents/architecture-decisions.md Roles section a reviewer can list and view documents not created by himself. But if a user has the publisher or editor role as well as the reviewer role he can move his own advisory from Review to Approved.

It's not clear from the roles description but it seams wrong from a security point of view if someone can approve his own work. The review step is senseless unless you have enough people were one half is only allowed to write and the other only to review. This means the administrator must manage users that way that authors/editors/publishers and reviewers are disjoint groups. But then its not a peer review.

Is this intentional?

[rainer-exxcellent]

The implementation in the csaf-cms-backend must comply with the CSAF specification. We check whether this is the case.
For a change to the specification, an issue must be addressed to oasis. (https://github.com/oasis-tcs/csaf)

[rainer-exxcellent]

Here is what the standard says:
(Chapter: 9.1.6 Conformance Clause 6: CSAF content management system)

provides a user management and support at least the following roles:

• Registered: Able to see all published CSAF documents (but only in the published version).
• Author: inherits Registered permissions and also can Create and Edit Own (mostly used for automated creation, see above)
• Editor: inherits Author permissions and can Edit (mostly used in PSIRT)
• Publisher: inherits Editor permissions and can Change state and Review any (mostly used as HEAD of PSIRT or team lead)
• Reviewer: inherits Registered permissions and can Review advisories assigned to him (might be a subject matter expert or management)
• Manager: inherits Publisher permissions and can Delete; User management up to Publisher
• Administrator: inherits Manager permissions and can Change the configuration

[rainer-exxcellent]

I think it is a case that is not covered by the standard
@tschmidtb51 what do you think?

[tommylehmann]

I was not aware of the fact that the CSAF spec also covers the roles of an CMS. The combination of both the architecture document of csaf-cms-backend and the CSAF spec make sense. From the phrase "can Change state and Review any" I interprete this as the correct behavior. I think the Documentation of csaf-cms-backend could be a bit more clear at this point.

Currently the section is the following:

Role: Publisher

• inherits the rights of the "Editor" role
• may list and view all CSAF documents in Approved status
• may change the workflow state of all CSAF documents from RfPublication to Published
• may change the workflow state of all CSAF documents from Approved to Draft

If that would be extended by

• may change the workflow state of all CSAF documents from Review to Approved

It would be more clear and follows the CSAF spec in "can Change state and Review any".

[tschmidtb51]

The intention is in fact to have a kind of peer review in the PSIRT group.

But if a user has the publisher or editor role as well as the reviewer role he can move his own advisory from `Review` to `Approved`.

For the publisher this is intentional. The idea behind this is, that a Head of PSIRT has (as a last resort) all power regarding the content in the system. This ultimately includes publishing his/her own advisories. As always: With great power comes great responsibility. If that is a concern to you, feel free to open an issue and suggest to have a technical role included that is just allowed to do the step from `RfPublication` to `Published`. However, I currently fail to see, how an editor should be able to push it through. Would he/she be able to request a review from himself/herself? IMHO, the standard does not explicitly prohibit it, but I guess it would be a good practice to at least have a (default active) config option to disallow requesting a review from themselves. @rainer-exxcellent is something like this implemented? Also, if you have a suggestion to improve the documentation - feel free to provide a PR.

[rainer-exxcellent]

No, there is no flag implemented.

The implementation is in:
https://github.com/secvisogram/csaf-cms-backend/blob/main/src/main/java/de/bsi/secvisogram/csaf_cms_backend/service/AdvisoryWorkflowUtil.java#L223

[tommylehmann]

While the CSAF Spec states that Reviewers can review advisories assigned to them, CSAF CMS Backend lacks an assignment function.
"Reviewer: inherits Registered permissions and can Review advisories assigned to him"

The design document on the other hand specifies the Reviewer role as follows. Yes there is no condition on the "change status" requirement, but I would assume that you can not change a documents status that you are not allowed to view. Since the Reviewer should only be able view Documents that are in Review state and not created by the user that would mean that he can only change the state of documents with that constraint. Am I wrong at this point?

Role: Reviewer

• inherits the rights of the "Registered" role
• may list and view all CSAF documents in the status Review and not created by the user
• May view, reply to and create comments on CSAF documents
• may change the status from Review to Draft or Approved.

The sourcecode checks only for the Reviewer Role, nothing more. That means to me: the sourcecode is currently neither adhering to the CSAF Spec nor its own architecture document.

        if (oldWorkflowState == WorkflowState.Review && newWorkflowState == WorkflowState.Draft) {
            canBeChanged = hasRole(REVIEWER, credentials);
        }

        if (oldWorkflowState == WorkflowState.Review && newWorkflowState == WorkflowState.Approved) {
            canBeChanged = hasRole(REVIEWER, credentials);
        }

[tommylehmann]

I would provide a PR but currently I don't see a clear direction. Could be changing the documentation or changing the sourcecode to check for the isOwnDocument condition and allowing Publisher explicitly to review own documents. Which way to go?

[rainer-exxcellent]

We will discuss this at our next status meeting

[rainer-exxcellent]

We decided to add an optional flag that regulates whether your own documents may be reviewed and approved or not. The default should be that own documents may be reviewed but not approved. Cases to be considered are: "Reviewer" and "Publisher".