diff --git a/app/assets/javascripts/discourse/initializers/strip-mobile-app-url-params.js.es6 b/app/assets/javascripts/discourse/initializers/strip-mobile-app-url-params.js.es6 new file mode 100644 index 00000000000..04f78881724 --- /dev/null +++ b/app/assets/javascripts/discourse/initializers/strip-mobile-app-url-params.js.es6 @@ -0,0 +1,30 @@ +export default { + name: "strip-mobile-app-url-params", + + initialize() { + let queryStrings = window.location.search; + + if (queryStrings.indexOf("user_api_public_key") !== -1) { + let params = queryStrings.startsWith("?") + ? queryStrings.substr(1).split("&") + : []; + + params = params.filter(param => { + return ( + !param.startsWith("user_api_public_key=") && + !param.startsWith("auth_redirect=") + ); + }); + + queryStrings = params.length > 0 ? `?${params.join("&")}` : ""; + + if (window.history && window.history.replaceState) { + window.history.replaceState( + null, + null, + `${location.pathname}${queryStrings}${location.hash}` + ); + } + } + } +}; diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6fd9b79daf3..3f257137647 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -731,6 +731,26 @@ class ApplicationController < ActionController::Base redirect_to path(redirect_path) end end + + # Used by clients authenticated via user API. + # Redirects to provided URL scheme if + # - request uses a valid public key and auth_redirect scheme + # - one_time_password scope is allowed + if !current_user && + params.has_key?(:user_api_public_key) && + params.has_key?(:auth_redirect) + begin + OpenSSL::PKey::RSA.new(params[:user_api_public_key]) + rescue OpenSSL::PKey::RSAError + return render plain: I18n.t("user_api_key.invalid_public_key") + end + + if UserApiKey.invalid_auth_redirect?(params[:auth_redirect]) + return render plain: I18n.t("user_api_key.invalid_auth_redirect") + end + redirect_to("#{params[:auth_redirect]}?otp=true") if UserApiKey.allowed_scopes.superset?(Set.new(["one_time_password"])) + end + end def block_if_readonly_mode diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index fa46aad1af6..52732b34c6f 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -12,7 +12,7 @@ class SessionController < ApplicationController before_action :check_local_login_allowed, only: %i(create forgot_password email_login) before_action :rate_limit_login, only: %i(create email_login) skip_before_action :redirect_to_login_if_required - skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy email_login) + skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy email_login one_time_password) ACTIVATE_USER_KEY = "activate_user" @@ -321,6 +321,20 @@ class SessionController < ApplicationController render layout: 'no_ember' end + def one_time_password + otp_username = $redis.get "otp_#{params[:token]}" + + if otp_username && user = User.find_by_username(otp_username) + log_on_user(user) + $redis.del "otp_#{params[:token]}" + return redirect_to path("/") + else + @error = I18n.t('user_api_key.invalid_token') + end + + render layout: 'no_ember' + end + def forgot_password params.require(:login) diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb index 30ea1290165..ab80568668f 100644 --- a/app/controllers/user_api_keys_controller.rb +++ b/app/controllers/user_api_keys_controller.rb @@ -2,11 +2,11 @@ class UserApiKeysController < ApplicationController layout 'no_ember' - requires_login only: [:create, :revoke, :undo_revoke] - skip_before_action :redirect_to_login_if_required, only: [:new] + requires_login only: [:create, :create_otp, :revoke, :undo_revoke] + skip_before_action :redirect_to_login_if_required, only: [:new, :otp] skip_before_action :check_xhr, :preload_json - AUTH_API_VERSION ||= 3 + AUTH_API_VERSION ||= 4 def new @@ -51,17 +51,15 @@ class UserApiKeysController < ApplicationController require_params - if params.key?(:auth_redirect) && SiteSetting.allowed_user_api_auth_redirects - .split('|') - .none? { |u| WildcardUrlChecker.check_url(u, params[:auth_redirect]) } - - raise Discourse::InvalidAccess + if params.key?(:auth_redirect) + raise Discourse::InvalidAccess if UserApiKey.invalid_auth_redirect?(params[:auth_redirect]) end raise Discourse::InvalidAccess unless meets_tl? validate_params @application_name = params[:application_name] + scopes = params[:scopes].split(",") # destroy any old keys we had UserApiKey.where(user_id: current_user.id, client_id: params[:client_id]).destroy_all @@ -72,7 +70,7 @@ class UserApiKeysController < ApplicationController user_id: current_user.id, push_url: params[:push_url], key: SecureRandom.hex, - scopes: params[:scopes].split(",") + scopes: scopes ) # we keep the payload short so it encrypts easily with public key @@ -87,8 +85,15 @@ class UserApiKeysController < ApplicationController public_key = OpenSSL::PKey::RSA.new(params[:public_key]) @payload = Base64.encode64(public_key.public_encrypt(@payload)) + if scopes.include?("one_time_password") + # encrypt one_time_password separately to bypass 128 chars encryption limit + otp_payload = one_time_password(public_key, current_user.username) + end + if params[:auth_redirect] - redirect_to("#{params[:auth_redirect]}?payload=#{CGI.escape(@payload)}") + redirect_path = "#{params[:auth_redirect]}?payload=#{CGI.escape(@payload)}" + redirect_path << "&oneTimePassword=#{CGI.escape(otp_payload)}" if scopes.include?("one_time_password") + redirect_to(redirect_path) else respond_to do |format| format.html { render :show } @@ -100,6 +105,38 @@ class UserApiKeysController < ApplicationController end end + def otp + require_params_otp + + unless current_user + cookies[:destination_url] = request.fullpath + + if SiteSetting.enable_sso? + redirect_to path('/session/sso') + else + redirect_to path('/login') + end + return + end + + @application_name = params[:application_name] + @public_key = params[:public_key] + @auth_redirect = params[:auth_redirect] + end + + def create_otp + require_params_otp + + raise Discourse::InvalidAccess if UserApiKey.invalid_auth_redirect?(params[:auth_redirect]) + raise Discourse::InvalidAccess unless meets_tl? + + public_key = OpenSSL::PKey::RSA.new(params[:public_key]) + otp_payload = one_time_password(public_key, current_user.username) + + redirect_path = "#{params[:auth_redirect]}?oneTimePassword=#{CGI.escape(otp_payload)}" + redirect_to(redirect_path) + end + def revoke revoke_key = find_key if params[:id] @@ -141,15 +178,30 @@ class UserApiKeysController < ApplicationController def validate_params requested_scopes = Set.new(params[:scopes].split(",")) - raise Discourse::InvalidAccess unless UserApiKey.allowed_scopes.superset?(requested_scopes) # our pk has got to parse OpenSSL::PKey::RSA.new(params[:public_key]) end + def require_params_otp + [ + :public_key, + :auth_redirect, + :application_name + ].each { |p| params.require(p) } + end + def meets_tl? current_user.staff? || current_user.trust_level >= SiteSetting.min_trust_level_for_user_api_key end + def one_time_password(public_key, username) + raise Discourse::InvalidAccess unless UserApiKey.allowed_scopes.superset?(Set.new(["one_time_password"])) + + otp = SecureRandom.hex + $redis.setex "otp_#{otp}", 10.minutes, username + + Base64.encode64(public_key.public_encrypt(otp)) + end end diff --git a/app/models/user_api_key.rb b/app/models/user_api_key.rb index 7a3747fec98..0895d2c0b7d 100644 --- a/app/models/user_api_key.rb +++ b/app/models/user_api_key.rb @@ -5,6 +5,7 @@ class UserApiKey < ActiveRecord::Base write: [:get, :post, :patch, :put, :delete], message_bus: [[:post, 'message_bus']], push: nil, + one_time_password: nil, notifications: [[:post, 'message_bus'], [:get, 'notifications#index'], [:put, 'notifications#mark_read']], session_info: [ [:get, 'session#current'], @@ -63,6 +64,11 @@ class UserApiKey < ActiveRecord::Base end end + def self.invalid_auth_redirect?(auth_redirect) + return SiteSetting.allowed_user_api_auth_redirects + .split('|') + .none? { |u| WildcardUrlChecker.check_url(u, auth_redirect) } + end end # == Schema Information diff --git a/app/views/session/one_time_password.html.erb b/app/views/session/one_time_password.html.erb new file mode 100644 index 00000000000..a0faef79d98 --- /dev/null +++ b/app/views/session/one_time_password.html.erb @@ -0,0 +1,5 @@ +<%if @error%> +
+ <%= @error %> +
+<%end%> diff --git a/app/views/user_api_keys/otp.html.erb b/app/views/user_api_keys/otp.html.erb new file mode 100644 index 00000000000..9c96d638d2d --- /dev/null +++ b/app/views/user_api_keys/otp.html.erb @@ -0,0 +1,9 @@ +

