jeffwils / grails-spring-security-saml

Grails Spring Security SAML2.0 Plugin for Grails 3
8 stars 24 forks source link

Update grails-spring-security-saml to support grails 3.3.x #13

Closed valentingoebel closed 6 years ago

valentingoebel commented 6 years ago

It is now finally possible to use grails-spring-security-saml with grails 3.3.x.

This pull request primarily updates the build.gradle and fixes the MetadataController and logoutSuccessHandler which broke because of API changes.

Note: I haven't changed the configuration handling and I still feel it is incomplete. Proper validation for the configuration values in applicaton.yml is missing. The error messages are still messy if the application.yml doesn't exactly match what the plugin expects. The configuration is easy to mess up and it is time consuming to figure out which of the dozens of settings was incorrect.

valentingoebel commented 6 years ago

To test the changes please follow these instructions:

git clone https://github.com/valentingoebel/grails-spring-security-saml
mv grails-spring-security-saml spring-security-saml
cd spring-security-saml
grails install

git clone https://github.com/valentingoebel/grails-spring-security-saml-test
grails run-app

You should see this Message (it will disappear if you correctly configure your metadata):

org.opensaml.saml2.metadata.provider.MetadataProviderException: Metadata for entity http://test.com and role {urn:oasis:names:tc:SAML:2.0:metadata}SPSSODescriptor wasn't found

If you see this Message then everything is working as expected.

irstevenson commented 6 years ago

Just a note (same as I mentioned over in #12), it'd be great if the gradle/wrapper/gradle-wrapper.jar was added back into this project (requiring also that .gitignore is fixed to stop ignoring *.jar). Without it, although this PR updates files like gradlew and gradle.properties they wont fully work.

Also, it'd be worthwhile to add the grails wrapper (grailsw, grailsw.bat and grails-wrapper.jar) back in, so that there is no confusion at all around grails versions for any future maintenance. (Mind you, adding gradle.wrapper goes a long way to helping that.) But is also helps with any CI type building.

This could be done as part of another PR I guess. However seeing this PR is about adding support for a new version of grails/gradles it seems to make sense to slip it in here alongside the above mentioned updated files.

irstevenson commented 6 years ago

Another a issue before releasing this, is to fix the issue with log in SpringSamlUserDetailsService as also detailed over in #12 .

But then I can confirm that the basics appear to work. That is, I successfully authenticated via SAML and arrived back at the index page.

valentingoebel commented 6 years ago

Ok, I have updated spring security core to the latest version.

irstevenson commented 6 years ago

Confirmed all working out of the box:

  1. Fresh clone;
  2. Removed installed plugin from my .m2/repository
  3. Ran grails install
  4. In test app, did a ./gradlew clean and confirmed build directory gone
  5. Ran ./grailsw run-app
  6. Completed successful authentication with no issues.

So it looks like it's up and running again in 3.3.x. Possibly ready to merge.

However, I thought I'd also go back to the plugin and see if I could figure out how to run the tests. But looks like to move to 3.3.0 has caused the tests to break due to changes in test framework:

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/MetadataControllerSpec.groovy: 6: unable to resolve class TestFor ,  unable to find class for annotation
 @ line 6, column 1.
   @TestFor(MetadataController)
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/MetadataControllerSpec.groovy: 7: unable to resolve class Mock ,  unable to find class for annotation
 @ line 7, column 1.
   @Mock(MetadataController)
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/SamlSecurityServiceSpec.groovy: 13: unable to resolve class TestFor ,  unable to find class for annotation
 @ line 13, column 1.
   @TestFor(SamlSecurityService)
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/SamlSecurityServiceSpec.groovy: 14: unable to resolve class Mock ,  unable to find class for annotation
 @ line 14, column 1.
   @Mock(TestSamlUser)
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/SamlTagLibSpec.groovy: 8: unable to resolve class TestFor ,  unable to find class for annotation
 @ line 8, column 1.
   @TestFor(SamlTagLib)
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/SpringSamlUserDetailsServiceSpec.groovy: 3: unable to resolve class grails.test.mixin.Mock
 @ line 3, column 1.
   import grails.test.mixin.Mock
   ^

/tmp/spring-security-saml/src/test/groovy/org/grails/plugin/springsecurity/saml/SpringSamlUserDetailsServiceSpec.groovy: 4: unable to resolve class grails.test.mixin.TestFor
 @ line 4, column 1.
   import grails.test.mixin.TestFor
   ^

/tmp/spring-security-saml/src/test/groovy/test/TestRoleSpec.groovy: 3: unable to resolve class grails.test.mixin.TestFor
 @ line 3, column 1.
   import grails.test.mixin.TestFor
   ^

/tmp/spring-security-saml/src/test/groovy/test/TestSamlUserSpec.groovy: 3: unable to resolve class grails.test.mixin.TestFor
 @ line 3, column 1.
   import grails.test.mixin.TestFor
   ^

/tmp/spring-security-saml/src/test/groovy/test/TestUserRoleSpec.groovy: 3: unable to resolve class grails.test.mixin.TestFor
 @ line 3, column 1.
   import grails.test.mixin.TestFor
   ^

10 errors

But I guess we could merge this, and then I can look at:

  1. Getting the wrappers up and running again; and then
  2. Getting the test suite to work again.

If you think that sounds workable, I guess seeing @jeffwils made us contributors you could merge this in @valentingoebel .

valentingoebel commented 6 years ago

It is possible to use the old unit tests in 3.3.x by including a new dependency. http://docs.grails.org/latest/guide/testing.html#unitTesting

I will merge this PR first and we will talk about the unit tests in a seperate issue.