feat: auth service-to-service api #3148

BlairCurrey · 2024-12-03T18:47:42Z

Changes proposed in this pull request

Adds a new ServiceAPIServer to auth
Adds new tenant CRUD routes to ServiceAPIServer
Adds new authServiceAPIClient to backend which is used by tenant service.

Context

fixes: #3125
Opened interledger/helm-charts#47 to follow up on the env vars in the helm charts

Checklist

Related issues linked using fixes #number
Tests added/updated
Make sure that all checks pass
Bruno collection updated (if necessary)
Documentation issue created with user-docs label (if necessary)
OpenAPI specs updated (if necessary)

BlairCurrey · 2024-12-05T03:00:14Z

packages/backend/src/auth-service-client/client.ts

+  private async request<T = any>(
+    path: string,
+    options: RequestInit
+  ): Promise<T> {
+    const response = await fetch(`${this.baseUrl}${path}`, options)
+
+    if (!response.ok) {
+      let errorDetails
+      try {
+        errorDetails = await response.json()
+      } catch {
+        errorDetails = { message: response.statusText }
+      }
+
+      throw new AuthServiceClientError(
+        `Auth Service Client Error: ${response.status} ${response.statusText}`,
+        response.status,
+        errorDetails
+      )
+    }
+
+    if (
+      response.status === 204 ||
+      response.headers.get('Content-Length') === '0'
+    ) {
+      return undefined as T
+    }
+
+    const contentType = response.headers.get('Content-Type')
+    if (contentType && contentType.includes('application/json')) {
+      try {
+        return (await response.json()) as T
+      } catch (error) {
+        throw new AuthServiceClientError(
+          `Failed to parse JSON response from ${path}`,
+          response.status
+        )
+      }
+    }
+
+    return (await response.text()) as T
+  }


I opted to use the native fetch here although there were some behaviors I wasnt quite sure about. I tested with axios and mirrored how that works:

not OK response (400, 500) etc. throws

204 returns undefined

Success without body (200, 201, etc.) returns the response.text (just '' if not set)

Interface accepts a generic which asserts the return type, although technically it could still return a string or undefined in the cases described above.

BlairCurrey · 2024-12-05T03:02:52Z

packages/auth/src/tenant/routes.ts

+async function createTenant(
+  deps: ServiceDependencies,
+  ctx: CreateContext
+): Promise<void> {
+  const { body } = ctx.request
+
+  await deps.tenantService.create(body)
+
+  ctx.status = 204
+}


@mkurapov influenced by the pattern we've been talking about in the POC meetings. Not 201 with the resource returned. Originally I went with 201 but that really should have a body: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201

thus the 204 No Content for this and the update and delete.

BlairCurrey · 2024-12-05T03:03:36Z

packages/auth/src/tenant/routes.ts

+function toTenantResponse(tenant: Tenant): TenantResponse {
+  return {
+    id: tenant.id,
+    idpConsentUrl: tenant.idpConsentUrl,
+    idpSecret: tenant.idpSecret
+  }
+}


to filter out the created, deleted, update timestamps basically. Figured we'd keep those in the return from the service layer and filter out here. Although I considered not returning them from the service since in theory maybe those are purely business logic concerns? Kinda academic at this point though.

BlairCurrey · 2024-12-05T03:05:22Z

packages/backend/src/auth-service-client/client.ts

+  public tenant = {
+    get: (id: string) =>
+      this.request<Tenant>(`/tenant/${id}`, { method: 'GET' }),
+    create: (data: Omit<Tenant, 'id'>) =>
+      this.request('/tenant', {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify(data)
+      }),
+    update: (id: string, data: Partial<Omit<Tenant, 'id'>>) =>
+      this.request(`/tenant/${id}`, {
+        method: 'PATCH',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify(data)
+      }),
+    delete: (id: string) => this.request(`/tenant/${id}`, { method: 'DELETE' })
+  }


could probably move these methods and the Tenant interface out of this file if we want but opted to just put it here since it's really the only thing we need for the forseeable future.

BlairCurrey · 2024-12-05T03:08:41Z

packages/backend/src/auth-service-client/client.ts

