feat(auth): use split auth specs in app #692

njlie · 2022-10-25T21:52:03Z

Changes proposed in this pull request

Updates the AS to use the split openAPI specs.

Context

With the updated OpenAPI specs, this branch now suffers from #630
Also, should be updated to use #684 once it is merged in.

Checklist

Related issues linked using fixes #number
Tests added/updated
Documentation added
Make sure that all checks pass

wilsonianb · 2022-10-27T15:11:46Z

Also, should be updated to use #684 once it is merged in.

I vote this waits for #684

sabineschaller · 2022-11-03T12:08:51Z

@njlie Can you please finish that now that #684 has been merged? I want to see if this solves my response must be null issue when requesting a grant.

wilsonianb · 2022-11-03T14:46:15Z

@njlie Want to open a pull request in https://github.com/interledger/open-payments removing/fixing whatever access restriction are causing problems (maybe just use the uniqueItems/maxItems for now), and update this to use the specs on main?

njlie · 2022-11-03T22:09:24Z

@njlie Want to open a pull request in interledger/open-payments removing/fixing whatever access restriction are causing problems (maybe just use the uniqueItems/maxItems for now), and update this to use the specs on main?

Sounds good.

@wilsonianb interledger/open-payments#208

wilsonianb · 2022-11-03T22:53:25Z

packages/auth/package.json

@@ -11,7 +11,7 @@
    "build": "pnpm build:deps && tsc --build tsconfig.json",
    "clean": "rm -fr dist/",
    "fetch-schemas": "./scripts/get-op-schema.sh",
-    "test": "jest --passWithNoTests --maxWorkers=50%",
+    "test": "pnpm fetch-schemas && jest --passWithNoTests --maxWorkers=50%",


In the spirit of #684 (comment), I think we shouldn't cause a local dev to inadvertently nuke their local schemas.

wilsonianb · 2022-11-03T22:57:27Z

packages/auth/src/index.ts

