Skip to content

On reload don't store duplicated keys#553

Merged
rohe merged 4 commits intoCZ-NIC:masterfrom
rohe:ignore_dup_keys
Jun 29, 2018
Merged

On reload don't store duplicated keys#553
rohe merged 4 commits intoCZ-NIC:masterfrom
rohe:ignore_dup_keys

Conversation

@rohe
Copy link
Contributor

@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
## 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
Contributor 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
[#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
Contributor 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
Contributor 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
Contributor 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