Ext auth #408
Ext auth #408
Conversation
Jyyjy
commented
Feb 6, 2024
Jyyjy
commented
- Adds a password setting to be used with userId for basic http auth.
- Exposes it to the browser plugin.
src/sendLogs.js
Outdated
| @@ -89,6 +89,12 @@ export function sendLogs(logs, config, retries) { | |||
| req.open("POST", config.url); | |||
|
|
|||
| // Update headers | |||
| if( config.userId && config.password) { | |||
There was a problem hiding this comment.
There's a bit of a code smell here. Silent, unexpected behavior may emerge.
If I set a password as well as an authHeader callback, the latter will overwrite the basic authorization with whatever my function returns. We now expose multiple ways to do the same thing: customer headers callback, authHeaders callback, and this password setting.
I don't expect users to use both password and the custom callback, but exposing too many ways to do things can be confusing. We really should force all of this into the callback strategy pattern. The problem is the callback doesn't accept arguments and we have no way to persist the password unless we store it on the config. However, as discussed, storing the password is also poor practice. That said, so is basic auth...
There was a problem hiding this comment.
Completely agree, I was thinking about either
- Removing static auth header option. Basic auth can be used with username/password, I think all other auth schemes are better served by a callback.
- Not including a password setting in the main package, only in the browser extension. But I am trying to keep settings consistent, so it's clear what does what.
