Merge pull request openshift#346 from smarterclayton/bug_927425_use_a…

…uthorization_tokens_not_strict_enough

Merged by openshift-bot

lib/rhc/commands.rb

@@ -232,7 +232,7 @@ def self.fill_arguments(cmd, options, args_metadata, options_metadata, args)
         Commander::Runner.instance.options.each do |opt|
           if opt[:context]
             arg = Commander::Runner.switch_to_sym(opt[:switches].last)
-            options[arg] ||= lambda{ cmd.send(opt[:context]) }
+            options.__hash__[arg] ||= lambda{ cmd.send(opt[:context]) }
           end
         end
 

lib/rhc/commands/account.rb

@@ -45,11 +45,11 @@ def logout
         rescue RHC::Rest::AuthorizationsNotSupported
           info "not supported"
         end
-      elsif options.token
+      elsif token_for_user
         options.noprompt = true
         say "Ending session on server ... "
         begin
-          rest_client.delete_authorization(options.token)
+          rest_client.delete_authorization(token_for_user)
           success "deleted"
         rescue RHC::Rest::AuthorizationsNotSupported
           info "not supported"

lib/rhc/commands/authorization.rb

@@ -12,7 +12,7 @@ class Authorization < Base
       remembering what is available.
       DESC
     def run
-      rest_client.authorizations.each{ |auth| paragraph{ display_authorization(auth, options.token) } } or info "No authorizations"
+      rest_client.authorizations.each{ |auth| paragraph{ display_authorization(auth, token_for_user) } } or info "No authorizations"
 
       0
     end

lib/rhc/commands/base.rb

