Offer an option to specify a cache for conditionally fetching public keys

[francisfuzz]

Summary

Closes #9

Previously, clients who fetched verification keys from the server had no way to cache those keys and reuse them for other requests.

This PR proposes a change using the GitHub API's conditional requests feature: clients can optionally specify a cache for their keys and only fetch new ones if the cache is outdated.

BREAKING CHANGE: verifyRequestByKeyId() now returns an object with a isValid property and acache` property.

Before

const isValid = await verifyRequestByKeyId();

After

const { isValid, cache } = await verifyRequestByKeyId();

BREAKING CHANGE: fetchVerificationKeys() now returns an object with a keys property and acache property.

Before

const keys = await fetchVerificationKeys();

After

const { keys, cache } = await fetchVerificationKeys();

[francisfuzz]

@gr2m - Thank you for all of your preliminary feedback in draft 🙇

I wrote the last test and left this comment about it for awareness. Happy to make any changes to get this over the line, especially around the way I've defined the types and the tests! 👂

[francisfuzz]

Leaving an update: I've updated the README.md with most of what's changed.

I'll have a final look over when I'm back online. ✌️

[gr2m]

Did you try if the changes work? I found the etag response and If-None-Match request header to be tricky in the past, there is some prefix that needs to be encoded ... just want to make sure we test the code in real-life before we merge the PR. If you run into any trouble let me know I can look what I did in other projects

[francisfuzz]

I was just thinking about test running the changes. Will report back! 🫡

[francisfuzz]

Example script:

import { request } from "@octokit/request";
import { fetchVerificationKeys } from "./index.js";

async function main() {
    const token = process.env.GITHUB_TOKEN;
    if (!token) throw new Error("GITHUB_TOKEN is not defined");

    const r = request.defaults({
        headers: {
            Authorization: `token ${token}`,
        }
    });

    const { id, keys } = await fetchVerificationKeys({ token });
    const cache = { id, keys };

    const firstRateLimitCheck = await r("GET /rate_limit");
    const firstRateLimitRemaining = firstRateLimitCheck.data.resources.core.remaining;
    console.log("Rate limit after fetching keys:", firstRateLimitRemaining);

    const response = await fetchVerificationKeys({ token, cache });

    const secondRateLimitCheck = await r("GET /rate_limit");
    const secondRateLimitRemaining = secondRateLimitCheck.data.resources.core.remaining;
    console.log("Rate limit after fetching keys from cache:", secondRateLimitRemaining);
}

main().catch(console.error);

Running it:

@francisfuzz ➜ /workspaces/preview-sdk.js (9-cache) $ node example.js 
Rate limit after fetching keys: 4953
Rate limit after fetching keys from cache: 4953

[gr2m]

can we add another test which sends the fetchVerificationKeys() request twice to simulate real-life behavior? The first one should respond with 200 and the body and a etag header. And the 2nd responds with a 304.

I'm not sure how undici is working with mocking, defining different replies for the same request can be tricky. We can also file a follow up issue and take care of it later

[francisfuzz]

Took a stab at this in fb4f95c

[francisfuzz]

Let me update the etag to be something that looks like what we'd expect from the GitHub API - please hold!

[francisfuzz]

@gr2m - This is ready for re-review! I've updated the README, incorporated your suggested feedback in today's most recent review, and re-ran the tests locally to ensure everything passes. I appreciate your time and can make any changes as needed to get this out (or, hold until after tomorrow's stream 😉 ).

[gr2m]

I don't think this is working yet.

Here is a test script

// .gregor/conditional-requests.js
import { request } from "@octokit/request";

import { fetchVerificationKeys } from "../index.js";

let counter = 0;

const requestWithLog = request.defaults({
  request: {
    hook(request, options) {
      counter += 1;
      return request(options);
    },
  },
});

const cache = await fetchVerificationKeys({ request: requestWithLog });
await fetchVerificationKeys({ request: requestWithLog, cache });

console.log({ counter });

This script currently logs out

{ counter: 2 }

But it should log out

{ counter: 1 }

[gr2m]

Ignore me, my script was incorrect, this one is correct

import { request } from "@octokit/request";

import { fetchVerificationKeys } from "../index.js";

const requestWithLog = request.defaults({
  request: {
    async hook(request, options) {
      try {
        const response = await request(options);
        console.log("no cache hit");
        return response;
      } catch (error) {
        if (error.status === 304) {
          console.log("cache hit");
          throw error;
        }

        console.log("unexpected error");
        throw error;
      }
    },
  },
});

const cache = await fetchVerificationKeys({ request: requestWithLog });
await fetchVerificationKeys({ request: requestWithLog, cache });

And the output is as expected

no cache hit
cache hit

[gr2m]

Great work on this one! We support caching now yay!

[github-actions]

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