Add support for read-only mode for web UI. #407

lukegb · 2020-06-18T01:57:06Z

Fixes #402.

Adds the ability to attach an identity to the context for a particular repository.
Modifies the GraphQL handlers to use the identity from the context for making mutations, as well as for looking up the current user's identity.
Modifies the file upload handler to require an identity when receiving file uploads (I'm not refactoring this to use the GraphQL file upload functionality yet).
Adds an error handler to the JS which detects a particular GraphQL error extension which indicates that the server would like the client to reset its store and rerun any active queries.
Adds a new component to the JS, "IfLoggedIn", which renders its children iff there is an active user.
Adds a flag to the WebUI command (--read-only), which disables attaching the default identity to the context.

tazjin · 2020-06-18T02:05:19Z

Neat. This will still require adopting an identity, right?

~~Some brief experimentation in our other repo shows that it also needs a worktree, I wonder if we can get around that.~~

Update: It actually works fine!

tazjin · 2020-06-18T02:17:11Z

nix-build yields:

golang.org/x/text@v0.3.3: is explicitly required in go.mod, but vendor/modules.txt indicates golang.org/x/text@v0.3.2

Bumped to this fork in this CL and I still see the "Comment" UI, but the user display in the corner is gone.

lukegb · 2020-06-18T02:26:44Z

Repacked the web UI (oops) - should work now.

tazjin · 2020-06-18T02:46:18Z

Cool, that version works fine. I've deployed this on camden as a test, and I'm wondering whether the identity and bug caches might be an issue for keeping this as a long-running process?

A workaround might be using systemd to watch the git folder and restarting appropriately, which would lead to intermittent downtime.

webui/src/pages/bug/CommentForm.graphql

webui/src/pages/bug/Bug.tsx

misc/powershell_completion/git-bug

sandhose

apollo-client caches query results and batches them. For that reason, it does not make much sense to add a new context: just use the useCurrentIdentityQuery() hook whenever you need access to the user identity.

MichaelMure

So much activity around here, that's awesome!

1. Adds a config concept to the GraphQL API core, which at present only supports setting the ReadOnly bit. This could potentially end up with more things later.

I guess that works.

2. Adds a GraphQL mutation resolver which returns an error for every request, and uses this implementation if ReadOnly mode is enabled.

This won't do. There is actually 3 modes for the GraphQL server (I missed one in #402 (comment)):

read-only: no git-bug user (ie, the "read-only" mode, even though IMHO this concept should be segregated in the webui command, leaving only "what's the current user if any" concept in the server)
personal local use git-bug user is the default git-bug user of the git repo
web portal git-bug user is provided through OAuth or similar: the user from the repo is ignored, each request either has authentication in some way (token, cookie) or the webUI fallback to read-only

My understanding is that you only want 1 and 2, but it should be done in a way that allow 3 in the future. In particular, this means that the GraphQL request would carry user information or not. To do that, you can't replace the whole mutation resolver with a read-only one as this is done only once when the server start. Instead it should be a unique resolver that return an error if there is no user. Without changing anything to the resolver you will get that error so maybe leave it like that until we figure out how to do 3. properly ?

7. Disable Git file uploads in read-only mode.

Make sense to me. Eventually this should also support OAuth or even be migrated into the GraphQL API itself now that it's possible (https://gqlgen.com/reference/file-upload/).

I'll defer to @sandhose for whatever touch the JS code.

misc/powershell_completion/git-bug

MichaelMure · 2020-06-18T09:35:01Z

nix-build yields:

golang.org/x/text@v0.3.3: is explicitly required in go.mod, but vendor/modules.txt indicates golang.org/x/text@v0.3.2

Bumped to this fork in this CL and I still see the "Comment" UI, but the user display in the corner is gone.

This might be my fault, I just bumped this package with #405

MichaelMure · 2020-06-18T09:38:37Z

Cool, that version works fine. I've deployed this on camden as a test, and I'm wondering whether the identity and bug caches might be an issue for keeping this as a long-running process?

A workaround might be using systemd to watch the git folder and restarting appropriately, which would lead to intermittent downtime.

There is no memory management in the cache at the moment (see #132). If something is loaded it will stay there forever. That said I would be surprised if it cause actual problem as this is fairly lightweight data. Contribution welcome :)

lukegb · 2020-06-18T14:45:42Z

nix-build yields:

golang.org/x/text@v0.3.3: is explicitly required in go.mod, but vendor/modules.txt indicates golang.org/x/text@v0.3.2

Bumped to this fork in this CL and I still see the "Comment" UI, but the user display in the corner is gone.

This might be my fault, I just bumped this package with #405

I think we determined this was just down to some nix stuff we'd written, rather than anything actually in this repo.

My understanding is that you only want 1 and 2, but it should be done in a way that allow 3 in the future. In particular, this means that the GraphQL request would carry user information or not. To do that, you can't replace the whole mutation resolver with a read-only one as this is done only once when the server start. Instead it should be a unique resolver that return an error if there is no user. Without changing anything to the resolver you will get that error so maybe leave it like that until we figure out how to do 3. properly ?

Makes sense - I think it makes sense to attach the identity to the context in a middleware. That way we can attach "nil" in the read-only case, and attach an actual user if we're not read only, and it's obvious where the extension point is for decoding an identity once proxy support is added (which is also something @tazjin and I are interested in)

MichaelMure · 2020-06-18T14:52:47Z

FYI, there is an example of such a middleware on the gqlgen website: https://gqlgen.com/recipes/authentication/

lukegb · 2020-06-18T19:09:07Z

Made some changes - I've updated the PR description with what this actually does now.

MichaelMure

It's going the right way but I think there is still some things that should be improved to have a sound architecture for later. Thank you for your work, it's very helpful :)

identity/context.go

graphql/resolvers/mutation.go

graphql/resolvers/repo.go

graphql/resolvers/mutation.go

tazjin · 2020-06-18T23:46:19Z

There is no memory management in the cache at the moment (see #132). If something is loaded it will stay there forever. That said I would be surprised if it cause actual problem as this is fairly lightweight data.

I think the problem we're more worried about is whether it actually refreshes the running web UI if the folder it looks at is changing simultaneously. Our setup would most likely point the running process directly at a Gerrit repository on disk, which is the actual .git that pushes end up happening to. This is fairly easy to figure out though.

MichaelMure · 2020-06-19T00:14:55Z

I think the problem we're more worried about is whether it actually refreshes the running web UI if the folder it looks at is changing simultaneously. Our setup would most likely point the running process directly at a Gerrit repository on disk, which is the actual .git that pushes end up happening to. This is fairly easy to figure out though.

So you want to accept edit from both the webUI and git bug push at the same time. That make sense.

When the webUI or any other command run, the cache lock the repo, which prevent any other command to process. Any ... but a push as this is git itself, not git-bug. It works well for a personal usage: you can't alter the repo concurrently and you can't pull new changes while something else happen. This mechanism doesn't protect from pushes though, which is a problem in hosting situation.

The other problem is indeed that the cache is at the moment not aware that a push happen and won't invalidate or update the entities in memory or update the index.

Maybe a server-side git hook could solve that ? This hook would monitor git pushes, look if a git-bug entity has been updated, and inform the running process (the one started with webui) of that through an additional API endpoint. The cache would then perform the required invalidation.

lukegb · 2020-06-20T13:46:36Z

There is no memory management in the cache at the moment (see #132). If something is loaded it will stay there forever. That said I would be surprised if it cause actual problem as this is fairly lightweight data.

I think the problem we're more worried about is whether it actually refreshes the running web UI if the folder it looks at is changing simultaneously. Our setup would most likely point the running process directly at a Gerrit repository on disk, which is the actual .git that pushes end up happening to. This is fairly easy to figure out though.

I think we should move this discussion out of band rather than here; it's conflating this PR with a different feature request specific to our deployment :^)

MichaelMure · 2020-06-21T15:56:57Z

Moved to #409

MichaelMure · 2020-06-21T20:25:25Z

So, this PR made me realize that the API/webUI code was quite tangled. I went ahead and massaged that until it looked better.

Included in the changes:

create a new /api root package to hold all API code, migrate /graphql in there, with /http for the git handlers and /auth for the authentication middleware and context stuff
git API handlers now use the cache instead of the repo directly
git API handlers are now tested
git API handlers now require a "repo" mux parameter
lots of untangling of API/handlers/middleware
less code in commands/webui.go
things should now be in a better position to accept external auth

Hopefully I didn't break anything in the process.

Sorry for making this PR basically unreadable. Please give it a look and let me know if that looks good to you.

sandhose · 2020-06-23T18:36:58Z

webui/src/apollo.ts

+        if (extensions && extensions.resetStore) {
+          console.log(
+            'remote end returned error requesting that we reset the store'
+          );
+          client.resetStore();
+        }


This does not seem necessary and may lead to request loops. If the server asks to reset the store for a query, the client would reset the store, and therefore re-run the same query, which would make a loop.

Hmm. This is specifically to make sure the UI updates in response to being told "no, actually, you're not logged in"

@lukegb is that a problem you saw with the current UI code or is it something you expect to be a problem in the future? As @sandhose said, this can trigger a request loop.

It's mostly something I expect to be a problem once "proper" auth exists - I mostly ran into it while restarting the git-bug server between readonly and non-readonly mode, although this is a bit of an edge case.

Ok, well, let's cross that bridge when we get there then.

webui/src/layout/IfLoggedIn.tsx

webui/src/pages/bug/Bug.tsx

MichaelMure · 2020-06-26T09:22:55Z

I rebased on master and added @sandhose 's suggestions.

We now just need to resolve this debate.

Fixes MichaelMure#402.

Don't use contexts, just raw Apollo, since it's cached anyway. Change "ReadonlyHidden" to "IfLoggedIn".

…ieving them from a context.Context

Included in the changes: - create a new /api root package to hold all API code, migrate /graphql in there - git API handlers all use the cache instead of the repo directly - git API handlers are now tested - git API handlers now require a "repo" mux parameter - lots of untangling of API/handlers/middleware - less code in commands/webui.go

… rendering Co-authored-by: Quentin Gliech <quentingliech@gmail.com>

MichaelMure · 2020-06-27T21:09:18Z

All good now 👍

Thanks for the help, sorry it became so messy.

lukegb force-pushed the fix-402 branch 2 times, most recently from b880c57 to 2576866 Compare June 18, 2020 02:02

lukegb force-pushed the fix-402 branch from 2576866 to 59980b4 Compare June 18, 2020 02:08

tazjin reviewed Jun 18, 2020

View reviewed changes

webui/src/pages/bug/CommentForm.graphql Outdated Show resolved Hide resolved

tazjin reviewed Jun 18, 2020

View reviewed changes

webui/src/pages/bug/Bug.tsx Outdated Show resolved Hide resolved

tazjin reviewed Jun 18, 2020

View reviewed changes

misc/powershell_completion/git-bug Outdated Show resolved Hide resolved

sandhose requested changes Jun 18, 2020

View reviewed changes

MichaelMure requested changes Jun 18, 2020

View reviewed changes

misc/powershell_completion/git-bug Outdated Show resolved Hide resolved

lukegb requested review from MichaelMure and sandhose June 18, 2020 19:12

MichaelMure requested changes Jun 18, 2020

View reviewed changes

lukegb requested a review from MichaelMure June 18, 2020 23:28

MichaelMure mentioned this pull request Jun 21, 2020

cache update on push in hosting situation #409

Open

MichaelMure force-pushed the fix-402 branch from 8b01c1c to 64a30d4 Compare June 21, 2020 20:22

MichaelMure force-pushed the fix-402 branch 2 times, most recently from d1231bf to 3dbf2e0 Compare June 21, 2020 20:29

MichaelMure force-pushed the fix-402 branch from 3dbf2e0 to 9d2555f Compare June 21, 2020 20:34

sandhose requested changes Jun 23, 2020

View reviewed changes

MichaelMure force-pushed the fix-402 branch from 85235ee to 9be558c Compare June 26, 2020 09:17

lukegb and others added 11 commits June 27, 2020 22:56

Add support for read-only mode for web UI.

4a28f25

Fixes MichaelMure#402.

Don't permit file uploads in read-only mode

43c78da

Verify that we have an identity only in read-write mode

dfdb5e0

Refactor webui changes.

cf67c78

Don't use contexts, just raw Apollo, since it's cached anyway. Change "ReadonlyHidden" to "IfLoggedIn".

Add context.go to identity, used for attaching identities to and retr…

8a38897

…ieving them from a context.Context

Change graphql Go handlers to pluck identity out of context instead.

766aff2

Pull out context-stuff from identity into graphqlidentity package

e5a316e

Use ErrNotAuthenticated

5f72b04

webui: render component's children as a function to avoid uncecessary…

07d6c6a

… rendering Co-authored-by: Quentin Gliech <quentingliech@gmail.com>

webui: pack

3aaf775

MichaelMure force-pushed the fix-402 branch from 9be558c to 3aaf775 Compare June 27, 2020 21:05

MichaelMure merged commit c0dbc14 into MichaelMure:master Jun 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for read-only mode for web UI. #407

Add support for read-only mode for web UI. #407

lukegb commented Jun 18, 2020 •

edited

tazjin commented Jun 18, 2020 •

edited

tazjin commented Jun 18, 2020 •

edited

lukegb commented Jun 18, 2020

tazjin commented Jun 18, 2020

sandhose left a comment

MichaelMure left a comment •

edited

MichaelMure commented Jun 18, 2020

MichaelMure commented Jun 18, 2020

lukegb commented Jun 18, 2020

MichaelMure commented Jun 18, 2020

lukegb commented Jun 18, 2020

MichaelMure left a comment

tazjin commented Jun 18, 2020

MichaelMure commented Jun 19, 2020

lukegb commented Jun 20, 2020

MichaelMure commented Jun 21, 2020

MichaelMure commented Jun 21, 2020 •

edited

sandhose Jun 23, 2020

lukegb Jun 23, 2020

MichaelMure Jun 26, 2020

lukegb Jun 27, 2020

MichaelMure Jun 27, 2020

MichaelMure commented Jun 26, 2020

MichaelMure commented Jun 27, 2020

Add support for read-only mode for web UI. #407

Add support for read-only mode for web UI. #407

Conversation

lukegb commented Jun 18, 2020 • edited

tazjin commented Jun 18, 2020 • edited

tazjin commented Jun 18, 2020 • edited

lukegb commented Jun 18, 2020

tazjin commented Jun 18, 2020

sandhose left a comment

Choose a reason for hiding this comment

MichaelMure left a comment • edited

Choose a reason for hiding this comment

MichaelMure commented Jun 18, 2020

MichaelMure commented Jun 18, 2020

lukegb commented Jun 18, 2020

MichaelMure commented Jun 18, 2020

lukegb commented Jun 18, 2020

MichaelMure left a comment

Choose a reason for hiding this comment

tazjin commented Jun 18, 2020

MichaelMure commented Jun 19, 2020

lukegb commented Jun 20, 2020

MichaelMure commented Jun 21, 2020

MichaelMure commented Jun 21, 2020 • edited

sandhose Jun 23, 2020

Choose a reason for hiding this comment

lukegb Jun 23, 2020

Choose a reason for hiding this comment

MichaelMure Jun 26, 2020

Choose a reason for hiding this comment

lukegb Jun 27, 2020

Choose a reason for hiding this comment

MichaelMure Jun 27, 2020

Choose a reason for hiding this comment

MichaelMure commented Jun 26, 2020

MichaelMure commented Jun 27, 2020

lukegb commented Jun 18, 2020 •

edited

tazjin commented Jun 18, 2020 •

edited

tazjin commented Jun 18, 2020 •

edited

MichaelMure left a comment •

edited

MichaelMure commented Jun 21, 2020 •

edited