-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: Show org token selector #7385
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
src/components/codeBlock.tsx
Outdated
|
||
const KEYWORDS_REGEX = /\b___(?:([A-Z_][A-Z0-9_]*)\.)?([A-Z_][A-Z0-9_]*)___\b/g; | ||
|
||
const ORG_AUTH_TOKEN_REGEX = /__ORG_AUTH_TOKEN__/g; |
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 inconsistently two underscores instead of three like the rest of these keywords
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 actually did this on purpose to avoid this being captured by the other regex. If we use triple underscore here, we need to make sure checking order is right (IMHO making this a bit more brittle for future refactorings), and it may also not be too clear that this does not go through the same code 🤔 maybe a completely different identifier would be even better, to make it more clear this is something else? For example +++ORG_AUTH_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.
In that case we should write tests.
If we're going to start having custom behavior for different tokens, we should implement it in a way that it's clear that this component supports handling that.
Right now using a different number of underscores is just going to be confusing, since it's really hard to notice (I already fixed a number of places where we were using 2 instead of 3).
I think the way to go here is to implement something a little bit more robust
return csrfCookie ? csrfCookie.split('=')[1] : null; | ||
} | ||
|
||
export async function createOrgAuthToken({ |
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 there a better place we can put this export? Doesn't feel right to have this inside codeContext.
I know this code is a bit of a mess right now, so it's probably unclear where this should go
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.
There isn't really a utils kind of place as far as I've seen so far? Just the components
, and this doesn't really have a dedicated component backing it 😬 it's only used in the codeBlock
as of now. Happy to move it to any place you think makes sense!
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.
Leave it here for now, we can move it around later once this repo is a little better organized.
} | ||
|
||
const currentSelectionIdx = sharedSelection.PROJECT ?? 0; | ||
const currentSelection = choices[currentSelectionIdx]; |
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.
Does it prompt you for which organization to create the token for?
I've been thinking about how we can generally make this experience better, so maybe the context would be a lot more obvious
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.
src/components/codeContext.tsx
Outdated
function getCsrfToken(cookies = document.cookie): string | null { | ||
// is sentry-sc in production, but may also be sc in other envs | ||
// So we just try both variants | ||
const cookieNames = ['sentry-sc', 'sc']; | ||
|
||
return cookieNames | ||
.map(cookieName => getCookie(cookies, cookieName)) | ||
.find(token => token !== null); | ||
} |
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.
Did this not already live somewhere else? Why does this have to be in here now?
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.
AFAIK this only exists in sentry frontend (where I kind of stole this from), not in docs yet! We have not been doing any POST stuff yet, only GET which does not need this.
Am I missing something, where does it actually show your tokens that you've already generated? We should add them to this endpoint https://github.com/getsentry/getsentry/blob/933c060c7efe4b90ab12cca434322f2443a4eadc/getsentry/web/docsupport.py |
export function SignedInCheck({ | ||
children, | ||
isUserAuthenticated, | ||
}: { | ||
children: React.ReactNode; | ||
isUserAuthenticated: boolean; | ||
}) { | ||
const {codeKeywords, status} = useContext(CodeContext); | ||
const {codeKeywords, isLoading} = useContext(CodeContext); |
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.
@evanpurkhiser refactored this based on your feedback from the other PR!
Tokens can only be viewed once after they are created, so we always have to create a new token (which is also why we have a button that has to be manually pressed in order to create/show the token). Every time you press the button it will generate a new token! |
Should we make this clear to the user? Seems a bit annoying that just clicking a button is going to pollute my token list. I'm not able to name it or give it a description here right? |
} catch { | ||
return null; | ||
} |
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.
will Sentry catch a bug here, in case it happens?
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.
No, we just ignore it in this case - not sure if it is worth it to capture this? May lead to some noise, not sure.
src/components/orgAuthTokenNote.tsx
Outdated
<ExternalLink href="https://sentry.io/settings/auth-tokens/" target="_blank"> | ||
manually create an Auth Token | ||
</ExternalLink>{' '} |
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.
doesn't the ExternalLink
component already open the link in a new tab? asking because I am not familiar with the docs UI/components
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.
as far as I can tell, in docs it does not open in a new tab by default..?
Is this PR still a draft? |
OK, so I added some tests for this for different scenarios! Thanks @priscilawebdev for helping me figure out the react test stuff 😅 But should be good now! |
We generate a (hopefully explanatory) name for the generated token, so it shouldn't be too confusing, hopefully. Also, since we track which token has been used when & where, it should be easy to clean up unusued tokens from the sentry UI. We can always add a further explanation here later when needed, IMHO. We could also eventually solve this in-product (by providing some auto-cleanup functionality or whatnot). |
+1 to making this clear in the doc here. The comment line above it now makes it sound like I can go retrieve the token later at the link provided. |
This adds a new org token generator for the source maps setup. For now, we only add it in one place (the webpack plugin configuration), if it works well we can roll it out to all places that use
SENTRY_AUTH_TOKEN
.If not signed in, we use this instead:
Note that this is blocked by the org tokens being rolled out to all orgs.
Closes #7226