Skip to content
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

Gitlab Support #183

Open
RichardJActon opened this issue Sep 1, 2022 · 4 comments
Open

Gitlab Support #183

RichardJActon opened this issue Sep 1, 2022 · 4 comments

Comments

@RichardJActon
Copy link

Hi I'm interested in contributing gitlab support for gert would you be open to this?
From my initial cursory assessment only 1 function needs modification make_cred_cb in credentials.R everything else seems to be only using core git functionality.

However the tests present more of a challenge as many depend on github repos on @jeroen's github to implement the same tests for gitlab would require a copy of the testing reop(s) on gitlab and presumably the development team would like these test repos in a group they control. I could just clone these to a gitlab remote of my own for testing and transfer the ownership.
test-auth.R, test-clone.R, test-rebase.R, & test-remotes.R appear to contain the affected tests.

For make_cred_cb() there is a question of how to handle the presence of a GITLAB_PAT & and GITHUB_PAT variable being set. Perhaps a warning and a default to github, with an injunction to unset the environment variable if you don't want to use the github PAT? In the case where only one is set I would assume default to the one that is set. I've prototyped this in my fork: https://github.com/RichardJActon/gert

This might help with issues #145, #174, and possibly also #111 which all involve gitlab

@jeroen
Copy link
Member

jeroen commented Sep 2, 2022

I'm not opposed to it, and we could probably support gitlab.com but supporting custom servers is very complicated.

Your implementation in RichardJActon@5542a63 is definitely a no go: IIUC, it just sends your GITHUB_PAT to any server that you connect to? That is a big security breach, you may leak your PAT to malicious parties. You should only share your PAT for a given gitlab with that particular host.

We actually discussed the same issue before with github-enterprise hosts (self-hosted github) and I think the gitcreds package supports tokens that are postfixed with the hostname, for example GITHUB_PAT_GITHUB_COM: https://cran.r-project.org/web/packages/gitcreds/vignettes/package.html . However I have never gotten around trying to support this in credentials/gert (also because I don't have access to any such server myself to test this).

@RichardJActon
Copy link
Author

RichardJActon commented Sep 2, 2022

Interesting, my use-case would very much benefit from support for custom domains.

So if my implementation reflects my intent it should:

  • Return a github pat if a github &/or gitlab pat environment variable are detected and if the url matches a github.com domain.
  • Return a gitlab PAT if a gitlab pat environment variable and no github PAT variable is detected irrespective of the url specified.

So would an acceptable solution to supporting gitlab.com be to check the url specified contains the gitlab.com domain as you did with github?

For custom domain support would some kind of fairly aggressive warning be sufficient let's say if a PAT environment variable is found that does not match either github or gitlab in the supplied url it errors out with a message saying something to the effect of "We found a PAT in your environment but you are using a custom domain that we don't know if it's legit so be sure you have the url correct, if you really want to do this set the allow_custom_domains_param to TRUE to suppress this error"?
Then have a param that lets you send a pat to a non pre-approved by the devs of gert' at your own risk', maybe with a warning in the docs about the associated risk of setting this param to TRUE?

@RichardJActon
Copy link
Author

So it seems that the approach {gitcreds} is taking is not just to store the PAT in the environment variable but also the protocol, host and username. I suppose this way you explicitly set the PAT and host at the same time in the same place, it doesn't stop you from accidentally sending the PAT to the wrong host. It just couples setting host and PAT more tightly which may make it less likely to do by accident relative to setting the PAT as an environment variable and the host URL as an argument to a function.

@RichardJActon
Copy link
Author

A version supporting only gitlab.com: RichardJActon@885b746

A version supporting custom domains only if a param is set explicitly allowing this:
RichardJActon@c4ad5a5

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

No branches or pull requests

2 participants