Skip to content
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

feat: header authentication service configuration #438

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

soullivaneuh
Copy link
Contributor

@soullivaneuh soullivaneuh commented Oct 31, 2023

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #437
Documentation php-http/documentation#313
License MIT

What's in this PR?

Exposes configuration for the header authentication plugin.

Why?

Allows the direct usage of this plugin through the bundle configuration.

Example Usage

  - authentication:
      header:
          type: header
          key: "ApiKey"
          value: "%env(SAMPLE_API_TOKEN)%"

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

To Do

N/A

@soullivaneuh
Copy link
Contributor Author

I am not sure of what I did wrong to make some tests crying. May you give me a hint @dbu?

@soullivaneuh
Copy link
Contributor Author

I found the reason. Apparently, it is because we are declaring the name scalar value that make the configuration process being confused as name is also used as a key attribute: https://github.com/pocivaneuh/HttplugBundle/blob/32caa9a2c95a4da393a0a91205ea904fe3c53960/src/DependencyInjection/Configuration.php#L640

I changed to key instead: 32caa9a

@soullivaneuh
Copy link
Contributor Author

All green! 👍 I also updated the documentation PR according to the configuration key name change.

@dbu
Copy link
Collaborator

dbu commented Oct 31, 2023

ah right, i think that is for xml where you can't have a custom xml element and we use name as identifier attribute. in yaml its the my_auth: part, and does not need an explicit name.

should we call the thing header_name instead of key? key is a bit generic and could be confusing in the context of security. it boils down to what header name to set in the request.

@soullivaneuh
Copy link
Contributor Author

Oh that case, header_name and header_value to make things consistent?

@dbu
Copy link
Collaborator

dbu commented Oct 31, 2023

yes, i like header_name and header_value. that makes it really clear and leaves no room for confusion (i hope :-) )

@soullivaneuh
Copy link
Contributor Author

@dbu keys named was changed in 04414b2.

Squashed and rebased, ready for review! 👍

Copy link
Collaborator

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks!

@dbu dbu merged commit 2af0706 into php-http:1.x Nov 6, 2023
12 checks passed
@soullivaneuh soullivaneuh deleted the authentication-header branch November 7, 2023 08:50
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.

Header authentication service configuration
2 participants