Skip to content

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Jun 29, 2018

In the present implementation verifyUserRelations(Model) assumes that
Model.relations.accessTokens is always set, and as a result may
crash when trying to access polymorphic property of that relation.

It seems the intention is to check whether the the following conditions are met:

  1. a model has a hasMany accessTokens relation
  2. that relation is polymorphic

This commit fixes the problem by accounting for the case where the
accessTokens relation was not correctly set up.

This pull request supersedes #3921, which was opened against a wrong branch (3.x-latest).

/cc @ryanxwelch

In the present implementation `verifyUserRelations(Model)` assumes that
`Model.relations.accessTokens` is always set, and as a result may
crash when trying to access `polymorphic` property of that relation.

It seems the intention is to check whether the the following conditions
are met:
 1. a model has a hasMany accessTokens relation
 2. that relation is polymorphic

This commit fixes the problem by accounting for the case where the
accessTokens relation was not correctly set up.
@bajtos bajtos force-pushed the fix/crash-in-verifyUserRelations branch from 3c75260 to 9d77ba2 Compare June 29, 2018 14:14
@bajtos bajtos merged commit cd1b319 into master Jun 29, 2018
@bajtos bajtos deleted the fix/crash-in-verifyUserRelations branch June 29, 2018 15:18
@bajtos
Copy link
Member Author

bajtos commented Jun 29, 2018

Landed. Thank you @ryanxwelch for the contribution!

@mcitdev
Copy link
Contributor

mcitdev commented Jul 2, 2018

Hi @bajtos, I think that the current fix works because of the merge of the settings of the child and the parent, more about merge settings in http://apidocs.loopback.io/loopback-datasource-juggler/#modelbuilder .

IMHO, the old version of the function was more accurate, because, what is certain is that the polymorphic relation comes from the child model (the polymorphic relation will be configured in the child model), so it makes more sense to test the relations of the child.

I think what should be used in this fix is ​​:

function verifyUserRelations(Model) {
    const hasManyTokens = Model.relations && Model.relations.accessTokens;

    // display a temp warning message for users using multiple users config
    if (hasManyTokens && hasManyTokens.polymorphic) { //  <---- the only change that should be done
      console.warn(
        'The app configuration follows the multiple user models setup ' +
          'as described in http://ibm.biz/setup-loopback-auth',
        'The built-in role resolver $owner is not currently compatible ' +
          'with this configuration and should not be used in production.');
    }

I think that is the only thing that should be changed in the old verifyUserRelations function to fix the issue.

@bajtos
Copy link
Member Author

bajtos commented Jul 2, 2018

@mcitdev thank you for investigating this problem further. Could you please open a pull request making the change you are suggesting?

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

Successfully merging this pull request may close these issues.

4 participants