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

fix JWK checking #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix JWK checking #2

wants to merge 1 commit into from

Conversation

t8
Copy link

@t8 t8 commented Jan 10, 2021

No description provided.

@t8
Copy link
Author

t8 commented Jan 24, 2021

@johnletey & @fabianriewe - Would love a review here 😅

This fixes environment variable issues for any new AuthNodes coming online.

@johnletey
Copy link
Collaborator

@t8 Sorry for the late reply here 😅 If I'm understanding this correctly, you want to put the contents of your keyfile into the config?

I figured that users would want to instead put a path to their keyfile instead 🤷🏻‍♂️

@t8
Copy link
Author

t8 commented Jan 28, 2021

Ahh sorry @johnletey, I may have misunderstood. You seem to be checking the location statically here:
image

Because of this, the subsequent readFileSync throws errors whenever someone configures using environment variables. That is what this fix was for. If you want to check a path and support environment variables, I can push an update to this PR.

EDIT:
I now see you're checking config.json statically and not the keyfile. My bad!

@johnletey
Copy link
Collaborator

Ah yeah we are only statically checking the configuration file. However, we pull the keyfile path from the config 😄

@t8
Copy link
Author

t8 commented Jan 28, 2021

🌴 to forehead. Will push a fix shortly!

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.

2 participants