@@ -31,7 +31,7 @@ def initialize(options=Commander::Command::Options.new,
     def rest_client(opts={})
       @rest_client ||= begin
           auth = RHC::Auth::Basic.new(options)
-          auth = RHC::Auth::Token.new(options, auth, token_store)
+          auth = RHC::Auth::Token.new(options, auth, token_store) if (options.use_authorization_tokens || options.token) && !(options.rhlogin && options.password)
           client_from_options(:auth => auth)
         end
     end

lib/rhc/context_helper.rb

@@ -12,10 +12,6 @@ def server_context
       ENV['LIBRA_SERVER'] || (!options.clean && config['libra_server']) || "openshift.redhat.com"
     end
 
-    def token_context
-      token_store.get(options.rhlogin, options.server) if options.rhlogin
-    end
-
     def app_context
       debug "Getting app context"
 

lib/rhc/helpers.rb

@@ -100,7 +100,7 @@ def user_agent
 
     global_option '-l', '--rhlogin LOGIN', "OpenShift login"
     global_option '-p', '--password PASSWORD', "OpenShift password"
-    global_option '--token TOKEN', "An authorization token for accessing your account.", :context => :token_context
+    global_option '--token TOKEN', "An authorization token for accessing your account."
 
     global_option '-d', '--debug', "Turn on debugging", :hide => true
 
@@ -154,6 +154,10 @@ def openshift_rest_endpoint
       uri.path = '/broker/rest/api' if uri.path.blank? || uri.path == '/'
       uri
     end
+
+    def token_for_user
+      options.token or (token_store.get(options.rhlogin, options.server) if options.rhlogin)
+    end
 
     def client_from_options(opts)
       RHC::Rest::Client.new({

lib/rhc/wizard.rb

@@ -99,7 +99,7 @@ def token_store
       end
 
       def username
-        auth.username if auth.respond_to?(:username)
+        options.rhlogin || (auth.username if auth.respond_to?(:username))
       end
 
       def print_dot
@@ -129,7 +129,8 @@ def greeting_stage
     end
 
     def login_stage
-      if options.token
+      if token_for_user
+        options.token = token_for_user
         say "Using an existing token for #{options.rhlogin} to login to #{openshift_server}"
       elsif options.rhlogin
         say "Using #{options.rhlogin} to login to #{openshift_server}"
@@ -161,6 +162,7 @@ def login_stage
       end
 
       self.user = rest_client.user
+      options.rhlogin = self.user.login unless username
 
       if rest_client.supports_sessions? && !options.token && options.create_token != false
         paragraph do

spec/rhc/command_spec.rb

@@ -122,18 +122,27 @@ def execute(testarg); 1; end
             summary "Test command execute-list"
             def execute_list(args); 1; end
 
+            RHC::Helpers.global_option '--test-context', 'Test', :context => :context_var
+            def execute_implicit
+            end
+
             def raise_error
               raise StandardError.new("test exception")
             end
             def raise_exception
               raise Exception.new("test exception")
             end
+
+            protected
+              def context_var
+                "contextual"
+              end
           end
         end
         Static
       end
 
-      it("should register itself") { expect { subject }.to change(commands, :length).by(5) }
+      it("should register itself") { expect { subject }.to change(commands, :length).by(6) }
       it("should have an object name of the class") { subject.object_name.should == 'static' }
 
       context 'and when test is called' do
@@ -169,6 +178,10 @@ def raise_exception
         it('should make the option available') { command_for('static', 'execute-list', '1', '2', '3').send(:options).tests.should == ['1','2','3'] }
       end
 
+      context 'and when execute is called with a contextual global option' do
+        it("calls the helper") { command_for('static', 'execute-implicit').send(:options).test_context.should == 'contextual' }
+      end
+
       context 'and when an error is raised in a call' do
         it { expects_running('static', 'raise-error').should raise_error(StandardError, "test exception") }
       end
@@ -200,15 +213,22 @@ def raise_exception
     let(:instance){ subject }
 
     context "when initializing the object" do
-      let(:auth){ mock }
-      let(:basic_auth){ mock }
-      before{ RHC::Auth::Basic.should_receive(:new).once.with{ |arg| arg.should == instance.send(:options) }.and_return(basic_auth) }
-      before{ RHC::Auth::Token.should_receive(:new).once.with{ |arg, arg2, arg3| [arg, arg2, arg3].should == [instance.send(:options), basic_auth, instance.send(:token_store)] }.and_return(auth) }
-
-      it "should create a new auth object" do
-        subject.should_receive(:client_from_options).with(:auth => auth)
-        subject.send(:rest_client)
+      let(:auth){ mock('auth') }
+      let(:basic_auth){ mock('basic_auth') }
+      before{ RHC::Auth::Basic.should_receive(:new).at_least(1).times.with{ |arg| arg.should == instance.send(:options) }.and_return(basic_auth) }
+      before{ RHC::Auth::Token.should_receive(:new).any_number_of_times.with{ |arg, arg2, arg3| [arg, arg2, arg3].should == [instance.send(:options), basic_auth, instance.send(:token_store)] }.and_return(auth) }
+
+      context "with no options" do
+        before{ subject.should_receive(:client_from_options).with(:auth => basic_auth) }
+        it("should create only a basic auth object"){ subject.send(:rest_client) }
+      end
+
+      context "with use_authorization_tokens" do
+        before{ subject.send(:options).use_authorization_tokens = true }
+        before{ subject.should_receive(:client_from_options).with(:auth => auth) }
+        it("should create a token auth object"){ subject.send(:rest_client) }
       end
+
       it { subject.send(:rest_client).should be_a(RHC::Rest::Client) }
       it { subject.send(:rest_client).should equal subject.send(:rest_client) }
     end
@@ -217,7 +237,7 @@ def raise_exception
       subject{ Class.new(RHC::Commands::Base){ object_name :test; def run; 0; end } }
       let(:instance) { subject.new }
       let(:rest_client){ command_for(*arguments).send(:rest_client) }
-      let(:basic_auth){ rest_client.send(:auth).send(:auth) }
+      let(:basic_auth){ auth = rest_client.send(:auth); auth.is_a?(RHC::Auth::Basic) ? auth : auth.send(:auth) }
       let(:stored_token){ nil }
       before{ instance.send(:token_store).stub(:get).and_return(nil) unless stored_token }
 
@@ -257,18 +277,26 @@ def raise_exception
         let(:username){ 'foo' }
         let(:stored_token){ 'a_token' }
         let(:arguments){ ['test', '-l', username, '--server', mock_uri] }
-        before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.and_return(stored_token) }
         before{ stub_api; stub_user(:token => stored_token) }
-        it("has token set") { command_for(*arguments).send(:options).token.should == stored_token }
-        it("calls the server") { rest_client.user }
+
+        context "when tokens are not allowed" do
+          it("calls the server") { rest_client.send(:auth).is_a? RHC::Auth::Basic }
+        end
+
+        context "when tokens are allowed" do
+          let!(:config){ base_config{ |c, d| d.add('use_authorization_tokens', 'true') } }
+          before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.and_return(stored_token) }
+          it("has token set") { command_for(*arguments).send(:token_for_user).should == stored_token }
+          it("calls the server") { rest_client.user }
+        end
       end
 
       context "with username and tokens enabled" do
         let!(:config){ base_config{ |c, d| d.add('use_authorization_tokens', 'true') } }
         let(:username){ 'foo' }
         let(:auth_token){ stub(:token => 'a_token') }
         let(:arguments){ ['test', '-l', username, '--server', mock_uri] }
-        before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.twice.and_return(nil) }
+        before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.and_return(nil) }
         before{ stub_api(false, true); stub_api_request(:get, 'broker/rest/user', false).to_return{ |request| request.headers['Authorization'] =~ /Bearer/ ? simple_user(username) : {:status => 401} } }
         it("should attempt to create a new token") do 
           rest_client.should_receive(:new_session).ordered.and_return(auth_token)
