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

On reload don't store duplicated keys #553

Merged
merged 4 commits into from
Jun 29, 2018
Merged

On reload don't store duplicated keys #553

merged 4 commits into from
Jun 29, 2018

Conversation

rohe
Copy link
Collaborator

@rohe rohe commented Jun 28, 2018

If a key bundle is reloaded care should be taken to not store keys that already are in the key bundle.

  • [X ] Any changes relevant to users are recorded in the CHANGELOG.md.
  • The documentation has been updated, if necessary.

rohe added 2 commits June 28, 2018 11:32
…at already are in the key bundle. Updated CHANGELOG.md .
Copy link
Collaborator

@schlenk schlenk left a comment

Choose a reason for hiding this comment

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

Mostly ok.

Needs some whitespace fixes and the changes to the CHANGELOG still.

We should consider if _keys could be a set(). From cursory looking it seems the order is not relevant or am i missing something?

CHANGELOG.md Outdated
@@ -8,6 +8,7 @@ The format is based on the [KeepAChangeLog] project.
## 0.14.0 [2018-05-15]

### Fixed
- [#553] Made sure a a reload would not lead to duplicated keys in a keybundle.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be in the 0.14.0 section.

Open a new section for still unreleased changes on top.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it

CHANGELOG.md Outdated
@@ -45,6 +46,7 @@ The format is based on the [KeepAChangeLog] project.
[#532]: https://github.com/OpenIDC/pyoidc/pull/532
[#498]: https://github.com/OpenIDC/pyoidc/issues/498
[#534]: https://github.com/OpenIDC/pyoidc/pull/534
[#553]: https://github.com/OpenIDC/pyoidc/pull/553
Copy link
Collaborator

Choose a reason for hiding this comment

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

see above, move to new section preparing for 0.15.0

"alg":"RS256",
"e":"AQAB",
"kty":"RSA",
"n":"wkpyitec6TgFC5G41RF6jBOZghGVyaHL79CzSjjS9VCkWjpGo2hajOsiJ1RnSoat9XDmQAqiqn18rWx4xa4ErdWVqug88pLxMVmnV9tF10uJNgIi_RSsIQz40J9aKrxOotN6Mnq454BpanAxbrbC5hLlp-PIGgmWzUDNwCSfnWBjd0yGwdYKVB6d-SGNfLvdMUhFiYIX0POUnJDNl_j3kLYQ0peYRbunyQzST5nLPOItePCuZ12G5e0Eo1meSF1Md3IkuY8paqKk-vsWrT22X7CUV3HZow06ogRcFMMzvooE7yDqS53I_onsUrqgQ2aUnoo8OaD0eLlEWdaTyeNAIw","use":"sig"
Copy link
Collaborator

Choose a reason for hiding this comment

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

put the jwks in an extra file or make pylama happy about the overlong lines.

@rohe
Copy link
Collaborator Author

rohe commented Jun 28, 2018

The order of the keys are irrelevant which make the idea about making it into a set pretty good :-)

@rohe
Copy link
Collaborator Author

rohe commented Jun 28, 2018

Hmm, making _keys a set demands that jwkest.jwk.Key is hashable which is doable.
Could use thumbprint().

@rohe
Copy link
Collaborator Author

rohe commented Jun 28, 2018

Let's do this first and then look at making _keys a set instead of a list.

@codecov-io
Copy link

Codecov Report

Merging #553 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
+ Coverage   59.58%   59.59%   +<.01%     
==========================================
  Files          62       62              
  Lines       11247    11248       +1     
  Branches     1981     1982       +1     
==========================================
+ Hits         6702     6703       +1     
  Misses       3987     3987              
  Partials      558      558
Impacted Files Coverage Δ
src/oic/utils/keyio.py 65.53% <100%> (+0.04%) ⬆️

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 69a7ef5...5c48955. Read the comment docs.

Copy link
Collaborator

@schlenk schlenk left a comment

Choose a reason for hiding this comment

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

LGTM.

@rohe rohe merged commit b544831 into CZ-NIC:master Jun 29, 2018
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.

3 participants