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

Add support for interacting with repositories by id. #1120

Closed
MicahZoltu opened this issue Mar 6, 2016 · 13 comments
Closed

Add support for interacting with repositories by id. #1120

MicahZoltu opened this issue Mar 6, 2016 · 13 comments
Labels
Type: Feature New feature or request

Comments

@MicahZoltu
Copy link
Contributor

All of the commands that work with https://api.github.com/repos/:owner/:repo/ also work with https://api.github.com/repositories/:id/. If one desires to build an application that is resilient to change (e.g., repositories changing names, changing owners, owners changing name, etc.) then the application should be built against the repositories API rather than the repos API. Unfortunately, it appears that this library only supports the repos API.

An application can be mostly resilient against change by saving the ID long term and then using the repositories API to convert it to an owner/repo right before making the API calls they really want. This is still not great though as it can race and lead to very unexpected behavior if you are in the middle of a complex transaction (spanning multiple API calls) when the repository moves out from under you.

Please add support for making API calls via ID rather than owner/repo so developers can avoid unexpected behavior.

Example:

public interface IBlobsClient
{
    Task<BlobReference> Create(String owner, String name, NewBlob newBlob);
    Task<BlobReference> Create(String id, NewBlob newBlob);

    Task<Blob> Get(String owner, String name, String reference);
    Task<Blob> Get(String id, String reference);
}

Even better would be to abstract out the concept into a Repo object of some kind that can either be an ID or an owner/name. That way the public surface area doesn't need to be twice as big. However, deleting the old methods would be a breaking change and require a major version bump.

@shiftkey
Copy link
Member

shiftkey commented Mar 7, 2016

Some initial thoughts:

If one desires to build an application that is resilient to change (e.g., repositories changing names, changing owners, owners changing name, etc.) then the application should be built against the repositories API rather than the repos API.

This should be covered by the API supporting redirects which Octokit now follows correctly.

This is still not great though as it can race and lead to very unexpected behavior if you are in the middle of a complex transaction (spanning multiple API calls) when the repository moves out from under you.

I'd love to hear more about this scenario and what you're looking to build here. With most of our operations (whether returning a task or an observable) we're interested in getting the data as soon as possible. If that scenario isn't suitable for your use case, perhaps there's something we can be doing in Octokit to address this.

@MicahZoltu
Copy link
Contributor Author

Unfortunately, I am pretty sure that repository redirects can be subverted by new repositories and new users. If I create Zoltu/Foo, then rename it to Zoltu/Bar, I can then create a new Zoltu/Foo. If someone tries to go to Zoltu/Foo before I created the new Zoltu/Foo they will correctly be redirected. If they try to navigate to Zoltu/Foo after I create the new repository though they will be taken to the new repository.

To make matters worse, in the case of a name change a malicious user can actually mount an attack. Lets say Google decides to change their name to Abc within the GitHub organization. If they don't have the foresight to also create a new user to reclaim the Google name then a malicious user can create a user named Google and copy all of Google's repositories over from Abc. Now they can inject subtle backdoors, security holes, etc. into every repository and anyone who is going to Google/Foo (because they have a bookmark or old link in a blogpost etc) they will instead get Google/Foo + evil code.

See: https://help.github.com/articles/what-happens-when-i-change-my-username/

You are right, the /repositories/:id endpoint is not documented well, and I am not sure why that is. I can only assume it is because most users are familiar with the owner/name nomenclature and having a second endpoint with an id that no one ever sees could be confusing to new API users.

As for what I am doing in my project, I am maniuplating repository objects via the GitHub API, primarily because library support for the GitHub API is superior to library support for the git protocol (which is a separate can of worms). In this case, interacting with a git repository via Octokit from an application is significantly easier than trying to use something like libgit2 (or any of the wrappers around it), especially when you want to work with the repository in-memory (not clone to disk).

The most important bit for my scenario is that I am saving repository references in my own database for extended periods of time that would make my application susceptible to the above type of attack. At the least, I would need to save the id and resolve it to an owner/repo before doing any real work (which occurs over the span of about a minute). This would limit the surface area of an attack down to a small enough window of time that it is no longer an immediate problem.

Perhaps as a middle-ground, there could be a single method in Octokit that allows me to resolve an id into a owner/repo? I believe at the moment I am going to have to manually hit the API (without Octokit) in order to do that resolution before proceeding to use Octokit.

