APP-7001: Add UpdateOauthApp CLI command #4641
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
| "os/signal" | ||
| "path/filepath" | ||
| "runtime/debug" | ||
| "slices" | ||
| "strings" | ||
| "time" | ||
|
|
||
|
|
@@ -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 | ||
|
There was a problem hiding this comment. @stuqdog is this ok? linter wants it to be URLValidation, but There was a problem hiding this comment. 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. There was a problem hiding this comment. |
||
| 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
There was a problem hiding this comment. At this point, I'd just make shadow types to wrap around the proto enum types. You can look at the clients in |
||
|
|
||
| // 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 app %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", | ||
| oauthAppFlagClientAuthentication, clientAuthentication) | ||
| } | ||
|
Comment on lines
+2127
to
+2132
There was a problem hiding this comment. Similar to what my above comment, I would just use a shadow type with |
||
|
|
||
| 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", | ||
|
|
||
| oauthAppFlagPKCE, 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", | ||
| oauthAppFlagURLValidation, 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", | ||
| oauthAppFlagEnabledGrants, eg) | ||
| } | ||
| enabledGrantsProto[i] = apppb.EnabledGrant(enum) | ||
| } | ||
| return enabledGrantsProto, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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