-
Notifications
You must be signed in to change notification settings - Fork 0
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
[GWS-3384] Implements automatic refresh and token management for system auth and company auth #7
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1 @@ | |||
/src/hooks/clientcredentials.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an auto-generated hook created by speakeasy when any endpoint in the API is configured to use a type: oauth2
security scheme. We are adding it to .genignore
because I had to modify the file to make the fetch token request with grant_type: system_access
instead of grant_type: client_credentials
, which is the standard for oauth. For now, this is our only option, but in the future we should update our backend to properly support the oauth client credentials flow.
gusto_embedded/src/companyAuth.ts
Outdated
clientSecret: string, | ||
accessToken: string, | ||
refreshToken: string, | ||
options: { tokenStore?: TokenStore; url?: string } = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider making this non-optional. This is what allows the developer to hook into a data store that can store access/refresh token pairs whenever they are refreshed.
eeb991b
to
3b479aa
Compare
c8d45d1
to
45704db
Compare
update: | ||
type: http | ||
scheme: custom | ||
x-speakeasy-custom-security-scheme: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overlays the SystemAccessAuth security scheme from our open api spec with a custom security scheme that takes a clientId and clientSecret as inputs. These are then used to fetch a system access token before each request, using the clientcredentials hooks.
scopes: string[]; | ||
}; | ||
|
||
export class ClientCredentialsHook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defines beforeRequest
and afterError
hooks.
beforeRequest
– Retrieves a saved system access token from an in-memorysession
or creates a new system access token if there isn't a saved one. Sets this token in the authorization header.afterError
– deletes a saved token from the session if the server returns a 401, indicating that the token is no longer valid.
export function initHooks(hooks: Hooks) { | ||
// Add hooks by calling hooks.register{ClientInit/BeforeCreateRequest/BeforeRequest/AfterSuccess/AfterError}Hook | ||
// with an instance of a hook that implements that specific Hook interface | ||
// Hooks are registered per SDK instance, and are valid for the lifetime of the SDK instance | ||
const presetHooks: Array<Hook> = [new ClientCredentialsHook()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registers the beforeRequest and afterError hooks
…ith token refresh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit rusty with TS so this admittedly isn't the most thorough review, but I left a few questions/comments and noted where I faced the most friction while testing.
Is it worth it to also add some test coverage here?
Don't want to block getting this out to Lattice, so I'm happy to approve in the meantime if you want to address the feedback in another PR
if (serverURL) { | ||
return `${serverURL}/oauth/token`; | ||
} | ||
|
||
if (url) { | ||
return url; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
curious why the need to support both serverURL
and url
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call – the original implementation of this security callback that Speakeasy recommended is built in a way that consumers of the client can configure a separate auth server (the oauth spec doesn't specify that the auth server should be on the same domain as the rest of the API). However, since our implementation of oauth has the auth server on the same domain as the rest of the API, this probably isn't necessary. I'll consolidate to just allow one of these.
type Session = { | ||
credentials: Credentials; | ||
token: string; | ||
expiresAt?: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this being optional to ensure support for something like a personal access token, which never expires?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking through the code and I'm not actually sure why I made this optional. But I think it would make sense to leave as optional for that exact case.
gusto_embedded/src/companyAuth.ts
Outdated
) { | ||
const { | ||
tokenStore = new InMemoryTokenStore(), | ||
url = "https://api.gusto-demo.com/oauth/token", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this (or at least the demo root) be stored as a constant in a separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also is this hard coding the demo environment? (sorry, it's been a minute since I've looked at TS code 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, this is defaulting to the demo environment. We default to demo in a few other places, most importantly here. So I just wanted to make that behavior consistent for auth code. I'm also changing this to use
url = ServerList[ServerDemo],
You're right that we should be using a constant!
options: TokenRefreshOptions & SDKOptions; | ||
}; | ||
|
||
export function CompanyAuthenticatedClient({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there plans to extend this to other token types? (essentially, will we offer this for system tokens as well?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clientcredentials.ts
file in this PR handles fetching and refreshing system access tokens. Consumers of the library only have to pass in a clientId and clientSecret and the clientcredentials
hooks will automatically fetch or refresh a system access token as needed.
This is relying on Speakeasy's concept of "hooks". clientcredentials.ts
defines a beforeRequest
hook, that checks for the existence of a system access token before the request and fetches one if one hasn't been generated yet. It also defines an afterError
hook that checks for a 401 response code, then deletes the saved system access token so the user of the client library can retry the request and create a new system access token.
}); | ||
} | ||
|
||
function constructAuthUrl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
during my local testing, I didn't realize I had multiple options for setting the server url/env. Will partners using this SDK also be testing locally? If so, it might be worth it to improve that feedback loop.
Workarounds I tried to get it working locally:
- setting
server
tolocal
(raised anUncaught TypeError: Invalid URL
error) - not passing in
options
(raised anUncaught: TypeError: Cannot destructure property 'server' of 'options' as it is undefined.
error)
Not sure if setting a default "server" value here would be best, or if improving the error messages makes more sense. The error message might be preferable since it gives the dev more insight as to what's being executed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, partners will not be testing against a local server, so I'm hesitant to index too much on the UX of the serverURL
argument. Partners should be using the server
argument (either 'demo'
or 'prod'
), which defaults to 'demo'
. If nothing is passed in for server
or serverURL
the client should still work.
setting
server
tolocal
This should both be caught by the type checker. The server
argument's type should be 'prod' | 'demo'
, so the type checker would not allow a value of 'local'
and should guide the developer toward the two possible values.
not passing in
options
Thanks for calling this out. The options
argument was not optional in the type signature, but in practice you should be able to use the client without setting any of these values. I'll change it to be optional, and update the code to avoid this scenario when the user doesn't pass in anything for options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, partners will not be testing against a local server
Could you explain a little more about how this is to work between prod/non-prod environments? Is it expected that the partner uses this only in prod and in non-prod comes up with their own solution?
} | ||
|
||
const client = new GustoEmbedded(); | ||
const clientId = process.env["GUSTOEMBEDDED_CLIENT_ID"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this module gets imported on the client, process.env
will likely not be defined. What if we deferred this until a particular action is taken instead of making it happen in the module import?
const gustoEmbedded = new GustoEmbedded({ | ||
companyAccessAuth: process.env["GUSTOEMBEDDED_COMPANY_ACCESS_AUTH"] ?? "", | ||
}); | ||
class PersistentTokenStore implements TokenStore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add some kind of documentation that this is insecure if you attempt to use it in the browser? Right now the SDK assumes it's running solely in the browser and not on the server. That's subject to change eventually but it might break some assumptions we currently make.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I bring up the SDK here is that it ships this library, so whatever imports we add here have a chance of intentionally or unintentionally impacting the SDK
*/ | ||
export interface TokenStore { | ||
get(): Promise< | ||
{ token: string; refreshToken: string; expires: number } | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the tuple token, refreshToken, expires
occurs together so often I wonder if making an interface/type for them would be appropriate. What do you think?
} | ||
|
||
private hasTokenExpired(expiresAt?: number): boolean { | ||
return !expiresAt || Date.now() + 60000 > expiresAt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about extracting the magic number 60000
into an explanatory constant?
{ | ||
requestBody: { | ||
user: { | ||
firstName: "Frank", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the implications of using these hardcoded values for all partners?
I am assuming that the purpose of this is to help partners who are already using NodeJS as their backend tech. Is that correct? Is there any guidance as to how to incorporate their own authentication system with this? |
} | ||
|
||
/** | ||
* A TokenStore is used to save and retrieve OAuth tokens for use across SDK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool idea; I like that it's flexible enough to be agnostic about the store implementation
Makes the auth interface, including token management and refresh simpler.
Ticket: https://gustohq.atlassian.net/browse/GWS-3384
Testing in node console
First, apply these changes to your local branch to enable a local build (copy and then use
pbpaste | git apply
)Generate code locally using
speakeasy run --target=local
Import the generated library as a linked dependency into a node project (I like to use embedded-react-sdk).
npm link --save ../gusto-typescript-client/gusto_embedded
open a node console in the node project's directory
Then try to initialize the client and start making requests.
CompanyAuthenticatedClient
withserverURL: "http://api.gusto-dev.com:3000"
instead ofserver: "demo"
. Change ZP'saccess_token_expires_in
configuration inconfig/initizializers/doorkeeper.rb
to something like30.seconds
.