Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add auth middleware to Users service #2116

Merged
merged 5 commits into from
Apr 13, 2021

Conversation

humphd
Copy link
Contributor

@humphd humphd commented Apr 10, 2021

This adds authentication (makes sure the user has a valid JWT token) and authorization (makes sure their token matches what we expect per route) middleware from Satellite to the Users service. It depends on DevelopingSpace/satellite#12 for some changes to the isAuthorized() middleware in Satellite.

I'm adding middleware to all of the routes, but it's different in each case:

  • GET / this can be done only by an admin or another microservice (e.g., Parser service). A regular user can't request this, nor can an anonymous, unauthenticated user.
  • GET /:id this can be done by the user who owns the data (e.g., I can request my own data), or by an admin or another microservice. A user can't request someone else's data, nor can an anonymous, unauthenticated user.
  • POST /:id this can be done by the user who owns the data (e.g., I can register my own data), or by an admin or another microservice. A user can't register someone else's data, nor can an anonymous, unauthenticated user.
  • PUT /:id this can be done by the user who owns the data (e.g., I can update my own data), or by an admin or another microservice. A user can't update someone else's data, nor can an anonymous, unauthenticated user.
  • DELETE /:id this can be done by the user who owns the data (e.g., I can delete my own data), or by an admin or another microservice. A user can't delete someone else's data, nor can an anonymous, unauthenticated user.

I haven't written pure e2e tests here, which would also test things like trying to hit the various routes as different users. That would require the tests to be updated to full e2e tests first, so we'll have to come back to this.

A note about Satellite's middleware: a route must call isAuthenticated() before isAuthorized(), since the latter uses the parsed token created by the former.

The tests are going to fail until the Satellite fix is included.

@humphd
Copy link
Contributor Author

humphd commented Apr 11, 2021

Getting these tests to pass was more challenging than I'd expected, and has resulted in me having to fix a bunch of things in our docker setup and jest configs:

  • I added the jest-playwright eslint plugin, so I could use the Playwright globals (page, browser, context)
  • I shuffled around a bunch of environment variables in Docker, so things were defined properly in both development and production. We had some things only being set in production. I need each microservice to be able to create a service token, which means they need JWT info. I also simplified some things that don't need to be set multiple times (NODE_ENV, ports we don't change) or at all (APM stuff, until we do it). @manekenpix I'm hoping you can help me review this part especially, so that I don't break production.
  • I moved the details for how to switch Playwright from headless to having a UI to the jest-playwright-config.js, since I don't create the browsers in the tests anymore (you are supposed to let Playwright do it for you, apparently).
  • I reduced our test timeout to 10s from 30s, since it made failed tests on CI take forever to finish.
  • I changed the e2e tests to runInBand vs using parallelization, which should help things not get so bogged down.
  • I refactored src/api/auth/test/e2e/auth-flows.test.js to use Playwright's global page and context instead of creating them manually. This also results in all 3 browsers getting used vs. Chrome being used 3 times, like I was doing before.
  • I added a service token to the user creation logic in src/api/auth/test/e2e/auth-flows.test.js so it would work with the new authenticated routes.

birtony
birtony previously approved these changes Apr 12, 2021
Copy link
Contributor

@HyperTHD HyperTHD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. A few stuff and some questions

package.json Show resolved Hide resolved
src/api/users/test/e2e/users.test.js Show resolved Hide resolved
manekenpix
manekenpix previously approved these changes Apr 13, 2021
@humphd
Copy link
Contributor Author

humphd commented Apr 13, 2021

FAIL src/api/auth/test/token.test.js
  ● createToken() › should return a JWT with an issued at claim

    expect(received).toBeLessThan(expected)

    Expected: < 1618322319
    Received:   1618322319

      79 |     // The issued at time should be in the past
      80 |     const nowSeconds = Date.now() / 1000;
    > 81 |     expect(iat).toBeLessThan(nowSeconds);
         |                 ^
      82 |   });
      83 |
      84 |   it('should return a JWT without the picture claim, if picture not defined', () => {

      at Object.<anonymous> (src/api/auth/test/token.test.js:81:17)

I need to fix this in a follow-up.

@humphd humphd merged commit 4be2c9c into Seneca-CDOT:master Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: microservices area: satellite Issues related to the Satellite microservice project type: security Security concerns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants