MM-69450: Notify users when GitHub SAML SSO or token auth fails#1032
Merged
Conversation
Surface hidden GitHub auth failures (401 and SAML 403) by routing them through the existing handleRevokedToken path, which force-disconnects the account and sends the bot DM prompting reconnect via /github connect.
Contributor
Author
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds GitHub auth-failure detection and uses it to revoke stored tokens or continue partial responses across PR detail, LHS, and sidebar flows. It also propagates the first fetch error from concurrent calls and adds tests for the new auth-handling paths. ChangesAuth failure detection and token handling
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/plugin/plugin_test.go (1)
412-459: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCoverage gap: wrapped errors and
saml_failuresubstring untested.
isGitHubAuthFailure(plugin.go) unwraps viaerrors.As, and the GraphQL path matches two distinct substrings,"SAML enforcement"and"saml_failure". This table only exercises directly-typed errors and the"SAML enforcement"substring — neither a wrapped*github.ErrorResponse(e.g.fmt.Errorf("...: %w", ghErr)) nor the"saml_failure"substring path is covered.✅ Suggested additional sub-tests
+ t.Run("401 wrapped ErrorResponse", func(t *testing.T) { + ghErr := &github.ErrorResponse{Response: &http.Response{StatusCode: http.StatusUnauthorized}} + err := fmt.Errorf("github request failed: %w", ghErr) + require.True(t, isGitHubAuthFailure(err)) + }) + + t.Run("403 saml_failure graphql string", func(t *testing.T) { + err := errors.New("error in executing query: GraphQL: saml_failure") + require.True(t, isGitHubAuthFailure(err)) + })🤖 Prompt for 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. In `@server/plugin/plugin_test.go` around lines 412 - 459, Add test coverage for the missing `isGitHubAuthFailure` paths in `plugin_test.go`: the current `TestIsGitHubAuthFailure` only checks direct errors and the `"SAML enforcement"` string, but not wrapped `*github.ErrorResponse` values or the `"saml_failure"` GraphQL substring. Extend the existing subtests to verify `isGitHubAuthFailure` still returns true when a `github.ErrorResponse` is wrapped with `fmt.Errorf(... %w ...)`, and add a separate case that exercises the `saml_failure` match in `isGitHubAuthFailure` so both `errors.As` unwrapping and all GraphQL substring branches are covered.
🤖 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/api.go`:
- Around line 788-793: The concurrent error capture in the collector flow
currently keeps the first error seen in the fetchErr/fetchErrMu block, which can
let a non-auth failure mask a later 401/SAML auth failure. Update the error
selection logic in the relevant collector paths so that auth-related errors take
precedence over previously stored non-auth errors, while still preserving the
first auth error once seen. Use the existing fetchErr handling around the
collector goroutines in api.go to locate the fix and apply the same priority
logic in both affected blocks.
- Around line 1124-1130: The GetLHSData call already returns partial LHS slices
alongside err, but this wrapper discards them by replacing the results with
empty slices on any error. Update the return path in this api wrapper to
preserve reviewResp, assignmentResp, and openPRResp even when err is non-nil,
while still calling handleRevokedToken(c.GHInfo) for isGitHubAuthFailure(err)
before propagating the error.
In `@server/plugin/graphql/lhs_request.go`:
- Around line 43-47: The LHS request error handling in lhs_request.go is only
preserving the first failure via firstErr, which can hide a later SAML 403/auth
failure from the caller. Update the logic around the org loop and the return
path in the relevant fetching function(s) to either aggregate all org errors or
explicitly prefer auth/SAML failures over earlier transient errors. Use the
existing symbols around the error handling block (including firstErr and the org
iteration in the LHS fetch function) to ensure the returned error reflects the
most important failure.
---
Nitpick comments:
In `@server/plugin/plugin_test.go`:
- Around line 412-459: Add test coverage for the missing `isGitHubAuthFailure`
paths in `plugin_test.go`: the current `TestIsGitHubAuthFailure` only checks
direct errors and the `"SAML enforcement"` string, but not wrapped
`*github.ErrorResponse` values or the `"saml_failure"` GraphQL substring. Extend
the existing subtests to verify `isGitHubAuthFailure` still returns true when a
`github.ErrorResponse` is wrapped with `fmt.Errorf(... %w ...)`, and add a
separate case that exercises the `saml_failure` match in `isGitHubAuthFailure`
so both `errors.As` unwrapping and all GraphQL substring branches are covered.
🪄 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: 550d8bef-4f04-44f6-980d-bd728dba579b
📒 Files selected for processing (4)
server/plugin/api.goserver/plugin/graphql/lhs_request.goserver/plugin/plugin.goserver/plugin/plugin_test.go
nang2049
reviewed
Jul 1, 2026
|
|
||
| wg.Wait() | ||
|
|
||
| if isGitHubAuthFailure(fetchErr) { |
Contributor
There was a problem hiding this comment.
Nit: return an HTTP error after handling revocation instead of shipping empty results.
nang2049
reviewed
Jul 1, 2026
nang2049
left a comment
nang2049
left a comment
Contributor
There was a problem hiding this comment.
Thanks @jgheithcock added a few comments :)
…serving partial LHS data Prioritize GitHub auth errors over transient failures in concurrent PR fetches and per-org LHS requests so 401/SAML failures are not masked. Return 401 from getPrsDetails after revocation, preserve partial sidebar data on auth errors, and add isGitHubAuthFailure test coverage for wrapped errors and saml_failure GraphQL strings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When GitHub API calls fail due to revoked tokens (401) or SAML SSO authorization issues (403), the plugin previously logged the errors server-side but gave users no indication they needed to reconnect. This routes those failures through the existing
handleRevokedTokenflow, which force-disconnects the account and sends the bot DM telling the user to run/github connectagain.Changes:
isGitHubAuthFailureto detect 401 and SAML-specific 403 errors from both go-github (REST) and githubv4 (GraphQL) error shapes.useGitHubClientand wire upgetLHSData/getPrsDetailspaths that previously bypassed it.GetLHSDataso SAML failures are not swallowed when other orgs succeed.Ticket Link
https://mattermost.atlassian.net/browse/MM-69450
Checklist
make check-stylego test ./server/plugin/...Change Impact: High 🔴
Reasoning: These changes alter GitHub authentication-failure handling in a user-facing, security-adjacent flow and affect multiple plugin paths that interact with GitHub APIs. They also change when users are disconnected and notified, which is a critical behavior change with broad blast radius.
Regression Risk: Moderate to high, because the new error classification spans REST and GraphQL clients, includes SAML/401/403 edge cases, and touches shared GitHub client logic used across PR/LHS data fetching paths. Some affected branches are error-driven and harder to exercise exhaustively.
** QA Recommendation:** Manual QA is recommended, especially for GitHub token revocation and SAML SSO failure scenarios. Validate that affected users are disconnected and notified correctly, and confirm unrelated GitHub errors do not trigger the revoked-token flow.