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

[fix/token-timetolerance] Retry requests that failed with a 401 during a token refresh #1105

Closed
wants to merge 3 commits into from

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Mar 10, 2022

Description

Re-schedules/retries requests that were:

  • sent before a token refresh AND
  • received a response after that token refresh AND
  • then failed with a 401 status code
    using the latest token.

Different tokens are identified by comparing the OCAuthenticationDataID of the authentication data used when authorizing the request with the latest OCAuthenticationDataID of the bookmark.

This PR also adds an improved representation of the Authorization header in the logs. Those now come with a counter that issues a new number whenever the content of the Authorization header changes.

Those counters are specific to every process, so they likely will not match between File Provider and app. But it allows to see in the logs when token changes occurred in the respective processes. The original idea - logging the OCAuthenticationDataID - would have provided the same IDs for all processes, but was thrown out for privacy/security concerns: OCAuthenticationDataID is a checksum computed from authenticationData - and there would have been the remote possibility that this would provide hints to the actually [redacted] content.

Example: the Authorization header before a change to authentication data:

# REQUEST ---------------------------------------------------------
…
PROPFIND /remote.php/dav/files/demo HTTP/1.1
Authorization: Bearer [redacted:0]

… and after:

# REQUEST ---------------------------------------------------------
…
PROPFIND /remote.php/dav/files/demo HTTP/1.1
Authorization: Bearer [redacted:1]

Related Issue

https://github.com/owncloud/enterprise/issues/5059

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)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@felix-schwarz felix-schwarz added this to the 11.9.0-Current milestone Mar 14, 2022
APP_SHORT_VERSION = 11.8.2;
APP_VERSION = 208;
APP_SHORT_VERSION = 11.8.3;
APP_VERSION = 210;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Versioning should be removed, because it will be also merged into the milestone/11.9.0

APP_SHORT_VERSION = 11.8.2;
APP_VERSION = 208;
APP_SHORT_VERSION = 11.8.3;
APP_VERSION = 210;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Versioning should be removed, because it will be also merged into the milestone/11.9.0

@hosy
Copy link
Collaborator

hosy commented Mar 14, 2022

@felix-schwarz please set the correct merge branch

@hosy hosy requested a review from jesmrec March 14, 2022 11:53
@jesmrec jesmrec mentioned this pull request Mar 15, 2022
36 tasks
@felix-schwarz
Copy link
Contributor Author

Merged as part of SDK changes into 11.9.0.

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.

None yet

3 participants