add ability to add custom hosts to BaseAuthenticationProvider #484
Conversation
ramsessanchez
requested review from
MIchaelMainer,
baywet,
ddyett,
nikithauc and
zengin
as code owners
MIchaelMainer
left a comment
MIchaelMainer
left a comment
There was a problem hiding this comment.
nit: missing param description for customHosts.
baywet
left a comment
baywet
left a comment
There was a problem hiding this comment.
This would be a breaking change. Add getters and setters on the base auth provider instead. And rely on instance field, not static. Existing value being the default.
Revert changes to the should method. Revert changes to the token auth provider
|
Kudos, SonarCloud Quality Gate passed! |
| this.customHosts = new HashSet<String>(); | ||
| } | ||
| for(String host: customHosts){ | ||
| this.customHosts.add(host.toLowerCase(Locale.ROOT)); |
There was a problem hiding this comment.
nit: According to this SO post, we should set this to Locale.ENGLISH in the very rare case we get to country specific endpoints (Turkey in the example) to avoid potential language specific user input error. With that stated, MG will likely define the host names in English so we should be safe even with Locale.ROOT.
No action required as I don't see this scenario happening, just thought I'd share my learning.
ramsessanchez
changed the title
Fixes #483