Skip to content

support cloning private repo with github app #382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jun 3, 2025

Conversation

KyriosGN0
Copy link
Contributor

@KyriosGN0 KyriosGN0 commented Mar 11, 2025

Signed-off-by: AvivGuiser [email protected]

should fix #350 #347

@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of pkg/git/repo.go

@@ -3,14 +3,18 @@ package git
 import (
 	"bufio"
 	"context"
+	"encoding/json"
 	"fmt"
+	"io"
 	"io/fs"
+	"net/http"
 	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog/log"
@@ -248,6 +252,51 @@ func getCloneUrl(user string, cfg config.ServerConfig) (string, error) {
 		scheme = parts.Scheme
 	}
 
+	if cfg.GithubAppID != 0 && cfg.GithubInstallationID != 0 && cfg.GithubPrivateKey != "" {
+		client := &http.Client{
+			Timeout: 5 * time.Minute,
+		}
+		stringAppId := fmt.Sprintf("%d", cfg.GithubAppID)
+		jwt, err := pkg.CreateJWT(cfg.GithubPrivateKey, stringAppId)
+		if err != nil {
+			return "", errors.Wrapf(err, "failed to create jwt")
+		}
+		url := fmt.Sprintf("https://api.github.com/app/installations/%d/access_tokens", cfg.GithubInstallationID)
+
+		req, err := http.NewRequest(http.MethodPost, url, nil)
+		if err != nil {
+			return "", errors.Wrapf(err, "failed to create request")
+		}
+		req.Header.Add("Accept", "application/vnd.github.v3+json")
+		req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", jwt))
+
+		resp, err := client.Do(req)
+		if err != nil {
+			return "", errors.Wrapf(err, "failed to get response")
+		}
+		defer resp.Body.Close()
+
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return "", errors.Wrapf(err, "failed to read response")
+		}
+
+		var result interface{}
+		err = json.Unmarshal(body, &result)
+		if err != nil {
+			return "", errors.Wrapf(err, "failed to unmarshal response")
+		}
+
+		data, ok := result.(map[string]interface{})
+		if !ok {
+			return "", errors.New("failed to convert response to map")
+		}
+
+		if token, exists := data["token"]; exists {
+			user = fmt.Sprintf("x-access-token:%s", token.(string))
+		}
+	}
+
 	return fmt.Sprintf("%s://%s:%s@%s", scheme, user, vcsToken, hostname), nil
 }
 

Feedback & Suggestions:

  1. Error Handling and Logging: The new code block for fetching the GitHub access token has several points where errors are wrapped and returned. It would be beneficial to log these errors before returning them to provide more context in the logs. Consider using log.Error().Err(err).Msg("context message") before returning the error.

  2. Type Assertion: The type assertion for data, ok := result.(map[string]interface{}) is currently unchecked. If the assertion fails, it returns a generic error message. Consider logging the actual type of result when the assertion fails to aid in debugging.

  3. Token Handling: The token is being directly inserted into the user string. Ensure that this token is handled securely and not logged or exposed inadvertently. Consider adding a logging statement to confirm that the token is being used securely without exposing it.

  4. Timeout Duration: The HTTP client timeout is set to 5 minutes, which might be excessive for this operation. Consider reducing the timeout to a more reasonable duration unless there is a specific reason for it to be this long.

  5. JSON Unmarshalling: The JSON unmarshalling process could be improved by defining a struct that matches the expected response format. This would make the code more robust and easier to maintain.

  6. Variable Naming: The variable stringAppId could be renamed to appIDStr for consistency with Go naming conventions and to improve readability.

  7. Security: Ensure that the GithubPrivateKey and any sensitive information are stored and accessed securely, following best practices for handling secrets.



Dependency Review

Click to read mergecats review!

No suggestions found

@KyriosGN0
Copy link
Contributor Author

@Greyeye @djeebus sorry for the tag
Can i get some eyes?

@KyriosGN0 KyriosGN0 requested a review from Greyeye June 3, 2025 08:17
Copy link
Collaborator

@Greyeye Greyeye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Greyeye Greyeye merged commit 1dc3068 into zapier:main Jun 3, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to clone repositories with GitHub App authentication - repeated authentication failures
3 participants