Skip to content
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

Update Content-Type for login request #7253

Merged
merged 6 commits into from Apr 5, 2024
Merged

Conversation

josunect
Copy link
Contributor

@josunect josunect commented Apr 4, 2024

Fixes #7252

@josunect josunect added the bug Something isn't working label Apr 4, 2024
@josunect josunect self-assigned this Apr 4, 2024
Comment on lines +105 to +106
const getHeaders = (urlEncoded?: boolean): Partial<AxiosHeaders> => {
if (apiProxy || urlEncoded) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this leaves off ...loginHeaders. Should those be included for both content types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought that loginHeaders contain the login information (Once logged in), but let me verify

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if OSSMC should also have those login headers? @ferhoyos?

Copy link
Contributor

@ferhoyos ferhoyos Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, as OSSMC leverages of Openshift login. Where loginHeaders 'X-Auth-Type-Kiali-UI': '1' is used?

Said that, including loginHeaders probably do not affect OSSMC. Maybe you can simplify the code:

    if (apiProxy || urlEncoded) {
      return { 'Content-Type': 'application/x-www-form-urlencoded', ...loginHeaders };
    } else {
      return { 'Content-Type': 'application/json', ...loginHeaders };
   }

@josunect josunect added backport needed Issue PRs require backport to versions specified in comments test-add-coverage 📎 this needs test coverage labels Apr 4, 2024
nrfox
nrfox previously approved these changes Apr 4, 2024
@josunect josunect changed the title Update encoding for login Update Content-Type for login request Apr 4, 2024
@josunect josunect marked this pull request as ready for review April 4, 2024 17:49
@josunect josunect requested a review from nrfox April 4, 2024 17:49
@josunect josunect merged commit b0de30b into kiali:master Apr 5, 2024
9 checks passed
@josunect josunect deleted the issue7252 branch April 5, 2024 06:34
josunect added a commit to josunect/kiali that referenced this pull request Apr 5, 2024
* Update encoding for login requests
josunect added a commit that referenced this pull request Apr 5, 2024
* Update encoding for login requests
@jshaughn jshaughn added test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation ) backport completed Issue PRs have been backported labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport completed Issue PRs have been backported backport needed Issue PRs require backport to versions specified in comments bug Something isn't working test: front-end/cypress PR adds/updates front-end tests (unit and/or cypress automation ) test-add-coverage 📎 this needs test coverage
Projects
Development

Successfully merging this pull request may close these issues.

token & OpenShift authentication not working
4 participants