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: composable refactoring, api tokens, default config #318

Closed
wants to merge 22 commits into from

Conversation

jiblett1000
Copy link
Contributor

@jiblett1000 jiblett1000 commented Feb 18, 2023

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

Resolves: #254

This PR essentially adds the ability to use an api token. It also allows the developer to choose which token they would like to use by default (api token or user token (jwt)). This is useful because depending on the developers use of Strapi, it might be desirable to not have all requests authenticated when a user is logged in. For example, if a Strapi instance has 10+ roles or numerous content-apis, this requires going in to each role and setting permissions for each when these requests don't need to be authorized in the first place. It could potentially get very tedious.

Also included in this PR is the removal of the useStrapi3 and useStrapi4 composables and moving the previous type refactoring into the useStrapi composables included with each version. I can of course add the previous code back in. I did this partially to help me wrap my head around things as I was thinking about how to refactor things, but at some point I reckon these should be removed anyways. Either way, I'm viewing this as somewhat of a preliminary PR, so I'm happy to change things around if necessary. I will of course need to follow up with a PR for documentation as well.

Continuing on with more details and code...

This PR adds two module options:

strapi: {
    apiToken: process.env.STRAPI_API_TOKEN || ' ',
    defaultToken: 'api',
}

The apiToken is as it sounds. A setting to include the api token you would like to use for requests.

The second is which token you would like to use by default for the strapi composables useStrapi and useStrapiClient.

  • If set to api (the module default), the composables will make requests with the apiToken (if set), otherwise no token will be passed. This means requests will use the public role.
  • If set to user, the composables will mimic the current module behavior, using the jwt if available (user logged in) and not passing the token if unavailable (user logged out).

However, regardless of which default is chosen, there are times when you will want to make requests with the alternate authorization method. This is where the composables useStrapiApiToken and useStrapiUserToken come in and also the removal of the useStrapiToken (which is now useStrapiUserToken). For example, say you have 'defaultToken' set to 'api', you can still authenticate with the user token as such:

const userToken = useStrapiUserToken()

const { find, update } = useStrapi({ token: userToken })

Of course the reverse can be done with useStrapiApiToken or with any custom token for that matter.

I've used a parameter object here partially because I am a fan of object parameters in general but also to allow the potential to further configure options per composable instance ( baseURL, prefix, version, etc ). Although, customizing such options would require further refactoring within the module and might not be necessary or desirable. I would love to hear feedback on that from users of this module.

There are some type fixes regarding the DefaultErrors in the useStrapiClient composable. Definitely could use another pair of eyes on those changes though.

I think that just about sums it up! Thanks for reading :)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes (if not applicable, please state why)

@jiblett1000
Copy link
Contributor Author

Hey @benjamincanac . Don't mean to be a pest. Just wanted to check in on this....comments, concerns, needs to be reworked?

@benjamincanac
Copy link
Member

Hello @jiblett1000, so sorry for the delay I completely missed this.

Honestly I'd rather not introduce any breaking change to the module right now, so if we could keep the old composables it would be better IMO.

Also, if I understand correctly, the need is to be able to use an api token for some requests. Why not just pass a token option to useStrapiClient() with a token like you did here https://github.com/nuxt-modules/strapi/pull/318/files#diff-8726078c9d7eb5ebbc8bd4098453bd6129e96d707458c073dd05928f8859beeeR42? These options could also be forwarded from useStrapi() to every use of useStrapiClient(). I don't see the need to create a breaking change for the useStrapiToken() as users could store this token in their runtime config and use it like this:

const config = useRuntimeConfig()
const { find, update } = useStrapi({ token: config.my.token })

And the useStrapiClient code could look like this:

if (options?.token) {
  headers.Authorization = `Bearer ${options.token}`
} else if (token.value) {
  headers.Authorization = `Bearer ${token.value}`
}

What do you think?

@jiblett1000
Copy link
Contributor Author

@benjamincanac No worries! Thanks for your thoughts and the other merge as well :)

Yeah, I will totally revert to the other useStrapi composables. No problem. I'm fine with eliminating the useStrapiApiToken and useStrapiUserToken as well. Although I do like the distinction of knowing what kind of token is being used over useStrapiToken (which is not entirely clear). But either way, I understand not wanting to introduce those breaking changes right now and using useRuntimeConfig is just as easy really.

I think you essentially get the need here. Especially considering api tokens are configurable from the Strapi admin interface, I think there needs to be an easier way to utilize them from the client.

