Skip to content

Conversation

@mattyod
Copy link
Contributor

@mattyod mattyod commented Jul 12, 2021

  Allow for a 'header' property to be specified in the options object.
  If present use the value specified as the name of the header from
  which to extract credentials for validation.

Checklist

@mcollina
Copy link
Member

Could you please add a test for this as well https://github.com/fastify/fastify-basic-auth/blob/master/index.test-d.ts?

@mattyod mattyod force-pushed the add-header-option-47 branch from 1d11227 to 52f9434 Compare July 13, 2021 09:14
@mattyod
Copy link
Contributor Author

mattyod commented Jul 13, 2021

Added an optional header to one of the existing tests, does that cover us?

```js
fastify.register(require('fastify-basic-auth'), {
validate,
header: 'x-forwarded-authorization'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we mention that it must be lowercase or add a lowercase function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it must be lowercased?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the lookup of the header will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the .toLowerCase() suggestion from @mcollina. Will update the test to reflect this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test to demonstrate the to lowercasing. Not sure it needs a whole test of it's own but let me know if that's not enough.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also need an added test

@mattyod mattyod force-pushed the add-header-option-47 branch from 0ab3580 to 0b4f90c Compare July 13, 2021 13:42
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

      Allow for a 'header' property to be specified in the options object.
      If present use the value specified as the name of the header from
      which to extract credentials for validation.
@mattyod mattyod force-pushed the add-header-option-47 branch from 0b4f90c to 4303901 Compare July 13, 2021 16:12
@mcollina mcollina merged commit fdfe2fb into fastify:master Jul 13, 2021
@mattyod mattyod deleted the add-header-option-47 branch July 14, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants