feathersjs-ecosystem / feathers-hooks-common

Useful hooks for use with FeathersJS services.
https://hooks-common.feathersjs.com
MIT License
193 stars 89 forks source link

fastJoin and context.params.provider #398

Closed sxwebdev closed 6 years ago

sxwebdev commented 6 years ago

Hey. I have a trouble when I use fastJoin hook in my after hooks.

In this example: image image

This operation works great, and all data fetch from organizations service. But in my organization after hooks a have this code

image

The problem is hook.params.provider = undefined and discard not works. Clients get not protected data.

I try pass provider from context in user after hooks. But it did not help.

How to resolve this? Thanks

bertho-zero commented 6 years ago

You should add the provider to the find method call in your join resolver.

user.organization = (await context.app
  .service("organizations")
  .find({
    query: { _id: user.organization },
    paginate: false,
    provider: context.provider
  }))[0];
sxwebdev commented 6 years ago

You should add the provider to the find method call in your join resolver.

I did it.

image

and this does not work

image

protected data return to client

sxwebdev commented 6 years ago

If I console hook provider in organization service

image

I get undefined

image

bertho-zero commented 6 years ago

You should move your when hook on the service organizations, or discard organization.*

sxwebdev commented 6 years ago

You should move your when hook on the service organizations, or discard organization.*

like this?

image

this not helped

eddyystop commented 6 years ago

I'm catching up on this issue. The original post reads like the issue is that context.provider === undefined ?!?

That is normal when the service call is made on the server. Its not undefined only when the service call is made by a client.

You may want to copy the original context.provider to the calls made within fastJoin if the hooks of those fastJoin calls check context.provider. You can use the makeCallingParams utility function for this https://feathers-plus.github.io/v1/feathers-hooks-common/index.html#makecallingparams

eddyystop commented 6 years ago

If you expected your service call's context.provider !== undefined but it is, is the service call being made within another hook?

sxwebdev commented 6 years ago

If you expected your service call's context.provider !== undefined but it is, is the service call being made within another hook?

Hey. Thanks for feedback.

I pass context to after hook in users service, and call this from client. Like this.

image

Then client context pass to find method by fastJoin.

image

That is normal when the service call is made on the server. Its not undefined only when the service call is made by a client.

Organizations hooks must define client context.provider. But this is not so.

eddyystop commented 6 years ago

Why do you have ...context in (...context) => async (user, context) => ? I suggest you use different names for every context variable. Your problem may be that one context is "shadowing" another context and your code is not using the context you expect.

I'm also confused why you are using fastJoin(context => joinOrg) rather than just fastJoin(joinOrg). The latter seems to do the same as the former.

sxwebdev commented 6 years ago

This was not a problem, but I updated the code as you described above

image

and after hook

image

This did not help

bertho-zero commented 6 years ago

You can share your code like this:

```js
// your code
sxwebdev commented 6 years ago

You can share your code like this

ok) I did not know how to do this

sxwebdev commented 6 years ago

You can use the makeCallingParams utility function for this https://feathers-plus.github.io/v1/feathers-hooks-common/index.html#makecallingparams

const joinOrg = {
    joins: {
        organization: () => async (user,context) =>  {
            user.organization = (await context.app.service('organizations').find(
                makeCallingParams(context,{_id:user.organization},'user',{paginate:false,_populate: false})
            ))[0]
        }
    }
}

This did not help. Client receives unprotected data.

eddyystop commented 6 years ago

You have said fastJoin works as expected. So the issue is the when.

Write a custom hook that logs context.provider. Add it as the first and last hook in all. Also add it between every hook in all.

If context.provider is undefined at the start, then you rproblem is elsewhere.

bertho-zero commented 6 years ago

You need to discard organization.dadata or add the discard hook directly on the organization service.

sxwebdev commented 6 years ago

Write a custom hook that logs context.provider. Add it as the first and last hook in all. Also add it between every hook in all.

ok. I got a strange sequence:

image

1 - React application authenticate in feathersjs. 2 - React application authenticated and get user data. But organization all hook get undefined provider

sxwebdev commented 6 years ago

You need to discard organization.dadata or add the discard hook directly on the organization service.

If I completely remove dadata, it will not be available on the server or on the client.

sxwebdev commented 6 years ago

I do not understand why it is not here

image

I checked this over socketio and rest

image

sxwebdev commented 6 years ago

If I don`t use fastJoin and use this example. All works expected

