oauth2 confirmation screen, closes #7762

codekitchen · codekitchen · commit ad8f17d07e16 · 2012-04-24T09:30:26.000-06:00
This explicit confirmation step is an improvement on our login-and-implicitly-accept workflow from before. And it allows us to do the oauth workflow without forcing a logout, which is much more ideal especially for embedded LTI tools that want to use oauth. Eventually this dialog will contain more information on the app and the permissions requested. test plan: As a client application, kick off the oauth workflow for a logged-in user, verify the user goes straight to the confirmation screen. Verify you only get a code back if they accept, and an error if they deny. Do the same without a web session, verify you go to the confirmation screen straight after logging in. Change-Id: Idf9905b795979339aec0cb5e4e058f4507a81bac Reviewed-on: https://gerrit.instructure.com/9804 Tested-by: Hudson <hudson@instructure.com> Reviewed-by: Brian Whitmer <brian@instructure.com>
diff --git a/app/controllers/pseudonym_sessions_controller.rb b/app/controllers/pseudonym_sessions_controller.rb
@@ -362,21 +362,7 @@ def successful_login(user, pseudonym)
     respond_to do |format|
       flash[:notice] = t 'notices.login_success', "Login successful." unless flash[:error]
       if session[:oauth2]
-        # this is where we will verify client authorization and scopes, once implemented
-        # .....
-        # now generate the temporary code, and respond/redirect
-        code = ActiveSupport::SecureRandom.hex(64)
-        code_data = { 'user' => user.id, 'client_id' => session[:oauth2][:client_id] }
-        Canvas.redis.setex("oauth2:#{code}", 1.day, code_data.to_json)
-        redirect_uri = session[:oauth2][:redirect_uri]
-        if redirect_uri == OAUTH2_OOB_URI
-          # destroy this user session, it's only for generating the token
-          @pseudonym_session.try(:destroy)
-          reset_session
-          format.html { redirect_to oauth2_auth_url(:code => code) }
-        else
-          format.html { redirect_to "#{redirect_uri}?code=#{code}" }
-        end
+        return redirect_to(oauth2_auth_confirm_url)
       elsif session[:course_uuid] && user && (course = Course.find_by_uuid_and_workflow_state(session[:course_uuid], "created"))
         claim_session_course(course, user)
         format.html { redirect_to(course_url(course, :login_success => '1')) }
@@ -396,26 +382,48 @@ def successful_login(user, pseudonym)
   OAUTH2_OOB_URI = 'urn:ietf:wg:oauth:2.0:oob'
 
   def oauth2_auth
-    if params[:code]
+    if params[:code] || params[:error]
       # hopefully the user never sees this, since it's an oob response and the
       # browser should be closed automatically. but we'll at least display
       # something basic.
       return render()
     end
 
-    key = DeveloperKey.find_by_id(params[:client_id]) if params[:client_id].present?
-    unless key
+    @key = DeveloperKey.find_by_id(params[:client_id]) if params[:client_id].present?
+    unless @key
       return render(:status => 400, :json => { :message => "invalid client_id" })
     end
 
     redirect_uri = params[:redirect_uri].presence || ""
-    unless redirect_uri == OAUTH2_OOB_URI || key.redirect_domain_matches?(redirect_uri)
+    unless redirect_uri == OAUTH2_OOB_URI || @key.redirect_domain_matches?(redirect_uri)
       return render(:status => 400, :json => { :message => "invalid redirect_uri" })
     end
 
-    session[:oauth2] = { :client_id => key.id, :redirect_uri => redirect_uri }
-    # force the user to re-authenticate
-    redirect_to login_url(:re_login => true)
+    session[:oauth2] = { :client_id => @key.id, :redirect_uri => redirect_uri }
+    if @current_pseudonym
+      redirect_to oauth2_auth_confirm_url
+    else
+      redirect_to login_url
+    end
+  end
+
+  def oauth2_confirm
+    @key = DeveloperKey.find(session[:oauth2][:client_id])
+    @app_name = @key.name.presence || @key.user_name.presence || @key.email.presence || t(:default_app_name, "Third-Party Application")
+  end
+
+  def oauth2_accept
+    # now generate the temporary code, and respond/redirect
+    code = ActiveSupport::SecureRandom.hex(64)
+    code_data = { 'user' => @current_user.id, 'client_id' => session[:oauth2][:client_id] }
+    Canvas.redis.setex("oauth2:#{code}", 1.day, code_data.to_json)
+    final_oauth2_redirect(session[:oauth2][:redirect_uri], :code => code)
+    session.delete(:oauth2)
+  end
+
+  def oauth2_deny
+    final_oauth2_redirect(session[:oauth2][:redirect_uri], :error => "access_denied")
+    session.delete(:oauth2)
   end
 
   def oauth2_token
