Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APP-7001: Add UpdateOauthApp CLI command #4641

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 91 additions & 5 deletions cli/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"github.com/urfave/cli/v2"
apppb "go.viam.com/api/app/v1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

// CLI flags.
Expand Down Expand Up @@ -124,11 +125,20 @@ const (

packageMetadataFlagFramework = "model_framework"

authApplicationFlagName = "application-name"
authApplicationFlagApplicationID = "application-id"
authApplicationFlagOriginURIs = "origin-uris"
authApplicationFlagRedirectURIs = "redirect-uris"
authApplicationFlagLogoutURI = "logout-uri"
authApplicationFlagName = "application-name"
authApplicationFlagApplicationID = "application-id"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

authApplicationFlagOriginURIs = "origin-uris"
authApplicationFlagRedirectURIs = "redirect-uris"
authApplicationFlagLogoutURI = "logout-uri"
authApplicationFlagClientID = "client-id"
authApplicationFlagClientName = "client-name"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
authApplicationFlagClientID = "client-id"
authApplicationFlagClientName = "client-name"
authApplicationFlagOAuthApplicationClientID = "client-id"
authApplicationFlagOAuthApplicationClientName = "client-name"

authApplicationFlagClientAuthentication = "client-authentication"
authApplicationFlagPKCE = "pkce"
authApplicationFlagEnabledGrants = "enabled-grants"
authApplicationFlagURLValidation = "url-validation"
authServiceFlag = "auth-service"
authOAuthAppFlag = "oauth-app"
unspecified = "unspecified"

cpFlagRecursive = "recursive"
cpFlagPreserve = "preserve"
Expand Down Expand Up @@ -424,6 +434,82 @@ var app = &cli.App{
Usage: "work with organizations",
HideHelpCommand: true,
Subcommands: []*cli.Command{
{
Name: "auth-service",
Usage: "manage auth-service",
Subcommands: []*cli.Command{
{
Name: "oauth-app",
Usage: "manage the oauth applications for an organization",
Subcommands: []*cli.Command{
{
Name: "update",
Usage: "update an oauth application",
Flags: []cli.Flag{
&cli.StringFlag{
Name: generalFlagOrgID,
Required: true,
Usage: "organization ID that is tied to the oauth application",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
&cli.StringFlag{
Name: authApplicationFlagClientID,
Usage: "id for the oauth application to be updated",
Required: true,
},
&cli.StringFlag{
Name: authApplicationFlagClientName,
Usage: "updated name for the oauth application",
Required: false,
},
&cli.StringFlag{
Name: authApplicationFlagClientAuthentication,
Usage: "updated client authentication policy for the oauth application. can be one of " +
allEnumValues(clientAuthenticationPrefix, apppb.ClientAuthentication_value),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Required: false,
Value: unspecified,
},
&cli.StringFlag{
Name: authApplicationFlagURLValidation,
Usage: "updated url validation for the oauth application. can be one of " +
allEnumValues(urlValidationPrefix, apppb.URLValidation_value),
Required: false,
Value: unspecified,
},
&cli.StringFlag{
Name: authApplicationFlagPKCE,
Usage: "updated pkce for the oauth application. can be one of " +
allEnumValues(pkcePrefix, apppb.PKCE_value),
Required: false,
Value: unspecified,
},
&cli.StringSliceFlag{
Name: authApplicationFlagOriginURIs,
Usage: "updated comma separated origin uris for the oauth application",
Required: false,
},
&cli.StringSliceFlag{
Name: authApplicationFlagRedirectURIs,
Usage: "updated comma separated redirect uris for the oauth application",
Required: false,
},
&cli.StringFlag{
Name: authApplicationFlagLogoutURI,
Usage: "updated logout uri for the oauth application",
Required: false,
},
&cli.StringSliceFlag{
Name: authApplicationFlagEnabledGrants,
Usage: "updated comma separated enabled grants for the auth application. values can be of " +
allEnumValues(enabledGrantPrefix, apppb.EnabledGrant_value),
Required: false,
},
},
Action: createCommandWithT[updateOAuthAppArgs](UpdateOAuthAppAction),
},
},
},
},
},
{
Name: "list",
Usage: "list organizations for the current user",
Expand Down
133 changes: 133 additions & 0 deletions cli/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"os/signal"
"path/filepath"
"runtime/debug"
"slices"
"strings"
"time"

Expand Down Expand Up @@ -2050,3 +2051,135 @@ func logEntryFieldsToString(fields []*structpb.Struct) (string, error) {
}
return message + "}", nil
}

type updateOAuthAppArgs struct {
OrgID string
ClientID string
ClientName string
ClientAuthentication string
Pkce string
LogoutURI string
UrlValidation string //nolint:revive,stylecheck
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OriginURIs []string
RedirectURIs []string
EnabledGrants []string
}

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, ", ") + "]"
}
Comment on lines +2068 to +2084
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


// UpdateOAuthAppAction is the corresponding action for 'oauth-app update'.
func UpdateOAuthAppAction(c *cli.Context, args updateOAuthAppArgs) error {
client, err := newViamClient(c)
if err != nil {
return err
}

return client.updateOAuthAppAction(c, args)
}

func (c *viamClient) updateOAuthAppAction(cCtx *cli.Context, args updateOAuthAppArgs) error {
if err := c.ensureLoggedIn(); err != nil {
return err
}

req, err := createUpdateOAuthAppRequest(args)
if err != nil {
return err
}

_, err = c.client.UpdateOAuthApp(c.c.Context, req)
if err != nil {
return err
}

printf(cCtx.App.Writer, "Successfully updated oauth application %s", args.ClientID)
return nil
}

func createUpdateOAuthAppRequest(args updateOAuthAppArgs) (*apppb.UpdateOAuthAppRequest, error) {
orgID := args.OrgID
clientID := args.ClientID
clientName := args.ClientName
clientAuthentication := args.ClientAuthentication
urlValidation := args.UrlValidation
pkce := args.Pkce
originURIs := args.OriginURIs
redirectURIs := args.RedirectURIs
logoutURI := args.LogoutURI
enabledGrants := args.EnabledGrants

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",
authApplicationFlagClientAuthentication, clientAuthentication)
}

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

authApplicationFlagPKCE, pkce)
}

