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

[POC] Support specifying an API key in network requests #1247

Closed
wants to merge 6 commits into from

Conversation

zone117x
Copy link
Member

@zone117x zone117x commented May 6, 2022

This is a proof-of-concept implementation of adding a fetch configuration option to the existing network object used by almost all functions in stacks.js to perform http requests. This is also related to issue #994 -- while this PR doesn't remove the implicit fetching, it at least makes it configurable.

The property is a simple interface matching the standardized fetch API:

type FetchFn = (url: string, init?: RequestInit) => Promise<Response>;

Additionally, two proof-of-concept implementations of API key middleware are also implemented. The first one, createApiKeyMiddleware, is a minimalistic function that can be used to specify an API key string. Example usage:

import { createApiKeyMiddleware, getDefaultFetchFn } from '@stacks/common';
import { StacksMainnet } from '@stacks/network';
import { getNonce } from '@stacks/transactions';

const apiKey = '1234-my-api-key-example';

// Create an instance of the API key middleware
const apiKeyMiddleware = createApiKeyMiddleware({ apiKey });

// Create fetch instance using the default `fetch` lib provided by `cross-fetch`,
// also providing the middleware
const fetchFn = getDefaultFetchFn(apiKeyMiddleware);

// Create a network instance using the configured fetch function
const network = new StacksMainnet({ fetchFn });

// Provide the network instance to a stacks.js function
const fetchNonce = await getNonce('SP3FGQ8Z7JY9BWYZ5WM53E0M9NK7WHJF0691NZ159', network);

// The http request made by the above function will then include the http header:
//     `x-api-key: 1234-my-api-key-example`

A second included middleware implements a proof-of-concept version of dynamic per-session API key authentication. It's located in fetchApiAuthMiddleware.ts. This one uses the middleware post callback to check for 401 Unauthorized responses, then requests a new API key, and retries the http request. The example usage is similar:

const authMiddleware = createApiSessionAuthMiddleware({
  // Optionally specify a storage interface to persist auth keys (defaults to in-memory)
  authDataStore: ...,
  // Optionally specify the endpoint path used to request API keys
  authPath: '/request_key',
  // Optionally include metadata in the auth request (free form object)
  authRequestMetadata: {
    productName: 'hiro-explorer',
    productVersion: 'v1.2.3'
  },
});

const network = new StacksMainnet({ fetchFn: getDefaultFetchFn(authMiddleware) });
const fetchNonce = await getNonce('SP3FGQ8Z7JY9BWYZ5WM53E0M9NK7WHJF0691NZ159', network);

Note that createApiSessionAuthMiddleware is an untested proof-of-concept, and the API backend is not yet ready anyway (as of creating this PR). It was implemented as a test to see if the fetch middleware approach is usable in the future for dynamic key authentication.

@vercel
Copy link

vercel bot commented May 6, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
stacks-js ✅ Ready (Inspect) Visit Preview May 9, 2022 at 0:32AM (UTC)

@zone117x zone117x requested review from janniks, kyranjamie and He1DAr May 6, 2022 07:31
Copy link
Contributor

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Exceptional work, Matt 🎉 I think this is all we need for the wallets.

#994 would've been too much work up front so def the right call to add configuration before thinking about that.

👏🏼 for interface SessionAuthDataStore{}. Regarding the situation of having a client app read from the stacks.js-created session key for non-stacks.js requests, maybe it'd also be helpful to have baked-in callbacks in the middleware? onTokenCreated, onTokenExpirary? 🤔 Not entirely sure if this is necessary, was thinking that we might need to call a ReactQuery method or something in some cases, but actually if each request reads from the store, that would also be fine.

@zone117x zone117x requested a review from CharlieC3 May 6, 2022 08:12
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2022

Codecov Report

Merging #1247 (86906bb) into master (49a094b) will decrease coverage by 0.32%.
The diff coverage is 62.92%.

@@            Coverage Diff             @@
##           master    #1247      +/-   ##
==========================================
- Coverage   64.11%   63.78%   -0.33%     
==========================================
  Files         124      124              
  Lines        8600     8657      +57     
  Branches     1838     1878      +40     
==========================================
+ Hits         5514     5522       +8     
- Misses       2841     2884      +43     
- Partials      245      251       +6     
Impacted Files Coverage Δ
packages/transactions/src/utils.ts 95.83% <ø> (+2.97%) ⬆️
packages/common/src/fetchUtil.ts 9.09% <6.97%> (-24.25%) ⬇️
packages/cli/src/network.ts 29.86% <14.28%> (-0.21%) ⬇️
packages/profile/src/profile.ts 68.33% <33.33%> (ø)
packages/auth/src/userSession.ts 53.74% <50.00%> (-0.06%) ⬇️
packages/wallet-sdk/src/utils.ts 75.47% <57.14%> (-2.96%) ⬇️
packages/transactions/src/builders.ts 73.33% <64.00%> (+0.31%) ⬆️
...ages/wallet-sdk/src/models/legacy-wallet-config.ts 83.33% <66.66%> (-4.91%) ⬇️
packages/wallet-sdk/src/models/profile.ts 75.55% <80.00%> (-1.19%) ⬇️
packages/keychain/src/utils/index.ts 90.97% <87.50%> (+0.20%) ⬆️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49a094b...86906bb. Read the comment docs.

@janniks janniks force-pushed the feat/fetch-interface branch from 9adc8a3 to d13e431 Compare May 9, 2022 09:57
@zone117x
Copy link
Member Author

zone117x commented May 13, 2022

These changes look good! Note that the createApiSessionAuthMiddleware function (should all be isolated to the fetchApiAuthMiddleware.ts file) is preliminary/demo/POC code that won't have a final spec or backend implementation ready soon enough.

The createApiSessionAuthMiddleware function, however, is essentially spec-finalized and should have back-end support soon (as in, we could reasonably do a pre-release with stacks.js including it, and merge this PR sooner rather than later).

I'd advocate for either A) branching off this, deleting fetchApiAuthMiddleware.ts, and opening a new PR, or B) deleting fetchApiAuthMiddleware.ts in this branch and just ensuring it's in git history for later when we introduce it.

EDIT: nvm, looks like you've already taken care of this in #1255 :)

@janniks
Copy link
Collaborator

janniks commented May 19, 2022

@janniks janniks closed this May 19, 2022
@janniks janniks deleted the feat/fetch-interface branch September 6, 2022 17:09
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.

4 participants