Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for batched token requests in privacypass-issuer #83

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lbaquerofierro
Copy link
Contributor

@lbaquerofierro lbaquerofierro commented Mar 5, 2025

This PR introduces several updates and improvements to the privacypass-issuer project. Key changes include:

  1. New Error Types: Added InvalidContentTypeError and MismatchedTokenKeyIDError to handle specific error scenarios.
  2. Token Request Handling
    • Modified handleTokenRequest to handle different content types (PRIVATE_TOKEN_REQUEST and ARBITRARY_BATCHED_TOKEN_REQUEST).
    • Introduced handleSingleTokenRequest for single token issuance.
    • Introduced handleBatchedTokenRequest for batched token issuance, including validation and key-pair retrieval.
  3. Key Management:
    • Refactored getBlindRSAKeyPair to handle key retrieval and error logging.
  4. New Test Cases:
    • Added new test cases in index.test.ts to verify the handling of batched token requests.
    • Added e2e test in e2e/issuer.ts to verify handling of batched token requests in a deployed worker.
  5. CLI Updates:
    - Updated testCommand in e2e/index.ts to accept the number of tokens for testing.
    • Added the --tokens option to specify the number of tokens to use in tests.

@kcxlee
Copy link

kcxlee commented Mar 6, 2025

Looks good to me. Just not sure about the indentation (tabs or spaces).

Copy link
Contributor

@thibmeu thibmeu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change looks great and functional.

main comment is on the RSA-RAW change

src/index.ts Outdated
Comment on lines 174 to 171
// name: ctx.isTest() ? 'RSA-PSS' : 'RSA-RAW',
// RSA-RAW is handled directlry by blindrsa-tsst()
// https://github.com/cloudflare/blindrsa-ts/blob/64dc207f079ca680d4db6cd5a22f5bce6f1b28e8/src/blindrsa.ts#L168
name: 'RSA-PSS',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not change
you should consider adding the importKey mock for the test, as is done on cloudflare/privacypass-ts

@lbaquerofierro lbaquerofierro force-pushed the arbitrary_batched_toknes branch 4 times, most recently from 321d396 to 2a7d82a Compare March 6, 2025 16:22
@lbaquerofierro lbaquerofierro force-pushed the arbitrary_batched_toknes branch from 2a7d82a to c632a40 Compare March 6, 2025 20:25
thibmeu and others added 2 commits March 7, 2025 11:57
This upgrades privacypass-ts to the latest version, and updates tests
accordingly
@lbaquerofierro lbaquerofierro force-pushed the arbitrary_batched_toknes branch 4 times, most recently from cb6889f to 3912840 Compare March 9, 2025 22:54
@lbaquerofierro lbaquerofierro force-pushed the arbitrary_batched_toknes branch from 3912840 to 18c7e8f Compare March 10, 2025 13:19
const client = new Client(BlindRSAMode.PSS);
export async function testE2E(issuerName: string, nTokens: number, mTLS?: MTLSConfiguration): Promise<boolean> {
return nTokens > 1
? testArbitraryBatchedRequest(issuerName, nTokens, mTLS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test must be split in two cases based on the type of token, not on the number of tokens requested.
Note that it's valid to perform a batchedTokenRequest with 0, 1, or more tokens.

'verify',
]);
}
const MODE = BlindRSAMode.PSS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is used only once, better to remove MODE

Copy link
Contributor

@armfazh armfazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's include a e2e test case for batchedtokens with 0,1, or more tokens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants