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

Add delete button for non one-time passwords #1026

Merged
merged 9 commits into from
Oct 29, 2021

Conversation

boekkooi-lengoo
Copy link
Contributor

Hey,

First of thank you for your work in YoPass and having a look at this PR!

As requested in #333 this PR adds a delete button for the viewer of a time bound Secret.

Please let me know if there is anything missing or something needs to be updated.
Thanks again and have a great day 😄


image

image

image

@boekkooi-lengoo boekkooi-lengoo force-pushed the feature-delete branch 2 times, most recently from 125c48a to 1573644 Compare August 30, 2021 15:31
@codecov
Copy link

codecov bot commented Aug 30, 2021

Codecov Report

Merging #1026 (7db758e) into master (70db8c0) will decrease coverage by 1.87%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
- Coverage   78.21%   76.34%   -1.88%     
==========================================
  Files           6        6              
  Lines         404      427      +23     
==========================================
+ Hits          316      326      +10     
- Misses         58       71      +13     
  Partials       30       30              
Impacted Files Coverage Δ
pkg/server/memcached.go 57.69% <0.00%> (-16.23%) ⬇️
pkg/server/redis.go 58.06% <0.00%> (-13.37%) ⬇️
pkg/server/server.go 85.81% <85.71%> (-0.48%) ⬇️

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 2b2ee4e...7db758e. Read the comment docs.

@boekkooi-lengoo
Copy link
Contributor Author

@jhaals I have updated the PR with the new translation method. Is there anything I can do to help get this reviewed and hopefully merged?

Copy link
Owner

@jhaals jhaals left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I'm going to take this for a proper test as soon as I get some time to spare 👍

@@ -6,5 +6,5 @@ import "github.com/jhaals/yopass/pkg/yopass"
type Database interface {
Get(key string) (yopass.Secret, error)
Put(key string, secret yopass.Secret) error
Delete(key string) error
Delete(key string) (bool, error)
Copy link
Owner

Choose a reason for hiding this comment

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

What is the bool suppose to signal? I'd say returning error is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The boolean indicated if the secret was deleted or not.
This allows the correct status code to be returned in pkg/server/server.go line110-115.

I'm fine removing but to me it just seems nicer since we already require the check in deploy/aws-lambda/main.go, pkg/server/redis.go etc.

In order to allow the deletion of secrets we first need to ensure that the endpoint with method DELETE is not handled by getSecret.
In order to know if an item was deleted or not found without implementing implementation based error checking a boolean should also be returned by Delete.

If an item is Deleted then the return will be `true, nil`, if an item is not found the return is `false, nil` and if an error correct the return is `false, error`.
@boekkooi-lengoo boekkooi-lengoo force-pushed the feature-delete branch 3 times, most recently from 5e4d572 to 429dc62 Compare October 28, 2021 17:10
In order to avoid the `mux: route doesn't have methods` error from the CORS Middleware because `/` matches everything we now only allow its GET Method.
Because the CORS middleware is not working correctly for our use case it's easier to manually implement it.
This avoids code duplication (aka DRY).
@boekkooi-lengoo
Copy link
Contributor Author

@jhaals I have rebased this PR. Should you find any spare time to look into this it would be much appreciated 😄

P.S. Thanks for merging the fetch on demand PR 😄

@jhaals
Copy link
Owner

jhaals commented Oct 29, 2021

Really nice work @boekkooi-lengoo!

@jhaals jhaals merged commit cca7367 into jhaals:master Oct 29, 2021
@boekkooi-lengoo boekkooi-lengoo deleted the feature-delete branch November 1, 2021 08:03
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.

2 participants