feat(auth): add subject to grant#3440
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec canceled.
|
github-actions
Bot
added
type: tests
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
| jest.mock('../access/types', () => ({ | ||
| isIncomingPaymentAccessRequest: (access: AccessRequest) => | ||
| access.type === 'incoming-payment', | ||
| isQuoteAccessRequest: (access: AccessRequest) => access.type === 'quote' | ||
| })) |
There was a problem hiding this comment.
Why do we need this mock?
There was a problem hiding this comment.
canSkipInteraction function uses them and the purpose of the unit test is to test only the unit and not the deps, but I see there are also benefits from using the unmocked functions. removed the mock
njlie
left a comment
njlie
left a comment
There was a problem hiding this comment.
Overall LGTM, just a couple things
| }) | ||
|
|
||
| describe('getByGrant', (): void => { | ||
| test('gets access', async () => { |
There was a problem hiding this comment.
nit: this is getting subject, not access
| 400, | ||
| GNAPErrorCode.InvalidRequest, | ||
| err.message || 'invalid request' |
There was a problem hiding this comment.
We should match GrantErrors to their respective HTTP codes and GNAPErrorCodes.
This is similar to how @oana-lolea maps AccessErrors to the respective code & error codes in her PR.
| const emptyAccess = (body.access_token?.access?.length ?? 0) === 0 | ||
|
|
||
| if (emptySubject && emptyAccess) { | ||
| throw new Error('subject or access_token required') |
There was a problem hiding this comment.
Should this throw a GrantError instead?
There was a problem hiding this comment.
yes, adapted to throw GrantError
| interface IntrospectBody { | ||
| access_token: string | ||
| access?: AccessItem[] | ||
| subjects?: SubjectItem[] |
There was a problem hiding this comment.
If we don't use subjects anywhere (given that we don't pass it in during the introspection request), then its OK to leave out
Co-authored-by: Max Kurapov <max@interledger.org>
njlie
left a comment
njlie
left a comment
There was a problem hiding this comment.
Just one suggestion to be applied accross all the places where trx was cast in order to silence a Typescript error.
| let appContainer: TestContainer | ||
| let accessService: AccessService | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = null as unknown as Knex.Transaction |
There was a problem hiding this comment.
It might be healthier to actually initialize trx in the beforeAll block so that the tables get properly truncated when the test is finished, but the editor also doesn't flag a Typescript error. I assume this type casting was done to address the TS error.
beforeAll(async (): Promise<void> => {
...
trx = await appContainer.knex.transaction()
})
There was a problem hiding this comment.
Yes, It was a fix to a TS error
Variable 'trx' is used before being assigned.ts(2454)
Using a transaction is tricky because it can deadlock tests because some queries are done within a transaction and others are not. Managed to fix it, but in another manner.
let trx: KnexOrTransaction
beforeAll(async (): Promise<void> => {
...
trx = await appContainer.knex
})
Is this ok with you?
| let deps: IocContract<AppServices> | ||
| let appContainer: TestContainer | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = undefined as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| let deps: IocContract<AppServices> | ||
| let appContainer: TestContainer | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = undefined as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| let deps: IocContract<AppServices> | ||
| let appContainer: TestContainer | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = undefined as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| let appContainer: TestContainer | ||
| let grantService: GrantService | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = null as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| @@ -0,0 +1,86 @@ | |||
| import { AccessRequest } from '../access/types' | |||
There was a problem hiding this comment.
Thanks for adding these tests! Especially since they cover logic that existed before this PR
| let interaction: Interaction | ||
| let token: AccessToken | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = null as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| let client: string | ||
| let amtDelivered: bigint | ||
| let trx: Knex.Transaction | ||
| const trx: Knex.Transaction = null as unknown as Knex.Transaction |
There was a problem hiding this comment.
Ditto to prior comment on initializing trx.
| client: grant.client, | ||
| access: grant.access?.map((item) => accessToGraphql(item)), | ||
| access: grant.access?.map((item) => accessToGraphql(item)) || [], | ||
| subject: grant.subjects?.map((item) => subjectToGraphql(item)) || [], |
There was a problem hiding this comment.
grant.subjects only exists if we add it to the withGraphFetched request in the service methods OR we have subjects as a separate "child" resolver under grants, and call the subjectService.getByGrantId directly.
An example of this is the Asset > sendingFee resolver in backend.
There was a problem hiding this comment.
fixed with withGraphFetched('subjects') in the service methods
| "Wallet address of the grantee's account." | ||
| client: String! | ||
| "Details of the access provided by the grant." | ||
| access: [Access!]! |
There was a problem hiding this comment.
I think we can keep it as [Access!]!, if there is no access it can simply be an empty array.
| 400, | ||
| GNAPErrorCode.InvalidRequest, | ||
| 'access identifier required' | ||
| err instanceof Error ? err.message : '' |
There was a problem hiding this comment.
to avoid empty string, let's provide some generic message instead at least. It's less important for the client, its more so for us to find where to debug it.
There was a problem hiding this comment.
changed to 'unknown error while checking interaction requirement'
| 'subject id must be a valid https url' | ||
| ) | ||
| } | ||
| if (subject.format != 'uri') { |
There was a problem hiding this comment.
| if (subject.format != 'uri') { | |
| if (subject.format !== 'uri') { |
|
|
||
| function validateSubjectRequest(subject: SubjectRequest): void { | ||
| try { | ||
| if (!subject.id.startsWith('https://')) throw 1 |
There was a problem hiding this comment.
not sure if @njlie agrees, but I think just checking that it is a valid URL is sufficient, without checking the https://. IMO it can be up to the ASE to determine how strict they want to be with the specifics, like protocol.
There was a problem hiding this comment.
For the record, I also agree.
| subjectItems: Subject[] | ||
| ): OpenPaymentsGrant { | ||
| return { | ||
| access_token: toOpenPaymentsAccessToken(accessToken, accessItems, { |
There was a problem hiding this comment.
access_token could be undefined now, just like subject
| let appContainer: TestContainer | ||
| let accessService: AccessService | ||
| let trx: Knex.Transaction | ||
| let trx: TransactionOrKnex |
There was a problem hiding this comment.
We should remove the import { Knex } from 'knex' import
| ctx.body = { | ||
| grantId: interaction.grant.id, | ||
| access: access.map(toOpenPaymentsAccess), | ||
| subjects: subjects.map(toOpenPaymentsSubject), |
There was a problem hiding this comment.
there can only be one subject for a grant, but the sub_ids can be many. So this can either be a subject object with sub_ids array, (like the Open Payments spec), or we can directly return sub_ids here.
There was a problem hiding this comment.
The same applies for the subject updates in the GraphQL schema
There was a problem hiding this comment.
changed to subject object with sub_ids array.
I'm not aware of a subject updates.
| properties: | ||
| access: | ||
| $ref: ./auth-server.yaml#/components/schemas/access | ||
| subject: |
There was a problem hiding this comment.
We should make this spec standalone, and move over the correct components (access/subject) into this file, similar to how it was done for token-introspection
github-actions
Bot
added
pkg: backend
| const accessToken = await accessTokenService.create(grant.id) | ||
| const access = await accessService.getByGrant(grant.id) |
There was a problem hiding this comment.
I think we should only create an accessToken if the access_token was provided in the initial grant request. We can use length of access to verify that, I believe. Otherwise, if the client is just requesting subject information, and not access_token, we would return an access_token still, eg:
{
"access_token": {
"access": [],
"value": "EC6884A68394DAC1E186",
"manage": "http://localhost:3006/token/965841c8-1671-4174-944d-56778083dfdb",
"expires_in": 600
},
"continue": {
"access_token": {
"value": "B785F0F5ED51E08A923E"
},
"uri": "http://localhost:3006/continue/b094d601-cd62-43fd-9389-5ed6628c1c1d"
},
"subject": {
"sub_ids": [
{
"id": "https://cloud-nine-wallet-backend/accounts/gfranklin",
"format": "uri"
}
]
}
}
CC @njlie to confirm as well
There was a problem hiding this comment.
Yeah agreed, there's nothing to access, hence no access token.
There was a problem hiding this comment.
fixed, access_token is no more when the grant doesn't contain access items
* feat(localenv): expose subject during consent in mock-ase * feat: include client name in subject grant line * fix(mase): grantId not being retrieved * fix(mase): consent and confirmation texts * fix: handle subject-only grants properly --------- Co-authored-by: Cozmin Ungureanu <ucozmin@gmail.com>
github-actions
Bot
removed
pkg: backend
mkurapov
left a comment
mkurapov
left a comment
There was a problem hiding this comment.
The auth server changes look good 👍
I found a few things in the mock ase, but I will put up a change separately
3 participants
Changes proposed in this pull request
Context
fixes #3343
Checklist
fixes #numberuser-docslabel (if necessary)