APP-6999: Implement “ReadOAuthApplication” CLI command #4652
Conversation
viambot
added
the
safe to test
| Usage: "manage the OAuth applications for an organization", | ||
| Subcommands: []*cli.Command{ | ||
| { | ||
| Name: "read", |
There was a problem hiding this comment.
Why are we changing get to read for pulling the config for an OAuth application. Copied this from the scope:
"viam organization auth-service oauth-app create / read / update / delete " is a very logical replacement for "*viam auth-app register / update / get*". From a UX perspective, it is cleaner to create oauth-apps within the auth-service.
The other changes make sense to me but read doesn't seem either industry or viam standard to encapsulate making a GET request for an object.
We use get for logo / support email and get-config for branded billing, and it seems the standard for our CLI is get or just the name of the object being returned (i.e. robot logs returns the robot logs) . I've also never seen read an alias in other SDKs (happy to be proven wrong here) but this alias doesn't make logical sense to me.
Something like config to show that we return the config for the auth application makes a lot more sense to me.
| return nil | ||
| } | ||
|
|
||
| func protoToString(protoString, prefixToTrim string) string { |
There was a problem hiding this comment.
super-nit: I would make this something like formatStringForOutput() or something more along those lines. This is both taking an input and returning a string as output so it not really doing the conversion / there isn't really a conversion to be done.
May also make sense to put this in utils.go file
There was a problem hiding this comment.
Also agree on changing the name of this function, since there's no conversion going on, but reformatting.
| config := resp.OauthConfig | ||
| printf(cCtx.App.Writer, "OAuth config for client ID %s:", clientID) | ||
| printf(cCtx.App.Writer, "") | ||
| printf(cCtx.App.Writer, "Client Authentication: %s", protoToString(config.ClientAuthentication.String(), clientAuthenticationPrefix)) |
There was a problem hiding this comment.
Do you know why we return these prefixes if we are going to be removing them? I know this isn't your problem but that is the case we should be doing this formatting in the server itself
There was a problem hiding this comment.
The prefix is there for all proto enums. In the Go SDK, we put these enums into shadow types with prefixes as well because funcCall(passwordRequired, methodUnspecified) is more readable than funcCall(required, unspecified ...). In this case, it's a little annoying to present, but I think it's good practice to keep these as is since the configs are returning specific enum values.
There was a problem hiding this comment.
Oh interesting, can you elaborate more on what this means for my general learning?
we put these enums into shadow types with prefixes
Where do we define how to do this?
There was a problem hiding this comment.
As a general rule, we avoid giving users direct access to proto types, so we define shadow types that wrap around proto types. There are a lot of examples in the app wrappers (i.e. FragmentVisibility). Then we have helper functions to convert them.
There was a problem hiding this comment.
You'll see that we keep these prefixes as it's a good idea to have an inkling of what public means when it's used. Which is why in this case when we're trying to present them, it's a little annoying.
| OauthConfig: &apppb.OAuthConfig{ | ||
| ClientAuthentication: apppb.ClientAuthentication_CLIENT_AUTHENTICATION_REQUIRED, | ||
| Pkce: apppb.PKCE_PKCE_REQUIRED, | ||
| UrlValidation: apppb.URLValidation_URL_VALIDATION_ALLOW_WILDCARDS, |
There was a problem hiding this comment.
could you add a test with some of the prefixes as well?
There was a problem hiding this comment.
Is this test not already testing the prefixes? These variables wouldn't pass otherwise (i.e. CLIENT_AUTHENTICATION_REQUIRED becoming required)
| return nil | ||
| } | ||
|
|
||
| func protoToString(protoString, prefixToTrim string) string { |
There was a problem hiding this comment.
Also agree on changing the name of this function, since there's no conversion going on, but reformatting.
| OauthConfig: &apppb.OAuthConfig{ | ||
| ClientAuthentication: apppb.ClientAuthentication_CLIENT_AUTHENTICATION_REQUIRED, | ||
| Pkce: apppb.PKCE_PKCE_REQUIRED, | ||
| UrlValidation: apppb.URLValidation_URL_VALIDATION_ALLOW_WILDCARDS, |
There was a problem hiding this comment.
Is this test not already testing the prefixes? These variables wouldn't pass otherwise (i.e. CLIENT_AUTHENTICATION_REQUIRED becoming required)
| config := resp.OauthConfig | ||
| printf(cCtx.App.Writer, "OAuth config for client ID %s:", clientID) | ||
| printf(cCtx.App.Writer, "") | ||
| printf(cCtx.App.Writer, "Client Authentication: %s", protoToString(config.ClientAuthentication.String(), clientAuthenticationPrefix)) |
There was a problem hiding this comment.
The prefix is there for all proto enums. In the Go SDK, we put these enums into shadow types with prefixes as well because funcCall(passwordRequired, methodUnspecified) is more readable than funcCall(required, unspecified ...). In this case, it's a little annoying to present, but I think it's good practice to keep these as is since the configs are returning specific enum values.
Adds
viam organizations auth-service oauth-app read --org-id=<org-id> --client-id=<client-id>Manual Testing:
