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

Implement stale PR #286: Bradley/dev mode credentials #371

Merged

Conversation

l4u532
Copy link
Contributor

@l4u532 l4u532 commented Dec 11, 2023

Hey there, since the conversation in #286 had died down, I wanted to continue the work of @bradleyDean, since it is tremendously helpful for local db development.

What was that again?

  • Introduce GOD_MODE env var, that circumvents permission checks when querying/mutating the db
  • set GOD-MODE via yarn serve-dev

I added a switch in permissions.ts to the original code, since otherwise queries/mutations via GraphQL would return unauthorised.

@l4u532 l4u532 force-pushed the l4u532/fix_bradley/dev-mode-credentials branch 5 times, most recently from 25f5c5d to d3c2bb3 Compare December 11, 2023 10:50
@l4u532 l4u532 marked this pull request as ready for review December 11, 2023 10:54
@l4u532
Copy link
Contributor Author

l4u532 commented Dec 11, 2023

Tests currently fail because

Run docker/login-action@v2
Error: Username and password required

Issue raised: #372

@enapupe
Copy link
Contributor

enapupe commented Dec 12, 2023

Hey, I'm just spitballing here, I'm not fully aware of the requirements for this and I'm not even familiar with the auth system currently implemented:
wouldn't it be easier if we just had a "superuser" (which is an actual user) rather than having many checks/ifs/elses checking for "god mode"? Instead it would just "force" login with this previously set user that has access to everything. The logic would be kept tight and less sparse.
Of course this user could be activated by setting that env, this bit would not change much.
I hope my point is not far from reality 😅

@l4u532
Copy link
Contributor Author

l4u532 commented Dec 12, 2023

Hey, I'm just spitballing here, I'm not fully aware of the requirements for this and I'm not even familiar with the auth system currently implemented:

wouldn't it be easier if we just had a "superuser" (which is an actual user) rather than having many checks/ifs/elses checking for "god mode"? Instead it would just "force" login with this previously set user that has access to everything. The logic would be kept tight and less sparse.

Of course this user could be activated by setting that env, this bit would not change much.

I hope my point is not far from reality 😅

When querying the database via the GraphQL endpoint, some kind of Auth is required (Token). I assume re-wiring that logic would be equally complex? Anyway, would he happy to see a more parsimonious solution to this -- once we get this baby here merged ;)

@vnugent vnugent mentioned this pull request Dec 12, 2023
@vnugent
Copy link
Contributor

vnugent commented Dec 12, 2023

Background:

  1. Most read queries don't require a JWT token.
  2. Write/update/delete require a JWT token. For verifying JWT/checking user roles, we use https://github.com/dimatill/graphql-shield
  3. JWT payload contains additional params for the API, eg: uuid of the climb to be deleted or user uuid of the account to be updated, uuid of the person doing the update (preventing one user from updating other user's profile.

I'm all for improving the developer experience. My only concern is the current PR introducing more complexity to the auth layer. How about

  1. When god_mode == true, programmatically add a hard-coded JWT token to the request
  2. Bypass the verifyJWT call here: https://github.com/OpenBeta/openbeta-graphql/blob/develop/src/auth/middleware.ts#L24

One of the pros of this approach is the auth code path in test or prod is nearly identical. A con is we need to include the right data in the JWT payload.

What are your thoughts?

@l4u532 l4u532 force-pushed the l4u532/fix_bradley/dev-mode-credentials branch from d3c2bb3 to 5681867 Compare December 12, 2023 19:22
@l4u532
Copy link
Contributor Author

l4u532 commented Dec 12, 2023

@vnugent Checks are passing ✅. Regarding your proposal: I am a bit on the fence, since I'd have to start over 😄 but I do agree that the auth layer should be as clean as possible. I'll try my luck next week. Thanks for the suggestion.

@Silthus
Copy link
Contributor

Silthus commented Dec 13, 2023

Wasn't the original PR doing that? Bypassing the JWT Validation with a static user?

I thought that did not work and didn't get merged. Also I don't like the idea of having to statically include a JWT in every request I do against localhost.

How about the following:

  1. The code of this PR gets extracted into its own middleware and permission rule files. This way @l4u532 does not loose his progress, and everything is nicely encapsulated.
  2. The middlewares are conditionally swapped out in the server initialization based on a env var BYPASS_AUTH=true (I don't like non speaking names like god mode).
  3. The current random uuid of the test user is replaced with the static uuid of: 00000000-0000-0000-0000-000000000001. This way the test user is immediately recognizable when inspecting the database.

What do you think?

@vnugent
Copy link
Contributor

vnugent commented Dec 13, 2023

To help @l4u532 move forward perhaps we can address two issues at hand separately:

  1. This PR, how to best enable superuser mode.
    I like @Silthus "plug-and-play" proposal. This way we don't have to deal with a bunch of branches in the auth code.
  2. How to test GQL mutations / protected endpoints in development in general.
    Until we resolve 1, I know it's an extra step / inconvenience to copy your API key from the UI and add it to the authorization header. The key is good for 30 days.

Screenshot 2023-12-13 at 8 31 25 AM

@l4u532 l4u532 force-pushed the l4u532/fix_bradley/dev-mode-credentials branch 2 times, most recently from 9b4ee77 to 2f3edee Compare December 19, 2023 16:07
@l4u532 l4u532 force-pushed the l4u532/fix_bradley/dev-mode-credentials branch from 2f3edee to 6846874 Compare December 19, 2023 16:12
@l4u532
Copy link
Contributor Author

l4u532 commented Dec 19, 2023

@vnugent reworked it according to @Silthus proposal. What do you think?

@vnugent
Copy link
Contributor

vnugent commented Dec 23, 2023

@l4u532 Thanks! Can you add a follow up PR, documenting the new server start cmd?

@vnugent vnugent merged commit c35e218 into OpenBeta:develop Dec 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants