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.

Sign up for GitHub

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

Jump to bottom

feat: Add keep pairs option for keys deletion #1570

Closed
wants to merge 7 commits into from
Closed

feat: Add keep pairs option for keys deletion #1570

wants to merge 7 commits into from

Conversation

sawadashota
Copy link
Contributor

@sawadashota sawadashota commented Sep 19, 2019 •
edited
Loading

Related issue

This PR will closes #1476

Proposed changes

  • Add tests for JWK API
  • Add keep pairs option for keys deletion API
  • Generate SDK
  • Add keep pairs option for keys deletion CLI

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

Further comments

@sawadashota sawadashota mentioned this pull request Sep 19, 2019
Closed
10 tasks
aeneasr
aeneasr reviewed Sep 25, 2019
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome! A couple of changes :)

jwk/handler.go Outdated

if err := h.r.KeyManager().DeleteKeySet(r.Context(), setName); err != nil {
if olderThan != "" {
date, err := time.Parse("2006-01-02", olderThan)
Copy link
Member

Choose a reason for hiding this comment

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

Would a unix timestamp make more sense here? Because here we don't know about timezones etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds better! I fixed it!

jwk/handler.go Outdated
@@ -327,8 +328,22 @@ func (h *Handler) UpdateKey(w http.ResponseWriter, r *http.Request, ps httproute
// 500: genericError
func (h *Handler) DeleteKeySet(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
var setName = ps.ByName("set")
var olderThan = r.URL.Query().Get("older-than")
Copy link
Member

Choose a reason for hiding this comment

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

This should be documented in swagger :)

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe rename this to before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I added the param to jwk/docs.go

jwk/handler_test.go
})

t.Run("DeleteJSONWebKeySet", func(t *testing.T) {
deleteJWKSetPath := "/keys/test-key"
Copy link
Member

Choose a reason for hiding this comment

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

The older-than/before feature should be tested here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I will add a test case :)

jwk/manager_sql.go Outdated
@@ -211,6 +211,13 @@ func (m *SQLManager) DeleteKey(ctx context.Context, set, kid string) error {
return nil
}

func (m *SQLManager) DeleteOldKeys(ctx context.Context, set string, date time.Time) error {
Copy link
Member

Choose a reason for hiding this comment

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

The manager_test should have a test for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also I will do it!

@aeneasr
Copy link
Member

aeneasr commented Sep 30, 2019

Let me know when this is good for another review :)

@sawadashota sawadashota force-pushed the feat-1476 branch 2 times, most recently from 9cd7cd6 to 3b46649 Compare October 5, 2019 09:55
@sawadashota sawadashota marked this pull request as ready for review October 5, 2019 10:55
@sawadashota
Copy link
Contributor Author

@aeneasr I have done! Could you review again?

@sawadashota
jwk: Add test for JWK Set handler
8c8f47a 
Signed-off-by: Shota Sawada <xiootas@gmail.com>
@sawadashota
jwk: Add key deletion API params by date
26fca87 
Signed-off-by: Shota Sawada <xiootas@gmail.com>
@sawadashota
jwk: Rename key deletion param to before
a46546e 
Signed-off-by: Shota Sawada <xiootas@gmail.com>
@sawadashota
jwk: Add tests for old keys deletion SQL manager
cb99205 
Signed-off-by: Shota Sawada <xiootas@gmail.com>
@sawadashota
jwk: Add a feature test for old keys deletion
4b4783c 
Signed-off-by: Shota Sawada <xiootas@gmail.com>
@sawadashota sawadashota changed the title jwk: Add keep pairs option for keys deletion feat: Add keep pairs option for keys deletion Feb 20, 2020
@sawadashota
Update SDK
1a9aefe 
Signed-off-by: Shota Sawada <shota@sslife.tech>
@sawadashota
cmd: Add --older-than option to keys deletion
00a4bab 
Signed-off-by: Shota Sawada <shota@sslife.tech>
@sawadashota
Copy link
Contributor Author

I couldn't generate SDK before but I get to do it now!
So I added a --older-than option to key deletion.

@aeneasr I'm so sorry to be late. Can you review again?

@aeneasr
Copy link
Member

aeneasr commented Nov 18, 2020

Closing due to inactivity - would probably need another overhaul to address this as we've changed quite a lot internally in the meanwhile.

@aeneasr aeneasr closed this Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

jwk: Add keep pairs option for keys delete
2 participants
@sawadashota @aeneasr