MM-69450: Surface SAML SSO authorization failures to users#1030
Closed
jgheithcock wants to merge 1 commit into
Closed
MM-69450: Surface SAML SSO authorization failures to users#1030jgheithcock wants to merge 1 commit into
jgheithcock wants to merge 1 commit into
Conversation
When GitHub rejects API calls due to missing SAML SSO authorization, notify users via a deduplicated bot DM and show guidance in the RHS and link tooltips.
📝 WalkthroughWalkthroughAdds GitHub SAML SSO error detection, threads SAML-required state through server data and API responses, sends one-time notifications, and renders SAML messages in the sidebar and link tooltip UI. ChangesSAML SSO handling
Sequence Diagram(s)sequenceDiagram
participant getIssueByNumber
participant writeSAMLSSOErrorIfNeeded
participant notifySAMLSSORequired
participant store
getIssueByNumber->>writeSAMLSSOErrorIfNeeded: pass GitHub error
writeSAMLSSOErrorIfNeeded->>notifySAMLSSORequired: SAML SSO required
notifySAMLSSORequired->>store: Get and Set notification key
writeSAMLSSOErrorIfNeeded-->>getIssueByNumber: return true after 403
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/saml_sso.go`:
- Around line 19-29: The SAML SSO notification flow in notifySAMLSSORequired is
racy because it does Get, then CreateBotDMPost, then Set, allowing concurrent
callers from the API path to send duplicate DMs. Update the logic to atomically
claim the samlSSONotifiedKey before posting, or otherwise serialize access
around the check-and-set sequence, so only one goroutine can proceed to
CreateBotDMPost. Keep the existing store and logging flow in the Plugin method,
but make the notification state transition happen as a single guarded operation.
In `@webapp/src/components/link_tooltip/link_tooltip.jsx`:
- Around line 70-75: The tooltip’s fetch effect in link_tooltip.jsx is being
short-circuited by a stale errorMessage, so reconnects never trigger initData
again. Update the logic around the effect and the data/error state handling in
LinkTooltip so a successful reconnect clears or ignores the previous error
before retrying, and ensure the early return only blocks when the current state
truly prevents a refetch. Apply the same stale-error handling to the related
tooltip rendering paths in the other affected section as well.
🪄 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: 10fb2f16-3aaf-421a-8f09-dbae26a68f16
📒 Files selected for processing (12)
server/githuberrors/saml.goserver/githuberrors/saml_test.goserver/plugin/api.goserver/plugin/graphql/lhs_request.goserver/plugin/plugin.goserver/plugin/saml_sso.goserver/plugin/saml_sso_test.gowebapp/src/components/link_tooltip/link_tooltip.jsxwebapp/src/components/sidebar_right/index.jsxwebapp/src/components/sidebar_right/sidebar_right.jsxwebapp/src/selectors.tswebapp/src/types/github_types.ts
Comment on lines
+19
to
+29
| func (p *Plugin) notifySAMLSSORequired(userID string) { | ||
| key := userID + samlSSONotifiedKey | ||
| var notified bool | ||
| if err := p.store.Get(key, ¬ified); err == nil && notified { | ||
| return | ||
| } | ||
|
|
||
| p.CreateBotDMPost(userID, samlSSOUserMessage, "custom_git_saml_sso") | ||
| if _, err := p.store.Set(key, true); err != nil { | ||
| p.client.Log.Warn("Failed to store SAML SSO notification state", "userID", userID, "error", err.Error()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make the notification claim atomic.
notifySAMLSSORequired does Get → CreateBotDMPost → Set, so two concurrent SAML failures can both observe "not notified" and each send a DM before either writes the flag. server/plugin/api.go, Lines 818-845 already invoke this path from parallel goroutines, so the dedup guarantee is racy. Claim the KV key atomically, or otherwise serialize this section across callers, before posting.
🤖 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/saml_sso.go` around lines 19 - 29, The SAML SSO notification
flow in notifySAMLSSORequired is racy because it does Get, then CreateBotDMPost,
then Set, allowing concurrent callers from the API path to send duplicate DMs.
Update the logic to atomically claim the samlSSONotifiedKey before posting, or
otherwise serialize access around the check-and-set sequence, so only one
goroutine can proceed to CreateBotDMPost. Keep the existing store and logging
flow in the Plugin method, but make the notification state transition happen as
a single guarded operation.
Comment on lines
+70
to
+75
| if (!connected || data || errorMessage || ((typeof (show) !== 'undefined' || show != null) && !show)) { | ||
| return; | ||
| } | ||
|
|
||
| initData(); | ||
| }, [connected, data, href, show, enterpriseURL]); | ||
| }, [connected, data, errorMessage, href, show, enterpriseURL]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale errorMessage blocks re-fetch after reconnect
Line 70 short-circuits when errorMessage is set, but nothing clears that state on later retries. After /github connect, this tooltip can remain stuck showing the old SAML error until remount.
Suggested fix
useEffect(() => {
+ // reset stale state when input context changes
+ setErrorMessage(null);
+ setData(null);
+
const initData = async () => {
...
};
// show is not provided for Mattermost Server < 5.28
- if (!connected || data || errorMessage || ((typeof (show) !== 'undefined' || show != null) && !show)) {
+ if (!connected || data || ((typeof (show) !== 'undefined' || show != null) && !show)) {
return;
}
initData();
}, [connected, data, errorMessage, href, show, enterpriseURL]);Also applies to: 137-154
🤖 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 `@webapp/src/components/link_tooltip/link_tooltip.jsx` around lines 70 - 75,
The tooltip’s fetch effect in link_tooltip.jsx is being short-circuited by a
stale errorMessage, so reconnects never trigger initData again. Update the logic
around the effect and the data/error state handling in LinkTooltip so a
successful reconnect clears or ignores the previous error before retrying, and
ensure the early return only blocks when the current state truly prevents a
refetch. Apply the same stale-error handling to the related tooltip rendering
paths in the other affected section as well.
Contributor
Author
|
Closing as this is too large of a change for what we are looking to resolve. |
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
1 participant
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
X-GitHub-SSOheader, GraphQL errors)/github disconnectand/github connectagainTest plan
/github disconnectthen/github connect, re-authorize SAML on GitHub — confirm warnings clear and data loads againgo test ./server/githuberrors/... ./server/plugin/... -run SAMLmake check-stylepassesChange Impact: High 🔴
Reasoning: This change affects GitHub authentication/authorization handling and surfaces failures across critical user-facing flows. It also spans backend error detection, notification persistence, API responses, and multiple UI entry points, increasing the blast radius.
Regression Risk: Moderate to high, since the new SAML SSO handling path is adjacent to existing GitHub request/error handling and introduces behavior changes in sidebar data loading, link previews, and bot DM notifications.
** QA Recommendation:** Manual QA is strongly recommended, especially for revoked/partial SSO authorization flows, repeated notifications, sidebar messaging, and link hover behavior. Skipping manual QA is not advised.
Generated by CodeRabbitAI