Copilot commented on code in PR #8667: URL: https://github.com/apache/incubator-devlake/pull/8667#discussion_r2638164530
########## backend/plugins/github/token/round_tripper.go: ########## @@ -0,0 +1,76 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "net/http" +) + +type RefreshRoundTripper struct { + base http.RoundTripper + tokenProvider *TokenProvider +} + +func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider) *RefreshRoundTripper { + return &RefreshRoundTripper{ + base: base, + tokenProvider: tp, + } +} + +func (rt *RefreshRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + // Get token + token, err := rt.tokenProvider.GetToken() + if err != nil { + return nil, err + } + + // Clone request before modifying + reqClone := req.Clone(req.Context()) + reqClone.Header.Set("Authorization", "Bearer "+token) + + // Execute request + resp, reqErr := rt.base.RoundTrip(reqClone) + if reqErr != nil { + return nil, reqErr + } + + // Reactive refresh on 401 + if resp.StatusCode == http.StatusUnauthorized { + // Close previous response body + resp.Body.Close() + + // Force refresh + if err := rt.tokenProvider.ForceRefresh(token); err != nil { + return nil, err + } + + // Get new token + newToken, err := rt.tokenProvider.GetToken() + if err != nil { + return nil, err + } + + // Retry request with new token + reqRetry := req.Clone(req.Context()) + reqRetry.Header.Set("Authorization", "Bearer "+newToken) + return rt.base.RoundTrip(reqRetry) + } + + return resp, nil +} Review Comment: The RefreshRoundTripper lacks test coverage. Tests should verify proper request cloning, authorization header injection, handling of 401 responses with retry logic, and proper cleanup of response bodies. This is particularly important given the concurrent access patterns and retry logic that could lead to subtle bugs. ########## backend/plugins/github/tasks/api_client.go: ########## @@ -34,6 +35,24 @@ func CreateApiClient(taskCtx plugin.TaskContext, connection *models.GithubConnec return nil, err } + // Inject TokenProvider if refresh token is present + if connection.RefreshToken != "" { + logger := taskCtx.GetLogger() + db := taskCtx.GetDal() + + // Create TokenProvider + tp := token.NewTokenProvider(connection, db, apiClient.GetClient(), logger) + + // Wrap the transport + baseTransport := apiClient.GetClient().Transport + if baseTransport == nil { + baseTransport = http.DefaultTransport + } + + rt := token.NewRefreshRoundTripper(baseTransport, tp) + apiClient.GetClient().Transport = rt + } Review Comment: The RefreshRoundTripper wraps the existing transport, but this could conflict with the existing authentication setup in SetupAuthentication method. The connection's SetupAuthentication is likely already being used elsewhere to set the Authorization header, and now the RoundTripper is also setting it. This could lead to duplicate or conflicting authorization headers depending on the order of middleware execution. Consider documenting this interaction or ensuring the RoundTripper is the exclusive mechanism for setting authorization when refresh tokens are present. ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} + +func (tp *TokenProvider) refreshToken() errors.Error { + tp.logger.Info("Refreshing GitHub token for connection %d", tp.conn.ID) + + data := map[string]string{ + "refresh_token": tp.conn.RefreshToken, + "grant_type": "refresh_token", + "client_id": tp.conn.AppId, + "client_secret": tp.conn.SecretKey, + } + jsonData, _ := json.Marshal(data) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, "POST", tp.refreshURL, bytes.NewBuffer(jsonData)) + if err != nil { + return errors.Convert(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + + resp, err := tp.httpClient.Do(req) + if err != nil { + return errors.Convert(err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return errors.Default.New(fmt.Sprintf("failed to refresh token: %d", resp.StatusCode)) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return errors.Convert(err) + } + + var result struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + ExpiresIn int `json:"expires_in"` + RefreshTokenExpiresIn int `json:"refresh_token_expires_in"` + } + if err := json.Unmarshal(body, &result); err != nil { + return errors.Convert(err) + } + + if result.AccessToken == "" { + return errors.Default.New("empty access token returned") + } + + tp.conn.UpdateToken( + result.AccessToken, + result.RefreshToken, + time.Now().Add(time.Duration(result.ExpiresIn)*time.Second), + time.Now().Add(time.Duration(result.RefreshTokenExpiresIn)*time.Second), + ) + + if tp.dal != nil { + err := tp.dal.UpdateColumns(tp.conn, []dal.DalSet{ + {ColumnName: "token", Value: tp.conn.Token}, + {ColumnName: "refresh_token", Value: tp.conn.RefreshToken}, + {ColumnName: "token_expires_at", Value: tp.conn.TokenExpiresAt}, + {ColumnName: "refresh_token_expires_at", Value: tp.conn.RefreshTokenExpiresAt}, + }) + if err != nil { + tp.logger.Warn(err, "failed to persist refreshed token") + } + } + + return nil +} + +func (tp *TokenProvider) ForceRefresh(oldToken string) errors.Error { + tp.mu.Lock() + defer tp.mu.Unlock() + + // If the token has changed since the request was made, it means another thread + // has already refreshed it. + if tp.conn.Token != oldToken { + return nil + } + + return tp.refreshToken() +} Review Comment: The new token refresh functionality lacks test coverage. Given that this plugin has comprehensive test coverage (including unit tests for models and tasks), the TokenProvider should have tests covering scenarios such as: successful token refresh, handling of expired refresh tokens, concurrent token refresh attempts (testing the mutex), retry logic on 401 errors, and database persistence failures. ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} Review Comment: The TokenProvider struct and its public methods lack documentation. Public types and methods should have documentation comments explaining their purpose, usage, and any important considerations. For example, the TokenProvider should document that it handles OAuth2 token refresh with automatic retry logic, and that it's thread-safe due to the internal mutex. ########## backend/plugins/github/models/connection_test.go: ########## @@ -227,3 +227,14 @@ func TestGithubConnection_Sanitize(t *testing.T) { }) } } + +func TestIsUserToServerToken(t *testing.T) { Review Comment: The test function name 'TestIsUserToServerToken' doesn't accurately reflect what the test is validating. The test is checking the 'typeIs' method which classifies token types (classical, fine-grained, unknown), not specifically testing if tokens are user-to-server tokens. Consider renaming to 'TestTokenTypeClassification' or 'TestTypeIs' to better describe the actual behavior being tested. ```suggestion func TestTypeIs(t *testing.T) { ``` ########## backend/plugins/github/models/connection.go: ########## @@ -56,6 +56,21 @@ type GithubConn struct { helper.MultiAuth `mapstructure:",squash"` GithubAccessToken `mapstructure:",squash" authMethod:"AccessToken"` GithubAppKey `mapstructure:",squash" authMethod:"AppKey"` + RefreshToken string `mapstructure:"refreshToken" json:"refreshToken" gorm:"type:text;serializer:encdec"` + TokenExpiresAt time.Time `mapstructure:"tokenExpiresAt" json:"tokenExpiresAt"` + RefreshTokenExpiresAt time.Time `mapstructure:"refreshTokenExpiresAt" json:"refreshTokenExpiresAt"` +} + +// UpdateToken updates the token and refresh token information +func (conn *GithubConn) UpdateToken(newToken, newRefreshToken string, expiry, refreshExpiry time.Time) { + conn.Token = newToken + conn.RefreshToken = newRefreshToken + conn.TokenExpiresAt = expiry + conn.RefreshTokenExpiresAt = refreshExpiry + + // Update the internal tokens slice used by SetupAuthentication + conn.tokens = []string{newToken} + conn.tokenIndex = 0 Review Comment: The UpdateToken method resets the tokens slice and tokenIndex, but this could cause issues with concurrent access. If PrepareApiClient has been called previously and split the Token into multiple tokens for rotation, UpdateToken will replace that slice with a single token. This could lead to unexpected behavior when tokens are rotated, especially since there's no synchronization mechanism to protect against concurrent modifications of these fields. ########## backend/plugins/github/token/round_tripper.go: ########## @@ -0,0 +1,76 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "net/http" +) + +type RefreshRoundTripper struct { + base http.RoundTripper + tokenProvider *TokenProvider +} + +func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider) *RefreshRoundTripper { + return &RefreshRoundTripper{ + base: base, + tokenProvider: tp, + } +} + +func (rt *RefreshRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + // Get token + token, err := rt.tokenProvider.GetToken() + if err != nil { + return nil, err + } + + // Clone request before modifying + reqClone := req.Clone(req.Context()) + reqClone.Header.Set("Authorization", "Bearer "+token) + + // Execute request + resp, reqErr := rt.base.RoundTrip(reqClone) + if reqErr != nil { + return nil, reqErr + } + + // Reactive refresh on 401 + if resp.StatusCode == http.StatusUnauthorized { + // Close previous response body + resp.Body.Close() + + // Force refresh + if err := rt.tokenProvider.ForceRefresh(token); err != nil { + return nil, err + } + + // Get new token + newToken, err := rt.tokenProvider.GetToken() + if err != nil { + return nil, err + } + + // Retry request with new token + reqRetry := req.Clone(req.Context()) + reqRetry.Header.Set("Authorization", "Bearer "+newToken) + return rt.base.RoundTrip(reqRetry) Review Comment: There is a potential infinite retry loop if the token refresh itself returns a 401 status. When ForceRefresh is called on line 59, if the refresh endpoint returns 401, the code will retry the original request with the same (still invalid) token, potentially causing repeated failed refresh attempts. Consider adding a guard to prevent retrying after a failed refresh or tracking whether a refresh has already been attempted for this request. ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} + +func (tp *TokenProvider) refreshToken() errors.Error { + tp.logger.Info("Refreshing GitHub token for connection %d", tp.conn.ID) + + data := map[string]string{ + "refresh_token": tp.conn.RefreshToken, + "grant_type": "refresh_token", + "client_id": tp.conn.AppId, + "client_secret": tp.conn.SecretKey, + } + jsonData, _ := json.Marshal(data) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, "POST", tp.refreshURL, bytes.NewBuffer(jsonData)) + if err != nil { + return errors.Convert(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + + resp, err := tp.httpClient.Do(req) + if err != nil { + return errors.Convert(err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return errors.Default.New(fmt.Sprintf("failed to refresh token: %d", resp.StatusCode)) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return errors.Convert(err) + } + + var result struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + ExpiresIn int `json:"expires_in"` + RefreshTokenExpiresIn int `json:"refresh_token_expires_in"` + } + if err := json.Unmarshal(body, &result); err != nil { + return errors.Convert(err) + } + + if result.AccessToken == "" { + return errors.Default.New("empty access token returned") Review Comment: The error returned when an empty access token is received lacks context about the actual API response. This makes debugging difficult when the GitHub API returns an unexpected response structure. Consider including relevant parts of the response body in the error message to provide better diagnostic information. ```suggestion bodyStr := string(body) const maxBodySnippet = 512 if len(bodyStr) > maxBodySnippet { bodyStr = bodyStr[:maxBodySnippet] + "…" } return errors.Default.New(fmt.Sprintf("empty access token returned; response body: %s", bodyStr)) ``` ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} + +func (tp *TokenProvider) refreshToken() errors.Error { + tp.logger.Info("Refreshing GitHub token for connection %d", tp.conn.ID) + + data := map[string]string{ + "refresh_token": tp.conn.RefreshToken, + "grant_type": "refresh_token", + "client_id": tp.conn.AppId, + "client_secret": tp.conn.SecretKey, + } + jsonData, _ := json.Marshal(data) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, "POST", tp.refreshURL, bytes.NewBuffer(jsonData)) + if err != nil { + return errors.Convert(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + + resp, err := tp.httpClient.Do(req) + if err != nil { + return errors.Convert(err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return errors.Default.New(fmt.Sprintf("failed to refresh token: %d", resp.StatusCode)) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return errors.Convert(err) + } + + var result struct { + AccessToken string `json:"access_token"` + RefreshToken string `json:"refresh_token"` + ExpiresIn int `json:"expires_in"` + RefreshTokenExpiresIn int `json:"refresh_token_expires_in"` + } + if err := json.Unmarshal(body, &result); err != nil { + return errors.Convert(err) + } + + if result.AccessToken == "" { + return errors.Default.New("empty access token returned") + } + + tp.conn.UpdateToken( + result.AccessToken, + result.RefreshToken, + time.Now().Add(time.Duration(result.ExpiresIn)*time.Second), + time.Now().Add(time.Duration(result.RefreshTokenExpiresIn)*time.Second), + ) + + if tp.dal != nil { + err := tp.dal.UpdateColumns(tp.conn, []dal.DalSet{ + {ColumnName: "token", Value: tp.conn.Token}, + {ColumnName: "refresh_token", Value: tp.conn.RefreshToken}, + {ColumnName: "token_expires_at", Value: tp.conn.TokenExpiresAt}, + {ColumnName: "refresh_token_expires_at", Value: tp.conn.RefreshTokenExpiresAt}, + }) + if err != nil { + tp.logger.Warn(err, "failed to persist refreshed token") Review Comment: Token refresh failures are only logged as warnings and do not propagate an error to the caller. This means the application will continue using an expired token, leading to authentication failures. The database persistence failure should return an error to prevent the system from believing the token has been successfully refreshed when it hasn't been persisted. ```suggestion tp.logger.Warn(err, "failed to persist refreshed token") return errors.Convert(err) ``` ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} + +func (tp *TokenProvider) refreshToken() errors.Error { + tp.logger.Info("Refreshing GitHub token for connection %d", tp.conn.ID) + + data := map[string]string{ + "refresh_token": tp.conn.RefreshToken, + "grant_type": "refresh_token", + "client_id": tp.conn.AppId, + "client_secret": tp.conn.SecretKey, + } Review Comment: Sensitive authentication credentials (client_id and client_secret) are logged in plain text in the request data during token refresh. If debug logging is enabled or if there's an error that logs the request, these credentials could be exposed in log files. Consider sanitizing or redacting sensitive fields before logging to prevent credential leakage. ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} Review Comment: The needsRefresh method doesn't check if TokenExpiresAt is a zero value before comparing it with the current time. If TokenExpiresAt is not set (zero value), the comparison 'time.Now().Add(buffer).After(tp.conn.TokenExpiresAt)' will always return true, causing unnecessary refresh attempts. Consider adding a check to return false if TokenExpiresAt.IsZero() to handle the case where expiry time is not set. ########## backend/plugins/github/token/token_provider.go: ########## @@ -0,0 +1,171 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "os" + "strconv" + "sync" + "time" + + "github.com/apache/incubator-devlake/core/dal" + "github.com/apache/incubator-devlake/core/errors" + "github.com/apache/incubator-devlake/core/log" + "github.com/apache/incubator-devlake/plugins/github/models" +) + +const ( + DefaultRefreshBuffer = 5 * time.Minute +) + +type TokenProvider struct { + conn *models.GithubConnection + dal dal.Dal + httpClient *http.Client + logger log.Logger + mu sync.Mutex + refreshURL string +} + +func NewTokenProvider(conn *models.GithubConnection, d dal.Dal, client *http.Client, logger log.Logger) *TokenProvider { + return &TokenProvider{ + conn: conn, + dal: d, + httpClient: client, + logger: logger, + refreshURL: "https://github.com/login/oauth/access_token";, + } +} + +func (tp *TokenProvider) GetToken() (string, errors.Error) { + tp.mu.Lock() + defer tp.mu.Unlock() + + if tp.needsRefresh() { + if err := tp.refreshToken(); err != nil { + return "", err + } + } + return tp.conn.Token, nil +} + +func (tp *TokenProvider) needsRefresh() bool { + if tp.conn.RefreshToken == "" { + return false + } + + buffer := DefaultRefreshBuffer + if envBuffer := os.Getenv("GITHUB_TOKEN_REFRESH_BUFFER_MINUTES"); envBuffer != "" { + if val, err := strconv.Atoi(envBuffer); err == nil { + buffer = time.Duration(val) * time.Minute + } + } + + return time.Now().Add(buffer).After(tp.conn.TokenExpiresAt) +} + +func (tp *TokenProvider) refreshToken() errors.Error { + tp.logger.Info("Refreshing GitHub token for connection %d", tp.conn.ID) + + data := map[string]string{ + "refresh_token": tp.conn.RefreshToken, + "grant_type": "refresh_token", + "client_id": tp.conn.AppId, + "client_secret": tp.conn.SecretKey, + } + jsonData, _ := json.Marshal(data) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, "POST", tp.refreshURL, bytes.NewBuffer(jsonData)) + if err != nil { + return errors.Convert(err) + } + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json") + + resp, err := tp.httpClient.Do(req) + if err != nil { + return errors.Convert(err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return errors.Default.New(fmt.Sprintf("failed to refresh token: %d", resp.StatusCode)) + } + + body, err := io.ReadAll(resp.Body) + if err != nil { + return errors.Convert(err) + } + Review Comment: The response body is read but potential error responses from the GitHub API are not logged or included in the error message. When token refresh fails, the response body often contains valuable information about why the refresh failed (e.g., expired refresh token, invalid client credentials). Consider logging the response body before returning the error to aid in debugging and troubleshooting token refresh failures. ```suggestion body, err := io.ReadAll(resp.Body) if err != nil { return errors.Convert(err) } if resp.StatusCode != http.StatusOK { // Log the response body to aid in debugging token refresh failures. if tp.logger != nil { tp.logger.Error(nil, "failed to refresh token from GitHub, status=%d, body=%s", resp.StatusCode, string(body)) } return errors.Default.New(fmt.Sprintf("failed to refresh token: %d, body: %s", resp.StatusCode, string(body))) } ``` ########## backend/plugins/github/token/round_tripper.go: ########## @@ -0,0 +1,76 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package token + +import ( + "net/http" +) + +type RefreshRoundTripper struct { + base http.RoundTripper + tokenProvider *TokenProvider +} + +func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider) *RefreshRoundTripper { + return &RefreshRoundTripper{ + base: base, + tokenProvider: tp, + } +} Review Comment: The RefreshRoundTripper struct and RoundTrip method lack documentation. These should include comments explaining that this is an HTTP transport middleware that automatically refreshes OAuth tokens and retries requests on authentication failures. The RoundTrip method should document its behavior of cloning requests, handling 401 responses, and potential retry attempts. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
