Support CAS Protocol 3.0 attributes #2

jbjonesjr · 2016-07-16T03:48:11Z

CAS 2.0 just returned username and ticket in the authenticationSuccess message

<cas:authenticationSuccess>
 <cas:user>username</cas:user>
 <cas:proxyGrantingTicket>PGTIOU-84678-8a9d...</cas:proxyGrantingTicket>
</cas:authenticationSuccess>

CAS Protocol 3.0 supports additional attributes

<cas:authenticationSuccess>
     <cas:user>username</cas:user>
     <cas:attributes>
       <cas:firstname>John</cas:firstname>
       <cas:lastname>Doe</cas:lastname>
       <cas:title>Mr.</cas:title>
       <cas:email>jdoe@example.org</cas:email>
       <cas:affiliation>staff</cas:affiliation>
       <cas:affiliation>faculty</cas:affiliation>
     </cas:attributes>
     <cas:proxyGrantingTicket>PGTIOU-84678-8a9d...</cas:proxyGrantingTicket>
   </cas:authenticationSuccess>
 </cas:serviceResponse>

This pull request updates the parsing of the <authenticationSuccess> element to include attributes.

Actions

Update parsing to support attribute detection, inclusion in user_info
Update mock authenticationSuccess models to include attributes
Update tests to look for said attributes

* general code cleanup * look for attributes element, new in CAS Protocol 3.0 * if found, add those attributes into the user_info hash

jbjonesjr · 2016-07-16T03:51:26Z

This omniauth-cas module (built for omniauth-1.0) had already integrated this change from the original (omniauth 0.3 code that is in use here), so i'm i'm just going to port it over.

This work was done in these commits: dlindahl/omniauth-cas@8de378c , dlindahl/omniauth-cas@8ec97af

This model is of a CAS Protocol 3.0-style authenticationSuccess response that includes additional attributes. Details on the format can be found here: https://apereo.github.io/cas/4.2.x/protocol/CAS-Protocol-Specification.html#attributes-cas-30

jbjonesjr · 2016-07-17T03:15:15Z

Notes

Schema for CAS Protocol 3.0

Schema for CAS Protocol 2.0

- abstract user_info a bit with raw_info - build out the proper hash for the omniauth schema requirements - expose uid in a managed way

jbjonesjr · 2016-07-18T14:56:47Z

Note, the CAS success mock defined dlindahl/omniauth-cas@8ec97af and previously in our test suite is not valid schema. The only valid elements under success (per the schema linked above for 2.0) was User, ProxyGrantingTicket and Proxies.

  <xs:complexType name="AuthenticationSuccessType">
        <xs:sequence>
            <xs:element name="user" type="xs:string"/>
            <xs:element name="proxyGrantingTicket" type="xs:string" minOccurs="0"/>
            <xs:element name="proxies" type="cas:ProxiesType" minOccurs="0"/>
        </xs:sequence>
    </xs:complexType>

I'm updating the Protocol 2.0 tests (and spec/mock) to reflect this.

jbjonesjr · 2016-08-06T14:26:41Z

cc/ @github/platform-iam

I believe this PR would fall under your purview. I'm pretty satisfied with the code as is, but can't figure out how we expect rspec to run appropriately. I'd feel 💯% better sending you a PR with passing tests, but am about at the end of my capability.

davesims · 2016-08-08T13:44:14Z

oa-enterprise/lib/omniauth/strategies/cas/service_ticket_validator.rb

+          {}.tap do |hash|
+            node.children.each do |e|
+              node_name = e.name.sub(/^cas:/, '')
+              unless e.kind_of?(Nokogiri::XML::Text) || node_name == 'proxies'


Nice cleanup @jbjonesjr! If we wanted to be even nitpickier, maybe use a next if here instead of the unless block for readability?

@davesims I copied this method from the upstream code. I have no clue as to our plan to ever sync with upstream again, so am not sure how much we care about being compatible. Thoughts?

davesims · 2016-08-08T13:48:57Z

Thanks for this @jbjonesjr! I took a quick first pass on it, I'll do more thorough 👀 later this week. Looks great so far.

jbjonesjr · 2016-08-29T08:39:59Z

oa-enterprise/lib/omniauth/strategies/cas.rb

+      attr_accessor :raw_info
+      alias_method :user_info, :raw_info
+
+      option :uid_key,              'user'


While the option method is available in modern implementations of OmniAuth::Strategy, it is not available in the github/omniauth version
Need to revert back to a regular attr_accessor with a default.

