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: allow configurable tokens in nextjs app example #666

Merged
merged 8 commits into from
Jul 31, 2023

Conversation

anitarua
Copy link
Contributor

@anitarua anitarua commented Jul 21, 2023

Addresses the first part of #385 but this time for the NextJS example app. Since we're really just setting two variables, I figured it would be overkill to try to extract the same code into a separate SDK function or library, but open to suggestions!

Similar to #663, I created a new config file where users can specify token permissions and expiry before deploying the token vending machine. Instructions are in the README and examples are in the src/app/api/momento/token/config.ts file. (I am also open to moving the config file out of the token directory. My first thought was that keeping them together would make more sense but if it seems too difficult for people to find, definitely open to moving it.)

@anitarua anitarua marked this pull request as ready for review July 21, 2023 21:51
export const tokenPermissions: TokenScope = {
permissions: [
{
role: CacheRole.ReadWrite,
Copy link
Contributor

Choose a reason for hiding this comment

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

This would actually be cool to be part of the api, maybe query params or something. The user could select a cache and a topic, and then we can make a request to get a scoped token for that specific cache and topic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that be similar to this?

Would there need to be a way to stack multiple permissions though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I was thinking for the demo would be cool for it to just scope it down as low as possible. But we could also just accept a post body of all the permissions that the user wants the token to have

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the point of Anita's config file is to show app owners/maintainers how to restrict the scope (at app deploy time) so that the browser can't get tokens that have permissions to unexpected/unwanted caches. If we make it super dynamic then it's not really that different from just giving out an AllDataReadWrite token.

We can discuss this more for follow-up PRs, if there is something dynamic that is cool and worth demo'ing I'm open to talking about it! But for now this is more about showing how to make it secure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue that I see here is that now the demo only works if a user has a cause default-cache already created

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I agree with that Matt, see my other comment and see if that addresses your concern

Copy link
Contributor

Choose a reason for hiding this comment

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

tbh i wonder if we should just take the cache chooser out of the app. doesn't have to be in this PR but just thinking.

Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

looks good! just one minor suggestion about the default state of the config file.

@anitarua anitarua requested a review from cprice404 July 25, 2023 21:36
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

one tiny nit in the doc comment and then we should be good to go!

@anitarua anitarua requested a review from cprice404 July 26, 2023 16:04
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

one minor wording suggestion then 🚢

@anitarua anitarua requested a review from cprice404 July 27, 2023 16:14
@anitarua anitarua merged commit 80ab1fb into main Jul 31, 2023
10 checks passed
@anitarua anitarua deleted the nextjs-configurable-tokens branch July 31, 2023 15:46
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