Skip to content

Commit

Permalink
Remove undocumented audiences mechanism (#2540)
Browse files Browse the repository at this point in the history
  • Loading branch information
timleslie authored Mar 18, 2020
1 parent a8463bf commit b6a555c
Show file tree
Hide file tree
Showing 9 changed files with 21 additions and 65 deletions.
7 changes: 7 additions & 0 deletions .changeset/eighty-phones-drum.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@keystonejs/app-admin-ui': patch
'@keystonejs/auth-passport': patch
'@keystonejs/keystone': patch
---

Removed the undocumented `audiences` feature.
6 changes: 6 additions & 0 deletions .changeset/lovely-beds-carry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@keystonejs/session': major
'@keystonejs/app-graphql': patch
---

Removed the undocumented `restrictAudienceMiddleware` function.
6 changes: 1 addition & 5 deletions packages/app-admin-ui/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ class AdminUIApp {

return (
req.user &&
this._isAccessAllowed({ authentication: { item: req.user, listKey: req.authedListKey } }) &&
req.session.audiences &&
req.session.audiences.includes('admin')
this._isAccessAllowed({ authentication: { item: req.user, listKey: req.authedListKey } })
);
}

Expand All @@ -80,8 +78,6 @@ class AdminUIApp {
// Short-circuit GET requests when the user already signed in (avoids
// downloading UI bundle, doing a client side redirect, etc)
app.get(signinPath, (req, res, next) =>
// This session is currently authenticated as part of the 'admin'
// audience.
this.isAccessAllowed(req) ? res.redirect(this.adminPath) : next()
);

Expand Down
8 changes: 0 additions & 8 deletions packages/app-graphql/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
const express = require('express');
const { restrictAudienceMiddleware } = require('@keystonejs/session');
const { GraphQLPlaygroundApp } = require('@keystonejs/app-graphql-playground');
const { createApolloServer } = require('./lib/apolloServer');
const validation = require('./validation');
Expand All @@ -22,10 +21,6 @@ class GraphQLApp {
*/
prepareMiddleware({ keystone, dev }) {
const server = createApolloServer(keystone, this._apollo, this._schemaName, dev);
// GraphQL API always exists independent of any adminUI or Session
// settings We currently make the admin UI public. In the future we want
// to be able to restrict this to a limited audience, while setting up a
// separate public API with much stricter access control.
const apiPath = this._apiPath;
const graphiqlPath = this._graphiqlPath;
const app = express();
Expand All @@ -40,9 +35,6 @@ class GraphQLApp {

// { cors: false } - prevent ApolloServer from overriding Keystone's CORS configuration.
// https://www.apollographql.com/docs/apollo-server/api/apollo-server.html#ApolloServer-applyMiddleware
// This probably isn't the right place to put this restriction middleware. -TL
const restrict = restrictAudienceMiddleware({ isPublic: true });
app.use(apiPath, restrict);
app.use(server.getMiddleware({ path: apiPath, cors: false }));
return app;
}
Expand Down
3 changes: 0 additions & 3 deletions packages/auth-passport/lib/Passport.js
Original file line number Diff line number Diff line change
Expand Up @@ -506,12 +506,9 @@ class PassportAuthStrategy {
}

async _authenticateItem(item, accessToken, isNewItem, req, res, next) {
const audiences = ['admin'];

const token = await startAuthedSession(
req,
{ item, list: this._getList() },
audiences,
this._cookieSecret
);
this._onAuthenticated({ token, item, isNewItem }, req, res, next);
Expand Down
4 changes: 2 additions & 2 deletions packages/keystone/lib/Keystone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ module.exports = class Keystone {

return {
schemaName,
startAuthedSession: ({ item, list }, audiences) =>
startAuthedSession(req, { item, list }, audiences, this._cookieSecret),
startAuthedSession: ({ item, list }) =>
startAuthedSession(req, { item, list }, this._cookieSecret),
endAuthedSession: endAuthedSession.bind(null, req),
authedItem: req.user,
authedListKey: req.authedListKey,
Expand Down
7 changes: 1 addition & 6 deletions packages/keystone/lib/providers/listAuth.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,13 @@ class ListAuthProvider {
const gqlName = this.gqlNames.authenticateMutationName;
this.checkAccess(context, 'mutation', { gqlName });

// This is currently hard coded to enable authenticating with the admin UI.
// In the near future we will set up the admin-ui application and api to be
// non-public.
const audiences = ['admin'];

// Verify incoming details
const { item, success, message } = await this.authStrategy.validate(args);
if (!success) {
throw new Error(message);
}

const token = await context.startAuthedSession({ item, list: this.list }, audiences);
const token = await context.startAuthedSession({ item, list: this.list });
return { token, item };
}

Expand Down
14 changes: 2 additions & 12 deletions packages/session/index.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,3 @@
const {
commonSessionMiddleware,
restrictAudienceMiddleware,
startAuthedSession,
endAuthedSession,
} = require('./lib/session');
const { commonSessionMiddleware, startAuthedSession, endAuthedSession } = require('./lib/session');

module.exports = {
commonSessionMiddleware,
restrictAudienceMiddleware,
startAuthedSession,
endAuthedSession,
};
module.exports = { commonSessionMiddleware, startAuthedSession, endAuthedSession };
31 changes: 2 additions & 29 deletions packages/session/lib/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,39 +90,17 @@ function populateAuthedItemMiddleware(keystone) {
}
req.user = item;
req.authedListKey = list.key;
req.audiences = req.session.audiences;

next();
};
}

function restrictAudienceMiddleware({ isPublic, audiences }) {
return (req, res, next) => {
if (isPublic) {
// If the session restriction is marked public, we let everything through.
next();
} else if (
req.audiences &&
audiences &&
Array.isArray(audiences) &&
req.audiences.some(audience => audiences.includes(audience))
) {
// Otherwise, if one of the session audiences matches one of the restriction audiences, we let them through.
next();
} else {
// If the don't make it through, we simply respond with a 403 Permission Denied
res.status(403).send();
}
};
}

function startAuthedSession(req, { item, list }, audiences, cookieSecret) {
function startAuthedSession(req, { item, list }, cookieSecret) {
return new Promise((resolve, reject) =>
req.session.regenerate(err => {
if (err) return reject(err);
req.session.keystoneListKey = list.key;
req.session.keystoneItemId = item.id;
req.session.audiences = audiences;
resolve(cookieSignature.sign(req.session.id, cookieSecret));
})
);
Expand All @@ -137,9 +115,4 @@ function endAuthedSession(req) {
);
}

module.exports = {
commonSessionMiddleware,
restrictAudienceMiddleware,
startAuthedSession,
endAuthedSession,
};
module.exports = { commonSessionMiddleware, startAuthedSession, endAuthedSession };

0 comments on commit b6a555c

Please sign in to comment.