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:add configurable state #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shtayerc
Copy link

@shtayerc shtayerc commented Oct 4, 2022

List of common tasks a pull request require complete

  • Changelog entry is added or the pull request don't alter library's functionality

@DeepDiver1975
Copy link
Collaborator

Can you please explain under which circumstance this is of interest? Thx

@shtayerc
Copy link
Author

shtayerc commented Oct 4, 2022

I want to know which requests belong together. Right now the state is generated on the fly, so it is not possible to save it before authenticating and matching it after. Also, with this pull request it is possible to pass additional data with state (jwt for example).

@azmeuk
Copy link
Collaborator

azmeuk commented Oct 19, 2022

I am not very comfortable with the state being a fixed string. Wouldn't it be better to set a custom user function that would be called to generate the state instead?

@shtayerc shtayerc force-pushed the configurable-auth-state branch 3 times, most recently from 44a4194 to 06597ab Compare October 20, 2022 10:16
@shtayerc
Copy link
Author

@azmeuk yes, custom function is more flexible. I changed the code.

@michaelcerne
Copy link

This would be beneficial to a project I am working on.

We need a reasonable way to identify a user before and after authentication without the use of additional session data.

@azmeuk
Copy link
Collaborator

azmeuk commented Oct 25, 2022

I wonder if that callback function should take parameters, such as clientID. This is an open question, I don't really have an advice, but I think this should be thank early to avoid break compatibility if we find this will be needed in the future.
What do you think?

@michaelcerne
Copy link

@azmeuk Looking at the codebase... the primary class is architected as a singleton, thus you can receive the clientId by calling getClientID() on the instance where the callback was set, negating most of the need for this, no?

I suppose maybe this would have use if you were to create a number of different "clients" and then utilize the same callback method for all. In that case, I would imagine the easiest (and most versatile) solution would be to simply pass the entire client class as a single parameter. All (public) interfaces would be accessible.

I am not sure if this is favored from a stylistic perspective.

@dmlang
Copy link

dmlang commented Mar 23, 2023

Hi everyone, is there anything new about this pr? It would also be beneficial for my use case.
Thanks!

@DeepDiver1975
Copy link
Collaborator

I am more in favor of overloading method implementations by sub classing - all these callables are highly type-unsafe.

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.

5 participants