-
Notifications
You must be signed in to change notification settings - Fork 77
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
Conversation
Hey @benjamincanac . Don't mean to be a pest. Just wanted to check in on this....comments, concerns, needs to be reworked? |
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
And the
What do you think? |
@benjamincanac No worries! Thanks for your thoughts and the other merge as well :) Yeah, I will totally revert to the other 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 My current implementation would introduce a small breaking change, but could easily revert to the current token behavior by setting the config to The solution you posted above at least makes the use of an api token possible for the Anyhow, I will start making some of those changes now and most likely update this PR today. |
Ok, So in order to eliminate breaking changes but allow for greater customization, how about this logic for 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 @benjamincanac Thoughts? |
@jiblett1000 Seems fine to me 😊 |
@benjamincanac Awesome. I'll get crackin' on an update to this. Thanks! |
@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 There are two possible solutions I see to this....
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}`
}
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. |
On second thought, maybe that's over complicating things. If someone has |
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Benjamin Canac <canacb1@gmail.com>
✅ Live Preview ready!
|
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 |
Types of changes
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
anduseStrapi4
composables and moving the previous type refactoring into theuseStrapi
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:
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
anduseStrapiClient
.api
(the module default), the composables will make requests with theapiToken
(if set), otherwise no token will be passed. This means requests will use thepublic
role.user
, the composables will mimic the current module behavior, using thejwt
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
anduseStrapiUserToken
come in and also the removal of theuseStrapiToken
(which is now useStrapiUserToken). For example, say you have 'defaultToken' set to 'api', you can still authenticate with the user token as such: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 theuseStrapiClient
composable. Definitely could use another pair of eyes on those changes though.I think that just about sums it up! Thanks for reading :)
Checklist: