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-6999: Implement “ReadOAuthApplication” CLI command #4652

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
31 changes: 31 additions & 0 deletions cli/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ const (
authApplicationFlagRedirectURIs = "redirect-uris"
authApplicationFlagLogoutURI = "logout-uri"

oauthAppFlagClientID = "client-id"

cpFlagRecursive = "recursive"
cpFlagPreserve = "preserve"

Expand Down Expand Up @@ -425,6 +427,35 @@ 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: "read",
Copy link
Member

Choose a reason for hiding this comment

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

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.

@maxhorowitz @ohEmily

Usage: "read the OAuth configuration details",
Flags: []cli.Flag{
&cli.StringFlag{
Name: generalFlagOrgID,
Required: true,
Usage: "organization ID that is tied to the OAuth application",
},
&cli.StringFlag{
Name: oauthAppFlagClientID,
Usage: "id for the OAuth application",
Required: true,
},
},
Action: createCommandWithT[readOAuthAppArgs](ReadOAuthAppAction),
},
},
},
},
},
{
Name: "list",
Usage: "list organizations for the current user",
Expand Down
58 changes: 58 additions & 0 deletions cli/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2050,3 +2050,61 @@ func logEntryFieldsToString(fields []*structpb.Struct) (string, error) {
}
return message + "}", nil
}

type readOAuthAppArgs struct {
OrgID string
ClientID string
}

const (
clientAuthenticationPrefix = "CLIENT_AUTHENTICATION_"
pkcePrefix = "PKCE_"
urlValidationPrefix = "URL_VALIDATION_"
enabledGrantPrefix = "ENABLED_GRANT_"
)

// ReadOAuthAppAction is the corresponding action for 'organizations auth-service oauth-app read'.
func ReadOAuthAppAction(c *cli.Context, args readOAuthAppArgs) error {
client, err := newViamClient(c)
if err != nil {
return err
}

return client.readOAuthAppAction(c, args.OrgID, args.ClientID)
}

func (c *viamClient) readOAuthAppAction(cCtx *cli.Context, orgID, clientID string) error {
if err := c.ensureLoggedIn(); err != nil {
return err
}

req := &apppb.ReadOAuthAppRequest{OrgId: orgID, ClientId: clientID}
resp, err := c.client.ReadOAuthApp(c.c.Context, req)
if err != nil {
return err
}

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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

@purplenicole730 purplenicole730 Dec 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@purplenicole730 purplenicole730 Dec 23, 2024

Choose a reason for hiding this comment

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

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.

printf(cCtx.App.Writer, "PKCE (Proof Key for Code Exchange): %s", protoToString(config.Pkce.String(), pkcePrefix))
printf(cCtx.App.Writer, "URL Validation Policy: %s", protoToString(config.UrlValidation.String(), urlValidationPrefix))
printf(cCtx.App.Writer, "Logout URL: %s", config.LogoutUri)
printf(cCtx.App.Writer, "Redirect URLs: %s", strings.Join(config.RedirectUris, ", "))
if len(config.OriginUris) > 0 {
printf(cCtx.App.Writer, "Origin URLs: %s", strings.Join(config.OriginUris, ", "))
}

var enabledGrants []string
for _, eg := range config.GetEnabledGrants() {
enabledGrants = append(enabledGrants, protoToString(eg.String(), enabledGrantPrefix))
}
printf(cCtx.App.Writer, "Enabled Grants: %s", strings.Join(enabledGrants, ", "))

return nil
}

func protoToString(protoString, prefixToTrim string) string {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Also agree on changing the name of this function, since there's no conversion going on, but reformatting.

return strings.ToLower(strings.TrimPrefix(protoString, prefixToTrim))
}
39 changes: 39 additions & 0 deletions cli/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1050,3 +1050,42 @@ func TestShellFileCopy(t *testing.T) {
})
})
}

func TestReadOAuthApp(t *testing.T) {
readOAuthAppFunc := func(ctx context.Context, in *apppb.ReadOAuthAppRequest, opts ...grpc.CallOption) (
*apppb.ReadOAuthAppResponse, error,
) {
return &apppb.ReadOAuthAppResponse{
ClientName: "clientname",
ClientSecret: "fakesecret",
OauthConfig: &apppb.OAuthConfig{
ClientAuthentication: apppb.ClientAuthentication_CLIENT_AUTHENTICATION_REQUIRED,
Pkce: apppb.PKCE_PKCE_REQUIRED,
UrlValidation: apppb.URLValidation_URL_VALIDATION_ALLOW_WILDCARDS,
Copy link
Member

Choose a reason for hiding this comment

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

could you add a test with some of the prefixes as well?

Copy link
Member

Choose a reason for hiding this comment

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

Is this test not already testing the prefixes? These variables wouldn't pass otherwise (i.e. CLIENT_AUTHENTICATION_REQUIRED becoming required)

LogoutUri: "https://my-logout-uri.com",
OriginUris: []string{"https://my-origin-uri.com", "https://second-origin-uri.com"},
RedirectUris: []string{"https://my-redirect-uri.com"},
EnabledGrants: []apppb.EnabledGrant{apppb.EnabledGrant_ENABLED_GRANT_IMPLICIT, apppb.EnabledGrant_ENABLED_GRANT_PASSWORD},
},
}, nil
}

asc := &inject.AppServiceClient{
ReadOAuthAppFunc: readOAuthAppFunc,
}

cCtx, ac, out, errOut := setup(asc, nil, nil, nil, nil, "token")

test.That(t, ac.readOAuthAppAction(cCtx, "test-org-id", "test-client-id"), test.ShouldBeNil)
test.That(t, len(out.messages), test.ShouldEqual, 9)
test.That(t, len(errOut.messages), test.ShouldEqual, 0)
test.That(t, out.messages[0], test.ShouldContainSubstring, "OAuth config for client ID test-client-id")
test.That(t, out.messages[2], test.ShouldContainSubstring, "Client Authentication: required")
test.That(t, out.messages[3], test.ShouldContainSubstring, "PKCE (Proof Key for Code Exchange): required")
test.That(t, out.messages[4], test.ShouldContainSubstring, "URL Validation Policy: allow_wildcards")
test.That(t, out.messages[5], test.ShouldContainSubstring, "Logout URL: https://my-logout-uri.com")
test.That(t, out.messages[6], test.ShouldContainSubstring, "Redirect URLs: https://my-redirect-uri.com")
test.That(t, out.messages[7], test.ShouldContainSubstring, "Origin URLs: https://my-origin-uri.com, https://second-origin-uri.com")
test.That(t, out.messages[8], test.ShouldContainSubstring, "Enabled Grants: implicit, password")

}
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)
ReadOAuthAppFunc func(ctx context.Context, in *apppb.ReadOAuthAppRequest,
opts ...grpc.CallOption) (*apppb.ReadOAuthAppResponse, 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...)
}

// ReadOAuthApp calls the injected ReadOAuthAppFunc or the real version.
func (asc *AppServiceClient) ReadOAuthApp(
ctx context.Context, in *apppb.ReadOAuthAppRequest, opts ...grpc.CallOption,
) (*apppb.ReadOAuthAppResponse, error) {
if asc.ReadOAuthAppFunc == nil {
return asc.AppServiceClient.ReadOAuthApp(ctx, in, opts...)
}
return asc.ReadOAuthAppFunc(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