@shiftkey
Copy link
Member

shiftkey commented Mar 7, 2016

@Zoltu thanks for the extra information - and yes, there are certainly ways to subvert the process.

I had a chat with one of the API team just now and he told me that we intend to support /repositories/:id (even if it isn't documented) in the long term. I think I'm going to spin this out into a GSoC task for someone who is interested in contributing - unless someone else wants to claim it...

@shiftkey
Copy link
Member

shiftkey commented Mar 7, 2016

I've opened this up as a potential GSoC activity over in github/mentorships#112

@heshuimu
Copy link

heshuimu commented Mar 8, 2016

Hi @shiftkey,
So I stumbled into this while browsing through the GSoC idea list, and yes I am interested in working on this. I have done C# programming for the past few years (in Unity3D, to be specific).
I am fairly new to this project, though. Are there any ways, may I ask, that I could catch up with the project other than reading through the repo?
Thanks! I really appreciate it!

@shiftkey
Copy link
Member

shiftkey commented Mar 8, 2016

@heshuimu have a look at our documentation - http://octokitnet.readthedocs.org/ - it's not 100% covered, but will give you an idea of the basics. Also, the open pull requests for the repository will give you an idea of what's in flight and how we work...

@heshuimu
Copy link

heshuimu commented Mar 8, 2016

@shiftkey Thank you sir! I appreciate it!

@l17719
Copy link

l17719 commented Mar 8, 2016

Hello.
While i was browsing the ideas list for GSoC i came across your project and i'm interested in working with this project. I've done some work in C# in the past years, some of it as a junior developer for some small to mid companies and also some internships in order to get my bearings on what's to come when i'm finished with my degree. That asides, i'm fairly new to the project as well and i'm now on the process of reading the docs and i've got the project migrated to my machine.
Also i'm currently checking the easy-fix items listed and see if i can help out any way. I think that's a good idea on how to connect with the project and see it's inner workings.

@alexander-efremov
Copy link
Contributor

Hello, @shiftkey! I want to implement this idea during GSoC 2016. I know that application period starting since 15 March and I will send my proposal after this date with usage of application form, but I have done some investigation of this task right now.

I think that we could create overloaded method that use repository Id as main parameter for each method that use owner and repo name as their parameters. This is @Zoltu example:

public interface IBlobsClient
{
    Task<BlobReference> Create(String owner, String name, NewBlob newBlob);
    Task<BlobReference> Create(String id, NewBlob newBlob);

    Task<Blob> Get(String owner, String name, String reference);
    Task<Blob> Get(String id, String reference);
}

I found about 29 client interfaces in Octokit project that use owner and name of repository parameters, so we could create overload for each of theirs methods (I have counted about 175 methods in these interfaces).
I think that usage of such overloads is more preferable than creation some kind of abstract concept of Repo (as was suggested by @Zoltu in first message), because we get more simple and clean interfaces.

So, my proposal is create overload for each method that use owner and name parameters now and create test case for each of them.

@shiftkey
Copy link
Member

@dampir that all sounds reasonable

@ryangribble
Copy link
Contributor

👍

@alexander-efremov
Copy link
Contributor

alexander-efremov commented Jun 8, 2016

This post describes the work I've done for the GSoC 2016. Here are presented all PRs that I've created in order to add support into Octokit.NET for lookup by repository Id throughout GitHub APIs. I'm going to send the link to this post as work product submission for final evaluation of GSoC 2016.

Here are additional links:

  • Octokit.NET 0.21 - first release of Octokit.NET with the repository Id support. Here is a cite from release notes:

    This release adds support across Octokit.net for providing the repository Id
    rather than a name/owner pair. The repository Id does not change when transferring
    ownership of a repository, and is more robust for API callers. This work
    was lead by @dampir as part of Google Summer of Code 2016.

  • Merged repository id PRs - the link on merged PRs that I've created during GSoC 2016.

I want to say big thank you for all people that help me during GSoC period, especially for my mentor @shiftkey. 👍 🙏 Also I want to say big thank you for @ryangribble for help in review and merge of so big amount of PRs. 👍 🙏

Full list of clients that were changed during GSoC 2016:

This was referenced Jun 10, 2016
@shiftkey
Copy link
Member

This will be available in Octokit 0.21 which is almost ready to 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants
@nickfloyd @shiftkey @MicahZoltu @alexander-efremov @ryangribble @heshuimu @l17719 and others