jbjonesjr · 2016-08-30T01:31:55Z

So I figured out how to run the tests, am working my way through confirming that this actually works. Will commit and make a comment when ready to go again

jbjonesjr · 2016-09-05T22:17:53Z

Woot woot!

Finished in 0.12449 seconds
18 examples, 1 failure

Failed examples:

rspec ./spec/omniauth/strategies/cas_spec.rb:129 # OmniAuth::Strategies::CAS GET /auth/cas/callback with a valid ticket and gzipped response from the server on ruby >1.8 should call through to the master app when response is gzipped

Got almost all the tests passing (pretty sure the failing tests failed before I started this experiment).
Note: Confirmed afterwards that the gzip test fails on the 0-3-stable branch as well

I need to now go back and make sure this is compatible with the omniauth.auth variable the app expects us this module to provide (I'm worried we might not be sticking to the Auth Hash Schema. I'm also going to add some comments and discussion around some of these changes to be document why I did things the way I did, and hopefully provide the needed care to 🚢.

jbjonesjr

annotated my changes for this codebase

jbjonesjr · 2016-09-20T03:12:42Z

oa-enterprise/lib/omniauth/strategies/cas.rb

+
+      @uid_key = 'user'
+
+      def userHash


add fields defined in the omniauth user_info hash. support multiple formats that it can be shared via

jbjonesjr · 2016-09-20T03:13:14Z

oa-enterprise/lib/omniauth/strategies/cas.rb

      def initialize(app, options = {}, &block)
        super(app, options[:name] || :cas, options.dup, &block)
-        @configuration = OmniAuth::Strategies::CAS::Configuration.new(options)
+          @configuration = OmniAuth::Strategies::CAS::Configuration.new(options)


just whitespace change

jbjonesjr · 2016-09-20T03:13:50Z

oa-enterprise/lib/omniauth/strategies/cas.rb

+        @ticket = request.params['ticket']
+        return fail!(:no_ticket, 'No CAS Ticket') unless @ticket
+
+        self.raw_info = ServiceTicketValidator.new(@configuration, callback_url, @ticket).user_info


use the raw_info accessor, also chain methods for readability

jbjonesjr · 2016-09-20T03:15:40Z

oa-enterprise/lib/omniauth/strategies/cas.rb

        super
      end

      def auth_hash
        OmniAuth::Utils.deep_merge(super, {
-          'uid' => @user_info.delete('user'),
-          'extra' => @user_info
+          'uid' => @raw_info['user'],


configure omniauth object as defined in Auth Hash Schema

jbjonesjr · 2016-09-20T03:16:02Z

oa-enterprise/lib/omniauth/strategies/cas.rb

+
+       # Deletes Hash pairs with `nil` values.
+       # From https://github.com/mkdynamic/omniauth-facebook/blob/972ed5e3456bcaed7df1f55efd7c05c216c8f48e/lib/omniauth/strategies/facebook.rb#L122-127
+        def prune!(hash)


clean up nil/null values

jbjonesjr · 2016-09-20T03:17:25Z

oa-enterprise/spec/omniauth/strategies/cas_spec.rb

-         to_return(:body => File.read(File.join(File.dirname(__FILE__), '..', '..', 'fixtures', 'cas_success.xml')))
-      get '/auth/cas/callback?ticket=593af'
-    end
+    shared_examples :successful_validation do


upstream tests are a bit cleaner and easier to follow.

jbjonesjr · 2016-09-20T03:17:58Z

oa-enterprise/spec/omniauth/strategies/cas_spec.rb

+          get "/auth/cas/callback?ticket=593af&url=#{return_url}"
+      end
+
+      it 'should strip the ticket parameter from the callback URL before sending it to the CAS server' do


most of these tests are from upstream, but some of my own intepretation based on our sample success criteria

jbjonesjr · 2016-09-20T03:18:15Z

oa-enterprise/spec/omniauth/strategies/cas_spec.rb

+      let(:return_url) { 'http://127.0.0.10/?some=parameter' }
+
+
+    context 'cas-protocol-2.0' do


support the two different protocols under test

jbjonesjr · 2016-09-20T03:18:25Z

oa-enterprise/spec/omniauth/strategies/cas_spec.rb

        end
-        stub_request(:get, /^https:\/\/cas.example.org(:443)?\/serviceValidate\?([^&]+&)?ticket=593af/).
+        stub_request(:get, /^http(s)?:\/\/cas.example.org(:8080)?\/serviceValidate\?([^&]+&)?ticket=593af/).


same as above.

jbjonesjr · 2016-09-20T03:18:54Z

oa-enterprise/spec/omniauth/strategies/cas_spec.rb

+    context 'cas-protocol-3.0' do
+      let(:xml_file_name) { 'cas_success_3.0.xml' }
+      it_behaves_like :successful_validation
+    end
  end

  unless RUBY_VERSION =~ /^1\.8\.\d$/
    describe 'GET /auth/cas/callback with a valid ticket and gzipped response from the server on ruby >1.8' do


note: this test fails on my branch, and on the branch i branched from.

¯_(ツ)_/¯

jbjonesjr · 2016-09-20T03:24:13Z

OK 👌 . I've taken advantage of the new code review functionality to annotate the work that I have performed in this PR. I'm happy to answer any questions that anyone may have.

cc/ @sbryant as some folks thought you might be an owner of this sort of thing.

update with changes from dlindahl/omniauth-cas

3b0edb4

* general code cleanup * look for attributes element, new in CAS Protocol 3.0 * if found, add those attributes into the user_info hash

cas protocol 3.0 style success response

bde4490

This model is of a CAS Protocol 3.0-style authenticationSuccess response that includes additional attributes. Details on the format can be found here: https://apereo.github.io/cas/4.2.x/protocol/CAS-Protocol-Specification.html#attributes-cas-30

jbjonesjr added 3 commits July 16, 2016 22:44

fixing to be cas-2.0 protocol compliant

01076b9

Rename cas_success.xml to cas_success_2.0.xml

f0f71b1

work on better protocol compliance

7e85fda

- abstract user_info a bit with raw_info - build out the proper hash for the omniauth schema requirements - expose uid in a managed way

jbjonesjr added 3 commits August 6, 2016 09:33

improve response of valid CAS token

834c4d1

add context to support the two cas protocols 4test

cad1ab4

update to use even better token checking

3582e05

davesims reviewed Aug 8, 2016
View reviewed changes

jbjonesjr reviewed Aug 29, 2016
View reviewed changes

Passing tests(mostly)git status 🎉 ✨

51df2cd

jbjonesjr added 3 commits September 5, 2016 22:14

remove unneccessary code

4146d2f

more cleanup

e0377a5

general cleanup

7712e4b

jbjonesjr commented Sep 20, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support CAS Protocol 3.0 attributes #2

Support CAS Protocol 3.0 attributes #2

jbjonesjr commented Jul 16, 2016 •

edited

Loading

jbjonesjr commented Jul 16, 2016 •

edited

Loading

jbjonesjr commented Jul 17, 2016 •

edited

Loading

jbjonesjr commented Jul 18, 2016

jbjonesjr commented Aug 6, 2016 •

edited

Loading

davesims Aug 8, 2016

jbjonesjr Aug 30, 2016

davesims commented Aug 8, 2016

jbjonesjr Aug 29, 2016

jbjonesjr commented Aug 30, 2016

jbjonesjr commented Sep 5, 2016 •

edited

Loading

jbjonesjr left a comment

jbjonesjr Sep 20, 2016 •

edited

Loading

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr Sep 20, 2016

jbjonesjr commented Sep 20, 2016

		let(:return_url) { 'http://127.0.0.10/?some=parameter' }


		context 'cas-protocol-2.0' do

Support CAS Protocol 3.0 attributes #2

Are you sure you want to change the base?

Support CAS Protocol 3.0 attributes #2

Conversation

jbjonesjr commented Jul 16, 2016 • edited Loading

Actions

jbjonesjr commented Jul 16, 2016 • edited Loading

jbjonesjr commented Jul 17, 2016 • edited Loading

Notes

jbjonesjr commented Jul 18, 2016

jbjonesjr commented Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davesims commented Aug 8, 2016

Choose a reason for hiding this comment

jbjonesjr commented Aug 30, 2016

jbjonesjr commented Sep 5, 2016 • edited Loading

jbjonesjr left a comment

Choose a reason for hiding this comment

jbjonesjr Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbjonesjr commented Sep 20, 2016

jbjonesjr commented Jul 16, 2016 •

edited

Loading

jbjonesjr commented Jul 16, 2016 •

edited

Loading

jbjonesjr commented Jul 17, 2016 •

edited

Loading

jbjonesjr commented Aug 6, 2016 •

edited

Loading

jbjonesjr commented Sep 5, 2016 •

edited

Loading

jbjonesjr Sep 20, 2016 •

edited

Loading