JetBrains / js-graphql-intellij-plugin

GraphQL language support for WebStorm, IntelliJ IDEA and other IDEs based on the IntelliJ Platform.
https://jimkyndemeyer.github.io/js-graphql-intellij-plugin/
MIT License
879 stars 97 forks source link

Support ssl config with certificates in http client for introspection. #502

Closed srinivasankavitha closed 2 years ago

srinivasankavitha commented 2 years ago

This PR adds basic functionality for the user to specify ssl config for using ssl certificates to set up authentication for the HttpClient. The is PR does not address all the formats (only PEM and not DER etc.). In addition, there is no support for specifying key store and private key passwords.

{
  "name": "Remote Schema",
  "schemaPath": "remote-schema.graphql",
  "sslConfiguration": {
    "clientCertificate": {
      "path": "./certificates/user.crt"
    },
    "clientCertificateKey": {
      "path": "./certificates/user.key",
      "format": "PEM"
    }
  }
}
srinivasankavitha commented 2 years ago

Note that this PR does not attempt to address all possible use cases, as is intended as a minimal implementation that allows us to use ssl certs and keys specified in PEM format.

vepanimas commented 2 years ago

Hi! I haven't had time to check it carefully yet, but I can already say that it's better to provide sslConfiguration key under extensions key, not on the root level. We still use a legacy configuration format https://github.com/kamilkisiela/graphql-config/tree/legacy#specifying-endpoint-info, but I'm gonna rewrite configuration management for the v3 in the near future, and it would be easier if we tried to conform to the specification https://graphql-config.com/usage/#extensions Also, it would be really great if you could write a few words about configuring and using it in the README.md 🙏

srinivasankavitha commented 2 years ago

Sounds good, I'll move it. Thanks.

On Mon, Oct 25, 2021 at 6:48 PM Vladimir Panimaskin < @.***> wrote:

Hi! I haven't had time to check it carefully yet, but I can already say that it's better to provide sslConfiguration key under extensions key, not on the root level. We still use a legacy configuration format https://github.com/kamilkisiela/graphql-config/tree/legacy#specifying-endpoint-info, but I'm gonna rewrite configuration management for the v3 in the near future, and it would be easier if we tried to conform to the specification https://graphql-config.com/usage/#extensions

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jimkyndemeyer/js-graphql-intellij-plugin/pull/502#issuecomment-951480024, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXJERG3N3M3X4R7YF4DUIYCH7ANCNFSM5GWJVIIQ .

srinivasankavitha commented 2 years ago

@vepanimas - Moved the ssl config out to be part of the extensions config. Please let me know if anything else is needed.

vepanimas commented 2 years ago

Please note that I've changed a lot in the main branch in the last few days, and it's better to take the latest version.

vepanimas commented 2 years ago

Hi @srinivasankavitha! Thank you for the patience and for the fixes you've done before 😊. There were some conflicts after my changes in the repo, so I've resolved them and cleaned up a bit after (formatting, optimized imports, and added some nullability checks). I'm going to create a release this week, so all those changes should be available soon.

srinivasankavitha commented 2 years ago

That's great! Thanks so much for the review and detailed feedback to get this merged.

srinivasankavitha commented 2 years ago

@vepanimas - is it feasible to enable federation in the language settings by default? Even though all users won't need it, it would be available out of the box for those who do, and not cause any issues for users who don't use federation in any case? If you agree, we can have that as well for the upcoming release.

vepanimas commented 2 years ago

@srinivasankavitha I'm not sure that it won't cause any issues. The first thing that comes to mind is "directive is already defined" errors for Federation directives, users can define their own which have the same name (e.g., @key and @extends directives have a very generic name, it could easily lead to collisions). I'm pretty sure we'll get bug reports right after the release if we enable it by default, unfortunately.

directive @external on FIELD_DEFINITION
directive @requires(fields: _FieldSet!) on FIELD_DEFINITION
directive @provides(fields: _FieldSet!) on FIELD_DEFINITION
directive @key(fields: _FieldSet!) repeatable on OBJECT | INTERFACE
directive @extends repeatable on OBJECT | INTERFACE

The right way is to detect the framework and show some kind of notification, it's not trivial, so it will take some time to implement properly. I have it on the roadmap, but not for that release.