Add auth options 🎉 #58

Bouncey · 2018-04-23T22:05:52Z

Closes #37
Closes #83

raisedadead · 2018-04-24T05:33:36Z

Quick dumb query, how do I set a JWT for testing?

ojongerius · 2018-04-24T06:08:58Z

Nice one 🙇 I'll dedicated some time to review this evening.

Bouncey · 2018-04-24T08:42:51Z

My bad, it was late at night.

You can

Use one of my expired tokens:

eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6Ik0wTTJOVU0yTWpaR1FqUkVRelV4TVRFME9VTTVOVGcwUmtFeE1rTTJOek5GUTBRelJUTkVOZyJ9.eyJuaWNrbmFtZSI6InN0dWFydCIsIm5hbWUiOiJzdHVhcnRAZnJlZWNvZGVjYW1wLm9yZyIsInBpY3R1cmUiOiJodHRwczovL3MuZ3JhdmF0YXIuY29tL2F2YXRhci85ZjhmY2YzY2MzN2QzYWY4NDg0NDg3OTk4OTBmZGRmZT9zPTQ4MCZyPXBnJmQ9aHR0cHMlM0ElMkYlMkZjZG4uYXV0aDAuY29tJTJGYXZhdGFycyUyRnN0LnBuZyIsInVwZGF0ZWRfYXQiOiIyMDE4LTA0LTIyVDEwOjA5OjEzLjkzN1oiLCJlbWFpbCI6InN0dWFydEBmcmVlY29kZWNhbXAub3JnIiwiZW1haWxfdmVyaWZpZWQiOnRydWUsImlzcyI6Imh0dHBzOi8vZnJlZWNvZGVjYW1wLmF1dGgwLmNvbS8iLCJzdWIiOiJlbWFpbHw1YWNiMmVhZGE2ODAzYTkxOTEyODgwMTciLCJhdWQiOiJ2RjcwQ0paeVBLYlpSNHkwYXZlY1hYTGtmeU1ObnlLbiIsImlhdCI6MTUyNDM5MTc1MywiZXhwIjoxNTI0NDI3NzUzLCJhdF9oYXNoIjoiZDZsSW9Ra19uWkwxakxGWlFNN01EdyIsIm5vbmNlIjoiMkJWbVlWOWkzckllSFVJZzhzUU5WTDZtXy1MZTREejUifQ.nw5kbnpqCZRrJN3qr9coUAMNCRRg3Vrs7jhSqyX_BqGNcxZRA0ZYovM0ZNHQBioy_XH0bEeIoy59Bgjzcd-hNFJyoa_8pXRbhJ4tBMYkdK7K8_d8rGdDdSDg2EkTTeeidH8uAx9E14nYCWUJdqRlD1YXp3XkqQtBPLkD5Fu296ZnGrZ8hCjqz78bvYEg-dXYd3NsuuplnbFTjhjhHJ57KxBvk6_2Xvg-eeNx73xOJBsscyStv6eVvzQKMLvEin7SbV4FA4C0cKlKZl7YVj2qVxehojxkDupnSxFCDb59gkf4Z99eOx8qQOE4lI5c3gKuoZ3sIYDd1aRgHXAsvl8Dag

Change this line to

 return jwt.verify(token.replace('Bearer ', ''), cert, { ignoreExpiration: true });

Or generate your own by:

run events locally
log in
access id_token from your localStorage

So, you have a token

Add your token to the request headers 👇

Play around with the directives on the User type and query.

The Idea is:

if a field has the @isAuthenticatedOnField directive, the value will be null if they are not authenticated, but other information in the request will be available.
if a query has the @isAuthenticatedOnQuery directive, the request will error out if they are not authenticated.

I hope this helps in your QA @raisedadead @ojongerius 😀

ojongerius

Nice work @Bouncey!

graphql/resolvers/directives.js

+import { AuthorizationError } from '../errors';
+
+const log = debug('fcc:resolvers:directives');
+const certPath = process.cwd() + '/public.pem';


