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

Api key support #768

Merged
merged 3 commits into from
Aug 22, 2023
Merged

Api key support #768

merged 3 commits into from
Aug 22, 2023

Conversation

loneil
Copy link
Collaborator

@loneil loneil commented Aug 21, 2023

Add plugin support for getting a token for a tenant via the API Key.

Modify Tenant UI login page to support both Wallet creds and API key options.

One caveat/discussion point for this is that there exists 2 token endpoints

  • {wallet_id}/token
  • {tenant_id}/token

Current token fetching via the tenant UI is the wallet_id one (a calling LOB could theoretically use either but we've generally gone with wallet_id).
What I modified here is the tenant_id endpoint, as that's the one that's actually in the Innkeeper plugin.
So what a caller would do is use the Tenant ID and API Key(s). Which is not totally ideal as it's a different ID than if they use the wallet ID.
But then I also kind of like it this way as it's a completely separate deliverable set of creds to a caller that don't associate with the wallet itself. It's tenant based, not wallet based... Probably some discussion to be had here. I think we should modify the {wallet_id}/token endpoint to support this as well, and fix a separate issue I've discussed with Sherman too around securing the call.

@loneil loneil temporarily deployed to development August 21, 2023 22:47 — with GitHub Actions Inactive
@github-actions
Copy link

@loneil loneil temporarily deployed to development August 22, 2023 04:39 — with GitHub Actions Inactive
@loneil loneil temporarily deployed to development August 22, 2023 16:48 — with GitHub Actions Inactive
@loneil loneil temporarily deployed to development August 22, 2023 17:00 — with GitHub Actions Inactive
Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil temporarily deployed to development August 22, 2023 17:19 — with GitHub Actions Inactive
@usingtechnology
Copy link
Collaborator

Just a comment. The tenant_id based token was added in traction because we cannot seed a wallet_id. So for the innkeeper tenant/wallet, we needed a way to fetch tokens with a known id. And wallet_id is not known until it is created. So that is really there for the innkeeper tenant itself to bootstrap.

@loneil
Copy link
Collaborator Author

loneil commented Aug 22, 2023

Just a comment. The tenant_id based token was added in traction because we cannot seed a wallet_id. So for the innkeeper tenant/wallet, we needed a way to fetch tokens with a known id. And wallet_id is not known until it is created. So that is really there for the innkeeper tenant itself to bootstrap.

Thanks @usingtechnology .
It's a convenient endpoint to encapsulate token fetching with an API Key in the Innkeeper plugin. As well allows us to have API access use Tenant based values rather than wallet (although someone with an API Key token can get the wallet ID so not a security thing or anything).
So leaning towards keeping API access through that endpoint only rather than extending to wallet_id/token as well.

Any concerns you can think of with using {tenant_id}/token given it's original intent? Or think it's all good repurpose for this as well?

@usingtechnology
Copy link
Collaborator

No, no real issues except for possible confusion? May want to think about not allowing token fetch by wallet_id? So using traction is tenant based only. Or just leave it and document the API Key is only used with the tenant_id endpoint. And if you are guiding developers to using that tenant_id endpoint, then you should update the tenant-ui to use it. It's the best example of using traction apis.

Copy link
Collaborator

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't run it, but the code looks good to me!

@esune
Copy link
Member

esune commented Aug 22, 2023

No, no real issues except for possible confusion? May want to think about not allowing token fetch by wallet_id? So using traction is tenant based only. Or just leave it and document the API Key is only used with the tenant_id endpoint. And if you are guiding developers to using that tenant_id endpoint, then you should update the tenant-ui to use it. It's the best example of using traction apis.

I think we could leave them both, but as suggested by @usingtechnology pivot teh Tenant Ui to use the tenant id and api-key where possible, as it provides a good abstraction layer for our users (something similar might be implemented for OIDC-based access btw). We can leave the "native" wallet id/key endpoint for specific use-cases (if any) and for compatibility reasons. We can always decide to enforce stricter rules in the future if we decide to go the way of fully managed tenants (i.e.: the tenant will not necessarily need to know wallet id/key to access their APIs).

@loneil
Copy link
Collaborator Author

loneil commented Aug 22, 2023

No, no real issues except for possible confusion? May want to think about not allowing token fetch by wallet_id? So using traction is tenant based only. Or just leave it and document the API Key is only used with the tenant_id endpoint. And if you are guiding developers to using that tenant_id endpoint, then you should update the tenant-ui to use it. It's the best example of using traction apis.

Yeah I like this idea too.

@loneil loneil merged commit f63a09b into main Aug 22, 2023
11 checks passed
@loneil loneil temporarily deployed to development August 22, 2023 23:22 — with GitHub Actions Inactive
@loneil loneil deleted the feature/apiKeyToken branch August 23, 2023 16:42
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.

3 participants