Site: Attach Database #2572

lukeed · 2019-04-26T08:12:03Z

Handle GitHub OAuth directly (no passport)
Create user records into a users table on PostgreSQL database
Handle client<->server authentication through JWT
Perform gists' CRUD operations thru database (no more GitHub gists)

All in all, this removes the flakiness of:

tunneling public/anonymous traffic through GitHub's free quota
AKA, at launch, so many people were getting (silent) rate-limit errors. No content appeared.
disappearing sessions, as the source of truth has moved to a long-lasting DB instance.
Currently, sessions get wiped on every new deployment & on new and/or dying GCR instances

I've also added a .env.example file to keep easier track of what's needed. Of course, NODE_ENV and PORT are optional, but still listed anyway.

Finally, I've already provisioned & migrated a DB instance for the website. The ENV vars are also already injected into GCR. Same goes for JWT configuration. Nothing else needs to be done for deployment if/when this is ready to go.

Caution: There's only a "production" DB, so launching a "staging" site will be connected to the same production database.

To run this locally, you need postgres installed. Then you need to create a new database and fill in the details as the DATABASE_URL key within .env file. Then you'll need to migrate/setup the database.

Full instructions may look like this:

# install postgres
$ brew install postgresql
$ brew services start postgresql

# create "svelte_demo" database
# this uses the system's current user, eg "lukeed"
$ createdb svelte_demo -U $(whoami)

# copy ".env.example" to ".env"
$ cp .env.example .env

# EDIT ".env" file~!

# setup db
$ npm install
$ npm run migrate up

# start sapper
$ npm run dev

Here's an example DATABASE_URL value for localhost:

# file: .env

DATABASE_URL=postgres://lukeed@127.0.0.1:5432/svelte_demo

These ENV keys must be filled in in order to have a working app:

DATABASE_URL
GITHUB_CLIENT_ID
GITHUB_CLIENT_SECRET
BASEURL
JWT_KEY

lukeed · 2019-04-26T08:13:54Z

site/src/routes/repl/index.svelte