+  }
+}
+
+export class AuthServiceClient {


Almost feels like this should be a different package outside of backend but this is the only consumer at this point so I felt like this keeps it simpler.

…e-api

BlairCurrey · 2024-12-20T19:26:24Z

Not able to add an integration test yet because the backend doesnt have any endpoints that uses the backend tenant service. However, I did do some manual integration tests by adding this to the start function the backend/src/index.ts and found everything to be working as expected:

  // Test the tenant stuff. Comment out client methods and inspect db as needed to confirm.
  const authServiceClient = container.use('authServiceClient')
  const tenant = await authServiceClient.tenant.get(config.operatorTenantId)
  console.log({ tenant }) // OK

  const id = '438fa74a-fa7d-4317-9ced-dde32ece1788'

  await authServiceClient.tenant.create({
    // OK
    id,
    idpConsentUrl: 'http://localhost:3031/mock-idp2/',
    idpSecret: '3pEcn2kkCclbOHQiGNEwhJ0rucATZhrA807HTm2rNXE='
  })

  await authServiceClient.tenant.update(id, {
    // OK
    idpConsentUrl: 'http://localhost:3031/mock-idp3/'
  })

  const deletedAt = new Date('2024-12-20T18:16:02.019Z')
  await authServiceClient.tenant.delete(id, deletedAt) // OK

njlie · 2025-01-07T19:35:33Z

Not able to add an integration test yet because the backend doesnt have any endpoints that uses the backend tenant service. However, I did do some manual integration tests by adding this to the start function the backend/src/index.ts and found everything to be working as expected:

  // Test the tenant stuff. Comment out client methods and inspect db as needed to confirm.
  const authServiceClient = container.use('authServiceClient')
  const tenant = await authServiceClient.tenant.get(config.operatorTenantId)
  console.log({ tenant }) // OK

  const id = '438fa74a-fa7d-4317-9ced-dde32ece1788'

  await authServiceClient.tenant.create({
    // OK
    id,
    idpConsentUrl: 'http://localhost:3031/mock-idp2/',
    idpSecret: '3pEcn2kkCclbOHQiGNEwhJ0rucATZhrA807HTm2rNXE='
  })

  await authServiceClient.tenant.update(id, {
    // OK
    idpConsentUrl: 'http://localhost:3031/mock-idp3/'
  })

  const deletedAt = new Date('2024-12-20T18:16:02.019Z')
  await authServiceClient.tenant.delete(id, deletedAt) // OK

We can add the proper test for this in #3124

njlie

LGTM

mkurapov · 2025-01-08T15:17:38Z

packages/auth/src/config/app.ts

@@ -43,6 +43,7 @@ export const Config = {
  authPort: envInt('AUTH_PORT', 3006),
  interactionPort: envInt('INTERACTION_PORT', 3009),
  introspectionPort: envInt('INTROSPECTION_PORT', 3007),
+  serviceAPIPort: envInt('SERVICE_API_PORT', 3011),


what do you think about calling this Tenant service instead?

Are you approaching this from this service being used for something else, for example?

Are you approaching this from this service being used for something else, for example?

Yes, that was the idea. Figured it can serve the purpose of any service-to-service communication. No specific things in mind but if we did want to expose some other resource I dont imagine we'd want a new api just for that. And probably better to avoid renaming everything including this env var (since it would be breaking).

mkurapov · 2025-01-08T15:23:18Z

packages/backend/src/auth-service-client/client.ts

+
+  public tenant = {
+    get: (id: string) =>
+      this.request<Tenant>(`/tenant/${id}`, { method: 'GET' }),


do we need the headers object in here as well?
Can also just have it in the request fn

The second arg in request? Yeah I think we need this. Its more than just the headers. request just wraps fetch and fetch needs the the method at the very least since its used for gets, posts, delete, etc. Or maybe I'm misunderstanding the comment?

As in, I think we need { 'Content-Type': 'application/json' } also for the GET method.

My suggestion is to just have { 'Content-Type': 'application/json' } once on L29

i added it in request as suggested. Actually I dont think we need it for the GET (just for specifying format of body no?) but it shouldn't hurt in any case.

mkurapov · 2025-01-08T15:31:11Z

packages/auth/src/shared/utils.ts

@@ -104,3 +104,8 @@ export async function verifyApiSignature(

  return verifyApiSignatureDigest(signature as string, ctx.request, config)
 }
+
+// Intended for Date strings like "2024-12-05T15:10:09.545Z" (e.g., from new Date().toISOString())
+export function isValidDateString(date: string): boolean {


small, but would be good to write a few tests for this

got em already here https://github.com/interledger/rafiki/pull/3148/files#diff-04126f4f8bb03998a1c86d2551e9442d11fab8403c4d7aff2031088f2dc36092R149

mkurapov · 2025-01-08T15:36:05Z

packages/backend/src/app.ts

@@ -265,6 +266,7 @@ export interface AppServices {
  paymentMethodHandlerService: Promise<PaymentMethodHandlerService>
  ilpPaymentService: Promise<IlpPaymentService>
  localPaymentService: Promise<LocalPaymentService>
+  authServiceClient: AuthServiceClient


Suggested change

authServiceClient: AuthServiceClient

authServiceClient: Promise<AuthServiceClient>

Technically I dont think I need this? Although I realize everything else is a promise but its driven by the factory function we pass into the singleton. I have:

container.singleton('authServiceClient', () => { return new AuthServiceClient(config.authServiceApiUrl) })

I didn't make that async since it didn't need to be, hence the AuthServiceClient and not a promise fort he type here. I do see that we return a promise elsewhere where it doesnt seem like we need to. Recent in memory cache for example.

container.singleton('tenantCache', async () => { return createInMemoryDataStore(config.localCacheDuration) })

So either I'm missing something or we just return a promise everywhere even when its not needed out of habit I guess.

Ah yeah, I think it's only out of habit. In that case, it's fine as is

BlairCurrey added 3 commits December 3, 2024 12:08

feat(auth): add service api with /healtz endpoint

f48bc8f

feat(auth): tenant routes

e593b2a

feat(auth): service api error handling

cab3e2c

github-actions bot added type: tests Testing related type: source Changes business logic pkg: auth Changes in the GNAP auth package. pkg: documentation Changes in the documentation package. labels Dec 3, 2024

BlairCurrey added 4 commits December 3, 2024 13:51

chore(auth): rm old todo

0ded2df

fix(auth): how errors are set

7cc448d

fix(auth): improve tenant tests, cleanup tenant get response,

d2b6411

feat(backend): auth service api client

b3eda45

github-actions bot added the pkg: backend Changes in the backend package. label Dec 5, 2024

BlairCurrey added 2 commits December 4, 2024 21:33

fix(auth): change status codes to 204 where no body

ecae9f2

fix(backend): format

8e51dff

BlairCurrey commented Dec 5, 2024

View reviewed changes

BlairCurrey added 10 commits December 5, 2024 11:04

feat(auth): add required deletedAt to DELETE /tenant body

0bab0c4

Merge branch '2893/multi-tenancy-v1' into bc/3125/auth-backend-servic…

9ce0f4d

…e-api

feat(backend): AUTH_SERVICE_API_URL env var

5fe1061

fix(backend): auth service client tests to mock codes correctly

dba2cd4

feat(backend): add AuthServiceClient dep

02cc1d5

feat(backend): use auth service client in tenant service

5635631

chore(auth): format

2742f1b

chore(auth): format

a499775

fix(integration,localenv): auth service api config

2f60a86

fix(backend,auth): update tenant api to support deletedAt

0188f2f

BlairCurrey mentioned this pull request Dec 20, 2024

Add new Auth Service API Environment variables interledger/helm-charts#47

Open

docs: update with env vars

eebb862

BlairCurrey added 2 commits December 20, 2024 14:26

fix(backend): dep container type

2212f88

fix(localenv): docker compose config

aa03813

BlairCurrey marked this pull request as ready for review December 20, 2024 19:34

BlairCurrey requested review from njlie, mkurapov, sanducb, oana-lolea and koekiebox December 20, 2024 19:35

njlie approved these changes Jan 7, 2025

View reviewed changes

mkurapov reviewed Jan 8, 2025

View reviewed changes

mkurapov linked an issue Jan 14, 2025 that may be closed by this pull request

[Mulit-Tenant] auth Tenant Admin Graphql API #3125

Closed

4 tasks

fix(backend): add default header to api client

8835a8c

BlairCurrey requested a review from mkurapov January 14, 2025 18:27

BlairCurrey merged commit fd8283b into 2893/multi-tenancy-v1 Jan 14, 2025
30 of 36 checks passed

BlairCurrey deleted the bc/3125/auth-backend-service-api branch January 14, 2025 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auth service-to-service api #3148

feat: auth service-to-service api #3148

BlairCurrey commented Dec 3, 2024 •

edited

Loading

BlairCurrey Dec 5, 2024

BlairCurrey Dec 5, 2024 •

edited

Loading

BlairCurrey Dec 5, 2024 •

edited

Loading

BlairCurrey Dec 5, 2024

BlairCurrey Dec 5, 2024

BlairCurrey commented Dec 20, 2024

njlie commented Jan 7, 2025

njlie left a comment

mkurapov Jan 8, 2025

BlairCurrey Jan 13, 2025 •

edited

Loading

mkurapov Jan 8, 2025

BlairCurrey Jan 14, 2025 •

edited

Loading

mkurapov Jan 14, 2025

BlairCurrey Jan 14, 2025

mkurapov Jan 8, 2025

BlairCurrey Jan 13, 2025

mkurapov Jan 8, 2025

BlairCurrey Jan 14, 2025 •

edited

Loading

mkurapov Jan 14, 2025

	authServiceClient: AuthServiceClient
	authServiceClient: Promise<AuthServiceClient>

feat: auth service-to-service api #3148

feat: auth service-to-service api #3148

Conversation

BlairCurrey commented Dec 3, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

Choose a reason for hiding this comment

BlairCurrey Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Dec 20, 2024

njlie commented Jan 7, 2025

njlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jan 14, 2025 • 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

BlairCurrey Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey commented Dec 3, 2024 •

edited

Loading

BlairCurrey Dec 5, 2024 •

edited

Loading

BlairCurrey Dec 5, 2024 •

edited

Loading

BlairCurrey Jan 13, 2025 •

edited

Loading

BlairCurrey Jan 14, 2025 •

edited

Loading

BlairCurrey Jan 14, 2025 •

edited

Loading