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

do not send www-authenticate basic for Api requests #5992

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

micbar
Copy link
Contributor

@micbar micbar commented Apr 3, 2023

Description

Bugfix: Fix authenticate headers for API requests

We changed the www-authenticate header which should not be sent for "basic-auth" when the XMLHttpRequest header is set.

Related Issue

Fixes #5986

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@ownclouders
Copy link
Contributor

ownclouders commented Apr 3, 2023

💥 Acceptance test localApiTests-apiSpacesShares-ocis failed. Further test are cancelled...

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

Just a few comments/questsions:
a) Why is this only relevent for basic auth?
b) The header is X-Requested-With and the value is XmlHttpRequest
c) There is no regression test :trollface:

@micbar
Copy link
Contributor Author

micbar commented Apr 3, 2023

a) Why is this only relevent for basic auth?
I am aware that the desktop client always checks the presence of the Www-authenticate header for bearer auth.

But we can completely not show any www-authenticate headers if ok for the clients.

b) The header is X-Requested-With and the value is XmlHttpRequest

Thanks. Will fix

c) we can do an API test. Let us tackle that separately.

@dschmidt
Copy link
Member

dschmidt commented Apr 3, 2023

a) Why is this only relevent for basic auth?
I am aware that the desktop client always checks the presence of the Www-authenticate header for bearer auth.

But we can completely not show any www-authenticate headers if ok for the clients.

Ah, interesting - from oC Web prespective: the www-authenticate header triggers a super annoying auth popup which we cannot surpress, so it would be great to avoid it completely ... and we don't use basic auth, so basically doing the check only for basic auth it kind of defeats the purpose of the issue we raised in the first place.

b) The header is X-Requested-With and the value is XmlHttpRequest

Thanks. Will fix

💪🏻

c) we can do an API test. Let us tackle that separately.

Yup, fine with me - I would just highly appreciate a regression test as it's so annoying in oC Web if for whatever reason a token refresh fails or some other condition leads to invalid tokens

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

Now I get no Www-Authenticate header at all. Not Basic and also not Bearer.

Is this the intended behaviour?

@micbar
Copy link
Contributor Author

micbar commented Apr 3, 2023

Now I get no Www-Authenticate header at all. Not Basic and also not Bearer.

Is this the intended behaviour?

If you are sending 'x-requested-with: XMLHttpRequest' you should not get any Www-Authenticate header at all.

@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@micbar micbar merged commit b256897 into master Apr 3, 2023
@delete-merged-branch delete-merged-branch bot deleted the basic-auth-header branch April 3, 2023 13:22
ownclouders pushed a commit that referenced this pull request Apr 3, 2023
do not send www-authenticate basic for Api requests
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
@ScharfViktor ScharfViktor mentioned this pull request May 4, 2023
86 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong WWW-Authenticate: Basic ... header with basic auth being disabled
4 participants