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

Multifactor authentication for yank command. #2514

Merged
merged 8 commits into from Feb 28, 2019

Conversation

@ecnelises
Copy link
Member

commented Dec 6, 2018

Description:

I added check for otp field, as multi-factor authentication feature to rubygems. Currently master branch of rubygems.org API has supported it. (See #1831) This PR will add respective support for client.


Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

cc: @sonalkr132

@@ -61,15 +62,23 @@ def execute

def yank_gem(version, platform)
say "Yanking gem from #{self.host}..."
yank_api_request(:delete, version, platform, "api/v1/gems/yank")
args = [:delete, version, platform, "api/v1/gems/yank"]
response = yank_api_request(*args)

This comment was marked as resolved.

Copy link
@sonalkr132

sonalkr132 Dec 8, 2018

Member

Why are we not passing options[:otp] here? I don't think we need use_otp argument. If options[:otp] is empty/nil, don't add it to request.
Please fix this for other commands as well.

@@ -57,6 +58,50 @@ def test_execute
assert_equal [yank_uri], @fetcher.paths
end

def test_execute_with_otp_success
response_fail = "You have enabled multifactor authentication but your request doesn't have the correct OTP code. Please check it and retry."

This comment was marked as outdated.

Copy link
@sonalkr132

sonalkr132 Dec 8, 2018

Member

single quotes here and at 86 please.

yank_uri = 'http://example/api/v1/gems/yank'
@fetcher.data[yank_uri] = proc do
@call_count ||= 0
(@call_count += 1).odd? ? [response_fail, 401, 'Unauthorized'] : ['Successfully yanked', 200, 'OK']

This comment was marked as outdated.

Copy link
@sonalkr132

sonalkr132 Dec 10, 2018

Member

what does this do again?

@hsbt

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

@sonalkr132 @ecnelises Please notify me when this pull-request is done status.


if data.respond_to?(:flatten!)
data.flatten!
data.size <= nargs ? data.last(nargs) : data.shift(nargs)

This comment was marked as resolved.

Copy link
@sonalkr132

sonalkr132 Jan 18, 2019

Member

Do we really need this ternary operator? data smaller than 3 values can never be of any use. Please change this to

return data.shift if data.respond_to?(:flatten!)
data

This comment was marked as resolved.

Copy link
@ecnelises

ecnelises Jan 28, 2019

Author Member

I mean the last 3 values should be kept regardless of how many times they are fetched. Otherwise the fake response array would be empty and cause error.

This comment was marked as outdated.

Copy link
@sonalkr132

sonalkr132 Jan 28, 2019

Member

the last 3 values should be kept regardless of how many times they are fetched

is this a requirement for tests you have added (since flatten would be true only for nested array) ? if yes, explicitly declare it in test. ie

ok_res = [response_success, 200, 'OK']
@stub_fetcher.data["..."] = [
     [response_fail, 401, 'Unauthorized'],
     ok_res,
     ok_res,
     ...
    ]

IMHO, when testing, some repetition is tolerable if it makes things simpler to understand.

@sonalkr132

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Please add comment about use of nested array when more than single response is expected from Gem::FakeFetcher. https://github.com/rubygems/rubygems/pull/2514/files#diff-ea68063aca43e347562c3b48fcaf82a9R13

@@ -87,8 +87,8 @@ def manage_owners(method, name, owners)
begin
response = send_owner_request(method, name, owner)

if need_otp? response
response = send_owner_request(method, name, owner, true)
if need_ask_otp? response

This comment was marked as outdated.

Copy link
@sonalkr132

sonalkr132 Jan 18, 2019

Member

AFAIR, I had suggested that checking if otp is needed and asking for otp should be separate function. This would cleaner when written like:

if need_otp? response
  ask_otp
  response = send_owner_request(method, name, owner)
end

This comment was marked as outdated.

Copy link
@ecnelises

ecnelises Jan 21, 2019

Author Member

Yes, I planned to commit this in another commit.

@ecnelises ecnelises force-pushed the ecnelises:multifactor-auth branch from ae8dad2 to e3ff746 Jan 28, 2019
@ecnelises ecnelises force-pushed the ecnelises:multifactor-auth branch from e3ff746 to f3595e6 Feb 1, 2019
@sonalkr132

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

ping!

@ecnelises

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

Updated.

Copy link
Member

left a comment

@hsbt This looks good to merge to me 👍

@hsbt

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

bundlerbot bot pushed a commit that referenced this pull request Feb 28, 2019
Merge #2514
2514: Multifactor authentication for yank command. r=hsbt a=ecnelises

# Description:

I added check for `otp` field, as multi-factor authentication feature to rubygems. Currently master branch of rubygems.org API has supported it. (See [#1831](rubygems/rubygems.org#1831)) This PR will add respective support for client. 
______________

# Tasks:

- [x] Describe the problem / feature
- [x] Write tests
- [x] Write code to solve the problem
- [ ] Get code review from coworkers / friends

I will abide by the [code of conduct](https://github.com/rubygems/rubygems/blob/master/CODE_OF_CONDUCT.md).

cc: @sonalkr132 

Co-authored-by: Qiu Chaofan <fwage73@gmail.com>
Co-authored-by: root <fwage73@gmail.com>
@bundlerbot

This comment has been minimized.

Copy link

commented Feb 28, 2019

@bundlerbot bundlerbot bot merged commit 9fd38a1 into rubygems:master Feb 28, 2019
3 checks passed
3 checks passed
bors Build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.