api: add gRPC calls to create and delete connectors

[sks]

• Following up on PR #925

• This change defines 4 new API calls:

1. CreateConnector
2. UpdateConnector
3. DeleteConnector
4. ListConnector

• Added unit tests for each.

[sks]

cc: @rithujohn191 / @srenatus

[srenatus]

Did not include ListConnectors from the original version as that seems like a security hole

Why is that? The GRPC API is not supposed to be public -- it's a inter-service API supporting stuff like your use case. How is exposing ListConnectors a security problem there? 🤔

[sks]

@srenatus : My initial thought was to have limited access to connectors.

But as i think about it more, it could be okay. But did not want to expose the connector config in the list connector API.

I am okay with implementing the list connector API as well, I don’t have a use case for it.

~Your call :)~

Added the API in https://github.com/dexidp/dex/pull/1489/commits/402a828a0fd29582e7c4afb93a396f86c0a08867

[sks]

~Not sure how to get the latest protoc and use V3,~

~I ran make proto, but it still generated the V2 API :/~

~Checking further~

Fixed in e8ab80ca0f9bae8063fea8e9b915ab8f1c8d5606,

git clean -xffd
make proto

[srenatus]

@sks yup, I'm afraid I've merged a PR bumping all dependencies this morning. So, the generated protobuf code would look different now. Thanks for bearing with me

[sks]

@srenatus : please :)

Thank you for maintaining the project , I am trying to do my part while solving my own problems :)

[jpatters]

Is there potential for this to get merged sometime soon?

[lewismarshall]

I've tested this implementation and looks good to me. @srenatus @sks are we waiting on more unit tests / or just a comment fix. Maybe able to help...

[sks]

Hi @srenatus / @bonifaido,

Not sure what needs to be done so as to get a closure on this one.\

I have been maintaining a fork with this change for my use case.

Would love to kill that fork once this PR is merged. :pray:

[sagikazarmark]

For a start: can you please rebase this PR from the latest master?

TBH I'm not sure I see the added value of configuring connectors through the API (yet). What's your use case for it?

Another thing I would consider (not sure if it's the right idea though) is creating a separate proto modul for connectors. Having one, single proto module for the entire API is usually an anti-pattern IMHO.

[jpatters]

What's your use case for it?

@sagikazarmark I want to provide my customers with a way to configure their own connectors through a web interface. If there is another way I can achieve this then I would be very interested in hearing about it. This is the only thing keeping me from using dex.

[sks]

What's your use case for it

Similar to @jpatters , our use case was in a multi-tenant environment, we wanted to add new connectors when onboarding new tenants.

[piotrmsc]

Any update on this one from reviewers? :) @sagikazarmark @srenatus

[sagikazarmark]

I got this on my radar again. First of all: thanks for the patience and the continuous work on this one @sks

Sadly, it's going to need some more rework: we've recently relocated the API package to a new place and decided not to add new features to the original one (which we kept for backward compatibility). You can read more about that here: https://github.com/dexidp/dex/releases/tag/api%2Fv2.0.0

tl;dr: the new API calls should be added to another proto file.

I would also take another stab at the connector configuration, to see if things improved since the last review.

But in general, this looks okay. So @sks can you take a look at this again?

It would also be nice to have a few simple, step-by-step instruction for trying it. I'd love to test some edge cases, like what happens if I remove a connector in the middle of an authentication flow. Stuff like that.

Thanks!

[sks]

@sagikazarmark : Thank you for your contribution in cleaning up the proto clutter.

I have made the changes in api/v2/api.proto and the changes got copied over to api/api.proto as soon as I ran the make proto command, the rest of the changes were not impacted.

This is how I tested the changes on my local:

make bin/dex


## Uncommented the GRPC section in config
# grpc:
# addr: 127.0.0.1:5557

rm examples/dex.db
./bin/dex serve examples/config-dev.yaml  

The example grpc client has been modified, it can be found here

https://gist.github.com/sks/2928ef8cc9488ebe7858e45139d0a0df

[cdoan1]

@sagikazarmark @sks can this still be merged ? Since we can already manage clients through grpc, it would be equally nice to manage the connectors through grpc.

[iverberk]

We would also really like to see this merged. Our use-case is similar: to be able to add a new connector for a new tenant without an elaborate config update and Dex restart.

[sks]

I have updated the branch with the latest from dex/master, Please let me know in case I have missed on anything

[iverberk]

Is there anything I can do to help get this reviewed / merged?

[iverberk]

@sks we've started to use your branch in our fork as well, until this (hopefully) gets merged. Let me know if you need any help keeping it up to date.

[sks]

@nabokihms / @sagikazarmark : ping

please let me know how we can get this PR to closure.

--Thanks Sabith

[iverberk]

@sagikazarmark any possibility of reviewing and merging this?

[iverberk]

@sks are you still maintaining the branch for this? To keep up with latest master?

[Idane]

@sagikazarmark Is there anything that needs to be done to proceed with this PR? We'd benefit from it immensely.

[sagikazarmark]

It needs a thorough review. I added it to my list, but my plate is quite full lately, so I don't know when I'll get to it.

[dplynskiy]

Is there potential for this to get merged sometime soon?