Fix bug in support for jwt.url_param customization #1025
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1025 +/- ##
=======================================
Coverage 72.27% 72.27%
=======================================
Files 87 87
Lines 1915 1915
Branches 249 249
=======================================
Hits 1384 1384
Misses 478 478
Partials 53 53 Continue to review full report at Codecov.
|
| ); | ||
|
|
||
| const url = new URL('http://localhost:5601/app/api/v1/auth/authinfo'); | ||
| url.searchParams.append('authorization', 'testtoken'); |
There was a problem hiding this comment.
Can you push this into the url like http://localhost:5601/app/api/v1/auth/authinfo?authorization=testtoken & http://localhost:5601/app/api/v1/auth/authinfo?q=1&authorization=testtoken
There was a problem hiding this comment.
I think this makes it easier to see what will/won't work for a real request
There was a problem hiding this comment.
Updated to add the url params in the url string
|
|
||
| import { getAuthenticationHandler } from '../../auth_handler_factory'; | ||
|
|
||
| describe('test jwt auth library', () => { |
There was a problem hiding this comment.
Nit: I like smaller test functions that make it clearer what is being tested, could you refactor the call for getAuthenticationHandler to be a private function to make the test case read more like Setup / Action / Verification
There was a problem hiding this comment.
I created a helper function in this fixture called getTestJWTAuthenticationHandlerWithConfig to reduce some duplication and make the tests tighter.
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com> (cherry picked from commit 5e4004f)
…t#1025) Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Craig Perkins cwperx@amazon.com
Description
There is a bug in our implementation for
jwt.url_paramcustomization that hardcodes theurl_paramsetting tourlParamName.This PR fixes the issue and adds tests. The issue is described in detail here: #872
Category
Bug fix
Why these changes are required?
There is a bug in an advertised feature for JWT authentication via the URL for opensearch dashboards.
What is the old behavior before changes and new behavior after changes?
The old behavior supports JWT as a URL Param, but the URL Param must be
urlParamNameIssues Resolved
#872
Testing
Verified by creating a cluster with JWT backend and additional unit tests.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.