graphql/resolvers/directives.js

+  }
+  try {
+    return jwt.verify(token.replace('Bearer ', ''), cert);
+  } catch (err) {


graphql/resolvers/directives.js

+const certPath = process.cwd() + '/public.pem';
+const cert = fs.readFileSync(certPath);
+
+function isAuth(ctx) {


graphql/resolvers/directives.js

+  }
+};
+
+// Credit: agonbina https://github.com/apollographql/graphql-tools/issues/212


graphql/resolvers/directives.js

+import { asyncErrorHandler } from '../../utils';
+
+const log = debug('fcc:resolvers:directives');
+const certPath = process.cwd() + '/public.pem';


.gitignore

@@ -54,6 +54,10 @@ typings/
 # Yarn Integrity file
 .yarn-integrity

+# certificates


README.md

@@ -25,8 +25,11 @@ docker pull lambci/lambda # Pull Docker image used to simulate an AWS Lambda con

 ```sh
 cp sample.env .env
+cp public.pem.sample public.pem


README.md

 ```

+Add your public key to `public.pem`, instructions can be found in `public.pem`.


graphql/resolvers/directives.js

+const log = debug('fcc:resolvers:directives');
+const {
+  NODE_ENV,
+  STAGING_JWT_CERT: stagingCert,


graphql/resolvers/directives.js

+      return asyncErrorHandler(next());
+    }
+    throw new AuthorizationError({
+      message: `You are not authorized, ${error.message}`


public.pem.sample

@@ -0,0 +1,4 @@
+-----BEGIN CERTIFICATE-----


raisedadead

package-lock.json should be deleted.

I would recommend this just to be sure:

rm -rf node_modules package-lock.json
yarn

do the git thing!

package.json

@@ -23,7 +23,7 @@
    "lint": "eslint ./**/*.js --fix",
    "prepare-production": "snyk protect",
    "start":
-      "nodemon ./node_modules/serverless/bin/serverless offline start --skipCacheInvalidation",
+      "DEBUG='fcc:*' nodemon ./node_modules/serverless/bin/serverless offline start --skipCacheInvalidation",


ojongerius · 2018-04-26T03:00:14Z

@raisedadead are you happy for this to go in? Mind you this will need conflicts resolved and I hope my tests will start failing 🤪

ojongerius · 2018-04-26T03:14:20Z

▶ yarn test
yarn run v1.6.0
$ jest --runInBand
 FAIL  test/user.test.js
  ✕ should return a user after one has been created (74ms)

  ● should return a user after one has been created

    TypeError: Cannot read property '0' of null

      40 |
      41 |   /* eslint-disable no-undef */
    > 42 |   expect(data.users[0].name).toBe(user.name);
      43 |   /* eslint-enable no-undef */
      44 | });
      45 |

      at Object.<anonymous>.it (test/user.test.js:42:10)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        1.394s, estimated 2s
Ran all test suites.
error Command failed with exit code 1.

When having a gander at what we do get back I see:

    { errors:
       [ GraphQLError {
           message: 'Cannot read property \'headers\' of undefined',
           locations: [Array],
           path: [Array] } ],
      data: { users: null } }

Looks like the contents of data are all good, but the error object could to with some attention, and, as expected, we need to adjust these tests to validate requests with, and without authentication and authorisation behave as expected.

For the record, the same code using /api in a browser does return exactly what we want when not authenticated 👍

{
  "data": {
    "users": null
  },
  "errors": [
    {
      "message": "You must supply a JSON Web Token for authorization!",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "users"
      ]
    }
  ]
}

