APP-7001: Add UpdateOauthApp CLI command #4641

gloriacai01 · 2024-12-18T17:13:49Z

Ticket here

maxhorowitz · 2024-12-19T20:34:05Z

cli/app.go

+	authApplicationFlagName                 = "application-name"
+	authApplicationFlagApplicationID        = "application-id"


Add a comment above these that they will be deprecated once https://viam.atlassian.net/browse/APP-6993 is complete. IDK what the standard is for adding comments in a public repo of ours, will defer to someone on the SDK/Netcode team for a decision here

maxhorowitz · 2024-12-19T20:34:35Z

cli/app.go

+	authApplicationFlagClientID             = "client-id"
+	authApplicationFlagClientName           = "client-name"


Suggested change

authApplicationFlagClientID = "client-id"

authApplicationFlagClientName = "client-name"

authApplicationFlagOAuthApplicationClientID = "client-id"

authApplicationFlagOAuthApplicationClientName = "client-name"

maxhorowitz · 2024-12-19T20:35:18Z

cli/app.go

+										&cli.StringFlag{
+											Name:     generalFlagOrgID,
+											Required: true,
+											Usage:    "organization ID that is tied to the oauth application",


Suggested change

Usage: "organization ID that is tied to the oauth application",

Usage: "organization ID that is tied to the OAuth application",

gloriacai01 · 2024-12-19T20:38:25Z

cli/client.go

+	ClientAuthentication string
+	Pkce                 string
+	LogoutURI            string
+	UrlValidation        string //nolint:revive,stylecheck


@stuqdog is this ok? linter wants it to be URLValidation, but parseStructFromCtx doesn't seem to recognize that the passed in flag in the cli is associated with this field when its URLValidation, so I have the field named as UrlValidation

Ooh yeah, that's a bit of a gotcha. I think what you did is correct, but thanks for flagging it! I'll file a ticket to revisit our struct parsing logic to see how we can avoid that in the future.

ticket: https://viam.atlassian.net/browse/RSDK-9592

maxhorowitz · 2024-12-19T20:39:55Z

cli/app.go

@@ -8,6 +8,7 @@ import (
 	"strings"

 	"github.com/urfave/cli/v2"
+	apppb "go.viam.com/api/app/v1"


Look into why we are adding this dependency now (I would think this would have existed already and adding it is a mistake).

Triple check to confirm the dependency on API is okay -- even though inherently this stuff is "dependent" on the API I feel like this is a red herring.

Starting to see that this may be okay - it is imported already in this package (only in a different file) - the real question becomes whether we want to hardcode the string values that we import here instead of importing them - to maybe check against breaking changes in our API ... ? Again - deferring to someone who owns this code path

I don't see an actual issue with this, but it is a bit jarring to see a proto being imported here

maxhorowitz · 2024-12-19T20:50:36Z

cli/client.go

+	pkceEnum, ok := apppb.PKCE_value[pkcePrefix+strings.ToUpper(pkce)]
+	if !ok {
+		return nil, errors.Errorf("%s must be a valid PKCE, got %s. "+
+			"See `viam organizations auth-service oauth-app update --help` for supported options",


maxhorowitz · 2024-12-19T20:51:01Z

cli/client_test.go

@@ -1001,3 +1001,31 @@ func TestShellFileCopy(t *testing.T) {
 		})
 	})
 }
