[cli] Add WorkOS identity provider support for Restate Cloud login #4061
Conversation
|
|
||
| use super::IdentityProvider; | ||
|
|
||
| const IDENTITY_PROVIDER_DISCOVERY_URL: &str = "https://cloud.restate.dev/.idp-config"; |
There was a problem hiding this comment.
here's what i do now to do dev logins:
cat ~/.config/restate-dev/config.toml
[global]
[global.cloud]
api_base_url = "https://api.dev.restate.cloud"
login_base_url = "https://auth-dev.restate.cloud"
client_id = "cua6drm2md11pur6d5blnj9d4"
could this idp url come from config too, so i could just have the dev url in my config and then be used to get the other values if they are unset?
cli/src/commands/cloud/mod.rs
Outdated
| fn default() -> Self { | ||
| Self { | ||
| environment_info: None, | ||
| provider: IdentityProvider::Cognito { client_id: None }, |
There was a problem hiding this comment.
could this be simpler:
- default provider is IdentityProvider::Idp with the prod idp url (but my dev cli config can override this)
- if provider is idp, load from idp, fail if this doesnt work
- otherwise, use the values from config, but a none client id isnt allowed
there would be no parsing the config file to see if an override is set, and dev cli configs just need a single field set. but you can still skip idp if you have a specific provider set in config
|
I haven't tested the individual field overrides, but your suggestion works great - I've added the API base URL so now you'll be able to just do this and be set up with the dev defaults: [global.cloud]
config_discovery_url = "https://dev.cloud.restate.dev/.auth-config"Will update https://github.com/restatedev/restate-cloud-ui/pull/133 to include the extra fields and new route path. |
| pub client_id: String, | ||
| pub redirect_ports: Vec<u16>, | ||
| pub config_discovery_url: Option<Url>, | ||
| pub provider: Option<String>, |
There was a problem hiding this comment.
maybe bikeshedding but i had imagined that we just have an IdentityProvider enum with Discover/Cognito/WorkOS all as options. so in your config you can either override the discover url, or you can manually set the provider to cognito at some client id, or manually set to workod at some client id
There was a problem hiding this comment.
It made a bit more sense to me to do it this way – basically override individual parameters of the discovered remote config. I think it makes a lot of sense to do what you want if we weren't ourselves in control of both ends, but since it's just us who will end up using this, I think it's more convenient the way it is, even if it makes it a bit easier to break things. But again, since it's only a handful of Restate engineers who will ever make use of these, I think it's ok. I know you're easy with this but if you prefer it the way you suggested, I'll happily change it – it's definitely a more sound approach.
There was a problem hiding this comment.
youre closer to the problem - lets go with your instinct!
jackkleeman
left a comment
jackkleeman
left a comment
There was a problem hiding this comment.
makes sense to me, left a stylistic comment about the overrides but since almost no one would set those, it matters very little
|
Ack! I'm not in a rush to merge this, and will happily change it if you feel it should work that way. |
pcholakov
force-pushed
the
pavel/zqqkursvynnq
branch
from
50787c1 to
ea36f66
Compare
With this change:
restate cloud loginwill always fetch https://cloud.restate.dev/.idp-config and attempt to configure itself based on that, unless there is a local override already; the thinking is users don't do this more than ~once a day, so we're not gaining much from waiting, and might confuse things when the default does eventually change