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

Use metadata['allowed_push_host'] to restrict gem pushing to that host only #603

Merged
merged 1 commit into from Sep 16, 2013

Conversation

seamusabshere
Copy link
Contributor

Defaults to nil, meaning any host is allowed. Includes tests.

Note that I had to change 2 tests in test_gem_commands_push_command.rb, namely test_execute and test_execute_host; they exploited the fact that gem push previously did not always attempt to load a gem's specification (it only loaded the gemspec if no host was specified). I used existing test helpers to fix those 2 tests.

I figure @rykov of @gemfury will appreciate this.

# spec.allowed_hosts = ['https://privategemserver.com']
#

attr_reader :allowed_hosts
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a new option can you use the metadata support? Metadata has the restriction that both the key and value must be a String. When do you allow pushing to multiple hosts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, it's unlikely there's more than one gemserver. Still, it feels a little weird to use key/value metadata instead of a "first-class option" to implement a relatively important form of security for private gems, but maybe that's just needless formalism. If you're sure this should be implemented with metadata, just let me know.

@seamusabshere
Copy link
Contributor Author

hey guys, should I go with @drbrain's suggestion?

@drbrain
Copy link
Member

drbrain commented Jul 30, 2013

We're preferring to add new fields in metadata as they don't require a release of RubyGems to use and the gems are buildable on older RubyGems.

@seamusabshere
Copy link
Contributor Author

@drbrain done!

@seamusabshere
Copy link
Contributor Author

would this have a higher chance of being accepted/merged if i changed the metadata key to something other than allowed_hosts? does it make sense to people more as allowed_gem_push_servers or something?

@drbrain
Copy link
Member

drbrain commented Aug 5, 2013

At this time I'm only applying bug fixes as RubyGems 2.1 is imminent. After 2.1 is out I'll flush the queue, this pull request included.

… come to think of it, I like allowed_push_servers, no need to throw "gem" in there everywhere.

…t only . Defaults to nil, meaning any host is allowed. Includes tests.

Note that I had to change 2 tests in test_gem_commands_push_command.rb, namely test_execute and test_execute_host; they exploited the fact that gem push previously did not always attempt to load a gem's specification (it only loaded the gemspec if no host was specified). I used existing test helpers to fix those 2 tests.

I figure @rykov of @gemfury will appreciate this. Thanks to @drbrain for implementation suggestions.
@seamusabshere
Copy link
Contributor Author

hi @drbrain - i changed the key to allowed_push_host, that seemed to be the most concise and accurate name.

@drbrain
Copy link
Member

drbrain commented Aug 31, 2013

After a quick glance, it looks good. 2.1 will be out next week, so I will merge this after the release.

drbrain added a commit that referenced this pull request Sep 16, 2013
Use metadata['allowed_push_host'] to restrict gem push to that host
@drbrain drbrain merged commit eb9a733 into rubygems:master Sep 16, 2013
drbrain added a commit that referenced this pull request Sep 16, 2013
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.

None yet

2 participants