feat(key-auth) skip authenticating preflight OPTIONS requests #2857
Conversation
kikito
added
pr/ready
| @@ -52,5 +52,9 @@ return { | |||
| type = "boolean", | |||
| default = false, | |||
| }, | |||
| run_on_preflight = { | |||
| type = "boolean", | |||
| default = true, | |||
There was a problem hiding this comment.
The issue with such "on by default" booleans is that they need a migration to be inserted.
There was a problem hiding this comment.
see other comment, a follow up PR.
| -- FIXME: the above `== false` test is because existing entries in the db will | ||
| -- have `nil` and hence will by default start passing the preflight request | ||
| -- This should be fixed by a migration to update the actual entries | ||
| -- in the datastore |
There was a problem hiding this comment.
I thought our agreed upon policy was to write migrations for such newly introduced fields instead of slowly digging ourselves into a hole of backwards-compatibility edge-cases?
There was a problem hiding this comment.
the PR's are separated. This goes without migration, and hence can land in 0.11.x, whereas the accompanying PR with the migration, and removal of this TODO can land in 0.12. (previously discussed)
There was a problem hiding this comment.
There is nowhere in this PR or in the commit message a reference to that other PR you are mentioning. Could you provide a link to it? Verbose commit messages are also encouraged.
There was a problem hiding this comment.
added to the initial message
There was a problem hiding this comment.
removal of this TODO can land in 0.12.
It would be nice if we decided on a convention on how to tag such temporary measures in the codebase, so that we grep through the code before a new version and make sure that all of these were taken care of (something more specific than FIXME).
fixes #1292, #1535
replaces #2743
follow up PR with the migrations: #2752