@@ -445,4 +453,13 @@ def oauth2_token
       'user' => user.as_json(:only => [:id, :name], :include_root => false),
     }
   end
+
+  def final_oauth2_redirect(redirect_uri, opts = {})
+    if redirect_uri == OAUTH2_OOB_URI
+      redirect_to oauth2_auth_url(opts)
+    else
+      has_params = redirect_uri =~ %r{\?}
+      redirect_to(redirect_uri + (has_params ? "&" : "?") + opts.to_query)
+    end
+  end
 end
diff --git a/app/views/pseudonym_sessions/oauth2_auth.html.erb b/app/views/pseudonym_sessions/oauth2_auth.html.erb
@@ -1 +1,2 @@
-<%= t 'oauth2_authorized', 'Your application has been authorized for access to your Canvas data. Your session should begin automatically.' %>
+<%# normally the native application will intercept the redirect and this will never be seen, this is just a failsafe %>
+<%= t 'oauth2_complete', 'The application has finished the login workflow, and should reactivate shortly.' %>
diff --git a/app/views/pseudonym_sessions/oauth2_confirm.html.erb b/app/views/pseudonym_sessions/oauth2_confirm.html.erb
@@ -0,0 +1,24 @@
+<%
+  jammit_css :login
+  @headers = false
+  @body_classes << "modal"
+  content_for :page_title, t(:page_title, "App Login")
+%>
+<div id="modal-box-top"></div>
+<div id="modal-box-arbitrary-size" class="modal-box-more-padding">
+  <h2><%= @app_name %></h2>
+  <p class="modal-box-content">
+  <%= mt 'details.allow_application', "%{app_name} is requesting access to your account.", :app_name => @app_name %>
+    <br/><br/>
+    <%= mt 'details.login_name', "You are logging into this app as %{user_name}.", :user_name => link_to(@current_user.short_name, profile_url, :popup => true) %>
+    <% if @current_user.email.present? && @current_user.email != @current_user.short_name %>
+      <br/>
+      <%= t 'details.email', "Your email address is %{email}.", :email => @current_user.email %>
+    <% end %>
+  </p>
+  <div class="button_box">
+    <%= link_to(t(:cancel, "Cancel"), oauth2_auth_deny_path, :class => "button") %>
+    <%= link_to(t(:log_in, "Login"), oauth2_auth_accept_path, :method => :post, :class => "button button-default") %>
+  </div>
+</div>
+<div id="modal-box-bottom"></div>
diff --git a/config/routes.rb b/config/routes.rb
@@ -831,7 +831,10 @@ def et_routes(route_object, context)
   map.api_v1_files_create 'files_api', :controller => 'files', :action => 'api_create', :conditions => { :method => :post }
 
   map.oauth2_auth 'login/oauth2/auth', :controller => 'pseudonym_sessions', :action => 'oauth2_auth', :conditions => { :method => :get }
-  map.oauth2_token 'login/oauth2/token',:controller => 'pseudonym_sessions', :action => 'oauth2_token', :conditions => { :method => :post }
+  map.oauth2_token 'login/oauth2/token', :controller => 'pseudonym_sessions', :action => 'oauth2_token', :conditions => { :method => :post }
+  map.oauth2_auth_confirm 'login/oauth2/confirm', :controller => 'pseudonym_sessions', :action => 'oauth2_confirm', :conditions => { :method => :get }
+  map.oauth2_auth_accept 'login/oauth2/accept', :controller => 'pseudonym_sessions', :action => 'oauth2_accept', :conditions => { :method => :post }
+  map.oauth2_auth_deny 'login/oauth2/deny', :controller => 'pseudonym_sessions', :action => 'oauth2_deny', :conditions => { :method => :get }
 
   ApiRouteSet.route(map, "/api/lti/v1") do |lti|
     lti.post "tools/:tool_id/grade_passback", :controller => :lti_api, :action => :grade_passback, :path_name => "lti_grade_passback_api"
diff --git a/doc/api/oauth.md b/doc/api/oauth.md
@@ -111,6 +111,16 @@ response:
 The app can then extract the code, and use it along with the
 client_id and client_secret to obtain the final access_key.
 
