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

Make use of AuthorizationProvider (and cache clients?) #382

Open
yrodiere opened this issue Sep 21, 2022 · 3 comments
Open

Make use of AuthorizationProvider (and cache clients?) #382

yrodiere opened this issue Sep 21, 2022 · 3 comments

Comments

@yrodiere
Copy link
Collaborator

yrodiere commented Sep 21, 2022

github-api supports self-refreshing authorization providers, as evidenced by org.kohsuke.github.extras.authorization.JWTTokenProvider and org.kohsuke.github.authorization.OrgAppInstallationAuthorizationProvider.

We don't need to use these implementations if they're not a good fit, but we could use our own implementations of org.kohsuke.github.authorization.AuthorizationProvider in order to create a client that refreshes its token automatically.

The main advantage would be that a given client could stay valid forever (because it automatically refreshes its token if necessary before each HTTP request), opening the door to caching clients (potentially forever, in the case of the application client).

@gsmet
Copy link
Member

gsmet commented Sep 21, 2022

I would like us to keep one client per installation as I want to make sure no information leak. IIRC there are a few caches in the client.

Also we would still have the problem with the SmallRye GraphQL Client, right?

@yrodiere
Copy link
Collaborator Author

I would like us to keep one client per installation as I want to make sure no information leak.

We have to anyway as each installation would require a different token and authorization provider. That's completely compatible with my proposal.

If we cached clients, there would be one cached client for the application client and one per installation ID for the installation clients. Which means we would use the same client across all requests for the same installation, but not across different installations.

And even if we don't cache the clients, authorization providers could help as they would ensure a client stays valid forever, even if event processing (or background processes) take a long time.

Also we would still have the problem with the SmallRye GraphQL Client, right?

I didn't look how that client work, so I don't know. If the "problem" you're mentioning is the leak, though, here too it's irrelevant as we'd still need one client per installation.

@gsmet
Copy link
Member

gsmet commented Sep 21, 2022

I didn't look how that client work, so I don't know. If the "problem" you're mentioning is the leak, though, here too it's irrelevant as we'd still need one client per installation.

No the problem was that we would probably have to keep the current behavior for this client if it doesn't provide a similar mechanism.
But not sure that's a showstopper for implementing a different behavior for the GitHub client.

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

No branches or pull requests

2 participants