oxctl / spring-security-lti13

A LTI 1.3 implementation for Spring Security that builds on the OAuth2 support
Apache License 2.0
14 stars 7 forks source link

Create a jakarta compatible version for Spring Framework 6 / Spring Boot 3 #26

Closed krusche closed 3 months ago

krusche commented 1 year ago

Most imports to javax need to be replaced with jakarta, otherwise this framework cannot be used with Spring Framework 6

buckett commented 1 year ago

Thanks @krusche, there are some notes about this on: https://github.com/spring-projects/spring-framework/wiki/Upgrading-to-Spring-Framework-6.x

I expect that we'll need to continue to update versions of the library that target older versions of spring so will need a branch to support releasing for < Spring 6.

None of our project that use this library have yet moved to Spring 6 so we aren't in a rush to get this done, but if this is a blocker for anyone please shout.

krusche commented 1 year ago

It's a blocker for us soon 🙈 we plan to support Spring 6 until the end of this year.

I created a fork and adapted some code, but I'm not sure if it stills works because there are many breaking changes with respect to this library. I hope I can get some assistance from a developer in our team with more expertise on LTI.

If we get it working, we will create a PR.

mitch30000 commented 1 year ago

@krusche did you manage to make any headway with this? A Spring Boot 3 version of this library is something I'm also interested in.

krusche commented 1 year ago

Not yet. But someone in m team is looking into it atm. However, I don't expect results in the very near future. I assume this will take some time

TomDoesb commented 1 year ago

@krusche Did you make any progress? I am also in the progress of moving to spring boot 3 and the main issue is run into is that the implicit AuthorizationGrantType has been deprecated and is no longer available. How did you adapt the OIDCInitiatingLoginRequestResolver?

buckett commented 1 year ago

I've taken the work that was in @krusche repo (nice work thanks) and updated the tests to work and there's a draft PR out on: https://github.com/oxctl/spring-security-lti13/pull/37

I'm hoping to get this updated, documentation sorted, demo updated soon, but if anyone want to contribute by all means help.

One thing that is going to cause problems is the original incorrect (I think) use of implicit grant in the configuration which is currently switched to authorization code in the draft PR. This will mean people will need to update their configuration when updating between the Spring 5 version and the Spring 6 one.

TomDoesb commented 1 year ago

@buckett You mean update the application.properties and add the grantType there as well? It does seem to be a little more complicated than that. If your application uses cookies you need to alter the OIDCLaunchFlowAuthenticationProvider or the Oauth2LoginAuthenticationFilter as well. In Spring Security 6 the cookie is not automatically set anymore it seems. Did you encounter this already?

buckett commented 1 year ago

So I think we were right in using the implicit grant, but this has now been removed from Spring Security under: https://github.com/spring-projects/spring-security/issues/11506

TomDoesb commented 1 year ago

I've made similar changes as you did in #37 and the OpenId authentication with brightspace seems to work fine. The issue I need to address now is that the session cookie is no longer set.. So the backend of my tool is authenticated, but without a cookie the browser is not. I had expected that line 324 and 325 in the AbstractAuthenticationProcessingFilter.class would still work. Specially because I had to call these functions to set a cookie for our normal login endpoint with Spring Security 6.0

TomDoesb commented 1 year ago

I fixed it by updating the constructor of the Oauth2LoginAuthenticationFilter and explicitly setting the HttpSessionSecurityContextRepository

public OAuth2LoginAuthenticationFilter(ClientRegistrationRepository clientRegistrationRepository,
                                           String filterProcessesUrl) {
        super(filterProcessesUrl);
        Assert.notNull(clientRegistrationRepository, "clientRegistrationRepository cannot be null");
        this.clientRegistrationRepository = clientRegistrationRepository;
        // Set a HttpSessionSecurityContextRepository to add the session cookie (Spring Security 6.0 fix)
        setSecurityContextRepository(new HttpSessionSecurityContextRepository());
    }

Now the cookie is set as before

jpyoder commented 1 year ago

@buckett, any update on when there might be a version available for Spring 6? We've been using an in-house LTI 1.1 library, but want to move to 1.3, and are looking for alternative libraries. However, we're also upgrading to Spring Boot 3, which depends on Spring Framework 6.

maurercw commented 1 year ago

Hey, I was just looking into this today too! We were starting some quarterly updates and I had looked to see if we were able to go to spring boot 3 (or 3.1), or if we needed to stay with the latest 2.7. I've got no pressing needs to upgrade, but was curious! So, watching this as well.

buckett commented 1 year ago

It's something I'd like to fix but it's not currently a priority at work, we're trying to move things to Spring Boot 3 but none of the things done so far have needed this library. I'd hope we'd need to in a few months....

TomDoesb commented 1 year ago

@jpyoder @maurercw you could use #37 as a starting point and go from there. I've been using the authentication part of this repo and was able to move to spring boot 3.1.2 with relatively small changes.

Biggest changes is that the implicit authorization grant is no longer supported by spring security. Moving to the authorization_code grant works fine for Brightspace. (not tested with others).

If you want to use cookies you now need to explicitly set the cookie as I did in the comment above.

Good luck

maurercw commented 5 months ago

FYI, we're finally starting to look into upgrading our tools to spring boot 3.2 and I'm using the branch mentioned as a base. Will report back how it goes!

buckett commented 5 months ago

We did start a discussion with Spring Security about bringing back support for implicit or custom grants, but it doesn't look hopeful. https://github.com/spring-projects/spring-security/issues/15111

maurercw commented 5 months ago

@TomDoesb I did use the cookie stuff you mentioned, and along with a tweak to a dependency in the pom, things seem to be working fine for the one app I've migrated.

@buckett Any thoughts on when we could get this moved along to an official release?

Zsupi commented 4 months ago

@maurercw Is there any update on the Spring 6 upgrade?

maurercw commented 3 months ago

I met with the maintainers yesterday and they were very receptive to making the changes in the PR, just have to convince their leadership to prioritize the work. Maybe if all interested parties spoke up here via comment or reaction, it might help show how many people are waiting on this?

buckett commented 3 months ago

Ok, the PR is merged, I'll make some documentation updates and then hopefully release shortly.

buckett commented 3 months ago

And it's live now:

Sorry it's taken so long and please report any issue you have with upgrading.

buckett commented 3 months ago

I've also updated the demo application to use the newly released version: https://github.com/oxctl/spring-security-lti13-demo