+
+func TestUpdateOAuthAppAction(t *testing.T) {


Please add cases which check that passing in bad values to each of these will result in a specific error message

Thanks for adding !

maxhorowitz

Waiting on a second set of eyes for some of the qs / comments I've left on the PR. Content-wise, everything is okay to me. Some nits / renaming suggestions, a comment about adding test case coverage, and a question about importing go.viam.com/api/app/v1

purplenicole730

Instead of converting the enums manually each time, it would be more readable to make a shadow type of each enum with a ToProto() function that you can use in the function.

purplenicole730 · 2024-12-20T20:50:02Z

cli/app.go

+										&cli.StringFlag{
+											Name: oauthAppFlagClientAuthentication,
+											Usage: "updated client authentication policy for the OAuth application. can be one of " +
+												allEnumValues(clientAuthenticationPrefix, apppb.ClientAuthentication_value),


Please provide an example of what this would look like in the description so we know what these values mean and look like. This applies to all the other flags that also have a similar setup.

purplenicole730 · 2024-12-20T20:55:01Z

cli/app.go

@@ -8,6 +8,7 @@ import (
 	"strings"

 	"github.com/urfave/cli/v2"
+	apppb "go.viam.com/api/app/v1"


I don't see an actual issue with this, but it is a bit jarring to see a proto being imported here

purplenicole730 · 2024-12-20T20:56:37Z

cli/client.go

+const (
+	clientAuthenticationPrefix = "CLIENT_AUTHENTICATION_"
+	pkcePrefix                 = "PKCE_"
+	urlValidationPrefix        = "URL_VALIDATION_"
+	enabledGrantPrefix         = "ENABLED_GRANT_"
+)
+
+// allEnumValues returns the possible values we accept for a given proto enum.
+func allEnumValues(prefixToTrim string, enumValueMap map[string]int32) string {
+	var formattedValues []string
+	for values := range enumValueMap {
+		formattedValue := strings.ToLower(strings.TrimPrefix(values, prefixToTrim))
+		formattedValues = append(formattedValues, formattedValue)
+	}
+	slices.Sort(formattedValues)
+	return "[" + strings.Join(formattedValues, ", ") + "]"
+}


At this point, I'd just make shadow types to wrap around the proto enum types. You can look at the clients in rdk/app/ folder to see examples. (i.e. app.Visibility).

purplenicole730 · 2024-12-20T20:57:42Z

cli/client.go

+	clientAuthenticationEnum, ok := apppb.ClientAuthentication_value[clientAuthenticationPrefix+strings.ToUpper(clientAuthentication)]
+	if !ok {
+		return nil, errors.Errorf("--%s must be a valid ClientAuthentication, got %s. "+
+			"See `viam organizations auth-service oauth-app update --help` for supported options",
+			oauthAppFlagClientAuthentication, clientAuthentication)
+	}


Similar to what my above comment, I would just use a shadow type with toProto functions. This applies to all of the enums.

purplenicole730

really close, just some nits here and there

cli/app.go

purplenicole730 · 2025-01-07T15:30:17Z

cli/app.go

+											Usage: "updated client authentication policy for the OAuth application. can be one of " +
+												formatAcceptedValues(string(ClientAuthenticationUnspecified), string(ClientAuthenticationRequired),
+													string(ClientAuthenticationNotRequired), string(ClientAuthenticationNotRequiredWhenUsingPKCE)),


Please provide an example in your description to how the usage text for this entire function looks like

I'm not sure why there's a helper function, as this can be easily printed by writing: fmt.Sprintf("updated comma separated enabled grants for the OAuth application. values can be of [%s, %s, %s, %s, %s]", string(...), ...). It doesn't seem to really save that much work, but does makes it harder to read for developers.
I guess one can argue that it would be a good practice to make sure future developers use this format, so I'm okay with keeping it the way this is or removing the helper function

cli/app.go

purplenicole730 · 2025-01-07T15:46:46Z

cli/client.go

+func formatAcceptedValues(values ...string) string {
+	joined := strings.Join(values, ", ")
+	return "[" + joined + "]"
+}


If we're keeping this function, I feel like this helper function belongs somewhere else. Maybe utils.go?

cli/client.go

maxhorowitz

To other reviewers: sync over zoom with Gloria tested manually. All looks great to me.

purplenicole730 · 2025-01-07T17:36:17Z

cli/utils.go

+func formatAcceptedValues(values ...string) string {
+	joined := strings.Join(values, ", ")
+	return "[" + joined + "]"
+}


Sorry for asking you to move this again when I'm the one who said to put it here, but I realized we could put this under createUsageText() in cli/app.go. And then add a comment to clarify what it does.

purplenicole730

LGTM!

add updateoauthapp cli

b842a80

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 18, 2024

add comments

5ff5353

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 18, 2024

gloriacai01 requested a review from maxhorowitz December 18, 2024 17:18

lint

a7cb473

gloriacai01 requested a review from a team as a code owner December 18, 2024 22:03

gloriacai01 requested review from njooma and purplenicole730 December 18, 2024 22:03

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 18, 2024

change wording

3b461aa

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 19, 2024

add oauth-app command flag

6680cbd

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 19, 2024

make client name not required

ec0b6ad

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 19, 2024

maxhorowitz reviewed Dec 19, 2024

View reviewed changes

gloriacai01 commented Dec 19, 2024

View reviewed changes

maxhorowitz reviewed Dec 19, 2024

View reviewed changes

add tests to check invalid enum value error messages

b1c6350

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Dec 20, 2024

purplenicole730 requested changes Dec 20, 2024

View reviewed changes

add shadow types for proto values

20d76db

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 3, 2025

gloriacai01 requested a review from purplenicole730 January 3, 2025 21:32

gloriacai01 added 2 commits January 7, 2025 10:17

Merge branch 'main' of github.com:viamrobotics/rdk into app-7001

5567c8a

lint

9526521