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

Persist Auth #54

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

Persist Auth #54

wants to merge 3 commits into from

Conversation

elkolotfi
Copy link
Contributor

Actually, client details and tokens are stored in memory. Naturally this is not the best as it creates a strong coupling between the service and the data it manages.

Hence, I made this pull request to manage the authentication data on a separate container. I also shared the image on a docker repo (feel free to use your own repo if you prefer).

At your disposition for further information and/or discussion :)

@vasilaio
Copy link

vasilaio commented Aug 8, 2019

In my opinion, persistent client store is best suitable when there are dynamic clients registration or when clients details can be updated in a running application. Since all clients are known upfront there is no need for such functionality. It introduces a new container just to store "static" oauth2 client configuration.

@elkolotfi
Copy link
Contributor Author

Hi @vasilaio I agree with you about client details. Even if using mysql allow to change client informations without needing to restart the auth service. It is true that it's also a use case that shouldn't happen often but it's a plus.

On the other hand, it's better to store tokens (and refresh tokens) on a database in spite of having them on memory. Thus, we keep the information even if auth stops for some reason. Of course, keeping tokens in memory should be okay for demo purpose, but it's hard to use the app that way on production.

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.

2 participants