graphql/resolvers/user.js


 export const userResolvers = {
  Query: {
-    users: (_, args) => dbUsers.getUsers(args)
+    users: getUsers
  },
  Mutation: {
    createUser: (_, { email }) => dbUsers.createUser(email)


handler.js

@@ -15,12 +20,12 @@ const connectToDatabase = require('./dataLayer');

 const server = require('apollo-server-lambda');

-exports.graphqlHandler = function graphqlHandler(event, context, callback) {
+export async function graphqlHandler(event, context, callback) {


handler.js


-exports.apiHandler = lambdaPlayground({
+export const apiHandler = lambdaPlayground({


Bouncey · 2018-04-26T05:29:05Z

I did a git pull --rebase upstream staging which must have must have reverted things and I didn't notice. I'll untangle this mess when I get in to work

Bouncey · 2018-04-26T05:41:09Z

I will take a look at the test suite. We may need to spilt the current tests in to smaller units, as currently it is a bit end-to-end -ish. The errors headers tells me that context hasn't been set in the handler correctly, for example.

raisedadead · 2018-04-26T06:29:13Z

Hi @ojongerius @Bouncey I just rebase and force pushed staging after #69 was merged.

@ojongerius For all intents and purposes, its best to rebase an pr against staging, to avoid nasty merge commits. Or the escape hatch is to simple use squash commit button.

Bouncey · 2018-04-27T12:44:03Z

I want to see if I can get gulp to handle the tests due to the exporting of the JWT_CERT during testing

Bouncey · 2018-04-27T14:33:44Z

@raisedadead after rebasing and merging #83 into this PR I kept on getting this weird gulp error before I even touched the gulpfile

AssertionError [ERR_ASSERTION]: Task function must be specified

I have replaced what gulp was doing for us with cross-env. Can you please check everything is OK on your end using Windows?

raisedadead · 2018-04-27T14:44:36Z

Hi @Bouncey

My primary reason for having a task runner was to have the ability to offload stuff from package.json. We could potentially use tasks for deployment and release (github, npm) automation instead of having raw shell files and workarounds in the run scripts.

But, I am OK with removing gulp as long as we are moving forward for now.

Bouncey · 2018-04-27T15:25:13Z

Sweet!

This is now ready for QA @raisedadead @ojongerius

auth/index.js

+  let error = null;
+  try {
+    decoded = jwt.verify(token.replace('Bearer ', ''), JWT_CERT, {
+      ignoreExpiration: NODE_ENV === 'test'


raisedadead · 2018-04-28T07:00:47Z

Other than that everything else LGTM. Go to merge.

Side note, I have to lookup but the headers variable like authorization should be capitalised?

authorization => Authorization

As a practice else where?

Bouncey · 2018-04-28T07:07:13Z

The headers are passed to us from serverless through context. We have no control over that.

I was using Authorization in the playground headers, and they were always showing up as authorization in the handler context

raisedadead · 2018-04-28T07:12:17Z

Ok perfect.

Bouncey · 2018-04-28T07:24:07Z

Travis is failing on commitlint-travis. There is an open issue to relax these rules #54, all tests pass

Bouncey · 2018-04-28T07:24:28Z

Sweet!

Bouncey requested review from ojongerius and raisedadead April 23, 2018 22:05

Bouncey force-pushed the feat/auth branch from 38c192d to 2483eee Compare April 24, 2018 08:47

Bouncey mentioned this pull request Apr 24, 2018

Feat: Expand User type #55

Merged

ojongerius suggested changes Apr 24, 2018

View reviewed changes

Bouncey force-pushed the feat/auth branch from 2483eee to 06125f6 Compare April 24, 2018 11:17

Bouncey added the WiP Work in Progress, not ready for QA/merge label Apr 24, 2018

Bouncey commented Apr 24, 2018

View reviewed changes

graphql/resolvers/directives.js Outdated

import { asyncErrorHandler } from '../../utils';

const log = debug('fcc:resolvers:directives');

const certPath = process.cwd() + '/public.pem';

This comment was marked as off-topic.

Sign in to view

Bouncey removed the WiP Work in Progress, not ready for QA/merge label Apr 24, 2018

ojongerius suggested changes Apr 24, 2018

View reviewed changes

Bouncey force-pushed the feat/auth branch from 1dd4779 to da72fba Compare April 25, 2018 09:38

raisedadead suggested changes Apr 25, 2018

View reviewed changes

ojongerius approved these changes Apr 25, 2018

View reviewed changes

ojongerius suggested changes Apr 26, 2018

View reviewed changes

ojongerius mentioned this pull request Apr 27, 2018

feat: support authentication in tests #83

Closed

1 task

Bouncey added 6 commits April 27, 2018 13:39

feat(auth): add auth options 🎉

076cb5e

feat(tools): add debug

a2bb7ce

fix(directives): allow graphql-tools to handle directive resolvers

6119221

fix(auth-errors): communicate verification errors to the client

17f2be5

feat(jwt-cert): add cert to process.env

87311b7

chore(pem): remove references to .pem files

ecbdf67

Bouncey and others added 7 commits April 27, 2018 13:39

chore(dependencies): add new packages to yarn.lock

ebddc65

fix(jwt-cert): add jwt-cert to serverless.yml

46927bb

chore(tools): add jwt-cert to depoly script

892d8e1

feat(tools): add initial directive tests scaffolding

affa862

feat: support authentication in tests

f9362bc

chore(tools): refactor integration tests

ea41974

feat(tools): add directive tests

c83b051

Bouncey force-pushed the feat/auth branch from 68f16be to c83b051 Compare April 27, 2018 12:41

chore(tools): remove gulp in favour of cross-env

571a6c5

Bouncey force-pushed the feat/auth branch from 221fd7d to 571a6c5 Compare April 27, 2018 14:34

chore(deps): remove package-lock.json

db07ab0

raisedadead suggested changes Apr 28, 2018

View reviewed changes

auth/index.js Outdated

let error = null;

try {

decoded = jwt.verify(token.replace('Bearer ', ''), JWT_CERT, {

ignoreExpiration: NODE_ENV === 'test'

This comment was marked as off-topic.

Sign in to view

remove dev-only code

168fca4

chore(tools): fix for ci

8dd4715

raisedadead approved these changes Apr 28, 2018

View reviewed changes

raisedadead merged commit 2b9f1a0 into freeCodeCamp:staging Apr 28, 2018

Bouncey deleted the feat/auth branch April 28, 2018 07:24

ojongerius mentioned this pull request Apr 29, 2018

Create integration test env that handles auth protected routes correctly #81

Closed

Bouncey mentioned this pull request Apr 30, 2018

Authorization header casing #88

Merged

		```

		Add your public key to `public.pem`, instructions can be found in `public.pem`.


		exports.apiHandler = lambdaPlayground({
		export const apiHandler = lambdaPlayground({

Add auth options 🎉 #58

Add auth options 🎉 #58

Conversation

Bouncey commented Apr 23, 2018 • edited Loading

raisedadead commented Apr 24, 2018

ojongerius commented Apr 24, 2018

Bouncey commented Apr 24, 2018

You can

Use one of my expired tokens:

Or generate your own by:

So, you have a token

Add your token to the request headers 👇

ojongerius left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

raisedadead left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

ojongerius commented Apr 26, 2018

ojongerius commented Apr 26, 2018 • edited Loading

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Bouncey commented Apr 26, 2018

Bouncey commented Apr 26, 2018 • edited Loading

raisedadead commented Apr 26, 2018

Bouncey commented Apr 27, 2018

Bouncey commented Apr 27, 2018 • edited Loading

raisedadead commented Apr 27, 2018

Bouncey commented Apr 27, 2018

This comment was marked as off-topic.

raisedadead commented Apr 28, 2018

Bouncey commented Apr 28, 2018 • edited Loading

raisedadead commented Apr 28, 2018

Bouncey commented Apr 28, 2018

Bouncey commented Apr 28, 2018

Bouncey commented Apr 23, 2018 •

edited

Loading

ojongerius commented Apr 26, 2018 •

edited

Loading

Bouncey commented Apr 26, 2018 •

edited

Loading

Bouncey commented Apr 27, 2018 •

edited

Loading

Bouncey commented Apr 28, 2018 •

edited

Loading