Copilot commented on code in PR #8667:
URL:
https://github.com/apache/incubator-devlake/pull/8667#discussion_r2656300323
##########
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
Review Comment:
Missing documentation comment for the exported method UpdateToken. According
to Go best practices, all exported methods should have documentation comments
explaining their parameters and behavior, especially for a method that modifies
critical security-related state.
```suggestion
// UpdateToken updates the access token, refresh token and their expiration
times
// for this GithubConn instance. newToken is the access token value,
newRefreshToken
// is the corresponding refresh token, expiry is the time at which the
access token
// expires, and refreshExpiry is the time at which the refresh token expires.
// In addition to updating the exported fields, this method also resets the
internal
// tokens slice and tokenIndex used by authentication logic to use the new
access token.
```
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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()
+
+ 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)))
+ }
+ 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 == "" {
+ 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))
+ }
+
+ 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
+}
+
Review Comment:
Missing documentation comment for the exported method ForceRefresh.
According to Go best practices, all exported methods should have documentation
comments explaining their behavior, especially explaining the purpose of the
oldToken parameter and how it's used to prevent redundant refreshes.
```suggestion
// ForceRefresh refreshes the access token if the current token is still
equal to oldToken.
// The oldToken parameter should be the token value observed by the caller
when it determined
// that a refresh might be needed; if the token has changed since then,
another goroutine has
// already refreshed it and this method returns without performing a
redundant refresh.
```
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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:
Missing documentation comment for the exported function NewTokenProvider.
According to Go best practices, all exported functions should have
documentation comments explaining their parameters and return values.
```suggestion
// NewTokenProvider creates a TokenProvider for the given GitHub connection
using
// the provided DAL, HTTP client, and logger, and returns a pointer to it.
```
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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
+ }
+ }
Review Comment:
The environment variable check is performed on every call to needsRefresh(),
which could be inefficient. Consider caching the buffer value once during
initialization or using sync.Once for lazy initialization to avoid repeated
environment variable lookups and parsing.
##########
backend/plugins/github/token/round_tripper.go:
##########
@@ -0,0 +1,90 @@
+/*
+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"
+)
+
+// RefreshRoundTripper is an HTTP transport middleware that automatically
manages OAuth token refreshes.
+// It wraps an underlying http.RoundTripper and provides token refresh on auth
failures.
+// On 401's the round tripper will:
+// - Force a refresh of the OAuth token via the TokenProvider
+// - Retry the original request with the new token
+type RefreshRoundTripper struct {
+ base http.RoundTripper
+ tokenProvider *TokenProvider
+}
+
+func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider)
*RefreshRoundTripper {
+ return &RefreshRoundTripper{
+ base: base,
+ tokenProvider: tp,
+ }
+}
+
+// RoundTrip implements the http.RoundTripper interface and handles automatic
token refresh on 401 responses.
+// It clones the request, adds the Authorization header, and retries once with
a refreshed token if needed.
+func (rt *RefreshRoundTripper) RoundTrip(req *http.Request) (*http.Response,
error) {
+ return rt.roundTripWithRetry(req, false)
+}
+
+// roundTripWithRetry performs the actual request with retry on 401.
+// The refreshAttempted parameter tracks whether a refresh has already been
tried for this request
+// to prevent infinite retry loops if token refresh itself fails.
+func (rt *RefreshRoundTripper) roundTripWithRetry(req *http.Request,
refreshAttempted bool) (*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())
Review Comment:
The req.Clone() method does not clone the request body, which means if the
original request has a body (e.g., POST/PUT requests), attempting to retry the
request after a 401 error will fail because the body has already been read. The
body needs to be preserved or made replayable for retry scenarios. Consider
using req.GetBody if available, or document that this implementation only
supports requests without bodies.
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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";,
+ }
+}
+
Review Comment:
Missing documentation comment for the exported method GetToken. According to
Go best practices, all exported methods should have documentation comments
explaining their behavior, return values, and any important side effects.
```suggestion
// GetToken returns the current access token for the GitHub connection.
// If the token is close to expiry and a refresh token is available, it
// refreshes the token under a mutex lock and returns the updated value.
// On failure to refresh, it returns a non-nil errors.Error.
```
##########
backend/plugins/github/token/round_tripper.go:
##########
@@ -0,0 +1,90 @@
+/*
+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"
+)
+
+// RefreshRoundTripper is an HTTP transport middleware that automatically
manages OAuth token refreshes.
+// It wraps an underlying http.RoundTripper and provides token refresh on auth
failures.
+// On 401's the round tripper will:
+// - Force a refresh of the OAuth token via the TokenProvider
+// - Retry the original request with the new token
+type RefreshRoundTripper struct {
+ base http.RoundTripper
+ tokenProvider *TokenProvider
+}
+
+func NewRefreshRoundTripper(base http.RoundTripper, tp *TokenProvider)
*RefreshRoundTripper {
+ return &RefreshRoundTripper{
+ base: base,
+ tokenProvider: tp,
+ }
+}
+
+// RoundTrip implements the http.RoundTripper interface and handles automatic
token refresh on 401 responses.
+// It clones the request, adds the Authorization header, and retries once with
a refreshed token if needed.
+func (rt *RefreshRoundTripper) RoundTrip(req *http.Request) (*http.Response,
error) {
+ return rt.roundTripWithRetry(req, false)
+}
+
+// roundTripWithRetry performs the actual request with retry on 401.
+// The refreshAttempted parameter tracks whether a refresh has already been
tried for this request
+// to prevent infinite retry loops if token refresh itself fails.
+func (rt *RefreshRoundTripper) roundTripWithRetry(req *http.Request,
refreshAttempted bool) (*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 && !refreshAttempted {
+ // 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())
Review Comment:
The req.Clone() method is called again for the retry request, but request
bodies cannot be replayed after being read. If this is a POST/PUT/PATCH request
with a body, the retry will fail or send an empty body. This is the same issue
as line 58 - the request body needs to be preserved or made replayable.
##########
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 new UpdateToken method lacks test coverage. Since this method modifies
critical security-related state (tokens and expiry times), it should have unit
tests to verify that all fields are updated correctly, including the internal
tokens slice and tokenIndex.
##########
backend/plugins/github/token/round_tripper.go:
##########
@@ -0,0 +1,90 @@
+/*
+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"
+)
+
+// RefreshRoundTripper is an HTTP transport middleware that automatically
manages OAuth token refreshes.
+// It wraps an underlying http.RoundTripper and provides token refresh on auth
failures.
+// On 401's the round tripper will:
+// - Force a refresh of the OAuth token via the TokenProvider
+// - Retry the original request with the new token
+type RefreshRoundTripper struct {
+ base http.RoundTripper
+ tokenProvider *TokenProvider
+}
+
Review Comment:
Missing documentation comment for the exported function
NewRefreshRoundTripper. According to Go best practices, all exported functions
should have documentation comments explaining their parameters and return
values.
```suggestion
// NewRefreshRoundTripper creates a new RefreshRoundTripper that wraps the
given base RoundTripper
// and uses the provided TokenProvider to obtain and refresh OAuth tokens.
It returns a configured
// RefreshRoundTripper that can be used as an http.RoundTripper.
```
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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)
Review Comment:
The error from json.Marshal is silently ignored. While marshaling a simple
map[string]string is unlikely to fail, errors should still be handled properly
for robustness and to follow best practices. Consider returning or logging the
error if it occurs.
```suggestion
jsonData, err := json.Marshal(data)
if err != nil {
return errors.Convert(err)
}
```
##########
backend/plugins/github/token/token_provider.go:
##########
@@ -0,0 +1,179 @@
+/*
+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
+)
+
Review Comment:
Missing documentation comment for the exported type TokenProvider. According
to Go best practices, all exported types should have documentation comments
that describe their purpose and usage.
```suggestion
// TokenProvider provides GitHub access tokens for a connection and
refreshes them when necessary.
```
--
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]
