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

Support for creating ServiceAccount from string instead of file path #28

Open
chris13524 opened this issue Apr 10, 2024 · 8 comments · May be fixed by #29
Open

Support for creating ServiceAccount from string instead of file path #28

chris13524 opened this issue Apr 10, 2024 · 8 comments · May be fixed by #29

Comments

@chris13524
Copy link

chris13524 commented Apr 10, 2024

My use case involves handling many auth credentials which are stored in a database. So reading from a file is not ideal.

I'm willing to contribute this support myself :)

@chris13524
Copy link
Author

chris13524 commented Apr 17, 2024

@makarski This crate currently will read the service account file each time an access token needs to be generated. I'm wondering if this capability is intentional i.e. to support hot swapping credential files? I can keep this capability during my refactor, but it would be simpler without this support.

@makarski
Copy link
Owner

@chris13524 thanks for bringing this up and apologies for the late reply. Indeed, I went for the baseline scenario with the files in the first place. However, I can clearly see that you have a valid use case. Help me understand whether I interpret your discussion in the linked issue correctly. The proposed initialization of a ServiceAccount would look like similar to ServiceAccount::from_credentials(creds) and creds are likely to be a string?

In that case I'd be happy to review your PR and make the library better 👍

@chris13524
Copy link
Author

chris13524 commented Apr 17, 2024

@makarski yes exactly, my goal is to provide a string/bytes/ServiceAccountKey value directly to this crate, rather than reading from a file.

I do have a design question for you: if you want to keep the behavior that for the file case that this crate will read the latest version of the file each time access_token() needs to request a new token token. Or if it would be better to read the file only once when constructing ServiceAccount? Either way is fine with me

@makarski
Copy link
Owner

Amazing! As to your question, I would imagine that on default the file would be read only once. It'd be interesting to implement a listener that could be instructed that the file should be reloaded, i.e. as you mentioned hotswapping the key.

Currently, if the file is swapped, one would need to implement logic on the caller side and create a new instance of a ServiceAccount.

Happy to brainstorm possible design solutions in this thread.

@chris13524
Copy link
Author

Currently, if the file is swapped, one would need to implement logic on the caller side and create a new instance of a ServiceAccount.

Unless I'm misunderstanding the code, this is not the case currently, which is why I'm raising the question. Inside access_token() here every time the current token is expired, it calls jwt_token(). This calls JwtToken::from_file() which loads the file fresh

on default the file would be read only once

I agree this would be simpler and is what I would expect

@chris13524
Copy link
Author

Right now the file is lazily evaluated and will error when the first request is performed e.g. if the credentials file is invalid. This would probably be better anyway if it was read at time of construction in order to detect for errors earlier. Sounds like this was not intended

@makarski
Copy link
Owner

You are right and I agree with you, that is clearly an improvement we could go for - load once, keep in memory and avoid reloading from disc. I could look into that on the weekend to implement the change, or alternatively will be happy to review a PR.

@chris13524
Copy link
Author

chris13524 commented Apr 18, 2024

Thanks! I'll include the change in my PR for this issue since it's highly related

@chris13524 chris13524 linked a pull request Apr 18, 2024 that will close this issue
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 a pull request may close this issue.

2 participants