-
Notifications
You must be signed in to change notification settings - Fork 932
Misc fixes in auth #157
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
Misc fixes in auth #157
Conversation
src/client/auth.ts
Outdated
try { | ||
response = await fetch(url); | ||
} catch { | ||
return 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.
Sure you don't want to log/throw the exception?
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.
Some comments, but the logic looks good 👍
src/server/auth/handlers/register.ts
Outdated
client_secret_expires_at: isPublicClient | ||
? clientSecretExpirySeconds > 0 | ||
? clientIdIssuedAt + clientSecretExpirySeconds | ||
: 0 | ||
: 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.
Nested ternaries is a bit much—can we pull some of this out into one or two intermediate variables?
src/client/auth.ts
Outdated
} | ||
}); | ||
} catch { | ||
try { |
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 want to conditionalize this based on what the error actually was?
src/client/auth.ts
Outdated
try { | ||
response = await fetch(url); | ||
} catch { | ||
return 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.
I think it should be fine to let this error propagate if there's an actual network error (or otherwise), rather than a 404.
…rray-proptype Add an array input field to the ToolsTab using DynamicJsonForm.
While hacking with this I ran into a few little issues, these are the fixes.