after: {
    all: [
        //fastJoin(joinOrg),
        protect('password'),
        async hook => {
            if(hook.result.data && hook.result.data.length) {
                for(const i of hook.result.data.keys()) {
                    hook.result.data[i].organization = (await hook.app.service('organizations').find({query:{_id:hook.result.data[i].organization},paginate:false,provider:hook.params.provider}))[0];
                }
            } else if(hook.result._id) {
                hook.result.organization = (await hook.app.service('organizations').find({query:{_id:hook.result.organization},paginate:false,provider:hook.params.provider}))[0]
            }
            return hook;
        }
    ]
]

Client get protected data

sxwebdev commented 6 years ago
alterItems( (u,hook) => {
    u.organization = (async () => await hook.app.service('organizations').get(u.organization,hook))();
})

and this example also does not work

eddyystop commented 6 years ago

Please stop providing fragments of code. Provide the whole hook module. We can't guess what you are doing wrong from fragments.

I also asked the following:

Why do you have ...context in (...context) => async (user, context) => ? I suggest you use different 
names for every context variable. Your problem may be that one context is "shadowing" another context
and your code is not using the context you expect.

I'm also confused why you are using fastJoin(context => joinOrg) rather than just fastJoin(joinOrg).
The latter seems to do the same as the former.

Have you done that?

sxwebdev commented 6 years ago

Of course. I did it and showed you in this post https://github.com/feathers-plus/feathers-hooks-common/issues/398#issuecomment-394380052

eddyystop commented 6 years ago

Once again, past the whole module here.

sxwebdev commented 6 years ago

users hooks

const { authenticate } = require('@feathersjs/authentication').hooks;
const verifyHooks = require('feathers-authentication-management').hooks;
const { iff,isProvider,preventChanges,disallow,when,discard, fastJoin, alterItems } = require('feathers-hooks-common');
const accountService = require('../auth-management/notifier');
const isEnabled = require('../../hooks/is-enabled');

const {
  hashPassword, protect
} = require('@feathersjs/authentication-local').hooks;

const joinOrg = {
    joins: {
        organization: () => async (user,context) =>  {
            user.organization = (await context.app.service('organizations').find({
                query:{_id:user.organization},
                                paginate:false,
                                provider:context.params.provider
                            })
            ))[0]
        }
    }
}