-
-				repl.set({ components });
+				} else {
+					console.warn('TODO: 404 Gist')


Didn't see how to plug in to the REPL's error toasts

lukeed · 2019-04-26T08:15:01Z

site/src/user.js

+	const storageKey = 'svelte-dev:token';
+
+	// On load, get the last-known user token (if any)
+	// Note: We can skip this all by writing User data?


We already have the complete user object. We could write it to localStorage and have the user-data immediately available. If/when this is the case, we fetch the /auth/me.json endpoint which will wipe/reset if the token was expired or invalid.

lukeed · 2019-04-26T08:16:52Z

site/src/backend/auth.js

+export function toUser(obj={}) {
+	const { uid, username, name, avatar } = obj;
+	const token = sign({ uid, username });
+	return { uid, username, name, avatar, token };


This is the only user data that ever makes it to the client. The uid is GitHub's internal ID for the user. Not sure how "okay" it to keep this in localStorage.

Currently we don't. See comment above.

lukeed · 2019-04-30T15:13:23Z

This is ready to be looked at~

Conduitry · 2019-04-30T15:18:50Z

Without looking at anything yet, I guess my main concern is will this mess up people running the site locally and trying to load stuff from the database? When someone posts a gist as part of a bug report, as part of testing my fix I will often run the site locally and check the REPL with my local copy of Svelte to see if the thing is fixed. I can create the secret keys or whatever is needed in my .env, but that's an extra barrier to entry, and I also don't think we want to limit read access when running the site locally to people with trust with all of the secret Google Cloud keys.

lukeed · 2019-04-30T15:27:45Z

I would feel that's a very small edge case? Perhaps just you? lol

They'd still post a gist link that can be opened on the live site by anybody. There is a slightly greater barrier of entry for local development, but presumably anyone who's wanting to contribute to the site is already willing to cross that threshold anyway.

Rich-Harris · 2019-05-04T20:11:38Z

This looks 😎. Couple of thoughts before merging:

Should we call gists something else, to allay confusion? Maybe /gist/create should become /repl/create, etc, and the table could be repl instead of gists. (We could improve the URL scheme around REPLs more generally — for example it'd be nice if you could middle-click on the fork icon, etc — but that needn't be part of this work).
So that existing URLs continue to work, maybe it should fall back to querying the gist API if nothing is found in the DB, at least for now? (It could automatically create a DB entry at that point, though one wrinkle: the author of the gist might not exist in the DB yet)
Looking ahead, it's conceivable that we'd want to do something like attach a thumbnail to a specific ID. Do we need to factor that sort of thing into the DB design now, or do you think we can worry about that later?

Rich-Harris · 2019-05-04T21:11:36Z

If you get an error error: function gen_random_uuid() does not exist, run the following:

CREATE EXTENSION pgcrypto;

Rich-Harris · 2019-05-04T21:18:51Z

This is maybe a minor thing but I like the fact that you can double click a gist ID in the URL bar to select the whole thing. With gen_random_uuid() you get something like e6d592ce-54c8-4044-86bc-88ba0af6a493 which doesn't have that quality. Is there a version of that function that doesn't include hyphens?

Rich-Harris · 2019-05-04T21:26:42Z

oh wow, I just learned the hyphens are optional. We can just remove them from the URL. Don't mind me

lukeed · 2019-05-04T21:32:19Z

I would want to keep the URLs the same, personally. Though of course you can do anything.

2a) Sure, I would probably want to query GistHub for the gist on server's 404. IMO, the gists table entry should be added on their next "save" or "fork" action

2b) I don't recall the gist's data from GitHub API, but I think the account/owner is included. The only non-public (at least, I think it's not "public") information we need is the id value we're grabbing at login

It's no problem at all to update the table with future migrations
Postgres doesn't care about hyphens. That's a formatting operation for display. I don't think we can opt out of it in pg module directly, but we can have the API always send down values with val.replace(/-/g, '') and it should be fine.

Rich-Harris · 2019-05-04T21:44:09Z

Yeah, I just realised that the account info is included, so we could create user objects if necessary when the gist is fetched, and add a token later if that user logs in, if we updated the DB automatically when the gist was requested. I guess it's a question of whether it's worse to increase the risk hitting API rate limits (because of requesting the same gist repeatedly), or to fill up the database unnecessarily.

What's the argument for keeping the URLs the same, if we redirect the old to the new?

lukeed · 2019-05-04T21:47:46Z

Cool. We can create the gist immediately. Filling out our DB is very cheap & more forgiving than GH's rate limit.

Just, familiarity. Avoiding redirects is also nice. They're still "gists" – it's just that they live in our domain now

Rich-Harris · 2019-05-04T21:53:21Z

Cool. Working on this stuff atm — doing it in a separate branch off of this one in case any of it turns out controversial

lukeed · 2019-05-04T22:00:38Z

Ah okay. I was thinking I was assigned :P Am around if you wanna ping

Rich-Harris · 2019-05-04T22:05:06Z

Ha, I'm being a little selfish — just figured you might have better things to do and that it'd be a good opportunity for me to fully wrap my head round the new stuff

lukeed · 2019-05-04T22:13:55Z

Haha, agreed – good opportunity. Not a problem at all~

add database entries for new gists, update REPL URLs

Rich-Harris · 2019-05-05T04:56:57Z

I can't seem to deploy this... getting the following error:

ERROR: (gcloud.beta.run.deploy) Container failed to start. Failed to start and then listen on the port defined by the PORT environment variable. Logs for this revision might contain more information.

@lukeed any clue how to debug this? There doesn't seem to be anything useful in the logs, but I might be looking in the wrong place

lukeed · 2019-05-05T06:32:03Z

I ran it from the Docker image directly. Here was the message:

Error: Cannot find module 'yootils'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:668:15)
    at Function.Module._load (internal/modules/cjs/loader.js:591:27)
    at Module.require (internal/modules/cjs/loader.js:723:19)
    at require (internal/modules/cjs/helpers.js:14:16)
    at Object.<anonymous> (/app/__sapper__/build/server/index-4f8ee8d0.js:6:1)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)
    at Module.require (internal/modules/cjs/loader.js:723:19)

