Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
920 stars 164 forks source link

TypeError: r is undefined; roles_common.js line 780 #311

Closed ggerber closed 4 years ago

ggerber commented 4 years ago

Hi,

I get the above TypeError.

To show what goes on I copied getRolesForUser and added some console.log statements

function getRolesForUserX(user, options) {
  console.log('user', user);
  console.log('options', options);
  let id;
  let selector;
  let filter;
  let roles;

  options = Roles._normalizeOptions(options);

  Roles._checkScopeName(options.scope);

  options = {
    fullObjects: false,
    onlyAssigned: false,
    anyScope: false,
    onlyScoped: false,
    ...options
  };

  if (user && typeof user === 'object') {
    id = user._id;
  } else {
    id = user;
  }

  if (!id) return [];

  selector = {
    'user._id': id
  };

  filter = {
    fields: {'inheritedRoles._id': 1}
  };

  if (!options.anyScope) {
    selector.scope = {$in: [options.scope]};

    if (!options.onlyScoped) {
      selector.scope.$in.push(null);
    }
  }

  if (options.onlyAssigned) {
    delete filter.fields['inheritedRoles._id'];
    filter.fields['role._id'] = 1;
  }

  if (options.fullObjects) {
    delete filter.fields;
  }

  roles = Meteor.roleAssignment.find(selector, filter).fetch();

  if (options.fullObjects) {
    return roles;
  }

  console.log('selector', selector);
  console.log('filter', filter);
  console.log('roles', roles);

  const x = roles.map(r => r.inheritedRoles || [r.role]);

  console.log('x>', x);

  const y = roles.map(r => r.inheritedRoles || [r.role]).reduce((rev, current) => rev.concat(current), []);

  console.log('y', y);

  return [
    ...new Set(
      roles
        .map(r => r.inheritedRoles || [r.role])
        .reduce((rev, current) => rev.concat(current), [])
        .map(r => r._id)
    )
  ];
}

The console.log statements are:

user TuzWxp58krZCDqGR9

options SamvgbLEh3WEDrBdp

selector {
  "user._id": "TuzWxp58krZCDqGR9",
  "scope": {
    "$in": [
      "SamvgbLEh3WEDrBdp",
      null
    ]
  }
}

filter {
  "fields": {
    "inheritedRoles._id": 1
  }
}

roles [
  {
    "_id": "xmWydv4ArwHv5wggF"
  }
]

x [
  [
    undefined
  ]
]

y [
  undefined
]

TypeError: r is undefined

A mongo console query gives

> db['role-assignment'].find({"user._id": "TuzWxp58krZCDqGR9","scope": {"$in": ["SamvgbLEh3WEDrBdp",null]}},{"inheritedRoles._id":1})
{ "_id" : "JdPQ8AZDndpnswthB", "inheritedRoles" : [ { "_id" : "control" } ] }
{ "_id" : "C3MYXcBRE5mdfzDJy", "inheritedRoles" : [ { "_id" : "edit" } ] }
{ "_id" : "xmWydv4ArwHv5wggF", "inheritedRoles" : [ { "_id" : "manage" } ] }
{ "_id" : "8C7mEENDLSvoewh9w", "inheritedRoles" : [ { "_id" : "view" } ] }

Could this be a latency issue, such that the subscription is not completely ready? Or does line 780 make invalid assumptions about the contents of the 'roles' array?

I use withTracker to check the signedin user's roles:

const withAccount = withTracker(() => {
  const user = Meteor.user();
  const loggingIn = Meteor.loggingIn();
  const verifiedEmail = user && 'emails' in user && user.emails[0].verified;

  const subscription = Meteor.subscribe('user.roles');
  const isloading = !subscription.ready();

  const account = {
    loggingIn,
    signedIn: !loggingIn && user && verifiedEmail,
    roles: {}
  };

  if (!isloading && populatedObject(user) && '_id' in user) {
    const {_id} = user;

    Roles.getScopesForUser(_id).forEach(group => {
      account.roles[group] = getRolesForUserX(_id, group);
    });
  }

  const resp = {
    account
  };
  console.log('resp', resp);
  return resp;
});

The publication looks so:

import {Meteor} from 'meteor/meteor';
import {populatedString} from '../utils/shared/types';

// publish all the roles of the signed-in user
Meteor.publish('user.roles', function userroles() {
  if (populatedString(this.userId)) {
    return Meteor.roleAssignment.find({'user._id': this.userId});
  }
  this.ready();
});
copleykj commented 4 years ago

@Meteor-Community-Packages/meteor-roles has anyone had time to look into this? I don't know the codebase well enough to respond properly.

copleykj commented 4 years ago

Could this be a latency issue, such that the subscription is not completely ready? Or does line 780 make invalid assumptions about the contents of the 'roles' array?

@ggerber Looking at your code for your withAccount hook, it seems like it might be possible that your subscription runs before there is a user, and therefore when the publish function runs it skips your if block and runs this.ready(). This could mean that your isLoading check returns true when you don't actually have any data and thus causing this error.

I can't say for sure that this is the cause without running your code live, but its a starting point.

alanning commented 4 years ago

Two issues, I think:

  1. Expected data not on client

Debug by removing the if (populatedString(this.userId)) check in the publish function and just always returning the find result. If the data is on the client then its the culprit as @copleykj hypothesized.

  1. Roles package should gracefully handle lack of data without throwing error

Would be great to have a reproduction for this.

SimonSimCity commented 4 years ago

I'll set aside some time in the coming days to dig into this ... related issue: https://github.com/Meteor-Community-Packages/meteor-roles/issues/308#issuecomment-566294899

ggerber commented 4 years ago

Hi,

Now I am no longer running into this issue. I am not sure what changed.

My publication is:

import {Meteor} from 'meteor/meteor';
import {populatedString} from '../utils/shared/types';

// publish all the roles of the signed-in user
Meteor.publish('user.roles', function userroles() {
  if (populatedString(this.userId)) {
    return Meteor.roleAssignment.find({'user._id': this.userId});
  }
  this.ready();
});

My container is:

const withAccount = withTracker(() => {
  const user = Meteor.user();
  const loggingIn = Meteor.loggingIn();
  const verifiedEmail = user && 'emails' in user && user.emails[0].verified;

  const subscription = Meteor.subscribe('user.roles');
  const isloading = !subscription.ready();

  const account = {
    loggingIn,
    signedIn: !loggingIn && populatedObject(user) && verifiedEmail,
    roles: {}
  };

  if (!isloading && populatedObject(user) && '_id' in user) {
    const {_id} = user;

    Roles.getScopesForUser(_id).forEach(group => {
      account.roles[group] = Roles.getRolesForUser(_id, group);
    });
  }

  const resp = {
    account
  };
  console.log('resp', resp);
  return resp;
});
copleykj commented 4 years ago

@ggerber just beware that this being solved now, without code modification, has a very good chance of being a race condition. The problem may resurface at random, especially in a production environment where network latency is likely to be quite a bit higher.

ggerber commented 4 years ago

Hi Kelly,

I plan to keep a keen eye on this thread and contribute if I see it re-occuring again.

Thanks

On Thu, 6 Feb 2020 at 13:57, Kelly Copley notifications@github.com wrote:

@ggerber https://github.com/ggerber just beware that this being solved now, without code modification, has a very good chance of being a race condition. The problem may resurface at random, especially in a production environment where network latency is likely to be quite a bit higher.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Meteor-Community-Packages/meteor-roles/issues/311?email_source=notifications&email_token=ACQLPZTI3QXZUBY44NO5KZ3RBP3KNA5CNFSM4KNURVC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK66UKA#issuecomment-582871592, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQLPZVKFMCOJID2JCLPHQLRBP3KNANCNFSM4KNURVCQ .

ggerber commented 4 years ago

Got the error again while I was using my app (which programmatically added a new scope to the current user).

Not sure if client console output is of any use...

The above error occurred in the <ForwardRef> component:
    in ForwardRef
    in ForwardRef
    in Suspense
    in ErrorBoundary

React will try to recreate this component tree from scratch using the error boundary you provided, ErrorBoundary. modules.js:26607:13
    logCapturedError modules.js:26607
    logError modules.js:26644
    callback modules.js:28032
    callCallback modules.js:18593
    commitUpdateEffects modules.js:18631
    commitUpdateQueue modules.js:18621
    commitLifeCycles modules.js:26899
    commitLayoutEffects modules.js:30108
    callCallback modules.js:5100
    invokeGuardedCallbackDev modules.js:5149
    invokeGuardedCallback modules.js:5204
    commitRootImpl modules.js:29846
    unstable_runWithPriority modules.js:33334
    runWithPriority$2 modules.js:16913
    commitRoot modules.js:29686
    finishSyncRender modules.js:29093
    performSyncWorkOnRoot modules.js:29071
    flushSyncCallbackQueueImpl modules.js:16963
    unstable_runWithPriority modules.js:33334
    runWithPriority$2 modules.js:16913
    flushSyncCallbackQueueImpl modules.js:16958
    flushSyncCallbackQueue modules.js:16946
    scheduleUpdateOnFiber modules.js:28473
    dispatchAction modules.js:21840
    tracked useTracker.js:86
    computation useTracker.js:129
    _compute tracker.js:308
    _recompute tracker.js:324
    _runFlush tracker.js:495
    onGlobalMessage meteor.js:515

