feat(basic-auth) don't trim whitespace from basic-auth passwords #3650
Conversation
| @@ -77,6 +77,30 @@ for _, strategy in helpers.each_strategy() do | |||
| } | |||
| assert.equal(hash, json.password) | |||
| end) | |||
| it("encrypts the password without trimming whitespace #only", function() | |||
There was a problem hiding this comment.
Could you remove the #only tag in this test's title?
| @@ -127,7 +127,9 @@ function _M.validate_entity(tbl, schema, options) | |||
| local is_valid_type | |||
| -- ALIASES: number, timestamp, boolean and array can be passed as strings and will be converted | |||
| if type(t[column]) == "string" then | |||
| t[column] = utils.strip(t[column]) | |||
| if schema.fields[column].trim_whitespace ~= false then | |||
There was a problem hiding this comment.
We would also need unit tests or two in the plugins' validator test suite for this new field option, please. Especially since we use a false comparison vs. falsy (which is a good call)!
thibaultcha
added
the
pr/changes requested
|
I removed the #only tag (whoops 😄) and added some tests. Let me know how it looks |
| assert.are.same("kong", values.string) | ||
| end) | ||
| it("should trim whitepsace from value with trim_whitespace = true", function() | ||
| local trimSchema = { |
There was a problem hiding this comment.
a nitpick, but can we use snake_case instead for these? Thanks in advance :)
There are a few typos in the tests name that we can fix ourselves before merging too, no worries.
| @@ -25,7 +25,7 @@ local SCHEMA = { | |||
| created_at = {type = "timestamp", immutable = true, dao_insert_value = true}, | |||
| consumer_id = {type = "id", required = true, foreign = "consumers:id"}, | |||
| username = {type = "string", required = true, unique = true }, | |||
| password = {type = "string", func = encrypt_password} | |||
| password = {type = "string", func = encrypt_password, trim_whitespace = false} | |||
There was a problem hiding this comment.
It would be nice if your PR was updated to have two commits:
- Adding support (+ unit tests) for the
trim_whitespaceattribute in the schema validator - Adding the
trim_whitespaceattribute to the basic-auth schema (+ tests)
As per our contributing guidelines, we try to respect strict atomicity in our commits :)
Thanks!
…bute in the schema validator
|
Ah crap. Yeah I was just being sloppy. Thanks for being patient. Now we should be good to go. |
|
|
||
| local values = {string = " kong "} | ||
|
|
||
| local valid, err = validate_entity(values, trimSchema) |
There was a problem hiding this comment.
Your patch is now failing to lint/pass CI (https://travis-ci.org/Kong/kong/jobs/409120621), because the name was not updated here and in the below test
…th schema (+ tests)
|
Haha alright I swear I'm normally more careful than that. I was coding on an airplane and rushing to get done before I landed. Anyway, now we're really good to go. Linting is good, all tests are passing, and commits are atomic. |
thibaultcha
added
pr/please review
and removed
pr/changes requested
|
Awesome, merged! Thanks a lot for your contribution! Reminder: as a Kong contributor, you can now receive your own Contributor T-shirt by claiming it :) |
password.
fix #3595