hyperstackjs / hyperstack

The pragmatic app framework for builders 🐶
https://hyperstackjs.io
MIT License
389 stars 11 forks source link

Allow X-Access-Token header to be used along side access_token query string #2

Closed screencomuser closed 1 year ago

screencomuser commented 1 year ago

To be able to perform the following action, i would like to propose the attached changes

curl -s -H "X-Access-Token: ${TOKEN}" "http://localhost:5150/notes" | jq -r

Since hyperstack is opinionated, I also propose to include the config by default in the templates

And I've also added a default simple prettierrc since vscode complains about double qoutes :-)

jondot commented 1 year ago

Hi there! Thank you so much for contributing to our project, we really appreciate your help. You may have modified the low-level JWT library, but should have looked at the higher level controller that uses it. Don't worry, this is a common mistake and it's easy to fix.

What I would suggest is looking at the controller side of things, here. and here is the wiring of the bearer configuration, you see it is undefined, probably needs just to take a value from configuration to customize the behavior (the default is Bearer ..), and configure permit in this way

If you have any questions or need help with this, please let me know. I'm here to help and I'm excited to see what you can contribute to our project. Thank you again for your contribution!

screencomuser commented 1 year ago

Hi,

How would I go about overriding / implenting bearer when it's hardcoded to be undefined at https://github.com/hyperstackjs/hyperstack/blob/c9e660821b92a89fbe442e1e51ee0043c596004f/packages/initializer-jwt/src/index.ts#L34

I've looked and looked but could not find any point where the bearer option can be passed along to the final permit creation.

I hope you can shed more light on this. What changes should I make to get it working without modifying any of the packages.

Thank you in advance :-)

jondot commented 1 year ago

No worries, it should be easy. First, see that configuration is already being pulled, like here: https://github.com/hyperstackjs/hyperstack/blob/c9e660821b92a89fbe442e1e51ee0043c596004f/packages/initializer-jwt/src/index.ts#L25 So now, you need to add a new configuration key, for bearer, and pull it the same way, if it exists

screencomuser commented 1 year ago

Ehrm, that's exactly what I did in my changes in the pull request.

You confuse me. First you say I shouldn't change the code, but then I should change the code.

Perhaps you can create an example where I don't have to modify any of the packages code, like you mentioned in your earlier comment?

jondot commented 1 year ago

Oh my, You're exactly right -- sorry about that 🤦 I think I got confused by the fact that there was no undefined path in the code anymore, my apologies! I will go ahead and review the file modification + approve the CI run - thanks!