celtic-project / LTI-PHP

PHP class library for building LTI integrations
GNU Lesser General Public License v3.0
48 stars 38 forks source link

lti2_nonce.consumer_pk with multi-tenant issuer #27

Closed danhammari closed 2 years ago

danhammari commented 2 years ago

I'm running into trouble with the lti2_nonce table's consumer_pk field. Here is the workflow:

  1. platform sends request to tool's login URL with payload that has property claim iss as https://schoology.schoology.com
  2. tool's static function fromPlatformId receives this as $platformId while $clientId and $deploymentId are null
  3. tool's static function fromPlatformId calls DataConnector loadPlatform method using only the $platformId value
  4. tool now has instance of platform with recordId value 25 and builds an lti2_nonce entry with index consumer_pk 25
  5. tool sends request to platform's authentication URL with lti2_nonce value as state in payload
  6. platform receives request and responds with claims iss, aud, and deploymentId and also returns state nonce
  7. tool's static function fromPlatformId receives all three values as $platformId, $clientId, and $deploymentId
  8. tool's static function fromPlatformId calls DataConnector loadPlatform method using all three values
  9. tool now has instance of platform with recordId value 36 since it found an alternate consumer signature in the lti2_consumer table
  10. tool tries to find the nonce record that matches the state claim that was returned, but cannot since the consumer_pk value is now 36 instead of the 25 that was used when creating the lti2_nonce record

The lti2_consumer table seems to be built with the intention of allowing multiple instance of a platform_id value, provided that an alternate composite key of fields platform_id, client_id, and deployment_id remains unique. The problem becomes disambiguating multiple records when only the iss field is provided as a lookup. In particular, the lti2_nonce table relies on the correct foreign key value of consumer_pk when validating the returned state claim.

My solution right now is to alter the DataConnector loadPlatformNonce method and have it ignore the consumer_pk column when validating the state nonce that the platform returns:

BEFORE:

// Load the nonce
        $id = $nonce->getPlatform()->getRecordId();
        $value = $nonce->getValue();
        $sql = "SELECT value T FROM {$this->dbTableNamePrefix}" . static::NONCE_TABLE_NAME . ' WHERE (consumer_pk = :id) AND (value = :value)';
        $query = $this->db->prepare($sql);
        $query->bindValue('id', $id, \PDO::PARAM_INT);
        $query->bindValue('value', $value, \PDO::PARAM_STR);
        $ok = $this->executeQuery($sql, $query, false);

AFTER:

// Load the nonce
        $value = $nonce->getValue();
        $sql = "SELECT value T FROM {$this->dbTableNamePrefix}" . static::NONCE_TABLE_NAME . ' WHERE (value = :value)';
        $query = $this->db->prepare($sql);
        $query->bindValue('value', $value, \PDO::PARAM_STR);
        $ok = $this->executeQuery($sql, $query, false);

Let me know if this is not the correct direction to go. I imagine I will run into this issue repeatedly when registering platforms that host multiple tenants. As in the example above, Schoology will always send the same iss claim value to represent its platform, but each tenant will likely have its own aud claim (clientId), and multiple deployment_id claims. I can understand registering each tenant customer on the platform with its own client_id value, but hesitate to also record a uniquedeployment_id as well. Would it perhaps be okay to alter the DataConnector loadPlatform method and make it so the deployment_id field is not considered when searching for a platform entry in the lti2_consumer table?

spvickers commented 2 years ago

Thanks for reporting this; I can see it is an issue. I'll fix this in the next release.

As an aside, do you know why the platform is choosing not to send the client ID and/or deployment ID in the initiate login request?

danhammari commented 2 years ago

This may be an issue with how the LTI security specification is written. When a platform begins the third-party initiated login process the only required parameters are iss, login_hint, and target_link_uri: https://www.imsglobal.org/spec/security/v1p0/#step-1-third-party-initiated-login

5.1.1.1 Step 1: Third-party Initiated Login

When a user wants to launch into a Tool, the Platform will start the OpenID Connect flow by redirecting the User Agent (UA) to the 3rd party initiated login end point. The redirect may be a form POST or a GET - a Tool must support either case, with the following parameters.

iss
    REQUIRED. The issuer identifier identifying the learning platform.
login_hint
    REQUIRED. Hint to the Authorization Server about the login identifier the End-User might use to log in. The permitted values will be defined in the host specification.
target_link_uri
    REQUIRED. The actual end-point that should be executed at the end of the OpenID Connect authentication flow.

NOTE: Other parameters MAY be supplied to provide important context for a specific message exchange i.e. as defined in the corresponding IMS specification.
spvickers commented 2 years ago

Yes, I accept that it is quite valid for the client ID and deployment ID to be omitted from the request; I was just curious to find out if you knew why the platform chooses to do so. Since the values are known and shared with the tool there is no obvious (to me) reason why they would not be passed to make its life easier.

But I will update the library to allow for this.

spvickers commented 2 years ago

Update included in latest release (4.7.0). Thanks.