TypeError: r is undefined roles_common.js:780:128
    getRolesForUser roles_common.js:780
    map self-hosted:251
    getRolesForUser roles_common.js:780
    withAccount account-context.js:30
    forEach self-hosted:235
    withAccount account-context.js:29
    data withTracker.jsx:10
    runReactiveFn useTracker.js:49
    tracked useTracker.js:79
    computation useTracker.js:129
    _compute tracker.js:308
    Computation tracker.js:206
    autorun tracker.js:576
    computation useTracker.js:128
    nonreactive tracker.js:603
    useTrackerClient useTracker.js:127
    updateMemo modules.js:21618
    useMemo modules.js:22098
    useMemo modules.js:2730
    useTrackerClient useTracker.js:118
    module useTracker.js:206
    WithTracker withTracker.jsx:10
    renderWithHooks modules.js:21024
    updateForwardRef modules.js:22907
    beginWork$1 modules.js:24975
    callCallback modules.js:5100
    invokeGuardedCallbackDev modules.js:5149
    invokeGuardedCallback modules.js:5204
    beginWork$$1 modules.js:30544
    performUnitOfWork modules.js:29462
    workLoopSync modules.js:29435
    performSyncWorkOnRoot modules.js:29034
    performSyncWorkOnRoot self-hosted:920
    flushSyncCallbackQueueImpl modules.js:16963
    unstable_runWithPriority modules.js:33334
    runWithPriority$2 modules.js:16913
    flushSyncCallbackQueueImpl modules.js:16958
    flushSyncCallbackQueue modules.js:16946
    scheduleUpdateOnFiber modules.js:28473
    dispatchAction modules.js:21840
    dispatchAction self-hosted:975
    tracked useTracker.js:86
    computation useTracker.js:129
    _compute tracker.js:308
    _recompute tracker.js:324
    _runFlush tracker.js:495
    onGlobalMessage meteor.js:515

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in _temp (created by ForwardRef)
    in ForwardRef
    in ForwardRef (created by Context.Consumer)
    in ErrorBoundary (created by Context.Consumer)
    in div (created by Context.Consumer)
    in Route (created by Context.Consumer)
    in Subscriber (created by App)
    in Switch (created by App)
    in Router (created by BrowserRouter)
    in BrowserRouter (created by App)
    in Suspense (created by App)
    in ErrorBoundary (created by App)
    in App
    in provider (created by ForwardRef)
    in ForwardRef
    in ForwardRef
    in Suspense modules.js:5294:32
    warningWithoutStack modules.js:5294
    warnAboutUpdateOnUnmountedFiberInDEV modules.js:30502
    scheduleUpdateOnFiber modules.js:28443
    enqueueSetState modules.js:18758
    setState modules.js:1412
    toggleAddModal system-list.jsx:135
    onSubmit system-add.jsx:58
    bindEnvironment meteor.js:1234
    _maybeInvokeCallback MethodInvoker.js:52
    dataVisible MethodInvoker.js:79
    _process_updated livedata_connection.js:1361
    _runAfterUpdateCallbacks livedata_connection.js:1227
    _runAfterUpdateCallbacks livedata_connection.js:1226
    _performWrites livedata_connection.js:1216
    _flushBufferedWrites livedata_connection.js:1167
    _livedata_data livedata_connection.js:1133
    onMessage livedata_connection.js:1663
    onmessage browser.js:186
    forEachCallback common.js:30
    onmessage browser.js:185
    dispatchEvent sockjs-0.3.4.js:87
    _dispatchMessage sockjs-0.3.4.js:1072
    _didMessage sockjs-0.3.4.js:1130
    onmessage sockjs-0.3.4.js:1277
SimonSimCity commented 4 years ago

@ggerber could you please provide us with a sample project where we can reproduce this error? I've not yet been able to reproduce it.

Just start a new Meteor project, make sure you have all the components needed to reproduce the error and upload it to e.g. github.

ggerber commented 4 years ago

At a conference at the moment, but will see what I can do, by end this week

On Tue, 18 Feb 2020, 8:30 AM Simon Schick, notifications@github.com wrote:

@ggerber https://github.com/ggerber could you please provide us with a sample project where we can reproduce this error? I've not yet been able to reproduce it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Meteor-Community-Packages/meteor-roles/issues/311?email_source=notifications&email_token=ACQLPZXS4HXMTTLKLFKU64TRDN6B3A5CNFSM4KNURVC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMAX73Q#issuecomment-587300846, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQLPZRP5QUBUUOIJDDRRPLRDN6B3ANCNFSM4KNURVCQ .

ggerber commented 4 years ago

Hi,

I made a repo: https://github.com/ggerber/roles_issue311

to start: npm run x

Then signin as: global@admin.com password: admin

Press the 'Add system' button Choose a name and press 'Save'

TypeError: r is undefined jumps out (most of the time, but not always)

SimonSimCity commented 4 years ago

Thanks a lot for providing the test scenario! I was now able to write a test for it and published it as version v3.2.1. Please reopen the ticket if it still persists.