fix(hmac-auth) generate a credential secret if none provided #2158
Conversation
p0pr0ck5
force-pushed
the
fix/hmac_default_key
branch
2 times, most recently
from
2217c1a to
5871443
Compare
kong/plugins/hmac-auth/daos.lua
Outdated
|
|
||
| local function random_secret(t) | ||
| return utils.random_string() | ||
| end |
There was a problem hiding this comment.
why wrapping a single function call into another function?
if it is to drop the argument, then maybe a small comment would make that clear
There was a problem hiding this comment.
yeah, silly mistake, we can just set default = utils.random_string. ill fix that
|
|
||
| local body = assert.res_status(201, res) | ||
| credential = cjson.decode(body) | ||
| assert.is.not_nil(credential.secret) |
There was a problem hiding this comment.
fyi; you could write this as:
assert.response(res).has.status(201)
local json = assert.response(res).has.jsonbody()
assert.is.not_nil(json.secret)these custom assertions (in spec/helpers.lua) generally provide better error messages when they fail.
There was a problem hiding this comment.
Thanks, this was a copypasta from the test above. Nice to know though!
|
@poprocks ^^ approved it, but assuming there will be another pr with the migrations |
p0pr0ck5
force-pushed
the
fix/hmac_default_key
branch
from
5871443 to
6adf631
Compare
|
@Tieske re-up'd with an adjustment to the dao. I'd be happy to work on the migration part of it, but I could use some hand holding there. |
|
@p0pr0ck5 checkout the 2 migration files for the Kong core in the dao. They provide quite some examples. Ping me if you need more. |
p0pr0ck5
force-pushed
the
fix/hmac_default_key
branch
2 times, most recently
from
138e290 to
4c4ff54
Compare
Since the credential secret is required to compute the signature, create a random secret which will be displayed back to the user as part of the response body. This fixes issue #2143.
p0pr0ck5
force-pushed
the
fix/hmac_default_key
branch
from
4c4ff54 to
31af708
Compare
|
@p0pr0ck5 Are you planning on providing the migration in this PR or another one? |
thibaultcha
added
the
pr/ready (but hold merge)
|
Will handle the migration in another PR, since it's fairly destructive (after talking to tieske it seems the best plan for that is to simply delete all entries with a null |
Summary
Since the credential secret is required to compute the signature, create a random secret which will be displayed back to the user as part of the response body.
Full changelog
Issues resolved
Fix #2143