I do still think there is value in being able to set the api_token and default_token config option however. This allows the developer to choose the default token behavior (depending on the majority and type of requests being made) and potentially not require having to pass a token per useStrapi instance as much. This would also allow the developer to use the api token set in the config essentially in place of the public role for some added api security without the verbosity of passing the api token every time.

My current implementation would introduce a small breaking change, but could easily revert to the current token behavior by setting the config to default_token: 'user'. I think I explained the behavior pretty good up above, but I can elaborate more on that if needed.

The solution you posted above at least makes the use of an api token possible for the useStrapi and useStrapiClient and that is definitely what's most important. I'm fine just going with that for now if you think that's best, but I think the default_token behavior is still worth considering (at some point at least).

Anyhow, I will start making some of those changes now and most likely update this PR today.

@jiblett1000
Copy link
Contributor Author

Ok, So in order to eliminate breaking changes but allow for greater customization, how about this logic for useStrapiClient?

if (options?.token) {
  headers.Authorization = `Bearer ${options.token}`
} else if (config.strapi.defaultToken === 'user' && userToken.value) {
  headers.Authorization = `Bearer ${userToken.value}`
} else if (config.strapi.defaultToken === 'api' && config.strapi.apiToken) {
  headers.Authorization = `Bearer ${config.strapi.apiToken}`
}

and in the default module config:

{
 apiToken: process.env.STRAPI_API_TOKEN || '',
 defaultToken: 'user',
}

So, essentially this will allow the developer to choose whether they would like to use an api token or user token by default for the composables and since the module default for the default_token is user, it will maintain the current behavior by using the user token when it is available and the public role otherwise. And of course, if/when the dev needs to override the token per composable instance, they are still able to pass in whatever token they desire.

@benjamincanac Thoughts?

Copy link
Member

@jiblett1000 Seems fine to me 😊

@jiblett1000
Copy link
Contributor Author

@benjamincanac Awesome. I'll get crackin' on an update to this. Thanks!

@jiblett1000
Copy link
Contributor Author

jiblett1000 commented Mar 6, 2023

@benjamincanac Sorry to keep buggin' you. I know you've probably got bigger and better things on your plate.

I realized my PR still doesn't account for a likely scenario someone will want to use an api key for, which is to add some light security to their api and not expose it publicly. The default token behavior is great, but when it is set to defaultToken: 'user' (which is now the default), it will fall back to not passing a token (public role). A dev might need to fall back to the api token set in there config rather than pass no token at all.

There are two possible solutions I see to this....

  1. If the apiToken is set in the config, pass it automatically rather than no token at all. This would change the client logic to:
    if (options?.token) {
      headers.Authorization = `Bearer ${options.token}`
    } else if (config.strapi.defaultToken === 'user' && userToken.value) {
      headers.Authorization = `Bearer ${userToken.value}`
    } else if ((config.strapi.defaultToken === 'api' && config.strapi.apiToken) || config.strapi.apiToken) {
      headers.Authorization = `Bearer ${config.strapi.apiToken}`
    }
  1. Add another config option (something like privateApi: true | false) which will fallback to the api token when the config is set to true, otherwise no token will be passed (the current behavior).
    if (options?.token) {
      headers.Authorization = `Bearer ${options.token}`
    } else if (config.strapi.defaultToken === 'user' && userToken.value) {
      headers.Authorization = `Bearer ${userToken.value}`
    } else if ((config.strapi.defaultToken === 'api' || config.strapi.privateApi) && config.strapi.apiToken) {
      headers.Authorization = `Bearer ${config.strapi.apiToken}`
    }

Personally, I would stray towards adding the additional config option so as to not impose a particular behavior on someone. Open to a different name for the config option as well. I just wanted to bring this to light before starting work on the documentation as this would change things a bit.

@jiblett1000
Copy link
Contributor Author

On second thought, maybe that's over complicating things. If someone has user as the default token and also set the apiToken in there config, chances are they're wanting to use the apiToken instead of the public role....

@nuxt-studio
Copy link

nuxt-studio bot commented Mar 7, 2023

Live Preview ready!

Name Edit Preview Latest Commit
Strapi Edit on Studio ↗︎ View Live Preview f0a5697

@jiblett1000 jiblett1000 mentioned this pull request Mar 7, 2023
6 tasks
@jiblett1000
Copy link
Contributor Author

I somehow managed to completely mess this PR up by rebasing or something. I've still got a ways to go before I'm a git wizard. Anyhow, I redid the PR nice and clean here: #326

@jiblett1000 jiblett1000 closed this Mar 7, 2023
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.

Allow the use of the API token by adding Authorization header
3 participants