@@ -104,7 +104,16 @@ export function initIocContainer(

  container.singleton('openApi', async (deps) => {
    const config = await deps.use('config')
-    return await createOpenAPI(config.authServerSpec)
+    const clientSpec = await createOpenAPI(config.authServerSpec)


What if get-op-schema.sh also fetched the auth server spec, and we just used the local file (remove config.authServerSpec) as with the other specs?
That way we ensure all the OpenAPI specs in auth are using the same version of the schemas.

wilsonianb · 2022-11-03T23:45:50Z

packages/auth/scripts/get-op-schema.sh

+curl -o "$OUTDIR/schemas.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/schemas.yaml
+curl -o "$OUTDIR/auth-server.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/auth-server.yaml


I think we should either pin to (current) head of main
or support an env var when running fetch-schemas, something like:

Suggested change

curl -o "$OUTDIR/schemas.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/schemas.yaml

curl -o "$OUTDIR/auth-server.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/auth-server.yaml

curl -o "$OUTDIR/schemas.yaml" https://raw.githubusercontent.com/interledger/open-payments/${OPEN_PAYMENTS_COMMIT:-main}/openapi/schemas.yaml

curl -o "$OUTDIR/client.yaml" https://raw.githubusercontent.com/interledger/open-payments/${OPEN_PAYMENTS_COMMIT:-main}/openapi/auth-server.yaml

wilsonianb · 2022-11-03T23:46:40Z

packages/auth/src/index.ts

@@ -103,8 +103,7 @@ export function initIocContainer(
  })

  container.singleton('openApi', async (deps) => {
-    const config = await deps.use('config')
-    const clientSpec = await createOpenAPI(config.authServerSpec)
+    const clientSpec = await createOpenAPI('./openapi/auth-server.yaml')


What if we rename the fetched AS spec to match the existing local specs:

Suggested change

const clientSpec = await createOpenAPI('./openapi/auth-server.yaml')

const clientSpec = await createOpenAPI('./openapi/client.yaml')

I feel compelled to use the original name since it's being pulled in from somewhere external. The association with that spec's externality seems stronger that way, and it's holding the actual GNAP implementation so I feel auth-server might be a better name anyways.

wilsonianb · 2022-11-04T00:19:13Z

.github/workflows/lint_test_build.yml

@@ -46,6 +46,7 @@ jobs:
      - uses: actions/checkout@v2
      - uses: ./.github/workflows/rafiki/env-setup
      - run: pnpm --filter openapi build
+      - run: pnpm --filter auth fetch-schemas


Actually, I wonder if fetch-schemas also needs to be run in the Dockerfile
https://github.com/interledger/rafiki/blob/main/packages/auth/Dockerfile#L18

Yeah, local env fails as is with:

local-peer-auth-1 | {"level":50,"time":1667568836819,"pid":1,"hostname":"f5cbb380f8b7","msg":"ResolverError: Error opening file \"/workspace/openapi/auth-server.yaml\" \nENOENT: no such file or directory, open '/workspace/openapi/auth-server.yaml'\n at ReadFileContext.callback (/workspace/node_modules/.pnpm/@[email protected]/node_modules/@apidevtools/json-schema-ref-parser/lib/resolvers/file.js:52:20)\n at FSReqCallback.readFileAfterOpen [as oncomplete] (node:fs:323:13)"}

I'm thinking instead of adding fetch-schemas to the Dockerfile, we just add it to https://github.com/interledger/rafiki#environment-setup
That way localenv can run with modified specs.

wilsonianb · 2022-11-04T13:50:00Z

.github/workflows/lint_test_build.yml

@@ -46,6 +46,7 @@ jobs:
      - uses: actions/checkout@v2
      - uses: ./.github/workflows/rafiki/env-setup
      - run: pnpm --filter openapi build
+      - run: pnpm --filter auth fetch-schemas


Yeah, local env fails as is with:

local-peer-auth-1 | {"level":50,"time":1667568836819,"pid":1,"hostname":"f5cbb380f8b7","msg":"ResolverError: Error opening file \"/workspace/openapi/auth-server.yaml\" \nENOENT: no such file or directory, open '/workspace/openapi/auth-server.yaml'\n at ReadFileContext.callback (/workspace/node_modules/.pnpm/@[email protected]/node_modules/@apidevtools/json-schema-ref-parser/lib/resolvers/file.js:52:20)\n at FSReqCallback.readFileAfterOpen [as oncomplete] (node:fs:323:13)"}

I'm thinking instead of adding fetch-schemas to the Dockerfile, we just add it to https://github.com/interledger/rafiki#environment-setup
That way localenv can run with modified specs.

wilsonianb · 2022-11-04T13:54:08Z

packages/auth/src/index.ts

+    const authServerSpec = await createOpenAPI('./openapi/auth-server.yaml')
+    const resourceServerSpec = await createOpenAPI(
+      './openapi/resource-server.yaml'
+    )
+    const idpSpec = await createOpenAPI('./openapi/id-provider.yaml')


auth build script needs to add these to the dist folder:

rafiki/packages/auth/package.json

Line 11 in 8898075

"build": "pnpm build:deps && tsc --build tsconfig.json",

See backend's copy-files:

rafiki/packages/backend/package.json

Lines 8 to 10 in 8898075

"build": "pnpm build:deps && tsc --build tsconfig.json && pnpm copy-files",

"clean": "rm -fr dist/",

"copy-files": "cp src/graphql/schema.graphql dist/graphql/",

(this is causing local env to fail even after running fetch-schemas)

wilsonianb · 2022-11-08T01:44:14Z

packages/auth/package.json

    "clean": "rm -fr dist/",
    "fetch-schemas": "./scripts/get-op-schema.sh",
    "test": "jest --passWithNoTests --maxWorkers=50%",
-    "prepack": "pnpm build"
+    "prepack": "pnpm build",
+    "copy-files": "pnpm fetch-schemas && cp -r ./openapi ./dist"


Suggested change

"copy-files": "pnpm fetch-schemas && cp -r ./openapi ./dist"

"copy-files": "cp -r ./openapi ./dist"

Otherwise copy-files & build nuke local modified specs.

wilsonianb

👍
local env works as long after running:

pnpm --filter auth fetch-schemas

github-actions bot added pkg: auth Changes in the GNAP auth package. type: source Changes business logic type: tests Testing related labels Oct 25, 2022

njlie force-pushed the nl-use-split-openapi branch from db2d126 to d68bd20 Compare October 26, 2022 19:23

sabineschaller requested a review from wilsonianb October 27, 2022 02:21

njlie force-pushed the nl-use-split-openapi branch from fd155a8 to 52547fb Compare November 2, 2022 00:01

njlie force-pushed the nl-use-split-openapi branch from 52547fb to da02c37 Compare November 3, 2022 21:16

wilsonianb reviewed Nov 3, 2022

View reviewed changes

github-actions bot added the type: ci Changes to the CI label Nov 3, 2022

wilsonianb reviewed Nov 3, 2022

View reviewed changes

njlie force-pushed the nl-use-split-openapi branch from d327912 to 21c3c1f Compare November 4, 2022 00:13

wilsonianb approved these changes Nov 4, 2022

View reviewed changes

wilsonianb reviewed Nov 4, 2022

View reviewed changes

wilsonianb requested changes Nov 4, 2022

View reviewed changes

wilsonianb mentioned this pull request Nov 4, 2022

chore: add validation for the OpenAPI specs to GitHub Actions #726

Merged

4 tasks

github-actions bot added the type: documentation label Nov 8, 2022

njlie requested a review from wilsonianb November 8, 2022 01:24

njlie force-pushed the nl-use-split-openapi branch from 983d511 to c7f1f8a Compare November 8, 2022 01:29

wilsonianb reviewed Nov 8, 2022

View reviewed changes

njlie requested a review from wilsonianb November 8, 2022 02:05

njlie added 6 commits November 7, 2022 18:06

feat(auth): use split auth specs in app

a71545f

feat: use less restrictive spec, fix tests

ddcdf54

chore: formatting

dabd913

feat: use split spec in new grant choice route handling

cea4e80

feat: change default idp port, use fixed OpenAPI specs

47ce825

feat: use local schemas

1f702e0

njlie added 8 commits November 7, 2022 18:06

chore: comments

0f0bf77

fix: fetch schemas before running tests

1709f87

feat: pull in auth spec and remove fetch-schemas from test command

12111c9

chore: remove unused param

7fb64af

feat: use new default mock idp port

ea957f6

feat: pin to commit on main

47e5c86

feat: add copy-files, better path for spec files

185dad2

fix: move openapi

00f7a8b

njlie force-pushed the nl-use-split-openapi branch from edd7d01 to 00f7a8b Compare November 8, 2022 02:06

wilsonianb approved these changes Nov 8, 2022

View reviewed changes

njlie merged commit d808c85 into main Nov 8, 2022

njlie deleted the nl-use-split-openapi branch November 8, 2022 02:46

wilsonianb mentioned this pull request Nov 10, 2022

Latest AS OpenAPI spec breaking auth middleware tests #630

Closed

wilsonianb mentioned this pull request Nov 30, 2022

chore(backend): update OpenAPI specs #799

Merged

4 tasks

huijing added the type: documentation (archived) Improvements or additions to documentation label Oct 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): use split auth specs in app #692

feat(auth): use split auth specs in app #692

njlie commented Oct 25, 2022 •

edited

Loading

wilsonianb commented Oct 27, 2022

sabineschaller commented Nov 3, 2022

wilsonianb commented Nov 3, 2022

njlie commented Nov 3, 2022 •

edited

Loading

wilsonianb Nov 3, 2022

wilsonianb Nov 3, 2022

wilsonianb Nov 3, 2022

wilsonianb Nov 3, 2022

njlie Nov 3, 2022

wilsonianb Nov 4, 2022

wilsonianb Nov 4, 2022

wilsonianb Nov 4, 2022

wilsonianb Nov 4, 2022

wilsonianb Nov 8, 2022

wilsonianb left a comment

		curl -o "$OUTDIR/schemas.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/schemas.yaml
		curl -o "$OUTDIR/auth-server.yaml" https://raw.githubusercontent.com/interledger/open-payments/main/openapi/auth-server.yaml

	const clientSpec = await createOpenAPI('./openapi/auth-server.yaml')
	const clientSpec = await createOpenAPI('./openapi/client.yaml')

	"build": "pnpm build:deps && tsc --build tsconfig.json && pnpm copy-files",
	"clean": "rm -fr dist/",
	"copy-files": "cp src/graphql/schema.graphql dist/graphql/",

	"copy-files": "pnpm fetch-schemas && cp -r ./openapi ./dist"
	"copy-files": "cp -r ./openapi ./dist"

feat(auth): use split auth specs in app #692

feat(auth): use split auth specs in app #692

Conversation

njlie commented Oct 25, 2022 • edited Loading

Changes proposed in this pull request

Context

Checklist

wilsonianb commented Oct 27, 2022

sabineschaller commented Nov 3, 2022

wilsonianb commented Nov 3, 2022

njlie commented Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilsonianb left a comment

Choose a reason for hiding this comment

njlie commented Oct 25, 2022 •

edited

Loading

njlie commented Nov 3, 2022 •

edited

Loading