This was my fault. I removed it in this PR though am not sure where it's being used. Investigate later when not so tired.

In the meantime, I pushed up a fix to master directly am now redeploying the site.

lukeed added 18 commits April 25, 2019 13:45

fix: move shelljs to dev-deps

2b45963

fix: move codemirror to “external” config only

b2927e2

fix: move yootils to “external” config only

d7fdd2c

chore: bump “sirv” dep

a516923

chore: regenerate pkg-lock

b4be051

chore: add .env.example file

083c4d9

chore: remove previous dependencies

2eacc35

chore: add users migration

e8adc9c

feat: add db util w/ pool

d6f0b69

feat: handle GitHub OAuth directly, thru JWTs

f22c22c

fix: hydrate user data & update keys

fdf7963

fix: add timestamp columns to “users" table

5298f8e

feat: save new gists to database

f672e6a

feat: find & update gists

a337dd2

fix: update client for new endpoints

7cc22eb

fix: always send { error } shape

35485f9

fix: rename “users.token” => "users.github_token”

e3331ac

chore: include node-fetch as dev-dep

8fba0ec

lukeed requested review from Rich-Harris and Conduitry April 26, 2019 08:12

lukeed commented Apr 26, 2019

View reviewed changes

lukeed added 4 commits April 26, 2019 10:39

fix: remove extra DATABASE_URL key

6ae893f

Merge branch 'master' into site/database

b724a3d

fix: upload OAuth popup message

3732b9c

chore: regenerate lock file

67d3fb1

merge master -> database

2e6f9c4

add database entries for new gists, update REPL URLs

a4f7954

Rich-Harris mentioned this pull request May 4, 2019

add database entries for new gists, update REPL URLs #2680

Merged

10 tasks

Rich-Harris added 5 commits May 4, 2019 19:50

implement saving and forking

fd1b444

insert history entry when forking

c3857ab

add logic for relaxed gists

9795ee8

remove unnecessary on conflict clause

f6fd1e2

Merge pull request #2680 from sveltejs/site/database-with-fallback

d654b34

add database entries for new gists, update REPL URLs

Rich-Harris merged commit 1d0da9d into master May 5, 2019

Rich-Harris deleted the site/database branch May 5, 2019 04:33

Conduitry mentioned this pull request May 20, 2020

[site] /repl/embed doesn't work with gists #4866

Closed

ajesse11x mentioned this pull request Feb 18, 2021

[Snyk] Security upgrade prismjs from 1.16.0 to 1.23.0 ajesse11x/svelte#9

Open

Junoburger mentioned this pull request Feb 19, 2021

[Snyk] Security upgrade prismjs from 1.16.0 to 1.23.0 Junoburger/svelte#4

Open

snyk-bot mentioned this pull request Apr 29, 2021

[Snyk] Upgrade prismjs from 1.21.0 to 1.23.0 konlanz/svelte#4

Merged

tomoam mentioned this pull request Jun 16, 2021

REPL GitHub integrationを設定する svelte-jp/svelte-site-jp#84

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site: Attach Database #2572

Site: Attach Database #2572

lukeed commented Apr 26, 2019

lukeed Apr 26, 2019

lukeed Apr 26, 2019

lukeed Apr 26, 2019 •

edited

lukeed commented Apr 30, 2019

Conduitry commented Apr 30, 2019

lukeed commented Apr 30, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 5, 2019

lukeed commented May 5, 2019

Site: Attach Database #2572

Site: Attach Database #2572

Conversation

lukeed commented Apr 26, 2019

lukeed Apr 26, 2019

Choose a reason for hiding this comment

lukeed Apr 26, 2019

Choose a reason for hiding this comment

lukeed Apr 26, 2019 • edited

Choose a reason for hiding this comment

lukeed commented Apr 30, 2019

Conduitry commented Apr 30, 2019

lukeed commented Apr 30, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 4, 2019

lukeed commented May 4, 2019

Rich-Harris commented May 5, 2019

lukeed commented May 5, 2019

lukeed Apr 26, 2019 •

edited