search

Cvmcosta / ltijs

Turn your application into a fully integratable LTI 1.3 tool provider.
https://cvmcosta.github.io/ltijs/
Apache License 2.0
299 stars 67 forks source link

OIDC login doesn't work when initiated with GET request #17

Closed esellin closed 4 years ago

esellin commented 4 years ago

I'm testing against the IMS reference platform https://lti-ri.imsglobal.org/

OIDC login works if initiated with a POST request, but not with a GET request, which I believe tools should also support.

The code handling the login request looks for stuff into req.body, but this will only work for POST requests. For GET requests, you will need to look into req.query instead.

I've created a: const params = { ...req.body, ...req.query } to merge both, and that seems to work okay. Can't believe Express doesn't do that for us in 2020!

Anyway, looks like a simple fix.

Thanks for your awesome work on this library!

Cvmcosta commented 4 years ago

Hello again!

You are right, i changed the type of request it supported but forgot to get the correct parameters. I've uploaded a patch that hopefully fixes the issue. Do you mind uploading ltijs to the newest version (2.4.3) and letting me know if it's working fine now?

Thanks for helping :smiley:

esellin commented 4 years ago

It's not working.

I forgot to mention that in your sessionValidator, you need to check for req.path === this.#loginUrl because req.url will contain all the GET parameters.

Also, it would be good to have a call to provMainDebug() before every return statement in the sessionValidator, so we know the exact flow it's going through.

Thanks!

Cvmcosta commented 4 years ago

Okay, second time's the charm.

Changed req.url to req.path when matching routes and added some additional logs. Also added a unit test specifically to test login flow initiated with a GET request.

Hopefully it all works fine now.

esellin commented 4 years ago

Yes, works fine now with 2.4.6.

Thanks for the quick fix, much appreciated!

esellin commented 4 years ago

Bad news I'm afraid, it's not working properly, I've had to add some more debug to your code to work it out, I'll try to explain :)

If the platform sends a GET request to the /login endpoint, it doesn't have a Origin header (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin), so the code ends up using the Host header, which will be my own tool's URL, and that will be used to generate the state and iss cookies.

Later on, when the platform sends a POST request to the / endpoint (it's always a POST apparently), it does have a Origin header correctly set to the platform's URL https://lti-ri.imsglobal.org, so it will use that to search for cookies, and fail.

The reason why I said it worked before (and probably why you also thought it worked!) is because I was testing against my tool running on my own computer, and this was over http rather than https, which means that the POST to / had its Origin set to null, so the code would use the Host header again, and find the cookies this time.

So... I don't know enough about the LTI spec, and the use of the Origin vs Host header, but it clearly doesn't work in the current state.

Hope my explanation is clear enough!

Cvmcosta commented 4 years ago

Hello again!

I've spent some time today working on this issue, and i saw it as an opportunity to make some changes to the overall login flow. I cleaned up the code and changed the order in which i validate the id_token and provide the user with an ltikey. This change not only made the code and authentication flow itself much simpler, but also allowed me to create the authentication tokens based on the iss parameter received through the login request and id_token, instead of the origin and host headers. This gives me a much more precise identification of the platform originating the requests and hopefully fixes the problem you mentioned.

Thanks! :+1:

esellin commented 4 years ago

Hello there Looks like it's working great now, both with GET and POST. Thanks! I would like to see a little more debug output around the names of cookies it's using, but other than that, it's perfect, well done!