feat(tooling): add SharePoint collector support to test_collector.py - #334
feat(tooling): add SharePoint collector support to test_collector.py#334yergaliev98 wants to merge 3 commits into
Conversation
Import SharePointClient, add get_sharepoint_tenant_name() reading M365_SHAREPOINT_TENANT_NAME (no hard-coded tenant details), and detect SharePoint collectors by module path in both single-collector testing and test_all_collectors(), so any SharePoint-based collector can be exercised the same way as Graph/PowerShell collectors are today.
Preview EnvironmentA preview environment can be spun up on demand for this PR.
|
CI: Engine
One or more checks failed. View logs |
…oint support Explicitly annotate client as PowerShellClient | SharePointClient | GraphClient instead of letting mypy infer its type from whichever if/elif branch it processes first. Add scoped type: ignore[arg-type] on the two collector.collect() calls, since BaseDataCollector.collect()'s declared signature only accepts GraphClient and hasn't been generalised for PowerShell/SharePoint collectors — a pre-existing base-class limitation out of scope for this change. Note: this file imports collectors.registry, which registers every collector in the project. mypy follows that import and reports ~100 pre-existing type errors across registry.py and ~15 other Exchange collector files that share an identical, pre-existing None/dict/list normalisation bug (the same class of issue fixed for one instance of it, owa_mailbox_policy.py, in PR #333). None of those errors are introduced by this change - test_collector.py itself now type-checks clean aside from one pre-existing issue in get_credentials() (line 75, also predating this diff). Raising the wider debt with the team separately rather than fixing ~16 unrelated files here.
|
@codex review |
![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: 23d8fc428d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif collector.__class__.__module__.startswith("collectors.sharepoint"): | ||
| tenant_name = get_sharepoint_tenant_name() | ||
| client = SharePointClient(tenant_id, client_id, client_secret, tenant_name) |
There was a problem hiding this comment.
This branch cannot be entered through either CLI mode: in this commit, DATA_COLLECTORS has no imports or mappings for any collectors.sharepoint.* classes, and test_collector() rejects an ID absent from that mapping before calling get_collector. Consequently, -c sharepoint.spo_tenant reports Unknown collector, while --all never iterates a SharePoint collector, so the new client-selection logic does not provide the advertised SharePoint testing support. Register the SharePoint collectors (or discover them) alongside this wiring.
Useful? React with 👍 / 👎.
2 participants
Summary
Extends the shared
test_collector.pydeveloper testing utility to support SharePoint collectors, which it previously had no awareness of at all. ImportsSharePointClient, addsget_sharepoint_tenant_name()(reads the tenant name fromM365_SHAREPOINT_TENANT_NAME, no hard-coded tenant details), and updates the collector-execution logic — in both single-collector testing andtest_all_collectors()— to detect SharePoint collectors by module path and initialise them with a correctly configuredSharePointClient. This unblocks manual testing forsharepoint.spo_tenantand any future SharePoint-based collector, the same way Graph and PowerShell collectors are already exercised.Type of Change
Affected Components
/backend-api/frontend/engine(collectors / policies)/security/infrastructure/.github/workflows/docsMotivation
While implementing control 7.2.9 (SharePoint guest access expiration), I found the referenced
sharepoint.spo_tenantcollector was an unimplemented stub, and before I could even test or build against it, discoveredtest_collector.py— the script the whole team uses to exercise any collector in isolation — had no SharePoint support at all: it had no way to construct aSharePointClientor resolve a target tenant. Rather than work around this just for my own task, I fixed the shared script directly, since it blocks anyone testing a SharePoint-based collector, not just control 7.2.9. Submitting this separately from 7.2.9 itself, which remains blocked on a SharePoint authentication issue I've raised with my mentor/team (see my 5.2C progress report).Testing Done
uv run python -m scripts.test_collector -c sharepoint.spo_tenantlocally. Confirmed it now prompts forM365_SHAREPOINT_TENANT_NAME(previously failed immediately with "Unknown collector: sharepoint.spo_tenant"), correctly initialises aSharePointClient, and surfaces the stub's intentionalNotImplementedError("This collector is not yet implemented") rather than crashing — confirming the wiring is correct end-to-end even though the underlying collector itself isn't built yet. Also re-ran a handful of existing Graph/PowerShell collectors (e.g.entra.devices.device_registration_policy,exchange.organization.owa_mailbox_policy) to confirm their testing path is unchanged, since the SharePoint client is only lazily initialised when a SharePoint collector is actually requested.test_collector.pyis a manual developer CLI utility, not covered by the project's automated pytest/OPA test suites.Security Considerations
Tenant name is read from an environment variable (
M365_SHAREPOINT_TENANT_NAME), consistent with howM365_TENANT_ID/M365_CLIENT_ID/M365_CLIENT_SECRETare already handled — nothing hard-coded, nothing logged. This only affects a local developer testing script, not any production API surface or credential handling path.Breaking Changes
Purely additive: existing Graph/PowerShell collector testing paths in
test_collector.pyare untouched; the SharePoint client is only constructed when a SharePoint collector is explicitly requested.Rollback Plan
No DB migrations, no config changes required to revert.
Checklist
Screenshots
N/A — CLI developer tooling change, no UI.