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

Duplicate Entry due to Short Index #9

Open
Souvent22 opened this issue May 18, 2021 · 1 comment
Open

Duplicate Entry due to Short Index #9

Souvent22 opened this issue May 18, 2021 · 1 comment

Comments

@Souvent22
Copy link

Souvent22 commented May 18, 2021

Due to this code snippet:

https://github.com/Cvmcosta/ltijs-sequelize/blob/master/src/DB.js#L298-L313

We are getting "Duplicate Entry" errors.
When a call to getLineitems is called, it gets an access token with the scope https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.raedyOnly . Trucating this at 50 chars is ``.
Next, if you try and send a score, the following logic/issue happens:

  1. It needs a scope of https://purl.imsglobal.org/spec/lti-ags/scope/lineitem https://purl.imsglobal.org/spec/lti-ags/scope/score. It so it fetches a new access token.
  2. This token has a scope field of https://purl.imsglobal.org/spec/lti-ags/scope/lineitem https://purl.imsglobal.org/spec/lti-ags/scope/score.
  3. It tries to insert this into the DB
  4. The DB has a UNIQUE on platformUrl, clientId, scopes. But because the index truncates the cope to 50, these 2 different scopes are seen as the same:

Scope 1:
https://purl.imsglobal.org/spec/lti-ags/scope/lineitem https://purl.imsglobal.org/spec/lti-ags/scope/score

Scope 2:
https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readyOnly 

## ^^ Both truncate to:

https://purl.imsglobal.org/spec/lti-ags/scope/line

Suggested Fix

A new column named token_hash or access_token_hash that is an md5 of the 3 fields (platformUrl, clientId, scopes) and that is used for the unique check.

Steps to reproduce

  1. Setup LTIJS
  2. Create a route like /send-grade
  3. Use the following snippet. This basically gets line items and then pushes a score.
router.post('/send-grade', async(req, res) => {
    try {
        const idtoken = res.locals.token // IdToken
        const score = req.body.grade // User numeric score sent in the body
        // Creating Grade object
        const gradeObj = {
          userId: idtoken.user,
          scoreGiven: score,
          scoreMaximum: 100,
          activityProgress: 'Completed',
          gradingProgress: 'FullyGraded'
        }
    
        // Selecting linetItem ID
        let lineItemId = idtoken.platformContext.endpoint.lineitem // Attempting to retrieve it from idtoken
        console.log('Line Item ID: ', lineItemId);
        // This call makes an access token with a scope: https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readOnly
        let response = await lti.Grade.getLineItems(idtoken, { resourceLinkId: true });
        const lineItems = response.lineItems;
        console.log('Line items:', response.lineItems);
        // Get the SAMPLE lineitem.
        let lineItemInfo = lineItems.find(item => item.resourceid === 'SAMPLE');
        if (!lineItemInfo) {
            res.send('Unable to find the SAMPLE lineItem resourceid');
        }
        console.log('Submitting a score:');
        // Demo lineItemID url. You will need to locate yours.
        const lineItemID = lineItemInfo.id;
        // This wants an access token with the scope https://purl.imsglobal.org/spec/lti-ags/scope/lineitem https://purl.imsglobal.org/spec/lti-ags/scope/score 
        // But when this is inserted, the index truncates at 50. This is the error line.
        const responseGrade = await lti.Grade.submitScore(idtoken, lineItemID, gradeObj);
        return res.send(responseGrade);
    } catch(e) {
        console.log(e);
        return res.send('Error');
    }
});
@cxn-mkalinovits
Copy link

cxn-mkalinovits commented Jan 30, 2022

Hi,
I bumped into the same issue.
I use [email protected] with [email protected] on [email protected]

Initially I thought that MySQL has a size limitation of composite keys at 64 characters.
It quickly turned out the limit is way above that.

Later I found this ticket and the related PR as well.
I found the following code sequence, that seems to be the source of this issue:

accesstoken: this.#sequelize.define('accesstoken', {
        platformUrl: {
          type: Sequelize.TEXT
        },
        clientId: {
          type: Sequelize.TEXT
        },
        scopes: {
          type: Sequelize.TEXT
        },
        iv: {
          type: Sequelize.TEXT
        },
        data: {
          type: Sequelize.TEXT
        }
      }, {
        indexes: [{
          fields: [{
            attribute: 'platformUrl',
            length: 50
          }, {
            attribute: 'clientId',
            length: 50
          }, {
            attribute: 'scopes',
            length: 50
          }],
          unique: true
        }, {
          fields: ['createdAt']
        }]
      }),

I do not know this codebase enough, is it really needed to truncate the composite key's building blocks?

EDIT
I found the reason:

BLOB/TEXT column 'platformUrl' used in key specification without a key length
Still maybe 50 is quite small. According to MySQL documentation key size is 768+ bytes.

I suggest the following migration script to handle the issue:

ALTER TABLE `accesstokens` DROP INDEX `accesstokens_platform_url_client_id_scopes`;
ALTER TABLE `accesstokens` ADD UNIQUE INDEX `accesstokens_platform_url_client_id_scopes` (`platformUrl`(255), `clientId`(255), `scopes`(255));

Suggestion for fix: #19

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

2 participants