Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookies are always empty. #54

Open
ronnela opened this issue May 3, 2022 · 10 comments
Open

Cookies are always empty. #54

ronnela opened this issue May 3, 2022 · 10 comments

Comments

@ronnela
Copy link

ronnela commented May 3, 2022

The cookie is always empty and the Launch endpoint is giving this error
State not found. Please make sure you have cookies enabled in this browser.

@dbhynds
Copy link
Member

dbhynds commented May 10, 2022

@ronnela Can you provide more context for how you encountered this error? Any details you can share about the requests being made during the launch would be helpful, along with your implementation of the ICookie object.

@ronnela
Copy link
Author

ronnela commented May 11, 2022

The Launch is coming from Canvas which is on different domain and that is why the cookie is always empty because of cross-origin rule.

@dbhynds
Copy link
Member

dbhynds commented May 20, 2022

This library isn't reliant on a cookie from Canvas, however it does set one for the tool provider domain. Can you provide more details of your implementation, specifically the ICookie object?

@dbhynds
Copy link
Member

dbhynds commented Jun 8, 2022

I'm marking this issue as closed for the time being. If we get more information, we can reopen it.

@dbhynds dbhynds closed this as completed Jun 8, 2022
@ronnela
Copy link
Author

ronnela commented Jul 19, 2022

Hi @dbhynds

Sorry I was assigned to work on another project but I am back on this now.

Here is the ICookie

namespace App\LTIStorage;

use Illuminate\Support\Facades\Cookie;
use Packback\Lti1p3\Interfaces\ICookie;

class Lti13Cookie implements ICookie
{
public function getCookie(string $name): ?string
{
return Cookie::get($name, false);
}

public function setCookie(string $name, string $value, $exp = 3600, $options = []): void
{
    // By default, make the cookie expire within a minute
    // die("$name, $value, $exp");
    Cookie::queue($name, $value, $exp / 60);
}

}

And here is what it looks like its requiring the use of cookie