+If the user doesn't accept the request for access, or if another error
+occurs, Canvas redirects back to your request\_uri with an `error`
+parameter, rather than a `code` parameter, in the query string.
+
+<div class="method_details">
+<h3>http://www.example.com/oauth2response?error=access_denied</h3>
+</div>
+
+`access_denied` is the only currently implemented error code.
+
 ### Step 3: Exchange the code for the final access token
 
 <div class="method_details">
@@ -226,6 +236,16 @@ changed to contain <code>code=&lt;code&gt;</code> somewhere in the query
 string. The app can then extract the code, and use it along with the
 client_id and client_secret to obtain the final access_key.
 
+If the user doesn't accept the request for access, or if another error
+occurs, Canvas will add an `error`
+parameter, rather than a `code` parameter, to the query string.
+
+<div class="method_details">
+<h3>/login/oauth2/auth?error=access_denied</h3>
+</div>
+
+`access_denied` is the only currently implemented error code.
+
 ### Step 3: Exchange the code for the final access token
 
 <div class="method_details">
diff --git a/spec/apis/auth_spec.rb b/spec/apis/auth_spec.rb
@@ -137,20 +137,21 @@ def flow(opts = {})
 
           # step 1
           get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => 'urn:ietf:wg:oauth:2.0:oob'
-          response.should redirect_to(login_url(:re_login => true))
+          response.should redirect_to(login_url)
 
           yield
 
-          # step 2
+          # step 3
           response.should be_redirect
-          response['Location'].should match(%r{/login/oauth2/auth?})
-          code = response['Location'].match(/code=([^\?&]+)/)[1]
-          code.should be_present
+          response['Location'].should match(%r{/login/oauth2/confirm$})
+          get response['Location']
+          response.should render_template("pseudonym_sessions/oauth2_confirm")
+          post "/login/oauth2/accept", { :authenticity_token => session[:_csrf_token] }
 
-          # make sure the user is now logged out, or the app also has full access to their session
-          get '/'
           response.should be_redirect
-          response['Location'].should == 'http://www.example.com/login'
+          response['Location'].should match(%r{/login/oauth2/auth\?})
+          code = response['Location'].match(/code=([^\?&]+)/)[1]
+          code.should be_present
 
           # we have the code, we can close the browser session
           if opts[:basic_auth]
@@ -241,11 +242,45 @@ def cas.validate_service_ticket(st)
 
           get '/login', :ticket => 'ST-abcd'
           response.should be_redirect
-          response['Location'].should match(%r{/login/oauth2/auth\?code=})
-          session.should be_blank
         end
       end
 
+      it "should not require logging in again, or log out afterwards" do
+        course_with_student_logged_in(:active_all => true, :user => user_with_pseudonym)
+        get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => 'urn:ietf:wg:oauth:2.0:oob'
+        response.should be_redirect
+        response['Location'].should match(%r{/login/oauth2/confirm$})
+        get response['Location']
+        response.should render_template("pseudonym_sessions/oauth2_confirm")
+        post "/login/oauth2/accept", { :authenticity_token => session[:_csrf_token] }
+        response.should be_redirect
+        response['Location'].should match(%r{/login/oauth2/auth\?})
+        code = response['Location'].match(/code=([^\?&]+)/)[1]
+        code.should be_present
+        get response['Location']
+        response.should be_success
+        # verify we're still logged in
+        get "/courses/#{@course.id}"
+        response.should be_success
+      end
+
+      it "should redirect with access_denied if the user doesn't accept" do
+        course_with_student_logged_in(:active_all => true, :user => user_with_pseudonym)
+        get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => 'urn:ietf:wg:oauth:2.0:oob'
+        response.should be_redirect
+        response['Location'].should match(%r{/login/oauth2/confirm$})
+        get response['Location']
+        response.should render_template("pseudonym_sessions/oauth2_confirm")
+        get "/login/oauth2/deny"
+        response.should be_redirect
+        response['Location'].should match(%r{/login/oauth2/auth\?})
+        error = response['Location'].match(%r{error=([^\?&]+)})[1]
+        error.should == "access_denied"
+        response['Location'].should_not match(%r{code=})
+        get response['Location']
+        response.should be_success
+      end
+
       it "should allow http basic auth for the app auth" do
         flow(:basic_auth => true) do
           get response['Location']
