Skip to content

Commit 91206ad

Browse files
nang2049Nevyana Angelova
andauthored
MM-69609: Recover from GitHub SAML SSO expiry without forcing a full reconnect (#1036)
* MM-69609: Recover from GitHub SAML SSO expiry without forcing a full reconnect * linT * Coderabbit suggestions --------- Co-authored-by: Nevyana Angelova <nevyangelova@192.168.100.47>
1 parent 651b02f commit 91206ad

3 files changed

Lines changed: 117 additions & 11 deletions

File tree

server/plugin/api.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -799,7 +799,7 @@ func (p *Plugin) getPrsDetails(c *UserContext, w http.ResponseWriter, r *http.Re
799799
wg.Wait()
800800

801801
if isGitHubAuthFailure(fetchErr) {
802-
p.handleRevokedToken(c.GHInfo)
802+
p.handleAuthFailure(c.GHInfo, fetchErr)
803803
p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "Not authorized.", StatusCode: http.StatusUnauthorized})
804804
return
805805
}
@@ -1124,7 +1124,7 @@ func (p *Plugin) getLHSData(c *UserContext) (reviewResp []*graphql.GithubPRDetai
11241124

11251125
reviewResp, assignmentResp, openPRResp, err = graphQLClient.GetLHSData(c.Ctx)
11261126
if isGitHubAuthFailure(err) {
1127-
p.handleRevokedToken(c.GHInfo)
1127+
p.handleAuthFailure(c.GHInfo, err)
11281128
}
11291129
if err != nil {
11301130
return reviewResp, assignmentResp, openPRResp, err

server/plugin/plugin.go

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1350,7 +1350,7 @@ func (p *Plugin) useGitHubClient(info *GitHubUserInfo, toRun func(info *GitHubUs
13501350
}
13511351

13521352
if isGitHubAuthFailure(err) {
1353-
p.handleRevokedToken(info)
1353+
p.handleAuthFailure(info, err)
13541354
}
13551355

13561356
return err
@@ -1424,3 +1424,57 @@ func (p *Plugin) handleRevokedToken(info *GitHubUserInfo) {
14241424
p.disconnectGitHubAccount(info.UserID)
14251425
p.CreateBotDMPost(info.UserID, "Your Github account was disconnected due to an invalid or revoked authorization token. Reconnect your account using the `/github connect` command.", "custom_git_revoked_token")
14261426
}
1427+
1428+
const (
1429+
authFailureDMKey = "auth_failure_dm_"
1430+
authFailureDMCooldown = time.Hour
1431+
)
1432+
1433+
func (p *Plugin) handleAuthFailure(info *GitHubUserInfo, err error) {
1434+
if ssoURL := extractSSOAuthorizeURL(err); ssoURL != "" {
1435+
p.dmSAMLReauthorize(info, ssoURL)
1436+
return
1437+
}
1438+
p.handleRevokedToken(info)
1439+
}
1440+
1441+
func (p *Plugin) dmSAMLReauthorize(info *GitHubUserInfo, ssoURL string) {
1442+
// Throttle only DM once per cooldown window per user.
1443+
ok, err := p.store.Set(authFailureDMKey+info.UserID, []byte("1"),
1444+
pluginapi.SetExpiry(authFailureDMCooldown),
1445+
pluginapi.SetAtomic(nil),
1446+
)
1447+
if err != nil {
1448+
p.client.Log.Warn("Failed to set SAML reauthorize DM cooldown", "error", err.Error())
1449+
}
1450+
if !ok {
1451+
return
1452+
}
1453+
msg := fmt.Sprintf(
1454+
"Your GitHub SAML SSO session expired, so GitHub temporarily blocked this plugin from acting on your behalf. "+
1455+
"Re-authorize SSO for your organization here: %s\n\n"+
1456+
"You do not need to run `/github connect` again — once SSO is re-authorized, this account will keep working.",
1457+
ssoURL,
1458+
)
1459+
p.CreateBotDMPost(info.UserID, msg, "custom_git_saml_reauth")
1460+
}
1461+
1462+
// extractSSOAuthorizeURL pulls the SSO reauthorization url that GitHub
1463+
// includes in the X-GitHub-SSO header on SAML enforced 403 responses
1464+
func extractSSOAuthorizeURL(err error) string {
1465+
var ghErr *github.ErrorResponse
1466+
if !errors.As(err, &ghErr) || ghErr.Response == nil {
1467+
return ""
1468+
}
1469+
header := ghErr.Response.Header.Get("X-GitHub-SSO")
1470+
if header == "" {
1471+
return ""
1472+
}
1473+
for part := range strings.SplitSeq(header, ";") {
1474+
part = strings.TrimSpace(part)
1475+
if u, ok := strings.CutPrefix(part, "url="); ok {
1476+
return strings.TrimSpace(u)
1477+
}
1478+
}
1479+
return ""
1480+
}

server/plugin/plugin_test.go

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,36 @@ func TestIsGitHubAuthFailure(t *testing.T) {
470470
})
471471
}
472472

473+
func TestExtractSSOAuthorizeURL(t *testing.T) {
474+
makeErr := func(header string) error {
475+
h := http.Header{}
476+
if header != "" {
477+
h.Set("X-GitHub-SSO", header)
478+
}
479+
return &github.ErrorResponse{
480+
Response: &http.Response{StatusCode: http.StatusForbidden, Header: h, Request: &http.Request{}},
481+
}
482+
}
483+
tests := []struct {
484+
name string
485+
err error
486+
want string
487+
}{
488+
{"nil", nil, ""},
489+
{"non-github error", errors.New("boom"), ""},
490+
{"no header", makeErr(""), ""},
491+
{"required with url", makeErr("required; url=https://github.com/orgs/foo/sso?authorization_request=abc"), "https://github.com/orgs/foo/sso?authorization_request=abc"},
492+
{"partial-results with url", makeErr("partial-results; url=https://github.com/orgs/foo/sso"), "https://github.com/orgs/foo/sso"},
493+
{"header without url", makeErr("required"), ""},
494+
{"whitespace tolerant", makeErr("required ; url=https://github.com/orgs/foo/sso "), "https://github.com/orgs/foo/sso"},
495+
}
496+
for _, tc := range tests {
497+
t.Run(tc.name, func(t *testing.T) {
498+
require.Equal(t, tc.want, extractSSOAuthorizeURL(tc.err))
499+
})
500+
}
501+
}
502+
473503
func connectedGitHubUserInfo(t *testing.T) *GitHubUserInfo {
474504
t.Helper()
475505
encryptedToken, err := encrypt([]byte(testNewKey), MockAccessToken)
@@ -510,21 +540,39 @@ func expectRevokedTokenNotification(api *plugintest.API, mockKvStore *mocks.Mock
510540
})).Return(&model.Post{}, nil).Once()
511541
}
512542

543+
func expectSAMLReauthorizeNotification(api *plugintest.API, mockKvStore *mocks.MockKvStore, userInfo *GitHubUserInfo) {
544+
mockKvStore.EXPECT().Set(authFailureDMKey+userInfo.UserID, gomock.Any(), gomock.Any(), gomock.Any()).Return(true, nil)
545+
api.On("GetDirectChannel", userInfo.UserID, MockBotID).Return(&model.Channel{Id: "dmchannel"}, nil)
546+
api.On("CreatePost", mock.MatchedBy(func(post *model.Post) bool {
547+
return post.UserId == MockBotID &&
548+
post.ChannelId == "dmchannel" &&
549+
post.Type == "custom_git_saml_reauth" &&
550+
strings.Contains(post.Message, "https://github.com/orgs/foo/sso")
551+
})).Return(&model.Post{}, nil).Once()
552+
}
553+
513554
func TestUseGitHubClient_AuthFailureNotifiesUser(t *testing.T) {
514555
samlGraphQLErr := errors.New("error in executing query: GraphQL: Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization.")
515556

557+
const (
558+
expectNone = ""
559+
expectRevoked = "revoked"
560+
expectReauthDM = "reauth"
561+
)
562+
516563
tests := []struct {
517564
name string
518565
err error
519-
notify bool
566+
expect string
520567
}{
521568
{
522569
name: "401 bad credentials",
523570
err: errors.New(invalidTokenError),
524-
notify: true,
571+
expect: expectRevoked,
525572
},
526573
{
527-
name: "403 SAML REST",
574+
// SSO URL is available: keep account connected, DM the reauthorize link.
575+
name: "403 SAML REST with SSO URL",
528576
err: &github.ErrorResponse{
529577
Message: "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization.",
530578
Response: &http.Response{
@@ -533,25 +581,26 @@ func TestUseGitHubClient_AuthFailureNotifiesUser(t *testing.T) {
533581
Request: &http.Request{},
534582
},
535583
},
536-
notify: true,
584+
expect: expectReauthDM,
537585
},
538586
{
587+
// GraphQL error string carries no header we can parse — fall back to disconnect.
539588
name: "403 SAML graphql",
540589
err: samlGraphQLErr,
541-
notify: true,
590+
expect: expectRevoked,
542591
},
543592
{
544593
name: "403 unrelated",
545594
err: &github.ErrorResponse{
546595
Message: "Forbidden",
547596
Response: &http.Response{StatusCode: http.StatusForbidden, Request: &http.Request{}},
548597
},
549-
notify: false,
598+
expect: expectNone,
550599
},
551600
{
552601
name: "generic error",
553602
err: errors.New("connection reset"),
554-
notify: false,
603+
expect: expectNone,
555604
},
556605
}
557606

@@ -561,8 +610,11 @@ func TestUseGitHubClient_AuthFailureNotifiesUser(t *testing.T) {
561610
defer ctrl.Finish()
562611

563612
userInfo := connectedGitHubUserInfo(t)
564-
if tc.notify {
613+
switch tc.expect {
614+
case expectRevoked:
565615
expectRevokedTokenNotification(api, mockKvStore, userInfo)
616+
case expectReauthDM:
617+
expectSAMLReauthorizeNotification(api, mockKvStore, userInfo)
566618
}
567619

568620
err := p.useGitHubClient(userInfo, func(_ *GitHubUserInfo, _ *oauth2.Token) error {

0 commit comments

Comments
 (0)