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

Fixes #86 and #106 - properly handle token expiry in the sabre dav au… #108

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Oct 14, 2020

…th backend for openidconnect

Description

token expiry shall not lead to 500 but a proper 401

Related Issue

How Has This Been Tested?

  • unit tests added

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

Checklist:

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

Open tasks:

  • ...

@DeepDiver1975 DeepDiver1975 self-assigned this Oct 14, 2020
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #108 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #108      +/-   ##
============================================
+ Coverage     93.33%   93.38%   +0.04%     
- Complexity      121      122       +1     
============================================
  Files            10       10              
  Lines           420      423       +3     
============================================
+ Hits            392      395       +3     
  Misses           28       28              
Impacted Files Coverage Δ Complexity Δ
lib/Sabre/OpenIdSabreAuthBackend.php 97.36% <100.00%> (+0.22%) 11.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fee4bce...20a004f. Read the comment docs.

Copy link
Contributor

@jnweiger jnweiger left a comment

Choose a reason for hiding this comment

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

I would expect code that checks for expired token and then do some actions to refresh it.
I don't understand what is going on here. some comments would be helpful.
I have an easy reproducer to test the 10min timeout, so whatever it does, I can easily veryfy, if it is successful afterwards :-)

$this->session->close();
return $this->principalPrefix . $userId;
} catch (\Exception $ex) {
$this->session->close();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect code that checks for expired token and then do some actions to refresh it.
I don't understand what is going on here. some comments would be helpful.
I have an easy reproducer to test the 10min timeout, so whatever it does, I can easily veryfy, if it is successful afterwards :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect code that checks for expired token and then do some actions to refresh it.

We as the reply party cannot refresh the token - this is upon the clients to take care.
(The server has no refresh token)

@micbar micbar merged commit 55fd4f7 into master Oct 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/86+106 branch October 15, 2020 08:33
@jnweiger jnweiger restored the bugfix/86+106 branch October 15, 2020 18:06
jnweiger added a commit that referenced this pull request Oct 15, 2020
@jnweiger jnweiger mentioned this pull request Oct 15, 2020
42 tasks
phil-davis pushed a commit that referenced this pull request Nov 6, 2020
@DeepDiver1975 DeepDiver1975 deleted the bugfix/86+106 branch November 6, 2020 09:23
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.

[QA] [OIDC] Token refresh fails Exception: OpenID Connect token expired leads to 500
3 participants