Skip to content

local_o365: only run notifysecretexpiry task for connected Microsoft IdP sites using client secret - #3772

Open
Lai Wei (weilai-irl) wants to merge 1 commit into
MOODLE_501_STABLEfrom
wip-140354-m501
Open

Lai Wei (weilai-irl) wants to merge 1 commit into
MOODLE_501_STABLEfrom
wip-140354-m501

Conversation

@weilai-irl

Copy link
Copy Markdown
Collaborator

The task now skips and succeeds unless the site uses a Microsoft IdP, the client secret authentication method, and a working Microsoft 365 connection. Remove the unused errorunsupportedsecretauthenticationmethod string.

…IdP sites using client secret

The task now skips and succeeds unless the site uses a Microsoft IdP, the client secret authentication method, and a working Microsoft 365 connection. Remove the unused errorunsupportedsecretauthenticationmethod string.
Copilot AI balanced review requested due to automatic review settings October 9, 2026 08:10
@weilai-irl Lai Wei (weilai-irl) self-assigned this Oct 9, 2026
@weilai-irl Lai Wei (weilai-irl) added this to the 2026-04 milestone Oct 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The functional behavior change and lang-string removal are not accompanied by the conventional $plugin->version bump in local/o365/version.php, which this repo expects for plugin changes.

0 open findings

What changed in this PR

This PR narrows the conditions under which the local_o365 notifysecretexpiry scheduled task does real work. Instead of throwing exceptions (which surface as recurring task failures) when the site isn't connected or isn't using the client-secret auth method, the task now mtraces an explanatory message and returns true (success) when any precondition isn't met. This avoids noisy, false-positive task failures on sites that legitimately have no expiring client secret to monitor (e.g. certificate-based auth or disconnected integrations).

Changes:

  • Added an early skip-and-succeed check for the client-secret auth method (clientauthmethod != AUTH_OIDC_AUTH_METHOD_SECRET), moved ahead of the connection check.
  • Changed the "not connected" branch from throwing error_not_connected to skipping and returning true, and refreshed the "not configured" messaging.
  • Removed the now-unused errorunsupportedsecretauthenticationmethod language string.
File Description
local/​o365/​classes/​task/​notifysecretexpiry.php Reorders preconditions and converts hard failures into graceful skips (return true) for non-secret auth and disconnected sites.
local/​o365/​lang/​en/​local_o365.php Removes the orphaned errorunsupportedsecretauthenticationmethod string that the deleted exception used.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

"Notify site admin about Microsoft Entra ID app secret expiry" task fails if local_o365 is not in use

2 participants