-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Hash App token #6724
Hash App token #6724
Conversation
What do you think about using bcyrpt with salt? |
I thought about that, but Gitea needs to search for token in DB, and if it is hashed with a salt, then Gitea would have to loop through all keys that exist in system and compute which one is the match. It could be simplified if the Typing this out now, has made me consider this even more, and good news is that it wouldn't require another PR to go-sdk. So I think I will make the change. Thanks for prompting me to re-consider. |
@techknowlogick , I left a comment #3789 (comment) |
Ready for review |
Codecov Report
@@ Coverage Diff @@
## master #6724 +/- ##
==========================================
- Coverage 41.24% 41.19% -0.06%
==========================================
Files 422 423 +1
Lines 58277 58394 +117
==========================================
+ Hits 24037 24055 +18
- Misses 31061 31161 +100
+ Partials 3179 3178 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think v85.go might need changes equivalent to #6823 (once we're sure that that is correct.)
@zeripath ahah yeah. I’ve been keeping an eye on those changes and I’ve just set this to blocked so it will only get merged once the other PR does |
Great that you introduce such a breaking change into a minor release, you just killed the current Drone authentication. |
@tboerger I think we've had a few of these problems lately - would it be possible to write an integration test for Gitea that would ensure that we don't kill drone authentication by mistake. |
Fix #3789