From ad3ce0d8aa121fd7c4e25fc5b40369c279c98d2e Mon Sep 17 00:00:00 2001 From: Leo McArdle Date: Mon, 25 Mar 2019 12:49:58 +0000 Subject: [PATCH] FEATURE: consume Person API v2 Adds an API class for Person API v2. Returns a profile with public and verified attributes. Returns a full_name based off first_name, last_name and alternative_name. Also adds better logging/error messages to failed OAuth requests. https://github.com/mozilla/discourse/issues/182 --- config/settings.yml | 6 + lib/mozilla_iam.rb | 1 + lib/mozilla_iam/api/oauth.rb | 15 +- lib/mozilla_iam/api/person_v2.rb | 63 +++++++ plugin.rb | 2 +- spec/components/mozilla_iam/api/oauth_spec.rb | 13 +- .../mozilla_iam/api/person_v2_spec.rb | 168 ++++++++++++++++++ 7 files changed, 263 insertions(+), 5 deletions(-) create mode 100644 lib/mozilla_iam/api/person_v2.rb create mode 100644 spec/components/mozilla_iam/api/person_v2_spec.rb diff --git a/config/settings.yml b/config/settings.yml index 320eb85..325f517 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -15,3 +15,9 @@ mozilla_iam: mozilla_iam_person_api_aud: default: 'https://person-api.sso.mozilla.com' shadowed_by_global: true + mozilla_iam_person_v2_api_url: + default: "https://person.api.test.sso.allizom.org" + shadowed_by_global: true + mozilla_iam_person_v2_api_aud: + default: "api.test.sso.allizom.org" + shadowed_by_global: true diff --git a/lib/mozilla_iam.rb b/lib/mozilla_iam.rb index dc10f0b..bfc06b1 100644 --- a/lib/mozilla_iam.rb +++ b/lib/mozilla_iam.rb @@ -5,6 +5,7 @@ require_relative 'mozilla_iam/serializer_extensions/mozilla_iam' require_relative 'mozilla_iam/api' require_relative 'mozilla_iam/api/oauth' require_relative 'mozilla_iam/api/person' +require_relative 'mozilla_iam/api/person_v2' require_relative 'mozilla_iam/api/management' require_relative 'mozilla_iam/application_extensions' require_relative 'mozilla_iam/authenticator' diff --git a/lib/mozilla_iam/api/oauth.rb b/lib/mozilla_iam/api/oauth.rb index 985fb4b..5c944fc 100644 --- a/lib/mozilla_iam/api/oauth.rb +++ b/lib/mozilla_iam/api/oauth.rb @@ -32,6 +32,11 @@ module MozillaIAM if res.code == '200' MultiJson.load(res.body, symbolize_keys: true) else + Rails.logger.error <<~EOF + Mozilla IAM: Error fetching /#{path} + client_id: #{@client_id}, url: #{@url} + response: #{res.body} + EOF {} end end @@ -63,7 +68,15 @@ module MozillaIAM audience: @aud } ) - MultiJson.load(response.body)['access_token'] + if response.status == 200 + MultiJson.load(response.body)['access_token'] + else + raise <<~EOF + Mozilla IAM: Error fetching OAuth token: + client_id: #{@client_id}, token_endpoint: #{@token_endpoint}, audience: #{@aud} + response: #{response.body} + EOF + end end def verify_token(token) diff --git a/lib/mozilla_iam/api/person_v2.rb b/lib/mozilla_iam/api/person_v2.rb new file mode 100644 index 0000000..ab96c67 --- /dev/null +++ b/lib/mozilla_iam/api/person_v2.rb @@ -0,0 +1,63 @@ +module MozillaIAM + module API + class PersonV2 < OAuth + + def initialize(config={}) + config = { + url: SiteSetting.mozilla_iam_person_v2_api_url + "/v2", + aud: SiteSetting.mozilla_iam_person_v2_api_aud + }.merge(config) + super(config) + end + + def profile(uid) + res = get("user/user_id/#{uid}") + Profile.new(res) + end + + class Profile + attr_reader :username + attr_reader :pronouns + attr_reader :full_name + attr_reader :fun_title + attr_reader :description + attr_reader :location + + def initialize(raw) + @raw = raw + @username = process :primary_username + @pronouns = process :pronouns + @full_name = process_full_name + @fun_title = process :fun_title + @description = process :description + @location = process :location + end + + private + + def process(name) + if field = @raw[name] + metadata = field[:metadata] + if metadata[:display] == "public" && metadata[:verified] == true + return field[:value] + end + end + nil + end + + def process_full_name + first = process :first_name + last = process :last_name + return "#{first} #{last}" unless first.blank? || last.blank? + + alternative = process :alternative_name + return "#{alternative}" unless alternative.blank? + + return "#{first}" unless first.blank? + return "#{last}" unless last.blank? + end + + end + end + end +end diff --git a/plugin.rb b/plugin.rb index 98bb865..077465f 100644 --- a/plugin.rb +++ b/plugin.rb @@ -1,6 +1,6 @@ # name: mozilla-iam # about: A plugin to integrate Discourse with Mozilla's Identity and Access Management (IAM) system -# version: 1.2.0-alpha.1 +# version: 1.2.0-alpha.2 # authors: Leo McArdle # url: https://github.com/mozilla/discourse-mozilla-iam diff --git a/spec/components/mozilla_iam/api/oauth_spec.rb b/spec/components/mozilla_iam/api/oauth_spec.rb index 50054d1..db4c428 100644 --- a/spec/components/mozilla_iam/api/oauth_spec.rb +++ b/spec/components/mozilla_iam/api/oauth_spec.rb @@ -85,9 +85,10 @@ describe MozillaIAM::API::OAuth do expect(api.send(:get, "", foo: 'bar')[:success]).to eq "true" end - it "should return an empty hash if the the status code isn't 200" do - stub_request(:get, "https://example.com/api/").to_return(status: 403) - expect(api.send(:get, "")).to eq({}) + it "logs error and returns empty hash if the the status code isn't 200" do + stub_request(:get, "https://example.com/api/user/uid").to_return(status: 403) + Rails.logger.expects(:error).with { |e| e.start_with? "Mozilla IAM: Error fetching /user/uid"} + expect(api.send(:get, "user/uid")).to eq({}) end end @@ -136,6 +137,12 @@ describe MozillaIAM::API::OAuth do token = api.send(:fetch_token) expect(token).to eq "fetched_token" end + + it "throws error if fetch isn't successful" do + stub_request(:post, "https://example.com/oauth/token") + .to_return(status: 403, body: '{"error":"Error"}') + expect { api.send(:fetch_token) }.to raise_error(/Mozilla IAM: Error fetching OAuth token:/) + end end context "#verify_token" do diff --git a/spec/components/mozilla_iam/api/person_v2_spec.rb b/spec/components/mozilla_iam/api/person_v2_spec.rb new file mode 100644 index 0000000..d99d56a --- /dev/null +++ b/spec/components/mozilla_iam/api/person_v2_spec.rb @@ -0,0 +1,168 @@ +require_relative "../../../iam_helper" + +describe MozillaIAM::API::PersonV2 do + let(:api) { described_class.new } + before do + SiteSetting.mozilla_iam_person_v2_api_url = "https://personv2.com" + SiteSetting.mozilla_iam_person_v2_api_aud = "personv2.com" + end + + context "#initialize" do + it "sets url and aud based on SiteSetting" do + expect(api.instance_variable_get(:@url)).to eq "https://personv2.com/v2" + expect(api.instance_variable_get(:@aud)).to eq "personv2.com" + end + end + + context "#profile" do + it "returns the profile for a specific user" do + api.expects(:get).with("user/user_id/uid").returns(profile: "profile") + expect(api.profile("uid").instance_variable_get(:@raw)).to eq({profile: "profile"}) + end + + it "returns an empty hash if a profile doesn't exist" do + api.expects(:get).with("user/user_id/uid").returns({}) + expect(api.profile("uid").instance_variable_get(:@raw)).to eq({}) + end + end + + describe described_class::Profile do + def single_attribute(value=nil, metadata={}) + metadata[:verified] = true if metadata[:verified].nil? + metadata[:public] = true if metadata[:public].nil? + { + metadata: { + verified: metadata[:verified], + display: metadata[:public] ? "public" : "staff" + }, + value: value + } + end + + def profile_with(attributes, value=nil, metadata={}) + raw = {} + if attributes.is_a? Hash + attributes.each do |name, value| + raw[name] = single_attribute(value) + end + else + raw[attributes] = single_attribute(value, metadata) + end + described_class.new(raw) + end + + shared_examples "one-to-one mapping" do |method, attribute, value| + describe "##{method}" do + context "with no #{attribute} attribute" do + let(:profile) { described_class.new({}) } + + it "returns nil" do + expect(profile.public_send(method)).to be_nil + end + end + + context "with unverified #{attribute} attribute" do + let(:profile) { profile_with(attribute, value, verified: false) } + + it "returns nil" do + expect(profile.public_send(method)).to be_nil + end + end + + context "with non-public #{attribute} attribute" do + let(:profile) { profile_with(attribute, value, public: false) } + + it "returns nil" do + expect(profile.public_send(method)).to be_nil + end + end + + let(:profile) { profile_with(attribute, value) } + + it "returns #{attribute}" do + expect(profile.public_send(method)).to eq value + end + end + end + + include_examples "one-to-one mapping", :username, :primary_username, "janedoe" + include_examples "one-to-one mapping", :pronouns, :pronouns, "she/her" + include_examples "one-to-one mapping", :fun_title, :fun_title, "Fun job title" + include_examples "one-to-one mapping", :description, :description, "I have a fun job" + include_examples "one-to-one mapping", :location, :location, "Somewhere" + + describe "#full_name" do + let(:profile) do + profile_with ({ + first_name: "Jane", + last_name: "Doe", + alternative_name: "Janette Smith" + }) + end + + it "returns first_name + last_name" do + expect(profile.full_name).to eq "Jane Doe" + end + + context "without first_name" do + let(:profile) do + profile_with ({ + last_name: "Doe", + alternative_name: "Janette Smith" + }) + end + + it "returns alternative_name" do + expect(profile.full_name).to eq "Janette Smith" + end + + context "without alternative_name" do + let(:profile) do + profile_with ({ + last_name: "Doe" + }) + end + + it "returns last_name" do + expect(profile.full_name).to eq "Doe" + end + end + end + + context "without last_name" do + let(:profile) do + profile_with ({ + first_name: "Jane", + alternative_name: "Janette Smith" + }) + end + + it "returns alternative_name" do + expect(profile.full_name).to eq "Janette Smith" + end + + context "without alternative_name" do + let(:profile) do + profile_with ({ + first_name: "Jane" + }) + end + + it "returns first_name" do + expect(profile.full_name).to eq "Jane" + end + end + end + + context "without first_name, last_name or alternative_name" do + let(:profile) do + profile_with({}) + end + + it "returns nil" do + expect(profile.full_name).to be_nil + end + end + end + end +end