From 6f3d934bc1c11e7c316e5956c59d4e37541c8737 Mon Sep 17 00:00:00 2001 From: Sorin Davidoi Date: Sat, 8 Sep 2018 23:54:28 +0200 Subject: feat(cookies): Use the same-site attribute to lax (#8626) CSFR-prevention is already implemented but adding this doesn't hurt. A brief introduction to Same-Site cookies (and the difference between strict and lax) can be found at https://blog.mozilla.org/security/2018/04/24/same-site-cookies-in-firefox-60/ TLDR: We use lax since we want the cookies to be sent when the user navigates safely from an external site. --- spec/rails_helper.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'spec') diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index c575128e4..79e80220c 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -29,6 +29,7 @@ Devise::Test::ControllerHelpers.module_eval do value: resource.activate_session(warden.request), expires: 1.year.from_now, httponly: true, + same_site: :lax, } end end -- cgit From 2288d50a7bb4afcec4cfbcaa4b5cffaabd1df437 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 9 Sep 2018 04:10:44 +0200 Subject: Add force_login option to OAuth authorize page (#8655) * Add force_login option to OAuth authorize page For when a user needs to sign into an app from multiple accounts on the same server * When logging out from modal header, redirect back after re-login --- app/controllers/api/base_controller.rb | 4 --- app/controllers/application_controller.rb | 4 +++ app/controllers/auth/sessions_controller.rb | 8 ++++++ app/controllers/oauth/authorizations_controller.rb | 14 ++++++++++ app/views/layouts/modal.html.haml | 2 +- spec/controllers/auth/sessions_controller_spec.rb | 7 +++++ .../oauth/authorizations_controller_spec.rb | 32 +++++++++++++++++++--- 7 files changed, 62 insertions(+), 9 deletions(-) (limited to 'spec') diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 0b3735087..90f42251e 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -53,10 +53,6 @@ class Api::BaseController < ApplicationController [params[:limit].to_i.abs, default_limit * 2].min end - def truthy_param?(key) - ActiveModel::Type::Boolean.new.cast(params[key]) - end - def current_resource_owner @current_user ||= User.find(doorkeeper_token.resource_owner_id) if doorkeeper_token end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d266fa1bd..fb4283da3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -58,6 +58,10 @@ class ApplicationController < ActionController::Base protected + def truthy_param?(key) + ActiveModel::Type::Boolean.new.cast(params[key]) + end + def forbidden respond_with_error(403) end diff --git a/app/controllers/auth/sessions_controller.rb b/app/controllers/auth/sessions_controller.rb index b0d974ff8..bc980009e 100644 --- a/app/controllers/auth/sessions_controller.rb +++ b/app/controllers/auth/sessions_controller.rb @@ -28,8 +28,10 @@ class Auth::SessionsController < Devise::SessionsController end def destroy + tmp_stored_location = stored_location_for(:user) super flash.delete(:notice) + store_location_for(:user, tmp_stored_location) if continue_after? end protected @@ -124,8 +126,14 @@ class Auth::SessionsController < Devise::SessionsController end def clear_site_data + return if continue_after? + # Should be '"*"' but that doen't work in Chrome (neither does '"executionContexts"') # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Clear-Site-Data response.headers['Clear-Site-Data'] = '"cache", "cookies", "storage"' end + + def continue_after? + truthy_param?(:continue) + end end diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index e9cdf9fa8..cebbdc4d0 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -13,4 +13,18 @@ class Oauth::AuthorizationsController < Doorkeeper::AuthorizationsController def store_current_location store_location_for(:user, request.url) end + + def render_success + if skip_authorization? || (matching_token? && !truthy_param?('force_login')) + redirect_or_render authorize_response + elsif Doorkeeper.configuration.api_only + render json: pre_auth + else + render :new + end + end + + def truthy_param?(key) + ActiveModel::Type::Boolean.new.cast(params[key]) + end end diff --git a/app/views/layouts/modal.html.haml b/app/views/layouts/modal.html.haml index b73068459..2ef49e413 100644 --- a/app/views/layouts/modal.html.haml +++ b/app/views/layouts/modal.html.haml @@ -8,7 +8,7 @@ .name = t 'users.signed_in_as' %span.username @#{current_account.local_username_and_domain} - = link_to destroy_user_session_path, method: :delete, class: 'logout-link icon-button' do + = link_to destroy_user_session_path(continue: true), method: :delete, class: 'logout-link icon-button' do = fa_icon 'sign-out' .container-alt= yield diff --git a/spec/controllers/auth/sessions_controller_spec.rb b/spec/controllers/auth/sessions_controller_spec.rb index 97719a606..b4f912717 100644 --- a/spec/controllers/auth/sessions_controller_spec.rb +++ b/spec/controllers/auth/sessions_controller_spec.rb @@ -30,6 +30,13 @@ RSpec.describe Auth::SessionsController, type: :controller do expect(response).to redirect_to(new_user_session_path) end + + it 'does not delete redirect location with continue=true' do + sign_in(user, scope: :user) + controller.store_location_for(:user, '/authorize') + delete :destroy, params: { continue: 'true' } + expect(controller.stored_location_for(:user)).to eq '/authorize' + end end context 'with a suspended user' do diff --git a/spec/controllers/oauth/authorizations_controller_spec.rb b/spec/controllers/oauth/authorizations_controller_spec.rb index 91c2d03ef..a84260a54 100644 --- a/spec/controllers/oauth/authorizations_controller_spec.rb +++ b/spec/controllers/oauth/authorizations_controller_spec.rb @@ -5,23 +5,25 @@ require 'rails_helper' RSpec.describe Oauth::AuthorizationsController, type: :controller do render_views - let(:app) { Doorkeeper::Application.create!(name: 'test', redirect_uri: 'http://localhost/') } + let(:app) { Doorkeeper::Application.create!(name: 'test', redirect_uri: 'http://localhost/', scopes: 'read') } describe 'GET #new' do subject do - get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/' } + get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read' } end shared_examples 'stores location for user' do it 'stores location for user' do subject - expect(controller.stored_location_for(:user)).to eq "/oauth/authorize?client_id=#{app.uid}&redirect_uri=http%3A%2F%2Flocalhost%2F&response_type=code" + expect(controller.stored_location_for(:user)).to eq "/oauth/authorize?client_id=#{app.uid}&redirect_uri=http%3A%2F%2Flocalhost%2F&response_type=code&scope=read" end end context 'when signed in' do + let!(:user) { Fabricate(:user) } + before do - sign_in Fabricate(:user), scope: :user + sign_in user, scope: :user end it 'returns http success' do @@ -35,6 +37,28 @@ RSpec.describe Oauth::AuthorizationsController, type: :controller do end include_examples 'stores location for user' + + context 'when app is already authorized' do + before do + Doorkeeper::AccessToken.find_or_create_for( + app, + user.id, + app.scopes, + Doorkeeper.configuration.access_token_expires_in, + Doorkeeper.configuration.refresh_token_enabled? + ) + end + + it 'redirects to callback' do + subject + expect(response).to redirect_to(/\A#{app.redirect_uri}/) + end + + it 'does not redirect to callback with force_login=true' do + get :new, params: { client_id: app.uid, response_type: 'code', redirect_uri: 'http://localhost/', scope: 'read', force_login: 'true' } + expect(response.body).to match(/Authorize/) + end + end end context 'when not signed in' do -- cgit