feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.08k stars 750 forks source link

authenticate Hook throwing NotFound when executed after application level hook #2436

Open BahaMagi opened 3 years ago

BahaMagi commented 3 years ago

When having a security hook in place in app/before/all (thus being executed before the service level hooks) using an authenticate('jwt') hook in other services screwed up the auth hook for us.

User information is handled by a users service. Having several other services in place that need authentication as well as further query filtering. Hence we put a hook like this on app/before/all:


export const appLevelFilter= async (context: HookContext) => {
  if(context.type !== "before") throw new Error("Hook used in wrong context");

  // Skip this hook for authentication calls
  const whiteList = [context.app.service("authentication"), context.app.service("users")];
  if(whiteList.includes(context.service)) return context;

  const user: IUser = checkUser(context);   // Throw Error if context.params.user isn't set

  if(context.method === "find") {
    // Get passed query
    const query: Knex = context.params.knex || context.service.createQuery(context.params);

    /* do something with the query */   

    // Apply new query to current context
    context.params.knex = query;
  } else if(["create", "update"].includes(context.method)) {
    // reject by throwing or pass without altering the query
  } else if (context.method === "patch") {
    // reject by throwing or pass without altering the query
  } else { // get and remove
    // Do nothing
  }
  return context;
}

Steps to reproduce:

Having appLevelFilter execute before the auth hook leads to the GET call to the users service during the auth hook to be mixed up with the original service FIND call. Reversing the order of the hooks, i.e. having auth run first in a wrapper that excludes the auth call for services/methods we dont want it for (like the authentication service) solves the problem.

Sorry for not providing a repo that reproduces this. Hope the information provided is enough to figure out what might have caused this.

Expected behavior:

Authenticating that rejects un-authenticated queries but accepts authenticated ones.

Actual Behavior:

NotFound Error is thrown by the users service when searching for the current user, canceling the auth hook.

The data returned by the GET request to the users service is the data intended for the service that called the auth hook. We looked at what was returned here in index.js in the feathers-knex package and for the users service the data comes from the calling service (hence data.length > 1, hence the error throw).

_get (id, params = {}) {
    return this._findOrGet(id, params).then(data => {
      if (data.length !== 1) {
        throw new errors.NotFound(`No record found for id '${id}', ${this.fullName} Data: ${data && JSON.stringify(data)}`);
      }
      return data[0];
    }).catch(errorHandler);
  }

Configuration:

Relevant backend dependencies:

"dependencies": {
    "@feathersjs/authentication": "^5.0.0-pre.9",
    "@feathersjs/authentication-local": "^5.0.0-pre.9",
    "@feathersjs/authentication-oauth": "^5.0.0-pre.9",
    "@feathersjs/configuration": "^5.0.0-pre.9",
    "@feathersjs/errors": "^5.0.0-pre.9",
    "@feathersjs/express": "^5.0.0-pre.9",
    "@feathersjs/feathers": "^5.0.0-pre.9",
    "@feathersjs/socketio": "^5.0.0-pre.9",
    "@feathersjs/transport-commons": "^5.0.0-pre.9",
    "@types/bcryptjs": "^2.4.2",
    "@types/ejs": "^3.0.6",
    "@types/uuid": "^8.3.1",
    "bcryptjs": "^2.4.3",
    "compression": "^1.7.4",
    "cors": "^2.8.5",
    "cross-env": "^7.0.3",
    "date-fns": "^2.22.1",
    "dotenv": "^10.0.0",
    "ejs": "^3.1.6",
    "express-session": "^1.17.2",
    "feathers-hooks-common": "^5.0.6",
    "feathers-knex": "^7.1.1",
    "helmet": "^4.6.0",
    "knex": "^0.95.6",
    "ts-node": "^10.0.0",
    "typescript": "^4.3.4",
    "uuid": "^8.3.2",
    "winston": "^3.3.3"
  },

App config:

const app: Application = express(feathers());

export type HookContext<T = any> = { app: Application } & FeathersHookContext<T>;