urlValidationEnum, ok := apppb.URLValidation_value[urlValidationPrefix+strings.ToUpper(urlValidation)]
if !ok {
return nil, errors.Errorf("%s must be a valid UrlValidation, got %s. "+
"See `viam organizations auth-service oauth-app update --help` for supported options",
authApplicationFlagURLValidation, urlValidation)
}

egProto, err := enabledGrantsToProto(enabledGrants)
if err != nil {
return nil, err
}

req := &apppb.UpdateOAuthAppRequest{
OrgId: orgID,
ClientId: clientID,
ClientName: clientName,
OauthConfig: &apppb.OAuthConfig{
ClientAuthentication: apppb.ClientAuthentication(clientAuthenticationEnum),
Pkce: apppb.PKCE(pkceEnum),
UrlValidation: apppb.URLValidation(urlValidationEnum),
OriginUris: originURIs,
RedirectUris: redirectURIs,
LogoutUri: logoutURI,
EnabledGrants: egProto,
},
}
return req, nil
}

func enabledGrantsToProto(enabledGrants []string) ([]apppb.EnabledGrant, error) {
if enabledGrants == nil {
return nil, nil
}
enabledGrantsProto := make([]apppb.EnabledGrant, len(enabledGrants))
for i, eg := range enabledGrants {
enum, ok := apppb.EnabledGrant_value[enabledGrantPrefix+strings.ToUpper(eg)]
if !ok {
return nil, errors.Errorf("%s must consist of valid EnabledGrants, got %s. "+
"See `viam organizations auth-service oauth-app update --help` for supported options",
authApplicationFlagEnabledGrants, eg)
}
enabledGrantsProto[i] = apppb.EnabledGrant(enum)
}
return enabledGrantsProto, nil
}
28 changes: 28 additions & 0 deletions cli/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1001,3 +1001,31 @@ func TestShellFileCopy(t *testing.T) {
})
})
}