if ($this->cookie->getCookie(LtiOidcLogin::COOKIE_PREFIX.$this->request['state']) !== $this->request['state']) {

@sarritalhami
Copy link

sarritalhami commented Nov 9, 2022

I'm having the same issue.

namespace App\LTI;

use Illuminate\Support\Facades\Cookie;
use Packback\Lti1p3\Interfaces\ICookie;

class Lti13Cookie implements ICookie
{
    public function getCookie(string $name): ?string
    {
        return Cookie::get($name, false);
    }

    public function setCookie(string $name, string $value, $exp = 3600, $options = []): void
    {
        // By default, make the cookie expire within a minute
        Cookie::queue($name, $value, $exp / 60);
    }
}

I think Cookie::queue is not actually attaching a cookie to the response

@dbhynds dbhynds reopened this Nov 9, 2022
@dbhynds
Copy link
Member

dbhynds commented Nov 9, 2022

Here's a high level overview of how it's working in our system:

  1. A user launches from the LMS.
  2. The LMS submits a request to the lti/login endpoint.
  3. We do:
function login(Request $request, IDatabase $db, ICache $cache, ICookie $cookie) {
    $redirectUrl = LtiOidcLogin::new($db, $cache, $cookie)
        ->doOidcLoginRedirect('https://ourdomain.com/lti/launch', $request->all())
        ->getRedirectUrl();
    return redirect($url);
}
  1. This redirects the use back to the LMS, which then submits a request to the /lti/launch endpoint.
  2. From there we do:
function launch(Request $request, IDatabase $db, ICache $cache, ICookie $cookie, ILtiServiceConnector $serviceConnector) {
    $launch = LtiMessageLaunch::new($db, $cache, $cookie, $serviceConnector)
            ->validate($request->all());
    $ltiData = $launch->getLaunchData();
    // Do whatever after that.
}

How does that compare to what you're doing? Are you able to see the cookie being attached to the redirect back to the LMS? What about the subsequent launch request?

As I mentioned, we have this working in our production infrastructure, so my guess is there some configuration issue with your app (or perhaps the LMS).

Unrelated (and I'll update the documentation), but I noticed some typing issues in the Lti13Cookie::getCookie() method. Minor differences, but here what it should be:

    public function getCookie(string $name): ?string
    {
        return Cookie::get($name);
    }

@sarritalhami
Copy link

Thanks for the reply. So after a bit of fiddling I managed to figure out what the issue was potentially. It seems to work as I get a successful launch. However, I'm not sure if my solution in optimal. I'll post my solution here for future reference.

So I'm pretty much I was doing the same thing as mentioned above. With slight deviations in the implementation of my login and launch endpoints. I'm using Laravel 9.x (fyi).

Here were my implementations of the endpoints:

public function login( )
{
    return LtiOidcLogin::new(new Lti13Database, new Lti13Cache, new Lti13Cookie)
            ->doOidcLoginRedirect(url("lti13/launch"))
            ->doRedirect();
}

public function launch( )
{
    $launch = LtiMessageLaunch::new(new Lti13Database, new Lti13Cache, new Lti13Cookie)->validate();
    $launchData = $launch->getLaunchData();
}

I wasn't passing instances of my interfaces or the request into my login/launch endpoints. From what I gathered from the documentation it seemed as though you could pass new definitions of those interfaces. Also, when I was digging through LtiMessageLaunch and LtiOidcLogin it seemed like it captured the necessary request data.

I.e. in LtiMessageLaunch:

public function validate(array $request = null)
    {
        if ($request === null) {
            $request = $_POST;
        }
        //etc...
    }

In LtiOdicLogin:

public function doOidcLoginRedirect($launch_url, array $request = null)
    {
        if ($request === null) {
            $request = $_REQUEST;
        }
        //etc
    }

With those previous implementations of login/launch endpoints I was getting through the login phase and getting a successful redirect from the LMS to the launch endpoint (with the correct data). I tried your endpoint examples and they fixed the issue of the missing lti1p3_state cookie. However, it seems I was still stuck with a 419 session expired issue. Firefox was discarding the cookies:

Cookie “lti1p3_state-{ .... }” with the “SameSite” attribute value “Lax” or “Strict” was omitted because of a cross-site redirect.

The cookie expires a minute from creation, and has these properties as defined in my config/session.php:

HostOnly:true
HttpOnly:true
SameSite:"Lax"
Secure:true

From my limited understanding of cookies nothing here seemed to be the issue. For the purposes of testing I omitted these routes in Middleware/VerifyCsrfToken.php which then allows LtiException to throw:
Please make sure you have cookies enabled in this browser and that you are not in private or incognito mode
Digging into SameSite a little I kept HTTPS Only Cookies, ('secure' => env('SESSION_SECURE_COOKIE', true)) - in config/session.php. Then changed 'same_site' => 'lax' to 'same_site' => 'none'. This works.

However, I'm not sure this is the most secure way to do this. Should I be omitting these routes from Csfr Token Verification?

@dbhynds
Copy link
Member

dbhynds commented Nov 11, 2022

Ahah! I totally forgot that we had to tinker with the config for cookies to get this to work, but that's totally it. Thank you, I'll update the documentation.

A few notes:

First, I see how--based on the interface of LtiMessageLaunch::validate() and LtiOdicLogin::doOidcLoginRedirect()--you would reasonably assume you could skip passing in the $request->all() data. The fact that we read from $_POST or $_REQUEST is not ideal, and is a holdover from the old library I based this one on. I think the ideal solution would be to make those arguments required, rather than optional. (This, unfortunately, would be a breaking change, so it might be a while before we roll it out if that's the way we go.) Basically:

public function validate(array $requestPostData);
// and
public function doOidcLoginRedirect($launch_url, array $requestPostData);

Thoughts on that?

Second, we also have 'same_site' => 'none'. This is necessary for LTI requests, as the user bounces back and forth between the your server and the LMS during the launch. We also have VerifyCsrfToken middleware applied elsewhere in our application, but not on our LTI routes. Let me actually do a little more research into this and get back to you.

@datavoyager
Copy link

Just in case it helps anyone (I spent a whole day on this), I had the same problems as detailed here with the additional problem that I was using Laravel's artisan serve functionality as a web server on localhost. As this doesn't have ssl support, the cookies were never set.

In the end, I used the instructions here to setup https on Vite and then use that as a proxy for Laravel's web server.

I then had to force Laravel to use https in boot method of the AppServiceProvider class.

public function boot()
    {

        \URL::forceScheme('https');

        JWT::$leeway = 5;
    }

Thanks to @dbhynds and @ronnela for the pointers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants