-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat: Add button to create an "automation token" and use it in SDF API #5143
feat: Add button to create an "automation token" and use it in SDF API #5143
Conversation
350b82b
to
3bd4f5f
Compare
3bd4f5f
to
ec6b468
Compare
const token = createSdfAuthToken({ | ||
userId: user.id, | ||
workspaceId, | ||
role: "web", |
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 really want to hard code a role of "web" - can't this at least be an enum we extend?
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 enum is here: https://github.com/systeminit/si/pull/5143/files#diff-45a5a885b573adda0e5349e1af441abf982f4f977141755f48dd5780d52a17bdR52
Basically in TS (at least in my experience) unless an enum has extra data they are generally a type union of the possible values, and you specify those values directly. TS will yell at you if you don't use one of them.
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.
github is being weird about the link, but the line is in auth.service.ts:
export type SdfAuthTokenRole = "web" | "automation";
And it's part of the type on the parameter you pass to createSdfAuthToken
, which is how TS knows what values are allowed.
const token = createSdfAuthToken({ | ||
userId: authUser.id, | ||
workspaceId: workspace.id, | ||
role: "automation", |
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 should also be part of the enum
const token = createSdfAuthToken({ | ||
userId: user.id, | ||
workspaceId, | ||
role: "web", |
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 little confused by this - we're scoping this token to only be used by the web app?
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.
Sort of. Roles are a general indication of what you're using the token for, and that tells you about both what you can access as well as the scope of the token. The current tokens we give people are designed for use during active sessions in a web browser, so we called it "web"
. Alternate names are absolutely accepted :) What we didn't want was to leave it blank for this purpose; giving you essentially maximal access by default seemed undesirable.
workspaceId: workspace.id, | ||
role: "automation", | ||
}, { | ||
expiresIn: "1 day", |
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.
it's been a while since I was in JWT spec land - does this add the exp
claim to the token? I would have expected that to get automatically enforced by SDF's middleware but I could be wayyy off here.
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 would have too; looking at the token at https://jwt.io
it appears correct, but I'll definitely be revisiting this.
ec6b468
to
5f45814
Compare
This adds the ability to create "automation tokens" intended for use with API clients in hands-free contexts like github.
Testing
I have tested:
Automated tests: added tests for the new token formats. There is no new coverage in the SDF, DAL or auth api automated tests.
It is critical we ensure SDF and module-index are working after deploy in preprod. I have not tested the new tokens against preprod or prod module-index / SDF, because that requires private keys from those environments to create the tokens.
Deployment
auth api, SDF, and module-index will all have to be deployed. It doesn't matter what order they are deployed in, but all need to be deployed to ensure the new code is generating and accepting tokens correctly to ensure service doesn't get interrupted.
Caveats
Quickstart
You too can have your API requests denied due to lack of permission:
bin/auth-api/README.md
).curl -H "Authorization: Bearer $MY_AUTH_API_TOKEN" http://localhost:5156/api/v2/workspaces/01JF8X1BCPAFTFR18J24JHQGBE/integrations
See your very own 401 saying you don't have a
web
role.