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

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

APP-7001: Add UpdateOauthApp CLI command #4641

APP-7001: Add UpdateOauthApp CLI command #4641

gloriacai01 commented Dec 18, 2024 •

edited

Loading

maxhorowitz Dec 19, 2024

maxhorowitz Dec 19, 2024

maxhorowitz Dec 19, 2024

gloriacai01 Dec 19, 2024

stuqdog Dec 19, 2024

stuqdog Dec 19, 2024

maxhorowitz Dec 19, 2024

maxhorowitz Dec 19, 2024

maxhorowitz Dec 19, 2024

purplenicole730 Dec 20, 2024

maxhorowitz Dec 19, 2024

maxhorowitz Dec 19, 2024

maxhorowitz left a comment

purplenicole730 left a comment

purplenicole730 Dec 20, 2024

purplenicole730 Dec 20, 2024

purplenicole730 Dec 20, 2024

purplenicole730 Dec 20, 2024

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

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

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

APP-7001: Add UpdateOauthApp CLI command #4641

Are you sure you want to change the base?

APP-7001: Add UpdateOauthApp CLI command #4641

Conversation

gloriacai01 commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhorowitz left a comment

Choose a reason for hiding this comment

purplenicole730 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gloriacai01 commented Dec 18, 2024 •

edited

Loading