Skip to content

Commit

Permalink
fix: update to check for supported scopes for OAuth destinations (#4585)
Browse files Browse the repository at this point in the history
  • Loading branch information
sanpj2292 committed Apr 22, 2024
1 parent a44f92e commit f1a8b8c
Show file tree
Hide file tree
Showing 7 changed files with 221 additions and 96 deletions.
2 changes: 1 addition & 1 deletion regulation-worker/internal/delete/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func (m *APIManager) deleteWithRetry(ctx context.Context, job model.Job, destina
Config: destination.Config,
DefinitionConfig: destination.DestDefConfig,
}
isOAuth, err := dest.IsOAuthDestination()
isOAuth, err := dest.IsOAuthDestination(common.RudderFlowDelete)
if err != nil {
pkgLogger.Error(err)
return model.JobStatus{Status: model.JobStatusFailed, Error: err}
Expand Down
85 changes: 26 additions & 59 deletions regulation-worker/internal/delete/api/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,13 @@ type oauthTestCases struct {
isOAuthV2Enabled bool
}

var defaultDestDefConfig = map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"rudderScopes": []interface{}{"delete"},
},
}

var oauthTests = []oauthTestCases{
{
name: "test with a valid token and successful response",
Expand Down Expand Up @@ -307,12 +314,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "xyz",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
deleteResponses: []deleteResponseParams{
{
Expand Down Expand Up @@ -359,12 +362,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "xyz",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
deleteResponses: []deleteResponseParams{
{
Expand Down Expand Up @@ -419,12 +418,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "xyz",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
cpResponses: []testutils.CpResponseParams{
{
Expand Down Expand Up @@ -466,12 +461,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "xyz",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
cpResponses: []testutils.CpResponseParams{
{
Expand Down Expand Up @@ -522,12 +513,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
cpResponses: []testutils.CpResponseParams{},
deleteResponses: []deleteResponseParams{{}},
Expand Down Expand Up @@ -570,11 +557,7 @@ var oauthTests = []oauthTestCases{
DestinationID: "1234",
Config: map[string]interface{}{},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
DestDefConfig: defaultDestDefConfig,
},
cpResponses: []testutils.CpResponseParams{},
deleteResponses: []deleteResponseParams{{}},
Expand Down Expand Up @@ -611,12 +594,8 @@ var oauthTests = []oauthTestCases{
Config: map[string]interface{}{
"rudderDeleteAccountId": "xyz",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},

oauthHttpClientTimeout: 1 * time.Second,
Expand Down Expand Up @@ -664,12 +643,8 @@ var oauthTests = []oauthTestCases{
"rudderDeleteAccountId": "xyz",
"authStatus": "active",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
deleteResponses: []deleteResponseParams{
{
Expand Down Expand Up @@ -715,12 +690,8 @@ var oauthTests = []oauthTestCases{
"rudderDeleteAccountId": "xyz",
"authStatus": "active",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
deleteResponses: []deleteResponseParams{
{
Expand Down Expand Up @@ -767,12 +738,8 @@ var oauthTests = []oauthTestCases{
"rudderDeleteAccountId": "xyz",
"authStatus": "active",
},
Name: "GA",
DestDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
Name: "GA",
DestDefConfig: defaultDestDefConfig,
},
deleteResponses: []deleteResponseParams{
{
Expand Down
3 changes: 2 additions & 1 deletion router/transformer/transformer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,8 @@ var oauthDests = []backendconfig.DestinationT{
Name: "SALESFORCE_OAUTH",
Config: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"type": "OAuth",
"rudderScopes": []interface{}{"delivery"},
},
},
},
Expand Down
48 changes: 41 additions & 7 deletions services/oauth/v2/destination_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ package v2
import (
"fmt"

"github.com/samber/lo"

"github.com/rudderlabs/rudder-server/services/oauth"
"github.com/rudderlabs/rudder-server/services/oauth/v2/common"
"github.com/rudderlabs/rudder-server/utils/misc"
)

Expand All @@ -15,7 +18,7 @@ type DestinationInfo struct {
Config map[string]interface{}
}

func (d *DestinationInfo) IsOAuthDestination() (bool, error) {
func (d *DestinationInfo) IsOAuthDestination(flow common.RudderFlow) (bool, error) {
authValue, _ := misc.NestedMapLookup(d.DefinitionConfig, "auth", "type")
if authValue == nil {
// valid use-case for non-OAuth destinations
Expand All @@ -26,22 +29,53 @@ func (d *DestinationInfo) IsOAuthDestination() (bool, error) {
// we should throw error here, as we expect authValue to be a string if present
return false, fmt.Errorf("auth type is not a string: %v", authValue)
}
return authType == string(oauth.OAuth), nil
isScopeSupported, err := d.IsOAuthSupportedForFlow(string(flow))
if err != nil {
return false, err
}
return authType == string(oauth.OAuth) && isScopeSupported, nil
}

func (d *DestinationInfo) IsOAuthSupportedForFlow(flow string) (bool, error) {
rudderScopesValue, _ := misc.NestedMapLookup(d.DefinitionConfig, "auth", "rudderScopes")
if rudderScopesValue == nil {
// valid use-case for non-OAuth destinations
// when the auth.type is OAuth and rudderScopes is not mentioned, we would assume oauth flow is to be used when it is in "delivery" flow
return flow == string(common.RudderFlowDelivery), nil
}
interfaceArr, ok := rudderScopesValue.([]interface{})
if !ok {
return false, fmt.Errorf("rudderScopes should be a interface[]")
}
var rudderScopes []string
for _, scopeInterface := range interfaceArr {
scope, ok := scopeInterface.(string)
if !ok {
return false, fmt.Errorf("%v in auth.rudderScopes should be string", scopeInterface)
}
rudderScopes = append(rudderScopes, scope)
}
return lo.Contains(rudderScopes, flow), nil
}

/*
GetAccountID Gets AccountId for OAuth destination based on if rudderFlow is `Delivery` or `Delete`
Example:
`GetAccountID(destDetail.Config, "rudderDeleteAccountId")` --> To be used when we make use of OAuth during regulation flow
`GetAccountID(destDetail.Config, "rudderAccountId")` --> To be used when we make use of OAuth during normal event delivery
`GetAccountID(common.RudderFlowDelete)` --> To be used when we make use of OAuth during regulation flow
`GetAccountID(common.RudderFlowDelivery)` --> To be used when we make use of OAuth during normal event delivery
*/
func (d *DestinationInfo) GetAccountID(idKey string) (string, error) {
rudderAccountIdInterface, found := d.Config[idKey]
oauthDest, err := d.IsOAuthDestination()
func (d *DestinationInfo) GetAccountID(flow common.RudderFlow) (string, error) {
oauthDest, err := d.IsOAuthDestination(flow)
if err != nil {
return "", fmt.Errorf("failed to check if destination is oauth destination: %v", err)
}

idKey := common.DeliveryAccountIDKey
if flow == common.RudderFlowDelete {
idKey = common.DeleteAccountIDKey
}
rudderAccountIdInterface, found := d.Config[idKey]
if !oauthDest || !found || idKey == "" {
return "", fmt.Errorf("destination is not an oauth destination or accountId not found")
}
Expand Down
133 changes: 133 additions & 0 deletions services/oauth/v2/destination_info_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
package v2_test

import (
"fmt"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

v2 "github.com/rudderlabs/rudder-server/services/oauth/v2"
"github.com/rudderlabs/rudder-server/services/oauth/v2/common"
)

type isOAuthResult struct {
isOAuth bool
err error
}

type destInfoTestCase struct {
description string
flow common.RudderFlow
inputDefConfig map[string]interface{}
expected isOAuthResult
}

var isOAuthDestTestCases = []destInfoTestCase{
{
description: "should pass for a destination which contains OAuth and rudderScopes",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"rudderScopes": []interface{}{"delivery"},
},
},
expected: isOAuthResult{
isOAuth: true,
},
},
{
description: "should pass for a destination which contains OAuth but not rudderScopes",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
expected: isOAuthResult{
isOAuth: true,
},
},
{
description: "should return 'false' without error for a destination which contains OAuth with delete rudderScopes when flow is delivery",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"rudderScopes": []interface{}{"delete"},
},
},
expected: isOAuthResult{
isOAuth: false,
},
},
{
description: "should return 'true' without error for a destination which contains OAuth withoutrudderScopes when flow is delivery",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
},
},
expected: isOAuthResult{
isOAuth: true,
},
},
{
description: "should return 'false' with error for a destination which contains OAuth with one of invalid rudderScopes when flow is delivery",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"rudderScopes": []interface{}{"delivery", 1},
},
},
expected: isOAuthResult{
isOAuth: false,
err: fmt.Errorf("1 in auth.rudderScopes should be string"),
},
},
{
description: "should return 'false' with error for a destination which contains OAuth with invalid rudderScopes type when flow is delivery",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{
"auth": map[string]interface{}{
"type": "OAuth",
"rudderScopes": []interface{}{"a"}[0],
},
},
expected: isOAuthResult{
isOAuth: false,
err: fmt.Errorf("rudderScopes should be a interface[]"),
},
},
{
description: "should return 'false' without error for a non-OAuth destination when flow is delivery",
flow: common.RudderFlowDelivery,
inputDefConfig: map[string]interface{}{},
expected: isOAuthResult{
isOAuth: false,
},
},
}

var _ = Describe("DestinationInfo tests", func() {
Describe("IsOAuthDestination tests", func() {
for _, tc := range isOAuthDestTestCases {
It(tc.description, func() {
d := &v2.DestinationInfo{
DefinitionName: "dest_def_name",
}
d.DefinitionConfig = tc.inputDefConfig
isOAuth, err := d.IsOAuthDestination(tc.flow)

Expect(isOAuth).To(Equal(tc.expected.isOAuth))
if tc.expected.err != nil {
Expect(err).To(Equal(tc.expected.err))
} else {
Expect(err).To(BeNil())
}
})
}
})
})
Loading

0 comments on commit f1a8b8c

Please sign in to comment.