MM-69609: Recover from GitHub SAML SSO expiry without forcing a full reconnect

[nang2049]

Summary

When GitHub returns a SAML SSO 403 it includes a one click reauthorize URL. Today we ignore it and force users through /github connect again. This change reads that URL and DMs it to the user instead of disconnecting them.

Ticket Link

https://mattermost.atlassian.net/browse/MM-69609

Manual QA Steps

• Revoke the SSO grant for the Mattermost GitHub OAuth app on GitHub (Settings → Applications → Authorized OAuth Apps → the app → revoke SSO for the org).
• In Mattermost, open the GitHub sidebar or run /github todo.
• Expect: a bot DM with a https://github.com/orgs/.../sso?... link. Account still shows as connected.
• Click the link, grant SSO works without running /github connect.

Change Impact: High 🔴

Reasoning: This change alters the GitHub authentication failure path, which is a user-facing critical flow tied to SSO/authorization recovery. It affects how failed auth is handled across multiple plugin paths and changes the recovery behavior from forced reconnect to SSO reauthorization messaging.

Regression Risk: Moderate-to-high, since it touches auth failure handling and notification behavior in shared plugin logic. The blast radius is contained to the GitHub integration, but it still impacts a high-risk flow and includes fallback behavior changes that could affect edge cases.

QA Recommendation: Manual QA is strongly recommended, especially around SAML SSO expiry, 403 handling, DM delivery, cooldown behavior, and fallback to reconnect when no reauthorize URL is present. Skipping manual QA is not recommended.
Generated by CodeRabbitAI

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 14e94540-1e2b-40a0-b589-62b9e79779dc

📥 Commits

Reviewing files that changed from the base of the PR and between 8551a8f and fdb4a67.

📒 Files selected for processing (2)

• server/plugin/plugin.go
• server/plugin/plugin_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• server/plugin/plugin_test.go
• server/plugin/plugin.go

---

📝 Walkthrough

Walkthrough

This PR adds SAML-aware GitHub auth-failure handling. GitHub auth errors can now trigger SSO reauthorization DMs when the response includes a reauthorize URL, otherwise the existing revoked-token flow runs. The API call sites and tests were updated to use the new routing.

Changes

SAML Reauthorization Handling

Layer / File(s)	Summary
Auth-failure routing and SSO parsing server/plugin/plugin.go	Adds handleAuthFailure, dmSAMLReauthorize, extractSSOAuthorizeURL, and cooldown constants; updates useGitHubClient to route auth errors through the new handler.
API auth-failure call sites server/plugin/api.go	PR details and left-hand sidebar data paths now call handleAuthFailure instead of handleRevokedToken on GitHub auth failures.
Parsing and notification tests server/plugin/plugin_test.go	Adds SSO URL parsing tests, a helper for SAML reauth DM expectations, and updated auth-failure cases for revoked, reauth, and no-notification outcomes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• mattermost/mattermost-plugin-github#1032: Both PRs change the GitHub auth-failure path in server/plugin, including the useGitHubClient flow and API-side callers.

Poem

A bunny saw SAML in the way,
ואז hopped a reauth DM today.
If the URL appears, the fix is near,
If not, the old reconnect path is clear.
Thump-thump, auth failures fade away 🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: handling GitHub SAML SSO expiry with reauthorization instead of a full reconnect.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch MM-69609-github-saml-reauth

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/plugin/plugin_test.go`:
- Around line 557-561: The const block in plugin_test.go has misaligned
assignment spacing that violates gofmt formatting. Update the const declarations
near the expectNone/expectRevoked/expectReauthDM symbols so the identifiers and
equals signs are formatted by gofmt, then run gofmt -w on the file to normalize
the alignment.

In `@server/plugin/plugin.go`:
- Around line 1428-1446: The auth failure DM throttle currently uses
`authFailureDMCooldown` as a plain integer, which makes `pluginapi.SetExpiry`
treat it as nanoseconds instead of one hour. Update the `authFailureDMCooldown`
constant in `handleAuthFailure`/`dmSAMLReauthorize` usage to a proper
`time.Duration` value such as `time.Hour`, so the KV expiry and DM throttle
window behave correctly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 145870d0-be59-48b3-ac58-16ca19d2db5f

📥 Commits

Reviewing files that changed from the base of the PR and between 651b02f and 8551a8f.

📒 Files selected for processing (3)

• server/plugin/api.go
• server/plugin/plugin.go
• server/plugin/plugin_test.go

[jgheithcock]

LGTM. I put in one nit re adding two tests, I do not think they are required but do make it more complete.

[jgheithcock]

Nit: There is no test for the actual throttle (not sending the DM) nor the test for not sending if there is a KV error. You could add a setOK and setErr here and change the "403 SAML REST with SSO URL" to pass in setOK: true and create two new ones:

{
			name:   "403 SAML REST during cooldown suppresses DM",
			err:    samlRESTErr,
			expect: expectReauth,
			setOK:  false,
		},
		{
			name:   "403 SAML REST KV error skips DM",
			err:    samlRESTErr,
			expect: expectReauth,
			setOK:  false,
			setErr: errors.New("kv unavailable"),
		},

expectSAMLReauthorizeNotification would then need to pass in the setOK and setErr and use those in your mockKvStore.EXPECT().Set call. (And return if setOK was false)

[avasconcelos114]

LGTM!