// Load app configuration
app.configure(configuration());

const customSecurityDirectives: {[key: string]: string[]} = {
/* ... */
}
app.use(helmet({
  contentSecurityPolicy: {
    directives: customSecurityDirectives
  }
}));

app.use(cors({
  origin: '*',
  optionsSuccessStatus: 200 // For legacy browser support
}));

app.use(compress());
app.use(express.json());
app.use(express.urlencoded({ extended: true }));

// Host the public folder
app.use('/', express.static(app.get('public')));

// For zero downtime deploys
app.get("/_checks", (req: any, res: any) => {
  res.send("1337");
});

// Set up Plugins and providers
app.configure(express.rest());

app.configure(socketio());

app.configure(knex);

app.configure(authentication);
// Set up our services (see `services/index.ts`)
app.configure(services);
// Set up event channels (see channels.ts)
app.configure(channels);

// Configure a middleware for 404s and the error handler
app.use(express.notFound());
app.use(express.errorHandler({ logger } as any));

app.hooks(appHooks);
matiaslopezd commented 3 years ago

@BahaMagi in short I understand that you want to bypass the authenticate('jwt') hook when the service which needs to call has another strategy?

The best way to do this is to create exceptions or whitelist where the next authenticate('jwt') hook will not execute. So if I understand your question:

hooks/exceptionsServices.js

module.exports = function checkIsException() {
  const exceptions = ['login', 'logout', 'service-with-own-strategy']; // Or context.app.get('exceptionServices') to get from env vars
  return context => {
     const serviceName = context.path;
     return exceptions.includes(serviceName);
  }
}

In app.hooks.js

const { iff,  isProvider } = require('feathers-hooks-common');
const checkIsException = require('./hooks/exceptionsServices');

module.exports = {
  before: {
    all: [
      iff(isProvider('external'),
       iff(!checkIsException(), authenticate('jwt')) // <-- Pay attention to "!". The conditional will evaluate if it's not exception!!
      )
    ],
   ...
  },
  ...
};

With this solution, you will be able to add an authorization to all services but with the flexibility of do exceptions with certain services.

BahaMagi commented 3 years ago

Sorry, I guess my explanation was not easily understandable :x

The problem was, that if we applied a hook in app hooks to be run before any other hooks (that included the auth hook), the database call of the auth hook that fetches the user data for authentication gets messed up.

So in app.hooks.ts we had ( see my post above for details on appLevelFilter):

export default {
  before: {
    all:    [  appLevelFilter ],
    ...
  },

And in the other data services in datatservice.hooks.ts we had something like:

import { authenticate } from '@feathersjs/authentication/lib';

export default {
  before: {
    all:    [  authenticate('jwt') ],
    ...
  },

We expected the authentication to work as intended. However, we found out that the authenticate makes a GET request to the users service in order to fetch the authentication data. This request also passes the appLevelFilter which should not alter the request at all. However, that call is then loaded with the data of the original service call to dataservice not(!) with user data. This, in turn, causes the authentication to fail, despite the requesting user being authenticated.

The error is thrown in feathers-knex and were able to find the spot in index.ts:

_get (id, params = {}) {
    return this._findOrGet(id, params).then(data => {
      if (data.length !== 1) {
        throw new errors.NotFound(`No record found for id '${id}', ${this.fullName} Data: ${data && JSON.stringify(data)}`);
      }
      return data[0];
    }).catch(errorHandler);
  }

This error (extended by us for debugging) shows that a service call to the users services is receiving data from the dataservice. Sorry if this issue is better of placed in feathers-knex. Wasnt quite sure where to put it.

We solved it by pretty much doing what you suggested. We reversed the order of the hooks, and provided a white list to the auth hook and it works.

// app.hooks.ts
export default {
  before: {
    all:    [ authenticateJWTWrapper, appLevelFilter ],

Just did not understand why the other order did not work and suspecting a bug in either feathers-knex or the authenticate hook itself.