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

Remove bearer token if cookie_enabled is true #1567

Merged
merged 10 commits into from
Apr 25, 2023

Conversation

rhiroshi
Copy link
Contributor

@rhiroshi rhiroshi commented Dec 6, 2022

Some people were asking for the bearer token to not be included when using cookie_enabled: true in the PR that added the bearer token

@MaicolBen
Copy link
Collaborator

@rhiroshi can you fix the specs?

@blackst0ne
Copy link

Any chance to get this merged?

@MaicolBen
Copy link
Collaborator

@blackst0ne sorry, just checking this, can you rebase to make sure the build pass?

@blackst0ne
Copy link

@MaicolBen

This is not my PR. :)

@MaicolBen
Copy link
Collaborator

Oh sorry, bad tag, @rhiroshi can you rebase?

@rhiroshi
Copy link
Contributor Author

@MaicolBen Rebased. Had some tests failing but was not related to my changes. I made another commit to trigger a rerun and the tests passed. So sorry for taking so long, had a heavy workload on the end of the last year and ended up forgetting to fix things here.

@MaicolBen MaicolBen merged commit 4d52a74 into lynndylanhurley:master Apr 25, 2023
@MaicolBen
Copy link
Collaborator

@rhiroshi no problem, thank you!

@blackst0ne
Copy link

@rhiroshi

Thanks for the merge!

Any ETA on a new release with this change? :)

@djpremier
Copy link
Contributor

djpremier commented May 7, 2023

@MaicolBen Could we be reviewing a way to keep the header even using cookies? Because using cookies does not eliminate the need for headers, since shell applications (and others) that do not support cookies need them, so there are applications (like the one I develop) that have both functionalities (authentication through cookies or headers).

As the main driver of this change is due to the size of the cookie, what do you think about following the pattern of other languages and even the documentation, returning only the "Authorization" header, that is, removing all others (access-token , token-type, client, expiry, uid) that we currently have, as they are "just" the "separate" form of the token, and in turn end up making the size bigger?

I understand that removing these other headers would imply a breaking change, but we can do this in parts, starting with a flag to return in this new format and opening pull requests for changes in libraries that use this lib (eg ng-token-auth, jToker, etc).

What do you think? Could we go down this path? If so, I can start crafting.

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