module.exports = {
  before: {
    all: [],
    find: [ authenticate('jwt'), isEnabled() ],
    get: [ authenticate('jwt'), isEnabled() ],
    create: [
        hashPassword(),
        when(
            hook => !hook.data.verifyPhone && !hook.data.verifyPhoneToken && !hook.data.checkData,
            verifyHooks.addVerification()
        )
    ],
    update: [ disallow('external') ],
    patch: [ authenticate('jwt'),

        iff(isProvider('external'), preventChanges(true,
          'isVerified',
          'verifyToken',
          'verifyShortToken',
          'verifyExpires',
          'verifyChanges',
          'resetToken',
          'resetShortToken',
          'resetExpires'
        )),
    ],
    remove: [ authenticate('jwt') ]
  },

  after: {
    all: [
        fastJoin(joinOrg),
        protect('password'),
        when(
            hook => hook.params.provider,
            discard('password', '_computed', 'verifyExpires', 'resetExpires', 'verifyChanges', 'verifyToken', 'verifyShortToken')
        )
    ],
    find: [],
    get: [],
    create: [
        async hook => {
            if (!hook.params.provider) return hook;
            const user = hook.result;

            if(hook.data && hook.data.email && user && !hook.data.verifyPhone && !hook.data.verifyPhoneToken && !hook.data.checkData) {
                accountService(hook.app).notifier('resendVerifySignup', hook.result);
                return hook;
            }
        },
        verifyHooks.removeVerification()
    ],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};

organization hooks

const { authenticate } = require('@feathersjs/authentication').hooks;
const hooks = require('feathers-authentication-hooks');
const { discard,when,fastJoin, disallow, alterItems } = require('feathers-hooks-common');

const restrictToRoles = [
    hooks.restrictToRoles({
            roles: ['director'],
            fieldName: 'position'
        })
    ];

const joinContacts = {
    joins: {
        contacts: () => async (org,context) => org.contacts = await context.app.service('organization-contacts').find({
            query: { organization: org._id },
            paginate: false
        })
    }
}

module.exports = {
  before: {
    all: [ authenticate('jwt') ],
    find: [],
    get: [],
    create: [
        hooks.associateCurrentUser()
    ],
    update: [ disallow('external') ],
    patch: [
        async hook => {
            hook.data.updatedAt = Date.now();
        }
    ],
    remove: [ ...restrictToRoles ]
  },

  after: {
    all: [
        when(
            hook => hook.params.provider,
            discard('kfocus', 'atiData', 'userId', 'dadata', 'verifyChanges', 'adminEmail', 'telegram', 'type')
        ),
        alterItems( o => { o.management = !o.management ? {} : o.management } ),
        fastJoin(joinContacts),
    ],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
};
eddyystop commented 6 years ago

Initial comment is makeCallingParams is not defined in users hooks module.

sxwebdev commented 6 years ago

Initial comment is makeCallingParams is not defined in users hooks module.

Excuse me. I updated code. I reduced the code for testing and forgot to edit this line.

eddyystop commented 6 years ago

Could you run the fastJoin hook twice, one right after the other, and see if that makes a difference? Thanks.

sxwebdev commented 6 years ago

I tried it like you said. I Launched fastJoin hook twice and it did not help

all: [
        fastJoin(joinOrg),
        protect('password'),
        when(
            hook => hook.params.provider,
            discard('password', '_computed', 'verifyExpires', 'resetExpires', 'verifyChanges', 'verifyToken', 'verifyShortToken')
        ),
        fastJoin(joinOrg)
]
sxwebdev commented 6 years ago

So. How I solved this problem. A few days ago I saw a tweet about the spring feathersJS update. There are changes params.connection.

https://blog.feathersjs.com/feathersjs-spring-update-af493f656910#566c

params.connection The Socket.io and Primus transport adapters now add the connection object to every service method call as params.connection allowing to join, update or leave its channels (see this section for more information on managing channels). [changelog]

I tried this code in my users after all hook and it works as expected

hook => { hook.provider = hook.params.connection ? hook.params.connection.provider : hook.params.provider}

my joinOrg hook looks like this:

const joinOrg = {
    joins: {
        organization: () => async (user,context) =>  {
            user.organization = await context.app.service('organizations').get(user.organization)
        }
    }
}

Full code:

after: {
    all: [
        hook => { hook.provider = hook.params.connection ? hook.params.connection.provider : hook.params.provider},
        fastJoin(joinOrg),
        protect('password'),
        when(
            hook => hook.provider,
            discard('password', '_computed', 'verifyExpires', 'resetExpires', 'verifyChanges', 'verifyToken', 'verifyShortToken')
        )
    ]
}

I also want to say thank you for last release of hooks-common. alterItems now supports asynchronous functions. And this example from this issue works as expected https://github.com/feathers-plus/feathers-hooks-common/issues/398#issuecomment-394522470

But with few changes:

alterItems( async (u,hook) => {
    u.organization = await hook.app.service('organizations').get(u.organization,hook);
})

It follows that this problem lies in the core of the feathersJS. Thank you for helping me with this.

eddyystop commented 6 years ago

If I understand correctly, you may be saying context.params.provider may not exist when context.params.connection exists. Is this correct?

That would be affect so many hooks.

eddyystop commented 6 years ago

Frankly, sorry about alterItems. I forgot to merge it quite a while ago.

sxwebdev commented 6 years ago

If I understand correctly, you may be saying context.params.provider may not exist when context.params.connection exists. Is this correct?

When a client makes a request to the server via rest or socketio, context.params.connection contains the correct value of provider. context.params.provider contains an invalid value and = undefined.

eddyystop commented 6 years ago

This is a big bug. Could you please post an issue in @ feathersjs/feathers ?

daffl commented 6 years ago

It might be easier to track down with a complete example but I am not able to reproduce this issue (https://github.com/feathersjs/feathers/issues/884).

eddyystop commented 6 years ago

Such a bug would have caused issues with other hooks and I'd expect them to have been reporetd by now.

daffl commented 6 years ago

Agreed. It is possible to accidentally remove or reset context.params.provider but without a minimal breaking example I don't think we can reproduce what's happening.

eddyystop commented 6 years ago

I'm closing this issue since an example has yet to be provided. Please open another issue with the provided example as this one is getting too long.

Thanks.