feat(auth): tenanted grants #3187
Conversation
67f3d21 to
1e383c1
Compare
1e383c1 to
d1785a2
Compare
88cf1f5 to
1410ab0
Compare
| * @param { import("knex").Knex } knex | ||
| * @returns { Promise<void> } | ||
| */ | ||
| exports.up = function (knex) { |
There was a problem hiding this comment.
Do we need to backfill old grants w tenantId as well?
There was a problem hiding this comment.
Outside the scope of this PR, but we should do some manual testing for the upgrade before merging the multi tenacny branch into main. So we can catch stuff like this.
packages/auth/src/grant/routes.ts
Outdated
| @@ -179,6 +196,15 @@ async function createPendingGrant( | |||
| ) | |||
| } | |||
|
|
|||
| const tenant = await deps.tenantService.get(tenantId) | |||
There was a problem hiding this comment.
Since we fetch the tenant in the createGrant function, can we avoid this lookup by passing tenant in directly?
| await trx.commit() | ||
|
|
||
| if (!isTenantWithIdp(grant.tenant)) throw new Error('invalid interaction') |
There was a problem hiding this comment.
Do we want this to be a GNAPServerRouteError or is this something that shouldn't really happen?
There was a problem hiding this comment.
This is something that shouldn't really happen - an interactive grant should only be created if the tenant it's for has its IDP configured. If this does happen, this block is wrapped in a try-catch that throws a GNAPServerRouteError in the catch.
| getBySession( | ||
| id: string, | ||
| nonce: string, | ||
| tenantId?: string |
There was a problem hiding this comment.
Looks like this isn't used: getBySession is always called without tenantId. I think you probably wanted to add this because of the interaction finish route?
| @@ -328,7 +328,7 @@ describe('Incoming Payment Routes', (): void => { | |||
| await expect(incomingPaymentRoutes.get(ctx)).resolves.toBeUndefined() | |||
| expect(ctx.response).toSatisfyApiSpec() | |||
| expect(ctx.body).toEqual({ | |||
| authServer: config.authServerGrantUrl, | |||
| authServer: config.authServerGrantUrl + '/' + config.operatorTenantId, // TODO: replace with incoming payment tenant id | |||
There was a problem hiding this comment.
FYI @oana-lolea, this will need to be updated if this PR goes in first
|
@koekiebox in the docs there is https://rafiki.dev/integration/deployment/services/auth-service/ we can go over something specific as well if you have other q's, maybe a team wide call would be helpful as well about auth? |
| @@ -290,7 +290,7 @@ export class App { | |||
|
|
|||
| // Grant Cancel | |||
| router.delete<DefaultState, GrantRevokeContext>( | |||
| '/continue/:id', | |||
| '/:tenantId/continue/:id', | |||
There was a problem hiding this comment.
What are we doing with this tenantId, or why do we need to include it here? I dont see it used in the grantRoutes.revoke method and I dont think it's used by the middleware. Should we be validating it in the revoke method? I do see we're checking it in POST /:tenantId/continue/:id (grantRoutes.continue).
Changes proposed in this pull request
tenantIdto grantsContext
Closes #3128.
Checklist
fixes #numberuser-docslabel (if necessary)