@@ -257,7 +292,7 @@ def cas.validate_service_ticket(st)
       it "should require the correct client secret" do
         # step 1
         get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => 'urn:ietf:wg:oauth:2.0:oob'
-        response.should redirect_to(login_url(:re_login => true))
+        response.should redirect_to(login_url)
 
         get response['Location']
         response.should be_success
@@ -268,7 +303,9 @@ def cas.validate_service_ticket(st)
 
         # step 2
         response.should be_redirect
-        response['Location'].should match(%r{/login/oauth2/auth?})
+        response['Location'].should match(%r{/login/oauth2/confirm$})
+        post "/login/oauth2/accept", { :authenticity_token => session[:_csrf_token] }
+
         code = response['Location'].match(/code=([^\?&]+)/)[1]
         code.should be_present
 
@@ -304,12 +341,17 @@ def cas.validate_service_ticket(st)
             @key.update_attribute :redirect_uri, 'http://www.example.com/oauth2response'
 
             get "/login/oauth2/auth", :response_type => 'code', :client_id => @client_id, :redirect_uri => "http://www.example.com/my_uri"
-            response.should redirect_to(login_url(:re_login => true))
+            response.should redirect_to(login_url)
 
             get response['Location']
             response.should be_success
             post "/login", :pseudonym_session => { :unique_id => 'test1@example.com', :password => 'test123' }
 
+            response.should be_redirect
+            response['Location'].should match(%r{/login/oauth2/confirm$})
+            get response['Location']
+            post "/login/oauth2/accept", { :authenticity_token => session[:_csrf_token] }
+
             response.should be_redirect
             response['Location'].should match(%r{http://www.example.com/my_uri?})
             code = response['Location'].match(/code=([^\?&]+)/)[1]
diff --git a/spec/selenium/oauth_spec.rb b/spec/selenium/oauth_spec.rb
@@ -0,0 +1,51 @@
+require File.expand_path(File.dirname(__FILE__) + '/common')
+
+if Canvas.redis_enabled?
+
+describe "oauth2 flow" do
+  it_should_behave_like "in-process server selenium tests"
+
+  before do
+    @key = DeveloperKey.create!(:name => 'Specs')
+    @client_id = @key.id
+    @client_secret = @key.api_key
+  end
+
+  describe "a logged-in user" do
+    before do
+      course_with_student_logged_in(:active_all => true)
+    end
+
+    it "should show the confirmation dialog without requiring login" do
+      get "/login/oauth2/auth?response_type=code&client_id=#{@client_id}&redirect_uri=urn:ietf:wg:oauth:2.0:oob"
+      f('#modal-box-arbitrary-size').text.should match(%r{Specs is requesting access to your account})
+      expect_new_page_load { f('#modal-box-arbitrary-size a.button-default').click() }
+      driver.current_url.should match(%r{/login/oauth2/auth\?})
+      code = driver.current_url.match(%r{code=([^\?&]+)})[1]
+      code.should be_present
+    end
+  end
+
+  describe "a non-logged-in user" do
+    before do
+      course_with_student(:active_all => true, :user => user_with_pseudonym)
+    end
+
+    it "should show the confirmation dialog after logging in" do
+      get "/login/oauth2/auth?response_type=code&client_id=#{@client_id}&redirect_uri=urn:ietf:wg:oauth:2.0:oob"
+      driver.current_url.should match(%r{/login$})
+      user_element = driver.find_element(:css, '#pseudonym_session_unique_id')
+      user_element.send_keys("nobody@example.com")
+      password_element = driver.find_element(:css, '#pseudonym_session_password')
+      password_element.send_keys("asdfasdf")
+      password_element.submit
+      f('#modal-box-arbitrary-size').text.should match(%r{Specs is requesting access to your account})
+      expect_new_page_load { f('#modal-box-arbitrary-size a.button-default').click() }
+      driver.current_url.should match(%r{/login/oauth2/auth\?})
+      code = driver.current_url.match(%r{code=([^\?&]+)})[1]
+      code.should be_present
+    end
+  end
+end
+
+end

Original file line number	Diff line number	Diff line change
`@@ -1 +1,2 @@`
`1`		`-<%= t 'oauth2_authorized', 'Your application has been authorized for access to your Canvas data. Your session should begin automatically.' %>`
	`1`	`+<%# normally the native application will intercept the redirect and this will never be seen, this is just a failsafe %>`
	`2`	`+<%= t 'oauth2_complete', 'The application has finished the login workflow, and should reactivate shortly.' %>`