Merge pull request #11 from RaspberryPiFoundation/hydrav1-test

Update Hydra v1 strategy and add tests

.tool-versions

@@ -1 +1 @@
-ruby 2.7.0
+ruby 2.7.2

lib/omniauth-rpi/version.rb

@@ -1,5 +1,5 @@
 module OmniAuth
   module Rpi
-    VERSION = '0.7.0'.freeze
+    VERSION = '1.0.0'.freeze
   end
 end

lib/omniauth/strategies/hydra1.rb

@@ -1,49 +1,73 @@
+# frozen_string_literal: true
+
 require 'omniauth-oauth2'
 require 'jwt'
 
-module OmniAuth::Strategies
-  class Hydra1 < OmniAuth::Strategies::OAuth2
-    option :client_options,
-           :site          => 'https://auth.raspberrypi.org',
-           :authorize_url => 'https://auth.raspberrypi.org/oauth2/auth',
-           :token_url     => 'https://auth.raspberrypi.org/oauth2/token'
-
-    def authorize_params
-      super.tap do |params|
-        %w[scope client_options].each do |v|
-          params[v.to_sym] = request.params[v] if request.params[v]
+module OmniAuth
+  module Strategies
+    class Hydra1 < OmniAuth::Strategies::OAuth2 
+      option :client_options, {
+        site:         'https://auth-v1.raspberrypi.org',
+        authorize_url:'https://auth-v1.raspberrypi.org/oauth2/auth',
+        token_url:    'https://auth-v1.raspberrypi.org/oauth2/token'
+      }
+
+      def authorize_params
+        super.tap do |params|
+          %w[scope client_options].each do |v|
+            params[v.to_sym] = request.params[v] if request.params[v]
+          end
         end
       end
-    end
 
-    def callback_url
-      full_host + callback_path
-    end
+      def callback_url
+        full_host + callback_path
+      end
 
-    uid { raw_info['uuid'].to_s }
+      uid { raw_info['user'].to_s }
 
-    info do
-      {
-        'email'    => email,
-        'nickname' => nickname,
-        'name'     => fullname,
-      }
-    end
+      info do
+        {
+          'email'    => email,
+          'username' => username,
+          'name'     => fullname,
+          'nickname' => nickname,
+          'image'    => image,
+        }
+      end
 
-    def raw_info
-      @raw_info ||= (JWT.decode access_token.params['id_token'], nil, false)[0]
-    end
+      extra do
+        {
+          'raw_info' => raw_info
+        }
+      end
 
-    def email
-      raw_info['email']
-    end
+      def raw_info
+        @raw_info ||= (JWT.decode access_token.params['id_token'], nil, false)[0]
+      end
 
-    def nickname
-      raw_info['nickname']
-    end
+      def email
+        raw_info['email']
+      end
+
+      # <13 accounts have username instead of email
+      def username
+        raw_info['username']
+      end
 
-    def fullname
-      raw_info['name']
+      def nickname
+        raw_info['nickname']
+      end
+
+      # use fullname to avoid clash with 'name'
+      def fullname
+        raw_info['name']
+      end
+
+      def image
+        # deserialise openid claim into auth schema
+        raw_info['picture']
+      end
     end
   end
 end

lib/omniauth/strategies/rpi.rb

@@ -1,8 +1,11 @@
+# frozen_string_literal: true
+
 require 'omniauth-oauth2'
 require 'jwt'
 
 module OmniAuth::Strategies
-  class Rpi < OmniAuth::Strategies::Hydra0
+  class Rpi < OmniAuth::Strategies::Hydra1
+    option name: "raspberrypi_accounts"
   end
 end
 

omniauth-rpi.gemspec

@@ -30,7 +30,7 @@ Gem::Specification.new do |spec|
   spec.executables   = spec.files.grep(%r{^bin/}) { |f| File.basename(f) }
 
   spec.add_runtime_dependency 'jwt'
-  spec.add_runtime_dependency 'omniauth', '~> 1.4'
+  spec.add_runtime_dependency 'omniauth', '~> 2.0'
   spec.add_runtime_dependency 'omniauth-oauth2', '~> 1.4'
 
   spec.add_development_dependency 'bundler', '~> 2.0'

spec/omniauth/strategies/hydra0_spec.rb

@@ -4,7 +4,7 @@ def provider_name(klass)
   klass.to_s.split("::")[-1].downcase
 end
 
