clean up logic for removing creds on delete user #862
Conversation
|
@XVincentX could you take a look at this one? This bug is the last thing that's blocking us from deploying this. |
|
I need to find some bandwidth but yeah I'll look into //cc @altsang |
|
I'm open to other ideas on the optional parameter I included. I figured there might be a case where you'd want to delete all credentials, but still keep the user "container". |
Codecov Report
@@ Coverage Diff @@
## master #862 +/- ##
==========================================
+ Coverage 89.29% 89.34% +0.05%
==========================================
Files 132 132
Lines 3633 3642 +9
==========================================
+ Hits 3244 3254 +10
+ Misses 389 388 -1
Continue to review full report at Codecov.
|
|
Separate bug... Looks like the keys also still exist under each scope as well. |
There was a problem hiding this comment.
Thanks for spending your time on this.
At first glance, it looks good. I'll pull down your branch and verify everything is in order. It's going to require some wore work though — this code is currently untested (Codecov is indeed complaining).
Would you be willing to complete the testing phase too?
|
Also — can you revert the |
Sounds good. For context, I'm used to Typescript these days and defining the type, which requires the extra parentheses.
So:
```
const awaitAllPromises = credentialTypes.map((type: Credential) => {
```
Co-Authored-By: mfrye <michael@michaelfrye.me>
|
Yes on the package-lock and your other recommended changes. Where should the tests be added for this? The code coverage % is still going to be below your threshold even if I add tests for this. |
Yeah good point. The problem is that sometimes Codecov reveals areas that have never been tested, thous reporting something wrong with the coverage. Add the tests you feel are useful and I'll check that out for you later and integrate in case it's needed. P.S: Nowadays I'm using Typescript too. I have opinions on it :) |
|
@XVincentX I attempted to add a test to I can try to look at this again, but I really don't have a ton of time to spend on this if it's going to be a bigger refactor. |
|
Do not worry, leave the tests to me and I'll figure this out. |
|
Cool, thanks. |
XVincentX
force-pushed
the
fix_creds_leak
branch
from
e80d068 to
a0a42fb
Compare
XVincentX
force-pushed
the
fix_creds_leak
branch
from
a0a42fb to
d7863c6
Compare
XVincentX
force-pushed
the
fix_creds_leak
branch
from
6e952c1 to
bf73168
Compare
|
@mfrye Thanks a lot for the contribution. Writing the tests ended up being a little bit more difficult than anticipated because of the horrible state of some parts of the codebase. This is going to require some other work on my side. |
XVincentX
force-pushed
the
fix_creds_leak
branch
from
af6ab85 to
76d87f0
Compare
|
@XVincentX awesome. Thanks for fast tracking this. |
clean up logic for removing creds on delete user
The problem with the current logic is that the Redis Set containing the credentials isn't deleted on user delete. So when deleting the user and user owned credentials, the set still exists.