Skip to content

Enable github private repos. #103

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.

Sign up for GitHub

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

Closed
jgoppert wants to merge 1 commit into from
Closed

Enable github private repos. #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 30 additions & 3 deletions src/rosdistro/loader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,46 @@

import socket
import time
import os
import sys
import base64

from . import logger

try:
from urllib.request import urlopen
from urllib.request import urlopen, Request
from urllib.error import HTTPError
from urllib.error import URLError
except ImportError:
from urllib2 import urlopen
from urllib2 import urlopen, Request
from urllib2 import HTTPError
from urllib2 import URLError


GITHUB_USER = os.getenv('GITHUB_USER', None)
GITHUB_PASSWORD = os.getenv('GITHUB_PASSWORD', None)


def auth_header(username=None, password=None, token=None):
Copy link
Contributor

@mikepurvis mikepurvis Jun 19, 2017

Choose a reason for hiding this comment

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

I don't understand what's going on in this function, and in particular why it only needs to happen here and not for the manifest providers as well. Should this be a generic thing that's used everywhere?

Copy link
Author

@jgoppert jgoppert Jun 19, 2017

Choose a reason for hiding this comment

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

It is just creating either a basic or token auth header. I had to have the same function in bloom and ros_biuldfarm as well. So if there is one module that all of these import, we can put it there.

if username and password:
if sys.version_info > (3, 0):
userandpass = base64.b64encode(bytes('%s:%s' % (username, password), 'utf-8'))
else:
userandpass = base64.b64encode('%s:%s' % (username, password))
userandpass = userandpass.decode('ascii')
return 'Basic %s' % userandpass
elif token:
return 'token %s' % token


def load_url(url, retry=2, retry_period=1, timeout=10, skip_decode=False):
req = Request(url)
if GITHUB_USER and GITHUB_PASSWORD:
Copy link
Contributor

@mikepurvis mikepurvis Jun 19, 2017

Choose a reason for hiding this comment

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

I don't care for passing my github credentials to an arbitrary URL— shouldn't we check here if the request URL is in fact Github before doing this?

Alternatively, maybe some of this logic should be hoisted into the auth_header function; pass it the URL and have it do the business of checking the host and then pulling the credentials or token from the environment as appropriate.

Copy link
Author

@jgoppert jgoppert Jun 19, 2017

Choose a reason for hiding this comment

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

GITHUB_USER is a bit mis-leading, this also works for bitbucket as far as I can tell from the existing usage of it, but it seems that naming convention was left over. https://developer.atlassian.com/bitbucket/server/docs/latest/how-tos/example-basic-authentication.html

The issue would be if someone was using gitlab. If we wanteed to do something like bloom does and keep the settings in a config file, we could have user enter auth for each url in a dict.

{
"https://www.github.com/my_company/my_repo": ["user", "password"]
}

Or something like that.

Copy link
Contributor

@mikepurvis mikepurvis Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Right, but for my use case, I have repos at Github, Bitbucket, and Gitlab, and I have different credentials for all three in the environment that fetches manifests to build the cache.

I would prefer if this logic followed the same pattern and used the correct set of credentials based on where the rosdistro is located (rather than forcing me to pass my Bitbucket credentials as GITHUB_USER/GITHUB_PASSWORD).

Copy link
Author

@jgoppert jgoppert Jun 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm suggesting we do something like the following, this would work for you correct?

cat .config/rosdistro
{
"https://www.github.com/my_company/my_repo": ["user", "password"],
"https://www.bitbucket.com/my_company/my_repo": [user="user", password="password"],
"https://www.mygitlab.com/my_team/my_repo": [user="user", password="password"],
"https://www.github.com/my_company/my_repo2": [token="token"],
}

Copy link
Contributor

@mikepurvis mikepurvis Jun 21, 2017

Choose a reason for hiding this comment

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

I'm not seeing a motivation to depart from the convention already established in this repo.

Copy link
Author

@jgoppert jgoppert Jun 21, 2017

Choose a reason for hiding this comment

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

OK, are automated pull requests working for you? I noticed most of it was broken for private repo and did all of this to get it working. Am I missing something? It would be good to add secure pr's to the unit tests to get coverage over this code.

Copy link
Author

@jgoppert jgoppert Jun 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

And what is the current method of handling raw files from github lIke for distro.yaml. I don't think there is any, correct?

Copy link
Contributor

@mikepurvis mikepurvis Jun 22, 2017

Choose a reason for hiding this comment

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

The existing convention works for building the rosdistro cache from private repos on Github and Bitbucket, and is implemented in the manifest providers:

https://github.com/ros-infrastructure/rosdistro/tree/master/src/rosdistro/manifest_provider

The envvars in question are GITHUB_USER, GITHUB_PASSWORD, BITBUCKET_USER, and BITBUCKET_PASSWORD. My request is that we use these same envvars for this new logic, but use the correct ones depending on where the rosdistro is hosted.

Copy link
Author

@jgoppert jgoppert Jun 22, 2017

Choose a reason for hiding this comment

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

Ah, OK, sorry for misunderstanding. I hadn't seen BITBUCKET_USER and BITBUCKET_PASSWORD yet.

Copy link
Contributor

@mikepurvis mikepurvis Jun 22, 2017

Choose a reason for hiding this comment

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

My apologies also; I should have been more explicit upfront.

logger.debug('- using http basic auth from supplied environment variables.')
authheader = auth_header(username=GITHUB_USER, password=GITHUB_PASSWORD)
req.add_header('Authorization', authheader)
try:
fh = urlopen(url, timeout=timeout)
fh = urlopen(req, timeout=timeout)
except HTTPError as e:
if e.code in [500, 502, 503] and retry:
time.sleep(retry_period)
Expand Down
2 changes: 1 addition & 1 deletion src/rosdistro/manifest_provider/bitbucket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def bitbucket_manifest_provider(_dist_name, repo, pkg_name):
req = Request(url)
if BITBUCKET_USER and BITBUCKET_PASSWORD:
logger.debug('- using http basic auth from supplied environment variables.')
authheader = 'Basic %s' % base64.b64encode('%s:%s' % (BITBUCKET_USER, BITBUCKET_PASSWORD))
authheader = 'Basic %s' % base64.b64encode(b'%s:%s' % (BITBUCKET_USER, BITBUCKET_PASSWORD))
req.add_header('Authorization', authheader)
package_xml = urlopen(req).read().decode('utf-8')
return package_xml
Expand Down
2 changes: 1 addition & 1 deletion src/rosdistro/manifest_provider/github.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def github_source_manifest_provider(repo):
req = Request(tree_url)
if GITHUB_USER and GITHUB_PASSWORD:
logger.debug('- using http basic auth from supplied environment variables.')
authheader = 'Basic %s' % base64.b64encode('%s:%s' % (GITHUB_USER, GITHUB_PASSWORD))
authheader = 'Basic %s' % base64.b64encode(b'%s:%s' % (GITHUB_USER, GITHUB_PASSWORD))
req.add_header('Authorization', authheader)
try:
tree_json = json.loads(urlopen(req).read().decode('utf-8'))
Expand Down