-[OmniAuth::Strategies::Hydra0, OmniAuth::Strategies::Rpi].each{|provider|
+[OmniAuth::Strategies::Hydra0].each{|provider|
   RSpec.describe provider do
     subject(:strategy) { described_class.new({}) }
 
@@ -130,6 +130,7 @@ def provider_name(klass)
     describe '#callback_url' do
       it 'is a combination of host and callback path' do
         allow(strategy).to receive(:full_host).and_return('https://example.com')
+        strategy.instance_variable_set(:@env, {})
 
         expect(strategy.callback_url).to eq("https://example.com/auth/#{provider_name provider}/callback")
       end

spec/omniauth/strategies/hydra1_spec.rb

@@ -1,98 +1,160 @@
 require 'spec_helper'
 
-RSpec.describe OmniAuth::Strategies::Hydra1 do
-  subject(:strategy) { described_class.new({}) }
-
-  let(:access_token) { instance_double('AccessToken', :options => {}) }
-  let(:parsed_response) { instance_double('ParsedResponse') }
-  let(:response) { instance_double('Response', :parsed => parsed_response) }
-
-  let(:development_site)          { 'http://localhost:9000/api/v3' }
-  let(:development_authorize_url) { 'http://localhost:9000/login/oauth/authorize' }
-  let(:development_token_url)     { 'http://localhost:9000/login/oauth/access_token' }
-
-  let(:development) do
-    described_class.new('RASPBERRY_KEY', 'RASPBERRY_SECRET',
-                        :client_options => {
-                          :site          => development_site,
-                          :authorize_url => development_authorize_url,
-                          :token_url     => development_token_url,
-                        })
-  end
+def provider_name(klass)
+  klass.to_s.split("::")[-1].downcase
+end
 
-  before(:each) do
-    allow(strategy).to receive(:access_token).and_return(access_token)
-  end
+def make_env(path = '/auth/rpi', props = {})
+  {
+    'REQUEST_METHOD' => 'POST',
+    'PATH_INFO' => path,
+    'rack.session' => {},
+    'rack.input' => StringIO.new('test=true')
+  }.merge(props)
+end
 
-  context 'client options' do
-    it 'has the correct site url' do
-      expect(strategy.options.client_options.site).to eq('https://auth.raspberrypi.org')
+[OmniAuth::Strategies::Hydra1, OmniAuth::Strategies::Rpi].each{|provider|
+  RSpec.describe provider do
+    before do
+      OmniAuth.config.test_mode = true
+    end
+
+    after do
+      OmniAuth.config.test_mode = false
     end
 
-    it 'has the correct authorize url' do
-      expect(strategy.options.client_options.authorize_url).to eq('https://auth.raspberrypi.org/oauth2/auth')
+    let(:app) do
+      lambda { |_env| [404, {}, ['Awesome']] }
+    end
+
+    subject(:strategy) { described_class.new(app) }
+
+    let(:access_token) { instance_double('AccessToken', :options => {}) }
+    let(:parsed_response) { instance_double('ParsedResponse') }
+    let(:response) { instance_double('Response', :parsed => parsed_response) }
+
+    let(:development_site)          { 'http://localhost:9000/api/v3' }
+    let(:development_authorize_url) { 'http://localhost:9000/login/oauth/authorize' }
+    let(:development_token_url)     { 'http://localhost:9000/login/oauth/access_token' }
+
+    let(:development) do
+      described_class.new('RASPBERRY_KEY', 'RASPBERRY_SECRET',
+        client_options: {
+          site: development_site,
+          authorize_url: development_authorize_url,
+          token_url: development_token_url
+      })
     end
 
-    it 'has the correct token url' do
-      expect(strategy.options.client_options.token_url).to eq('https://auth.raspberrypi.org/oauth2/token')
+    before(:each) do
+      allow(strategy).to receive(:access_token).and_return(access_token)
     end
 
-    describe 'defaults are overrideable' do
-      it 'for site' do
-        expect(development.options.client_options.site).to eq(development_site)
+    context 'client options' do
+      it 'has the correct site url' do
+        expect(strategy.options.client_options.site).to eq('https://auth-v1.raspberrypi.org')
       end
 
-      it 'for authorize url' do
-        expect(development.options.client_options.authorize_url).to eq(development_authorize_url)
+      it 'has the correct authorize url' do
+        expect(strategy.options.client_options.authorize_url).to eq('https://auth-v1.raspberrypi.org/oauth2/auth')
       end
 
-      it 'for token url' do
-        expect(development.options.client_options.token_url).to eq(development_token_url)
+      it 'has the correct token url' do
+        expect(strategy.options.client_options.token_url).to eq('https://auth-v1.raspberrypi.org/oauth2/token')
       end
-    end
-  end
 
-  context '#email' do
-    it 'returns email from raw_info if available' do
-      allow(strategy).to receive(:raw_info).and_return('email' => '[email protected]')
-      expect(strategy.email).to eq('[email protected]')
-    end
+      describe 'defaults are overrideable' do
+        it 'for site' do
+          expect(development.options.client_options.site).to eq(development_site)
+        end
+
+        it 'for authorize url' do
+          expect(development.options.client_options.authorize_url).to eq(development_authorize_url)
+        end
 
-    it 'returns nil if there is no raw_info and email access is not allowed' do
-      allow(strategy).to receive(:raw_info).and_return({})
-      expect(strategy.email).to be_nil
+        it 'for token url' do
+          expect(development.options.client_options.token_url).to eq(development_token_url)
+        end
+      end
     end
-  end
 
-  context '#fullname' do
-    it 'returns fullname from raw_info if available' do
-      allow(strategy).to receive(:raw_info).and_return('name' => 'Jane Doe')
-      expect(strategy.fullname).to eq('Jane Doe')
+    context '#email' do
+      it 'returns email from deserialised info if available' do
+        allow(strategy).to receive(:raw_info).and_return('email' => '[email protected]')
+        expect(strategy.email).to eq('[email protected]')
+        expect(strategy.info['email']).to eq('[email protected]')
+      end
+
+      it 'returns nil if there is no email in raw_info' do
+        allow(strategy).to receive(:raw_info).and_return({})
+        expect(strategy.email).to be_nil
+        expect(strategy.info['email']).to be_nil
+      end
     end
 
-    it 'returns nil if there is no raw_info and fullname access is not allowed' do
-      allow(strategy).to receive(:raw_info).and_return({})
-      expect(strategy.fullname).to be_nil
+    context '#name' do
+      it 'returns name from deserialised info if available' do
+        allow(strategy).to receive(:raw_info).and_return({'name' => 'Jane Doe'})
+        expect(strategy.fullname).to eq('Jane Doe')
+        expect(strategy.info['name']).to eq('Jane Doe')
+      end
+
+      it 'returns nil if there is no name in raw_info' do
+        allow(strategy).to receive(:raw_info).and_return({})
+        expect(strategy.fullname).to be_nil
+        expect(strategy.info['name']).to be_nil
+      end
     end
-  end
 
-  context '#nickname' do
-    it 'returns nickname from raw_info if available' do
-      allow(strategy).to receive(:raw_info).and_return('nickname' => 'Raspberry Jane')
-      expect(strategy.nickname).to eq('Raspberry Jane')
+    context '#nickname' do
+      it 'returns nickname from deserialised info if available' do
+        allow(strategy).to receive(:raw_info).and_return({'nickname' => 'Raspberry Jane'})
+        expect(strategy.nickname).to eq('Raspberry Jane')
+        expect(strategy.info['nickname']).to eq('Raspberry Jane')
+      end
+
+      it 'returns nil if there is no nickname in raw_info' do
+        allow(strategy).to receive(:raw_info).and_return({})
+        expect(strategy.nickname).to be_nil
+        expect(strategy.info['nickname']).to be_nil
+      end
     end
 
-    it 'returns nil if there is no raw_info and nickname access is not allowed' do
-      allow(strategy).to receive(:raw_info).and_return({})
-      expect(strategy.nickname).to be_nil
+    context '#image' do
+      it 'returns image from deserialised info if available' do
+        allow(strategy).to receive(:raw_info).and_return({'picture' => '/profile/1234/avatar'})
+        expect(strategy.image).to eq('/profile/1234/avatar')
+        expect(strategy.info['image']).to eq('/profile/1234/avatar')
+      end
+
+      it 'returns nil if there is no picture in raw_info' do
+        allow(strategy).to receive(:raw_info).and_return({})
+        expect(strategy.image).to be_nil
+        expect(strategy.info['image']).to be_nil
+      end
+    end      
+
+    context '#extra' do
+      it 'returns roles and picture from deserialised extra if available' do
+        allow(strategy).to receive(:raw_info).and_return({'roles' => 'admin', 'picture' => '/profile/1234/avatar'})
+        expect(strategy.extra['raw_info']['roles']).to eq('admin')
+        expect(strategy.extra['raw_info']['picture']).to eq('/profile/1234/avatar')
+      end
+
+      it 'returns nil if no roles or picture in raw_info' do
+        allow(strategy).to receive(:raw_info).and_return({})
+        expect(strategy.extra['raw_info']['roles']).to be_nil
+        expect(strategy.extra['raw_info']['picture']).to be_nil
+      end
     end
-  end
 
-  describe '#callback_url' do
-    it 'is a combination of host and callback path' do
-      allow(strategy).to receive(:full_host).and_return('https://example.com')
+    describe '#callback_url' do
+      it 'is a combination of host and callback path' do
+        allow(strategy).to receive(:full_host).and_return('https://example.com')
+        strategy.instance_variable_set(:@env, {})
 
-      expect(strategy.callback_url).to eq('https://example.com/auth/hydra1/callback')
+        expect(strategy.callback_url).to eq("https://example.com/auth/#{provider_name provider}/callback")
+      end
     end
   end
-end
+}