FEATURE: Allow /u/by-external to work for all managed authenticators (#11168)

Previously, `/u/by-external/{id}` would only work for 'Discourse SSO' systems. This commit adds a new 'provider' parameter to the URL: `/u/by-external/{provider}/{id}`

This is compatible with all auth methods which have migrated to the 'ManagedAuthenticator' pattern. That includes all core providers, and also popular plugins such as discourse-oauth2-basic and discourse-openid-connect.

The new route is admin-only, since some authenticators use sensitive information like email addresses as the external id.
This commit is contained in:
David Taylor 2020-11-10 10:41:46 +00:00 committed by GitHub
parent ffc3da35a6
commit a7adf30357
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 46 additions and 1 deletions

View File

@ -502,7 +502,14 @@ class ApplicationController < ActionController::Base
result.find_by(find_opts) result.find_by(find_opts)
elsif params[:external_id] elsif params[:external_id]
external_id = params[:external_id].chomp('.json') external_id = params[:external_id].chomp('.json')
SingleSignOnRecord.find_by(external_id: external_id).try(:user) if provider_name = params[:external_provider]
raise Discourse::InvalidAccess unless guardian.is_admin? # external_id might be something sensitive
provider = Discourse.enabled_authenticators.find { |a| a.name == provider_name }
raise Discourse::NotFound if !provider&.is_managed? # Only managed authenticators use UserAssociatedAccount
UserAssociatedAccount.find_by(provider_name: provider_name, provider_uid: external_id)&.user
else
SingleSignOnRecord.find_by(external_id: external_id).try(:user)
end
end end
raise Discourse::NotFound if user.blank? raise Discourse::NotFound if user.blank?

View File

@ -492,6 +492,7 @@ Discourse::Application.routes.draw do
get "#{root_path}/:username/notifications/:filter" => "users#show", constraints: { username: RouteFormat.username } get "#{root_path}/:username/notifications/:filter" => "users#show", constraints: { username: RouteFormat.username }
delete "#{root_path}/:username" => "users#destroy", constraints: { username: RouteFormat.username } delete "#{root_path}/:username" => "users#destroy", constraints: { username: RouteFormat.username }
get "#{root_path}/by-external/:external_id" => "users#show", constraints: { external_id: /[^\/]+/ } get "#{root_path}/by-external/:external_id" => "users#show", constraints: { external_id: /[^\/]+/ }
get "#{root_path}/by-external/:external_provider/:external_id" => "users#show", constraints: { external_id: /[^\/]+/ }
get "#{root_path}/:username/flagged-posts" => "users#show", constraints: { username: RouteFormat.username } get "#{root_path}/:username/flagged-posts" => "users#show", constraints: { username: RouteFormat.username }
get "#{root_path}/:username/deleted-posts" => "users#show", constraints: { username: RouteFormat.username } get "#{root_path}/:username/deleted-posts" => "users#show", constraints: { username: RouteFormat.username }
get "#{root_path}/:username/topic-tracking-state" => "users#topic_tracking_state", constraints: { username: RouteFormat.username } get "#{root_path}/:username/topic-tracking-state" => "users#topic_tracking_state", constraints: { username: RouteFormat.username }

View File

@ -1,6 +1,12 @@
# frozen_string_literal: true # frozen_string_literal: true
class Auth::ManagedAuthenticator < Auth::Authenticator class Auth::ManagedAuthenticator < Auth::Authenticator
def is_managed?
# Tells core that it can safely assume this authenticator
# uses UserAssociatedAccount
true
end
def description_for_user(user) def description_for_user(user)
associated_account = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id) associated_account = UserAssociatedAccount.find_by(provider_name: name, user_id: user.id)
return "" if associated_account.nil? return "" if associated_account.nil?

View File

@ -3246,6 +3246,37 @@ describe UsersController do
get "/u/by-external/99.json" get "/u/by-external/99.json"
expect(response).not_to be_successful expect(response).not_to be_successful
end end
context "for an external provider" do
before do
sign_in(Fabricate(:admin))
SiteSetting.enable_google_oauth2_logins = true
UserAssociatedAccount.create!(user: user, provider_uid: 'myuid', provider_name: 'google_oauth2')
end
it "doesn't work for non-admin" do
sign_in(user)
get "/u/by-external/google_oauth2/myuid.json"
expect(response.status).to eq(403)
end
it "can fetch the user" do
get "/u/by-external/google_oauth2/myuid.json"
expect(response.status).to eq(200)
expect(response.parsed_body["user"]["username"]).to eq(user.username)
end
it "fails for disabled provider" do
SiteSetting.enable_google_oauth2_logins = false
get "/u/by-external/google_oauth2/myuid.json"
expect(response.status).to eq(404)
end
it "returns 404 for missing user" do
get "/u/by-external/google_oauth2/myotheruid.json"
expect(response.status).to eq(404)
end
end
end end
describe "include_post_count_for" do describe "include_post_count_for" do