Update admin/mkassoc to enable to work #103

Merged
merged 1 commit into from Feb 25, 2017

Conversation

Projects
None yet
2 participants
@serihiro
Contributor

serihiro commented Feb 23, 2017

What will this pr change ?

  • Enable to work admin/mkassoc.
    • When I tried to use this script with the newest version of ruby-openid, errors occurred for some interface mismatch.
@@ -1,11 +1,11 @@
#!/usr/bin/env ruby
require "openid/consumer/associationmanager"
-require "openid/store/memstore"
+require "openid/store/memory"

This comment has been minimized.

@tobiashm

tobiashm Feb 23, 2017

Contributor

👍

@tobiashm

tobiashm Feb 23, 2017

Contributor

👍

admin/mkassoc
ARGV.each do |server_url|
- mgr = OpenID::Consumer::AssociationManager.new(store, URI.parse(server_url))
+ mgr = OpenID::Consumer::AssociationManager.new(store, server_url)

This comment has been minimized.

@tobiashm

tobiashm Feb 23, 2017

Contributor

Why have you removed the URI.parse call?
I would guess that it's there to ensure that the arguments are valid URIs?

@tobiashm

tobiashm Feb 23, 2017

Contributor

Why have you removed the URI.parse call?
I would guess that it's there to ensure that the arguments are valid URIs?

This comment has been minimized.

@serihiro

serihiro Feb 23, 2017

Contributor

@tobiashm
Thank you for your comment!

If OpenID::Consumer::AssociationManager#get_association with @server_url of URI object is called, bad URI error occured like the following:

$ ruby --version
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin16]

$ bundle exec ruby admin/mkassoc http://auth.livedoor.com/openid/server
==================================================
Server: http://auth.livedoor.com/openid/server
/Users/seri/Documents/ruby-work/ruby-openid/lib/openid/kvpost.rb:54:in `rescue in make_kv_post': Unable to contact OpenID server: bad URI(is not URI?): http://
auth.livedoor.com/openid/server (OpenID::KVPostNetworkError)
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/kvpost.rb:51:in `make_kv_post'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:195:in `request_association'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:130:in `negotiate_association'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:118:in `get_association'
        from admin/mkassoc:11:in `block in <main>'
        from admin/mkassoc:7:in `each'
        from admin/mkassoc:7:in `<main>

This is because URI.parse is called twice, 1st time is here, and second time is here.

So, if you want to validate server_url in admin/mkassoc, how about adding this?

ARGV.each do |server_url|
  unless URI::regexp =~ server_url
    puts "`#{server_url}` will be skipped for invalid URI format."
    next
  end
  ...

end
@serihiro

serihiro Feb 23, 2017

Contributor

@tobiashm
Thank you for your comment!

If OpenID::Consumer::AssociationManager#get_association with @server_url of URI object is called, bad URI error occured like the following:

$ ruby --version
ruby 2.3.3p222 (2016-11-21 revision 56859) [x86_64-darwin16]

$ bundle exec ruby admin/mkassoc http://auth.livedoor.com/openid/server
==================================================
Server: http://auth.livedoor.com/openid/server
/Users/seri/Documents/ruby-work/ruby-openid/lib/openid/kvpost.rb:54:in `rescue in make_kv_post': Unable to contact OpenID server: bad URI(is not URI?): http://
auth.livedoor.com/openid/server (OpenID::KVPostNetworkError)
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/kvpost.rb:51:in `make_kv_post'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:195:in `request_association'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:130:in `negotiate_association'
        from /Users/seri/Documents/ruby-work/ruby-openid/lib/openid/consumer/associationmanager.rb:118:in `get_association'
        from admin/mkassoc:11:in `block in <main>'
        from admin/mkassoc:7:in `each'
        from admin/mkassoc:7:in `<main>

This is because URI.parse is called twice, 1st time is here, and second time is here.

So, if you want to validate server_url in admin/mkassoc, how about adding this?

ARGV.each do |server_url|
  unless URI::regexp =~ server_url
    puts "`#{server_url}` will be skipped for invalid URI format."
    next
  end
  ...

end

This comment has been minimized.

@tobiashm

tobiashm Feb 24, 2017

Contributor

Really like your last suggestion for validation.
Could you add that? Then I'll merge this.

@tobiashm

tobiashm Feb 24, 2017

Contributor

Really like your last suggestion for validation.
Could you add that? Then I'll merge this.

@tobiashm

This comment has been minimized.

Show comment
Hide comment
@tobiashm

tobiashm Feb 24, 2017

Contributor

Looking into the Travis CI build failure, which doesn't seem to have anything to do with this code.

Contributor

tobiashm commented Feb 24, 2017

Looking into the Travis CI build failure, which doesn't seem to have anything to do with this code.

@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Feb 25, 2017

Contributor

@tobiashm
I added validation and squashed into 6bcd3d0 . Please check this 🙇

Contributor

serihiro commented Feb 25, 2017

@tobiashm
I added validation and squashed into 6bcd3d0 . Please check this 🙇

@tobiashm tobiashm merged commit b7b3316 into openid:master Feb 25, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
@tobiashm

This comment has been minimized.

Show comment
Hide comment
@tobiashm

tobiashm Feb 25, 2017

Contributor

@serihiro thank you

Contributor

tobiashm commented Feb 25, 2017

@serihiro thank you

@serihiro

This comment has been minimized.

Show comment
Hide comment
@serihiro

serihiro Feb 25, 2017

Contributor

Thank you for your merging! 🍻

Contributor

serihiro commented Feb 25, 2017

Thank you for your merging! 🍻

@serihiro serihiro deleted the serihiro:follow-up-newest-version branch Feb 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment