Added path and method in to express request for passport

[fazilvp]

Summary

• This patch is for helping custom authentication strategy based on path and method. Presently we are not able to do it because these attributes are missing in request object. As per express documentation, an express request is supposed to have these fields.

  Method: https://expressjs.com/en/api.html#req.method
  Path: https://expressjs.com/en/api.html#req.path

  But this fields are not included while creating an express like request object for passport in
  @feathersjs/authentication hook.authenticate() function.

• Currently there are no open issues for this.

• This PR has no dependency to other PRs in other repos.

[regmish]

+1

[daffl]

Thank you for the PR and fixing the test. I believe the method will be the Feathers service method name though. Not sure if it needs to be normalized into something that Passport understands. The mapping would be:

const methodMap = {
  find: 'GET',
  get: 'GET',
  create: 'POST',
  update: 'PUT',
  patch: 'PATCH',
  remove: 'DELETE'
};

const httpMethod = methodMap[hook.method] || hook.method;

[fazilvp]

@daffl You are right. I have added the normalization. Will the same be applicable to path also? Because normally express path is starting with a front slash and feathers path is not. Please share your thoughts.

[subodhpareek18]

I don't think baking normalized method in is a good idea. At the least there should be the original feathers method somewhere.

find and get calls mean different things. And in my custom auth module I have different permissions for service:find and service:get.

And as far as path pased or method based strategies are concerned, can't they be done with hooks?

export const authorize = () =>
  iff(
    every(ctx => !ctx.path.startsWith('auth'), isProvider('external')),
    async ctx => {
      let userPermissions = R.path(
        ['params', 'user', 'role', 'permissions'],
        ctx
      );

      if (!userPermissions) {
        throw new errors.Forbidden('This user has no permissions');
      }

      if (userPermissions) {
        const hasPermissions = R.any(
          p => p === `${ctx.path}:${ctx.method}`,
          userPermissions
        );
        if (!hasPermissions) {
          throw new errors.Forbidden('Not allowed to execute this action.');
        }
      }

      return ctx;
    }
  );

[daffl]

@subodhpareek18 This change is for the authenticate hook which passes a fake request to the Passport strategy you indicated (usually jwt or local). This fake request object should be as close as possible to a normal request that a Passport strategy expects otherwise some may not work. Although I'm open to deprecating Passport (see discussion in #844) until then it makes sense to fake it as good as possible.

@fazilvp I think either way works, it depends what use-case prompted this PR.

[fazilvp]

@daffl Path without slash is perfectly OK for our use-case. Let me know if you think any other changes are needed in this PR.

[subodhpareek18]

@daffl that makes sense. Though I would still suggest the original feathersMethod can be passed without any normalization separately. But I guess that can always be done through middlewares for now. I'm having to do a similar thing for origin based auth.

The code below is for an old app on feathers@2.x.x though

export const rest = () => (req, res, next) => {
  req.feathers = {
    ...req.feathers,
    headers: req.headers,
    origin: req.get('origin')
  };
  next();
};

export const socketio = () => (socket, next) => {
  socket.feathers = {
    ...socket.feathers,
    headers: socket.handshake.headers,
    origin: socket.handshake.headers.origin
  };
  next();
};