<%= t("user_api_key.otp_description", application_name: @application_name) %>

+
+<%= form_tag(user_api_key_otp_path) do %> + <%= hidden_field_tag 'application_name', @application_name %> + <%= hidden_field_tag 'public_key', @public_key%> + <%= hidden_field_tag('auth_redirect', @auth_redirect) %> + <%= submit_tag t('user_api_key.authorize'), class: 'btn btn-danger' %> +<% end %> +
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 34d793b808d..d3d6ba842d0 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -912,6 +912,7 @@ en: read_write: "read/write" description: '"%{application_name}" is requesting the following access to your account:' instructions: 'We just generated a new user API key for you to use with "%{application_name}", please paste the following key into your application:' + otp_description: 'Would you like to allow "%{application_name}" to access this site?' no_trust_level: "Sorry, you do not have the required trust level to access the user API" generic_error: "Sorry, we are unable to issue user API keys, this feature may be disabled by the site admin" scopes: @@ -921,7 +922,10 @@ en: session_info: "Read user session info" read: "Read all" write: "Write all" - + one_time_password: "Create a one-time login token" + invalid_public_key: "Sorry, the public key is invalid." + invalid_auth_redirect: "Sorry, this auth_redirect host is not allowed." + invalid_token: "Missing, invalid or expired token." flags: errors: already_handled: "Flag was already handled" diff --git a/config/routes.rb b/config/routes.rb index 0787d9eeb85..08233e7b0c6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -331,6 +331,7 @@ Discourse::Application.routes.draw do get "session/csrf" => "session#csrf" get "session/email-login/:token" => "session#email_login" post "session/email-login/:token" => "session#email_login" + get "session/otp/:token" => "session#one_time_password", constraints: { token: /[0-9a-f]+/ } get "composer_messages" => "composer_messages#index" post "composer/parse_html" => "composer#parse_html" @@ -841,6 +842,8 @@ Discourse::Application.routes.draw do post "/user-api-key" => "user_api_keys#create" post "/user-api-key/revoke" => "user_api_keys#revoke" post "/user-api-key/undo-revoke" => "user_api_keys#undo_revoke" + get "/user-api-key/otp" => "user_api_keys#otp" + post "/user-api-key/otp" => "user_api_keys#create_otp" get "/safe-mode" => "safe_mode#index" post "/safe-mode" => "safe_mode#enter", as: "safe_mode_enter" diff --git a/config/site_settings.yml b/config/site_settings.yml index 0132538bbd2..60d4f2eff1c 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1109,7 +1109,7 @@ files: regex: '^((https?:)?\/)?\/.+[^\/]' shadowed_by_global: true restrict_letter_avatar_colors: - default: '' + default: "" type: list list_type: compact validator: "ColorListValidator" @@ -1231,11 +1231,11 @@ security: enforce_second_factor: client: true type: enum - default: 'no' + default: "no" choices: - - 'no' - - 'staff' - - 'all' + - "no" + - "staff" + - "all" force_https: default: false shadowed_by_global: true @@ -1919,7 +1919,7 @@ user_api: allow_user_api_keys: default: true allow_user_api_key_scopes: - default: "read|write|message_bus|push|notifications|session_info" + default: "read|write|message_bus|push|notifications|session_info|one_time_password" type: list max_api_keys_per_user: default: 10 diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb index 06a2719519c..83327e81cae 100644 --- a/spec/requests/application_controller_spec.rb +++ b/spec/requests/application_controller_spec.rb @@ -345,6 +345,61 @@ RSpec.describe ApplicationController do end end + describe 'Delegated auth' do + let :public_key do + <<~TXT + -----BEGIN PUBLIC KEY----- + MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDh7BS7Ey8hfbNhlNAW/47pqT7w + IhBz3UyBYzin8JurEQ2pY9jWWlY8CH147KyIZf1fpcsi7ZNxGHeDhVsbtUKZxnFV + p16Op3CHLJnnJKKBMNdXMy0yDfCAHZtqxeBOTcCo1Vt/bHpIgiK5kmaekyXIaD0n + w0z/BYpOgZ8QwnI5ZwIDAQAB + -----END PUBLIC KEY----- + TXT + end + + let :args do + { + auth_redirect: 'http://no-good.com', + user_api_public_key: "not-a-valid-public-key" + } + end + + it 'disallows invalid public_key param' do + args[:auth_redirect] = "discourse://auth_redirect" + get "/latest", params: args + + expect(response.body).to eq(I18n.t("user_api_key.invalid_public_key")) + end + + it 'does not allow invalid auth_redirect' do + args[:user_api_public_key] = public_key + get "/latest", params: args + + expect(response.body).to eq(I18n.t("user_api_key.invalid_auth_redirect")) + end + + it 'does not redirect if one_time_password scope is disallowed' do + SiteSetting.allow_user_api_key_scopes = "read|write" + args[:user_api_public_key] = public_key + args[:auth_redirect] = "discourse://auth_redirect" + + get "/latest", params: args + + expect(response.status).to_not eq(302) + expect(response).to_not redirect_to("#{args[:auth_redirect]}?otp=true") + end + + it 'redirects correctly with valid params' do + args[:user_api_public_key] = public_key + args[:auth_redirect] = "discourse://auth_redirect" + + get "/categories", params: args + + expect(response.status).to eq(302) + expect(response).to redirect_to("#{args[:auth_redirect]}?otp=true") + end + end + describe 'Content Security Policy' do it 'is enabled by SiteSettings' do SiteSetting.content_security_policy = false diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 443c1e801bf..2795ffdb09f 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1288,6 +1288,45 @@ RSpec.describe SessionController do end end + describe '#one_time_password' do + context 'missing token' do + it 'returns the right response' do + get "/session/otp" + expect(response.status).to eq(404) + end + end + + context 'invalid token' do + it 'returns the right response' do + get "/session/otp/asd1231dasd123" + + expect(response.status).to eq(404) + end + + context 'when token is valid' do + it 'should authenticate user and delete token' do + user = Fabricate(:user) + + get "/session/current.json" + expect(response.status).to eq(404) + + token = SecureRandom.hex + $redis.setex "otp_#{token}", 10.minutes, user.username + + get "/session/otp/#{token}" + + expect(response.status).to eq(302) + expect(response).to redirect_to("/") + expect($redis.get("otp_#{token}")).to eq(nil) + + get "/session/current.json" + expect(response.status).to eq(200) + end + end + end + + end + describe '#forgot_password' do it 'raises an error without a username parameter' do post "/session/forgot_password.json" diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb index e2cce3d8a36..c66e5b6faee 100644 --- a/spec/requests/user_api_keys_controller_spec.rb +++ b/spec/requests/user_api_keys_controller_spec.rb @@ -48,7 +48,7 @@ describe UserApiKeysController do it "supports a head request cleanly" do head "/user-api-key/new" expect(response.status).to eq(200) - expect(response.headers["Auth-Api-Version"]).to eq("3") + expect(response.headers["Auth-Api-Version"]).to eq("4") end end @@ -156,7 +156,7 @@ describe UserApiKeysController do expect(parsed["nonce"]).to eq(args[:nonce]) expect(parsed["push"]).to eq(false) - expect(parsed["api"]).to eq(3) + expect(parsed["api"]).to eq(4) key = user.user_api_keys.first expect(key.scopes).to include("push") @@ -168,7 +168,7 @@ describe UserApiKeysController do SiteSetting.allowed_user_api_auth_redirects = args[:auth_redirect] SiteSetting.allowed_user_api_push_urls = "https://push.it/here" - args[:scopes] = "push,notifications,message_bus,session_info" + args[:scopes] = "push,notifications,message_bus,session_info,one_time_password" args[:push_url] = "https://push.it/here" user = Fabricate(:user, trust_level: 0) @@ -193,7 +193,7 @@ describe UserApiKeysController do api_key = UserApiKey.find_by(key: parsed["key"]) expect(api_key.user_id).to eq(user.id) - expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info"].sort) + expect(api_key.scopes.sort).to eq(["push", "message_bus", "notifications", "session_info", "one_time_password"].sort) expect(api_key.push_url).to eq("https://push.it/here") uri.query = "" @@ -204,6 +204,14 @@ describe UserApiKeysController do post "/user-api-key.json", params: args expect(response.status).to eq(302) + + one_time_password = query.split("oneTimePassword=")[1] + encrypted_otp = Base64.decode64(CGI.unescape(one_time_password)) + + parsed_otp = key.private_decrypt(encrypted_otp) + redis_key = "otp_#{parsed_otp}" + + expect($redis.get(redis_key)).to eq(user.username) end it "will just show the payload if no redirect" do @@ -251,4 +259,78 @@ describe UserApiKeysController do expect(response.status).to eq(302) end end + + context '#create-one-time-password' do + let :otp_args do + { + auth_redirect: 'http://somewhere.over.the/rainbow', + application_name: 'foo', + public_key: public_key + } + end + + it "does not allow anon" do + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(403) + end + + it "refuses to redirect to disallowed place" do + sign_in(Fabricate(:user)) + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(403) + end + + it "will allow one-time-password for staff without TL" do + SiteSetting.min_trust_level_for_user_api_key = 2 + SiteSetting.allowed_user_api_auth_redirects = otp_args[:auth_redirect] + + user = Fabricate(:user, trust_level: 1, moderator: true) + + sign_in(user) + + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(302) + end + + it "will not allow one-time-password unless TL is met" do + SiteSetting.min_trust_level_for_user_api_key = 2 + SiteSetting.allowed_user_api_auth_redirects = otp_args[:auth_redirect] + + user = Fabricate(:user, trust_level: 1) + sign_in(user) + + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(403) + end + + it "will not allow one-time-password if one_time_password scope is disallowed" do + SiteSetting.allow_user_api_key_scopes = "read|write" + SiteSetting.allowed_user_api_auth_redirects = otp_args[:auth_redirect] + user = Fabricate(:user) + sign_in(user) + + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(403) + end + + it "will return one-time-password when args are valid" do + SiteSetting.allowed_user_api_auth_redirects = otp_args[:auth_redirect] + user = Fabricate(:user) + sign_in(user) + + post "/user-api-key/otp", params: otp_args + expect(response.status).to eq(302) + + uri = URI.parse(response.redirect_url) + + query = uri.query + payload = query.split("oneTimePassword=")[1] + encrypted = Base64.decode64(CGI.unescape(payload)) + key = OpenSSL::PKey::RSA.new(private_key) + + parsed = key.private_decrypt(encrypted) + + expect($redis.get("otp_#{parsed}")).to eq(user.username) + end + end end