func TestUpdateOAuthAppAction(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

updateOAuthAppFunc := func(ctx context.Context, in *apppb.UpdateOAuthAppRequest,
opts ...grpc.CallOption,
) (*apppb.UpdateOAuthAppResponse, error) {
return &apppb.UpdateOAuthAppResponse{}, nil
}
asc := &inject.AppServiceClient{
UpdateOAuthAppFunc: updateOAuthAppFunc,
}

flags := make(map[string]any)
flags[generalFlagOrgID] = "org-id"
flags[authApplicationFlagClientID] = "client-id"
flags[authApplicationFlagClientName] = "client-name"
flags[authApplicationFlagClientAuthentication] = "required"
flags[authApplicationFlagURLValidation] = "allow_wildcards"
flags[authApplicationFlagPKCE] = "not_required"
flags[authApplicationFlagOriginURIs] = []string{"https://woof.com/login", "https://arf.com/"}
flags[authApplicationFlagRedirectURIs] = []string{"https://woof.com/home", "https://arf.com/home"}
flags[authApplicationFlagLogoutURI] = "https://woof.com/logout"
flags[authApplicationFlagEnabledGrants] = []string{"implicit", "password"}
cCtx, ac, out, errOut := setup(asc, nil, nil, nil, flags, "token")

test.That(t, ac.updateOAuthAppAction(cCtx, parseStructFromCtx[updateOAuthAppArgs](cCtx)), test.ShouldBeNil)
test.That(t, len(errOut.messages), test.ShouldEqual, 0)
test.That(t, out.messages[1], test.ShouldContainSubstring, "Successfully updated oauth application")
}
12 changes: 12 additions & 0 deletions testutils/inject/app_service_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ type AppServiceClient struct {
opts ...grpc.CallOption) (*apppb.OrganizationSetLogoResponse, error)
OrganizationGetLogoFunc func(ctx context.Context, in *apppb.OrganizationGetLogoRequest,
opts ...grpc.CallOption) (*apppb.OrganizationGetLogoResponse, error)
UpdateOAuthAppFunc func(ctx context.Context, in *apppb.UpdateOAuthAppRequest,
opts ...grpc.CallOption) (*apppb.UpdateOAuthAppResponse, error)
CreateLocationFunc func(ctx context.Context, in *apppb.CreateLocationRequest,
opts ...grpc.CallOption) (*apppb.CreateLocationResponse, error)
GetLocationFunc func(ctx context.Context, in *apppb.GetLocationRequest,
Expand Down Expand Up @@ -403,6 +405,16 @@ func (asc *AppServiceClient) OrganizationGetLogo(
return asc.OrganizationGetLogoFunc(ctx, in, opts...)
}

// UpdateOAuthApp calls the injected UpdateOAuthAppFunc or the real version.
func (asc *AppServiceClient) UpdateOAuthApp(
ctx context.Context, in *apppb.UpdateOAuthAppRequest, opts ...grpc.CallOption,
) (*apppb.UpdateOAuthAppResponse, error) {
if asc.UpdateOAuthAppFunc == nil {
return asc.AppServiceClient.UpdateOAuthApp(ctx, in, opts...)
}
return asc.UpdateOAuthAppFunc(ctx, in, opts...)
}

// CreateLocation calls the injected CreateLocationFunc or the real version.
func (asc *AppServiceClient) CreateLocation(
ctx context.Context, in *apppb.CreateLocationRequest, opts ...grpc.CallOption,
Expand Down
Loading