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

AuthTokenHandler to always send an auth header unless specified otherwise #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fperreaultnv
Copy link

@fperreaultnv fperreaultnv commented Sep 15, 2021

GitHub Issue: #62

Proposed Changes

  • Feature

What is the current behavior?

AuthenticationTokenHandler does not include a token if the request doesn't already contain an Auth header.

What is the new behavior?

AuthenticationTokenHandler always includes a token unless it finds Authorization: Anonymous in the headers.

Checklist

  • Documentation has been added/updated
  • Automated Unit / Integration tests for the changes have been added/updated
  • Contains NO breaking changes
  • Updated the Changelog
  • Associated with an issue

@jeanplevesque
Copy link
Member

I unchecked Contains NO breaking changes because this PR currently reverses a default behavior which will break all applications using AuthenticationTokenHandler.
I wonder if we should keep the default behavior AND introduce a new class with the behavior that you want?

Copy link
Member

@jeanplevesque jeanplevesque left a comment

Choose a reason for hiding this comment

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

I unchecked Contains NO breaking changes because this PR currently reverses a default behavior which will break all applications using AuthenticationTokenHandler.
I wonder if we should keep the default behavior AND introduce a new class with the behavior that you want?

Note however that the breaking change is probably going to be silent... meaning that it will provide a header that wasn't provided previously, which shouldn't actually break functionalities.
In any case, it's still a breaking change because the behavior changes.

CHANGELOG.md Outdated Show resolved Hide resolved
@fperreaultnv
Copy link
Author

I unchecked Contains NO breaking changes because this PR currently reverses a default behavior which will break all applications using AuthenticationTokenHandler.
I wonder if we should keep the default behavior AND introduce a new class with the behavior that you want?

As you mentioned, it's a breaking change that doesn't break anything. The behavior is subtly different, yes, but I believe this is more of a fix to an undesirable behavior rather than a completely different implementation. Apps don't even have to react to this change, or very slightly if they desire not to try to send an auth header when sending a login request (for example), by adding [Headers("Authorization: Anonymous")] instead of nothing.

Let me know what you think.

@Soap-141
Copy link

Soap-141 commented Nov 3, 2023

@jeanplevesque Should we close this?

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.

3 participants