mirror of https://github.com/coder/coder.git
* fix: always attempt external auth refresh when fetching * refactor validate to check expiry when considering "valid"
This commit is contained in:
parent
eeef56a655
commit
d66e6e78ee
|
@ -6,6 +6,7 @@ import (
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"golang.org/x/exp/maps"
|
"golang.org/x/exp/maps"
|
||||||
|
"golang.org/x/oauth2"
|
||||||
|
|
||||||
"github.com/coder/coder/v2/coderd/database/dbtime"
|
"github.com/coder/coder/v2/coderd/database/dbtime"
|
||||||
"github.com/coder/coder/v2/coderd/rbac"
|
"github.com/coder/coder/v2/coderd/rbac"
|
||||||
|
@ -268,6 +269,14 @@ func (u ExternalAuthLink) RBACObject() rbac.Object {
|
||||||
return rbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String())
|
return rbac.ResourceUserData.WithID(u.UserID).WithOwner(u.UserID.String())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (u ExternalAuthLink) OAuthToken() *oauth2.Token {
|
||||||
|
return &oauth2.Token{
|
||||||
|
AccessToken: u.OAuthAccessToken,
|
||||||
|
RefreshToken: u.OAuthRefreshToken,
|
||||||
|
Expiry: u.OAuthExpiry,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func (u UserLink) RBACObject() rbac.Object {
|
func (u UserLink) RBACObject() rbac.Object {
|
||||||
// I assume UserData is ok?
|
// I assume UserData is ok?
|
||||||
return rbac.ResourceUserData.WithOwner(u.UserID.String()).WithID(u.UserID)
|
return rbac.ResourceUserData.WithOwner(u.UserID.String()).WithID(u.UserID)
|
||||||
|
|
|
@ -57,7 +57,7 @@ func (api *API) externalAuthByID(w http.ResponseWriter, r *http.Request) {
|
||||||
}
|
}
|
||||||
var eg errgroup.Group
|
var eg errgroup.Group
|
||||||
eg.Go(func() (err error) {
|
eg.Go(func() (err error) {
|
||||||
res.Authenticated, res.User, err = config.ValidateToken(ctx, link.OAuthAccessToken)
|
res.Authenticated, res.User, err = config.ValidateToken(ctx, link.OAuthToken())
|
||||||
return err
|
return err
|
||||||
})
|
})
|
||||||
eg.Go(func() (err error) {
|
eg.Go(func() (err error) {
|
||||||
|
|
|
@ -138,7 +138,7 @@ func (c *Config) RefreshToken(ctx context.Context, db database.Store, externalAu
|
||||||
retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second)
|
retryCtx, retryCtxCancel := context.WithTimeout(ctx, time.Second)
|
||||||
defer retryCtxCancel()
|
defer retryCtxCancel()
|
||||||
validate:
|
validate:
|
||||||
valid, _, err := c.ValidateToken(ctx, token.AccessToken)
|
valid, _, err := c.ValidateToken(ctx, token)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
|
return externalAuthLink, false, xerrors.Errorf("validate external auth token: %w", err)
|
||||||
}
|
}
|
||||||
|
@ -179,7 +179,14 @@ validate:
|
||||||
|
|
||||||
// ValidateToken ensures the Git token provided is valid!
|
// ValidateToken ensures the Git token provided is valid!
|
||||||
// The user is optionally returned if the provider supports it.
|
// The user is optionally returned if the provider supports it.
|
||||||
func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *codersdk.ExternalAuthUser, error) {
|
func (c *Config) ValidateToken(ctx context.Context, link *oauth2.Token) (bool, *codersdk.ExternalAuthUser, error) {
|
||||||
|
if link == nil {
|
||||||
|
return false, nil, xerrors.New("validate external auth token: token is nil")
|
||||||
|
}
|
||||||
|
if !link.Expiry.IsZero() && link.Expiry.Before(dbtime.Now()) {
|
||||||
|
return false, nil, nil
|
||||||
|
}
|
||||||
|
|
||||||
if c.ValidateURL == "" {
|
if c.ValidateURL == "" {
|
||||||
// Default that the token is valid if no validation URL is provided.
|
// Default that the token is valid if no validation URL is provided.
|
||||||
return true, nil, nil
|
return true, nil, nil
|
||||||
|
@ -189,7 +196,7 @@ func (c *Config) ValidateToken(ctx context.Context, token string) (bool, *coders
|
||||||
return false, nil, err
|
return false, nil, err
|
||||||
}
|
}
|
||||||
|
|
||||||
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
|
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", link.AccessToken))
|
||||||
res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req)
|
res, err := c.InstrumentedOAuth2Config.Do(ctx, promoauth.SourceValidateToken, req)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, nil, err
|
return false, nil, err
|
||||||
|
@ -396,10 +403,15 @@ func (c *DeviceAuth) ExchangeDeviceCode(ctx context.Context, deviceCode string)
|
||||||
if body.Error != "" {
|
if body.Error != "" {
|
||||||
return nil, xerrors.New(body.Error)
|
return nil, xerrors.New(body.Error)
|
||||||
}
|
}
|
||||||
|
// If expiresIn is 0, then the token never expires.
|
||||||
|
expires := dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second)
|
||||||
|
if body.ExpiresIn == 0 {
|
||||||
|
expires = time.Time{}
|
||||||
|
}
|
||||||
return &oauth2.Token{
|
return &oauth2.Token{
|
||||||
AccessToken: body.AccessToken,
|
AccessToken: body.AccessToken,
|
||||||
RefreshToken: body.RefreshToken,
|
RefreshToken: body.RefreshToken,
|
||||||
Expiry: dbtime.Now().Add(time.Duration(body.ExpiresIn) * time.Second),
|
Expiry: expires,
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -75,7 +75,7 @@ func TestInstrument(t *testing.T) {
|
||||||
require.Equal(t, count("TokenSource"), 1)
|
require.Equal(t, count("TokenSource"), 1)
|
||||||
|
|
||||||
// Try a validate
|
// Try a validate
|
||||||
valid, _, err := cfg.ValidateToken(ctx, refreshed.AccessToken)
|
valid, _, err := cfg.ValidateToken(ctx, refreshed)
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.True(t, valid)
|
require.True(t, valid)
|
||||||
require.Equal(t, count("ValidateToken"), 1)
|
require.Equal(t, count("ValidateToken"), 1)
|
||||||
|
|
|
@ -2037,78 +2037,26 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
if listen {
|
var previousToken *database.ExternalAuthLink
|
||||||
// Since we're ticking frequently and this sign-in operation is rare,
|
// handleRetrying will attempt to continually check for a new token
|
||||||
// we are OK with polling to avoid the complexity of pubsub.
|
// if listen is true. This is useful if an error is encountered in the
|
||||||
ticker, done := api.NewTicker(time.Second)
|
// original single flow.
|
||||||
defer done()
|
//
|
||||||
var previousToken database.ExternalAuthLink
|
// By default, if no errors are encountered, then the single flow response
|
||||||
for {
|
// is returned.
|
||||||
select {
|
handleRetrying := func(code int, response any) {
|
||||||
case <-ctx.Done():
|
if !listen {
|
||||||
return
|
httpapi.Write(ctx, rw, code, response)
|
||||||
case <-ticker:
|
|
||||||
}
|
|
||||||
externalAuthLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{
|
|
||||||
ProviderID: externalAuthConfig.ID,
|
|
||||||
UserID: workspace.OwnerID,
|
|
||||||
})
|
|
||||||
if err != nil {
|
|
||||||
if errors.Is(err, sql.ErrNoRows) {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
|
||||||
Message: "Failed to get external auth link.",
|
|
||||||
Detail: err.Error(),
|
|
||||||
})
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
// Expiry may be unset if the application doesn't configure tokens
|
|
||||||
// to expire.
|
|
||||||
// See
|
|
||||||
// https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app.
|
|
||||||
if externalAuthLink.OAuthExpiry.Before(dbtime.Now()) && !externalAuthLink.OAuthExpiry.IsZero() {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
// Only attempt to revalidate an oauth token if it has actually changed.
|
|
||||||
// No point in trying to validate the same token over and over again.
|
|
||||||
if previousToken.OAuthAccessToken == externalAuthLink.OAuthAccessToken &&
|
|
||||||
previousToken.OAuthRefreshToken == externalAuthLink.OAuthRefreshToken &&
|
|
||||||
previousToken.OAuthExpiry == externalAuthLink.OAuthExpiry {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
|
|
||||||
valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthAccessToken)
|
|
||||||
if err != nil {
|
|
||||||
api.Logger.Warn(ctx, "failed to validate external auth token",
|
|
||||||
slog.F("workspace_owner_id", workspace.OwnerID.String()),
|
|
||||||
slog.F("validate_url", externalAuthConfig.ValidateURL),
|
|
||||||
slog.Error(err),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
previousToken = externalAuthLink
|
|
||||||
if !valid {
|
|
||||||
continue
|
|
||||||
}
|
|
||||||
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
|
|
||||||
if err != nil {
|
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
|
||||||
Message: "Failed to create external auth response.",
|
|
||||||
Detail: err.Error(),
|
|
||||||
})
|
|
||||||
return
|
|
||||||
}
|
|
||||||
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
api.workspaceAgentsExternalAuthListen(ctx, rw, previousToken, externalAuthConfig, workspace)
|
||||||
}
|
}
|
||||||
|
|
||||||
// This is the URL that will redirect the user with a state token.
|
// This is the URL that will redirect the user with a state token.
|
||||||
redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s", externalAuthConfig.ID))
|
redirectURL, err := api.AccessURL.Parse(fmt.Sprintf("/external-auth/%s", externalAuthConfig.ID))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
handleRetrying(http.StatusInternalServerError, codersdk.Response{
|
||||||
Message: "Failed to parse access URL.",
|
Message: "Failed to parse access URL.",
|
||||||
Detail: err.Error(),
|
Detail: err.Error(),
|
||||||
})
|
})
|
||||||
|
@ -2121,6 +2069,75 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||||
})
|
})
|
||||||
if err != nil {
|
if err != nil {
|
||||||
if !errors.Is(err, sql.ErrNoRows) {
|
if !errors.Is(err, sql.ErrNoRows) {
|
||||||
|
handleRetrying(http.StatusInternalServerError, codersdk.Response{
|
||||||
|
Message: "Failed to get external auth link.",
|
||||||
|
Detail: err.Error(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
|
||||||
|
URL: redirectURL.String(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
|
externalAuthLink, valid, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
|
||||||
|
if err != nil {
|
||||||
|
handleRetrying(http.StatusInternalServerError, codersdk.Response{
|
||||||
|
Message: "Failed to refresh external auth token.",
|
||||||
|
Detail: err.Error(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if !valid {
|
||||||
|
// Set the previous token so the retry logic will skip validating the
|
||||||
|
// same token again. This should only be set if the token is invalid and there
|
||||||
|
// was no error. If it is invalid because of an error, then we should recheck.
|
||||||
|
previousToken = &externalAuthLink
|
||||||
|
handleRetrying(http.StatusOK, agentsdk.ExternalAuthResponse{
|
||||||
|
URL: redirectURL.String(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
|
||||||
|
if err != nil {
|
||||||
|
handleRetrying(http.StatusInternalServerError, codersdk.Response{
|
||||||
|
Message: "Failed to create external auth response.",
|
||||||
|
Detail: err.Error(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
||||||
|
}
|
||||||
|
|
||||||
|
func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.ResponseWriter, previous *database.ExternalAuthLink, externalAuthConfig *externalauth.Config, workspace database.Workspace) {
|
||||||
|
// Since we're ticking frequently and this sign-in operation is rare,
|
||||||
|
// we are OK with polling to avoid the complexity of pubsub.
|
||||||
|
ticker, done := api.NewTicker(time.Second)
|
||||||
|
defer done()
|
||||||
|
// If we have a previous token that is invalid, we should not check this again.
|
||||||
|
// This serves to prevent doing excessive unauthorized requests to the external
|
||||||
|
// auth provider. For github, this limit is 60 per hour, so saving a call
|
||||||
|
// per invalid token can be significant.
|
||||||
|
var previousToken database.ExternalAuthLink
|
||||||
|
if previous != nil {
|
||||||
|
previousToken = *previous
|
||||||
|
}
|
||||||
|
for {
|
||||||
|
select {
|
||||||
|
case <-ctx.Done():
|
||||||
|
return
|
||||||
|
case <-ticker:
|
||||||
|
}
|
||||||
|
externalAuthLink, err := api.Database.GetExternalAuthLink(ctx, database.GetExternalAuthLinkParams{
|
||||||
|
ProviderID: externalAuthConfig.ID,
|
||||||
|
UserID: workspace.OwnerID,
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
if errors.Is(err, sql.ErrNoRows) {
|
||||||
|
continue
|
||||||
|
}
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||||
Message: "Failed to get external auth link.",
|
Message: "Failed to get external auth link.",
|
||||||
Detail: err.Error(),
|
Detail: err.Error(),
|
||||||
|
@ -2128,35 +2145,45 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{
|
// Expiry may be unset if the application doesn't configure tokens
|
||||||
URL: redirectURL.String(),
|
// to expire.
|
||||||
})
|
// See
|
||||||
return
|
// https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app.
|
||||||
}
|
if externalAuthLink.OAuthExpiry.Before(dbtime.Now()) && !externalAuthLink.OAuthExpiry.IsZero() {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
externalAuthLink, updated, err := externalAuthConfig.RefreshToken(ctx, api.Database, externalAuthLink)
|
// Only attempt to revalidate an oauth token if it has actually changed.
|
||||||
if err != nil {
|
// No point in trying to validate the same token over and over again.
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
if previousToken.OAuthAccessToken == externalAuthLink.OAuthAccessToken &&
|
||||||
Message: "Failed to refresh external auth token.",
|
previousToken.OAuthRefreshToken == externalAuthLink.OAuthRefreshToken &&
|
||||||
Detail: err.Error(),
|
previousToken.OAuthExpiry == externalAuthLink.OAuthExpiry {
|
||||||
})
|
continue
|
||||||
|
}
|
||||||
|
|
||||||
|
valid, _, err := externalAuthConfig.ValidateToken(ctx, externalAuthLink.OAuthToken())
|
||||||
|
if err != nil {
|
||||||
|
api.Logger.Warn(ctx, "failed to validate external auth token",
|
||||||
|
slog.F("workspace_owner_id", workspace.OwnerID.String()),
|
||||||
|
slog.F("validate_url", externalAuthConfig.ValidateURL),
|
||||||
|
slog.Error(err),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
previousToken = externalAuthLink
|
||||||
|
if !valid {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
|
||||||
|
if err != nil {
|
||||||
|
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
||||||
|
Message: "Failed to create external auth response.",
|
||||||
|
Detail: err.Error(),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
|
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if !updated {
|
|
||||||
httpapi.Write(ctx, rw, http.StatusOK, agentsdk.ExternalAuthResponse{
|
|
||||||
URL: redirectURL.String(),
|
|
||||||
})
|
|
||||||
return
|
|
||||||
}
|
|
||||||
resp, err := createExternalAuthResponse(externalAuthConfig.Type, externalAuthLink.OAuthAccessToken, externalAuthLink.OAuthExtra)
|
|
||||||
if err != nil {
|
|
||||||
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
|
|
||||||
Message: "Failed to create external auth response.",
|
|
||||||
Detail: err.Error(),
|
|
||||||
})
|
|
||||||
return
|
|
||||||
}
|
|
||||||
httpapi.Write(ctx, rw, http.StatusOK, resp)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// createExternalAuthResponse creates an ExternalAuthResponse based on the
|
// createExternalAuthResponse creates an ExternalAuthResponse based on the
|
||||||
|
|
|
@ -1577,7 +1577,7 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) {
|
||||||
},
|
},
|
||||||
ExternalAuthConfigs: []*externalauth.Config{
|
ExternalAuthConfigs: []*externalauth.Config{
|
||||||
fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) {
|
fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) {
|
||||||
cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String()
|
cfg.Type = codersdk.EnhancedExternalAuthProviderGitLab.String()
|
||||||
}),
|
}),
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
|
@ -1623,7 +1623,8 @@ func TestWorkspaceAgentExternalAuthListen(t *testing.T) {
|
||||||
ticks <- time.Now()
|
ticks <- time.Now()
|
||||||
}
|
}
|
||||||
cancel()
|
cancel()
|
||||||
// We expect only 1
|
// We expect only 1. One from the initial "Refresh" attempt, and the
|
||||||
|
// other should be skipped.
|
||||||
// In a failed test, you will likely see 9, as the last one
|
// In a failed test, you will likely see 9, as the last one
|
||||||
// gets canceled.
|
// gets canceled.
|
||||||
require.Equal(t, 1, validateCalls, "validate calls duplicated on same token")
|
require.Equal(t, 1, validateCalls, "validate calls duplicated on same token")
|
||||||
|
|
Loading…
Reference in New Issue