26T1-BAC-TR-005 - Collector creation for SharePoint#136
Conversation
tinar10
requested review from
6igby,
du-dhartley and
liyunze-coding
as code owners
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5fa11738
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| is_restricted(sharing_capability, normalized) if { | ||
| sharing_capability != null | ||
| not contains(normalized, "anyone") |
There was a problem hiding this comment.
This rule currently treats any value that does not contain "anyone" as compliant, so settings like externalUserSharingOnly or existingExternalUserSharingOnly will pass even though the control text in this file says the recommended state is Only people in your organization. In practice, tenants that still allow external sharing can be reported as compliant, which produces false positives for this benchmark check.
Useful? React with 👍 / 👎.
| "sharepoint_sharing_capability": tenant_settings.get( | ||
| "CoreSharingCapability", | ||
| tenant_settings.get("SharingCapability"), |
There was a problem hiding this comment.
get_tenant_settings() returns the raw Microsoft Graph payload, but this collector only reads PascalCase keys (for example CoreSharingCapability/SharingCapability). With normal Graph responses (sharingCapability, oneDriveSharingCapability, etc.), these fields resolve to null, so site_sharing_settings loses the sharing configuration data and downstream evaluations from this collector cannot make accurate decisions.
Useful? React with 👍 / 👎.
|
@tinar10 Did you need to assign any new permissions for this collector to work? |
|
It's worth reviewing the feedback that codex has added re casing |
Hi @du-dhartley , yes, needed to enable an additional Microsoft Graph API permission for this collector. Specifically, |
sure, let me review and address the feedback from Codex. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43206fa6d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| is_restricted(sharing_capability, normalized) if { | ||
| sharing_capability != null | ||
| not contains(normalized, "anyone") |
There was a problem hiding this comment.
is_restricted currently marks the control compliant for any non-null value that does not literally contain "anyone", but Microsoft Graph returns the Anyone-links state as externalUserAndGuestSharing; that string bypasses this check and causes false passes for tenants that still allow anonymous sharing. This affects control 7.2.3 accuracy and should be fixed by explicitly evaluating known enum values (or explicitly failing externalUserAndGuestSharing).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3981f07318
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| is_restricted(sharing_capability, normalized) if { | ||
| sharing_capability != null | ||
| restricted_sharing_values[normalized] | ||
| } |
There was a problem hiding this comment.
is_restricted is currently a partial rule, so when sharing_capability is null or a non-restricted value (for example the non-compliant externalUserAndGuestSharing case), compliant := is_restricted(...) becomes undefined and the result rule does not evaluate. In those paths the policy falls back to the default {"compliant": false, "message": "Evaluation failed"} and loses the intended control-specific message and affected resource details, which degrades scan accuracy and troubleshooting output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97d1d1d812
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "onedrive_sharing_capability": self._first_present( | ||
| tenant_settings, | ||
| "OneDriveSharingCapability", | ||
| "oneDriveSharingCapability", |
There was a problem hiding this comment.
SpoTenantDataCollector.collect now reads onedrive_sharing_capability only from OneDriveSharingCapability/oneDriveSharingCapability, but this collector is fed by SharePointClient.get_tenant_settings() (GET /admin/sharepoint/settings), whose Graph payload does not expose those keys. That means this field is always null, and 7.2.4_onedrive_content_sharing_restricted.rego will consistently evaluate as non-compliant/unknown after 7.2.4 was marked ready, creating false failures for all tenants.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@tinar10 I haven't checked this particular case mentioned here but it seems like one to double check
There was a problem hiding this comment.
@tinar10 Can you please check the validity of this? Does the endpoint actually return this key?
There was a problem hiding this comment.
@du-dhartley , validated this Graph response from GET /admin/sharepoint/settings. The response includes sharingCapability, but it does not include oneDriveSharingCapability or OneDriveSharingCapability.
This confirms that the current collector mapping will resolve onedrive_sharing_capability to NULL only when using this endpoint, so the control for 7.2.4 may produce false failures unless we identify another data source for the OneDrive-specific sharing setting.
2 participants
Summary of the Changes
The pull request introduces new SharePoint-specific collectors required for compliance scanning of Section 7 in Microsoft 365. Additionally, the connection method uses Microsoft Graph, with the necessary SharePoint API permissions enabled in Entra ID (Azure AD).
Components
/engine (collectors / policies)Testing Done
Tested run locallyRollback
Revert the commit changesScreenshots