-
Notifications
You must be signed in to change notification settings - Fork 72
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
AN-614: Allowed security scheme must exist in config.json #1102
Conversation
a7554cf
to
7dc733c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM.
I think we should also verify that the the security scheme value is set via the apiCredentials
https://docs.api3.org/airnode/v0.6/grp-providers/guides/build-an-airnode/api-security.html#step-3-specify-the-values-for-the-defined-security-schemes but we can do that as a separate issue? WDYT?
Right, I thought about doing that at first but then I realized that |
Mhh, actually maybe we can bring this up at the call. I can imagine an user taking full OAS of their API, making a few changes to convert it to OIS and enable only a subset of triggers and security schemes. That suggests we should not do this check and allow "unused" security schemes. But for other users this will be an annoying error they'll need to debug 🤷 . However, for enabled security schemes (via the |
Yes, and this has been how I've been approaching "reference" validation in Airkeeper and Airkeeper. For those config files I'm only adding checks for things being referenced but unused things are not being validated for convenience since having those config values there don't really hurt and makes testing easier when switching between config values.
The thing is that we only care about |
No. I mean when you have a security scheme that is defined and also enabled. Such as this then we should verify the value is also supplied in |
But what if the Airnode is only relaying metadata using securitySchemes? https://docs.api3.org/airnode/v0.4/grp-providers/guides/build-an-airnode/api-security.html#relayed-meta-data-security-schemes |
Oh, I see what you mean now. Yes, we only care about those two. But if they are defined and used, there should be a value defined for them in |
78e2ef4
to
fcaa29e
Compare
Thanks for the thorough and detailed code review @Siegrift 🙏🏻 |
fd2f848
to
e236447
Compare
… in enabled and it is of type 'apiKey' or 'http'
e236447
to
decc200
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
AN-614: used zod.superRefine() in order to validate that the security scheme enabled under
ois.apiSpecifications.security
is defined inois.apiSpecifications.components
.