Add scope override for Azure Entra OAuth SASL config - #411
Conversation
|
@mostafa can you take a look? |
mostafa
left a comment
There was a problem hiding this comment.
Review
This is a real gap and the approach is right: optional SASLConfig.scope for sasl_azure_entra, hostname-derived https://<host>/.default when empty. Default behavior stays the same, and the unit tests cover both paths.
Workflows were waiting on first-time-contributor approval. I approved them; Test xk6-kafka, lint-pr, and zizmor are queued.
Blocking
-
Public JS API docs are missing.
scopeis a user-facingSASLConfigfield, and the FAQ already links toapi-docs/v2/docs/interfaces/SASLConfig.md, which will not list it. Please addscope?: stringtoapi-docs/index.d.tsand regenerate (yarn --cwd api-docs run generate-docs:v2). CI'scheck-docs:v2will not catch this ifindex.d.tsis left unchanged. -
FAQ overstates GCP support. The new FAQ says the override helps "Azure Entra OAuth (or GCP OAuth)", but
scopeis only threaded intonewAzureEntraOAuthTokenProvider. GCP still uses a hardcodedhttps://www.googleapis.com/auth/cloud-platform. Please drop the GCP mention (or implement a GCP override separately).
Non-blocking
- Wiring
scopeas a new positional arg onNewOAuthProvider(algorithm, scope, brokers, opts)works. Putting it onOAuthProviderOptswould keep the constructor smaller as more providers grow options. scope == ""does not trim whitespace." "would be sent as the Azure scope. Fine unless you wantstrings.TrimSpace.- GitHub reports the PR as mergeable onto current
main(Kerberos/GCP). Worth a rebase if CI merge produces a conflict inoauth.go/auth.go.
After docs + FAQ wording this looks good to merge.
|
Thanks for looking into this @mostafa. I've added the missing Docs & updated to FAQ to clarify that custom scope is only supported for Azure at the moment. |
|
@Waleed2660 Please fix the failing test/lint. |
|
@mostafa I've pushed the fix now. |
mostafa
left a comment
There was a problem hiding this comment.
LGTM! Thank you for your contribution.
Summary
Hi @mostafa, hope you're doing well!
I've added an optional
scopefield toSASLConfigso we can override the OAuth scope/audience requested forsasl_azure_entra. Currently the scope is always derived from the broker hostnamehttps://<broker-host>/.default, which doesn't work for setups where the token is tied to an Azure App ID e.g.api://<app-id>/.defaultrather than the broker's connection endpoint. Hence, auth fails with no workaround.When
scopeis left empty, the existing hostname-derived behaviour is unchanged.We've tested my fork with Azure & it worked with our custom scope.
Note: I haven't regenerated
api-docs/to keep this diff focused, happy to generate docs if you'd like that included in this PR.