@@ -280,7 +308,7 @@ def raise_exception
         let!(:config){ base_config{ |c, d| d.add('use_authorization_tokens', 'true') } }
         let(:username){ 'foo' }
         let(:arguments){ ['test', '-l', username, '--server', mock_uri] }
-        before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.twice.and_return(nil) }
+        before{ instance.send(:token_store).should_receive(:get).with{ |user, server| user.should == username; server.should == instance.send(:openshift_server) }.and_return(nil) }
         before{ stub_api(false, false); stub_api_request(:get, 'broker/rest/user', false).to_return{ |request| request.headers['Authorization'] =~ /Basic/ ? simple_user(username) : {:status => 401} } }
         it("should prompt for password") do 
           basic_auth.should_receive(:ask).once.and_return('password')

spec/rhc/commands/account_spec.rb

@@ -42,7 +42,7 @@
   describe '#logout' do
     let(:arguments) { ['account', 'logout'] }
     let(:username) { 'foo' }
-    let(:password) { 'pass' }
+    let(:password) { nil }
     let(:supports_auth) { false }
     let(:server) { mock_uri }
     let!(:token_store) { RHC::Auth::TokenStore.new(Dir.mktmpdir) }

spec/rhc/wizard_spec.rb

@@ -97,13 +97,14 @@ def mock_config
     let(:password){ 'test pass' }
     let(:rest_client){ stub }
     let(:auth){ subject.send(:auth) }
+    let(:user_obj){ stub(:login => user) }
 
     subject{ described_class.new(config, options) }
 
     def expect_client_test(with_sessions=false)
       subject.should_receive(:new_client_for_options).ordered.and_return(rest_client)
       rest_client.should_receive(:api).ordered
-      rest_client.should_receive(:user).ordered.and_return(true)
+      rest_client.should_receive(:user).ordered.and_return(user_obj)
       rest_client.should_receive(:supports_sessions?).ordered.and_return(with_sessions)
     end
     def expect_raise_from_api(error)
@@ -114,6 +115,7 @@ def expect_raise_from_api(error)
     it "should prompt for user and password" do
       expect_client_test
       subject.send(:login_stage).should be_true
+      subject.send(:options).rhlogin.should == user
     end
 
     context "with token" do
@@ -181,6 +183,7 @@ def expect_raise_from_api(error)
         before{ RHC::Auth::TokenStore.should_receive(:new).any_number_of_times.and_return(store) }
 
         it "should not generate a token if the user does not request it" do
+          store.should_receive(:get).and_return(nil)
           subject.should_receive(:info).with(/OpenShift can create and store a token on disk/).ordered
           subject.should_receive(:agree).with(/Generate a token now?/).ordered.and_return(false)
 
@@ -189,6 +192,7 @@ def expect_raise_from_api(error)
         end
 
         it "should generate a token if the user requests it" do
+          store.should_receive(:get).and_return(nil)
           subject.should_receive(:info).with(/OpenShift can create and store a token on disk/).ordered
           subject.should_receive(:agree).with(/Generate a token now?/).ordered.and_return(true)
           subject.should_receive(:say).with(/Generating an authorization token for this client /).ordered