Skip to content

Fix context passing in OWNER role resolver - #3230

Merged
bajtos merged 1 commit into
masterfrom
fix/context-passing-for-isOwner
Mar 3, 2017
Merged

Fix context passing in OWNER role resolver#3230
bajtos merged 1 commit into
masterfrom
fix/context-passing-for-isOwner

Conversation

@bajtos

Copy link
Copy Markdown
Member

Description

With the move to explicit context passing of an options object, internal calls to find etc need to be updated to pass the necessary information along, too. In this case, isOwner calls findById on a Model, but does not pass an access token in options.

cc @DaGaMs@ebarault

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

Comment threadcommon/models/role.js
Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, options, callback) {
if (!callback && typeof options === 'function') {
callback = options;
options = {};

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This branch is not covered by any test, coveralls reports "Coverage decreased (-0.04%) to 89.558%" as a result. I think we can ignore that, but please let me know if you disagree!

(The solution is to add a test that explicitly calls Role.isOwner(modelClass, modelId, userId, principalType, callback).)

@bajtos
bajtos merged commit 5ebc9b6 into masterMar 3, 2017
@bajtos
bajtos deleted the fix/context-passing-for-isOwner branch March 3, 2017 15:19
@bajtosbajtos removed the review label Mar 3, 2017
@ebarault

Copy link
Copy Markdown
Contributor

sorry to wake after the battle @bajtos
i see here that you fix the content of passed options to solely the accessContext (which i agree is the base configuration) when registering the $owner role resolver.
What if a user wants extra customization on top of that to get extra parameters in the passed options? Should he then override the Role model to specify his own way of registering this role resolver?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@bajtos@ebarault@DaGaMs