[TT-8351] fix jwt_scope_to_policy_mapping backwards compatibility. (T…

…ykTechnologies#4895)

<!-- Provide a general summary of your changes in the Title above -->

## Description

This PRs fixes broken backward compatibility for
`jwt_scope_to_policy_mapping`.
- added migration to new fields in `Migrate()` method.
- keeping `jwt_scope_to_policy_mapping` to be considered in middlewares
when the new fields (`scopes.jwt` / `scopes.oidc`) are not configured.

## Related Issue
https://tyktech.atlassian.net/browse/TT-8351

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [x] Refactoring or add test (improvements in base code or adds test
coverage to functionality)

## Checklist

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the checklist. -->

- [ ] I ensured that the documentation is up to date
- [ ] I explained why this PR updates go.mod in detail with reasoning
why it's required
- [ ] I would like a code coverage CI quality gate exception and have
explained why

apidef/api_definition_test.go

@@ -29,37 +29,6 @@ func TestSchema(t *testing.T) {
 }
 
 func TestEncodeForDB(t *testing.T) {
-	t.Run("update ScopeClaim when Scopes.JWT is not empty and OIDC is not enabled", func(t *testing.T) {
-		spec := DummyAPI()
-		defaultScopeName := "scope"
-		scopeToPolicyMap := map[string]string{
-			"user:read": "pID1",
-		}
-		spec.Scopes.JWT = ScopeClaim{
-			ScopeClaimName: defaultScopeName,
-			ScopeToPolicy:  scopeToPolicyMap,
-		}
-		spec.EncodeForDB()
-		assert.Equal(t, defaultScopeName, spec.JWTScopeClaimName)
-		assert.Equal(t, scopeToPolicyMap, spec.JWTScopeToPolicyMapping)
-	})
-
-	t.Run("update ScopeClaim when Scopes.OIDC is not empty and OpenID is enabled", func(t *testing.T) {
-		spec := DummyAPI()
-		defaultScopeName := "scope"
-		scopeToPolicyMap := map[string]string{
-			"user:read": "pID1",
-		}
-		spec.Scopes.OIDC = ScopeClaim{
-			ScopeClaimName: defaultScopeName,
-			ScopeToPolicy:  scopeToPolicyMap,
-		}
-		spec.UseOpenID = true
-		spec.EncodeForDB()
-		assert.Equal(t, defaultScopeName, spec.JWTScopeClaimName)
-		assert.Equal(t, scopeToPolicyMap, spec.JWTScopeToPolicyMapping)
-	})
-
 	t.Run("EncodeForDB persist schema objects from extended path", func(t *testing.T) {
 		spec := DummyAPI()
 		spec.EncodeForDB()
@@ -74,39 +43,6 @@ func TestEncodeForDB(t *testing.T) {
 }
 
 func TestDecodeFromDB(t *testing.T) {
-	t.Run("update Scopes.JWT when JWTScopeClaimName is not empty", func(t *testing.T) {
-		spec := DummyAPI()
-		defaultScopeName := "scope"
-		spec.JWTScopeClaimName = defaultScopeName
-		scopeToPolicyMap := map[string]string{
-			"user:read": "pID1",
-		}
-		spec.JWTScopeToPolicyMapping = scopeToPolicyMap
-		spec.DecodeFromDB()
-		expectedJWTScope := ScopeClaim{
-			ScopeClaimName: defaultScopeName,
-			ScopeToPolicy:  scopeToPolicyMap,
-		}
-		assert.Equal(t, expectedJWTScope, spec.Scopes.JWT)
-	})
-
-	t.Run("update Scopes.OIDC when JWTScopeClaimName is not empty and OpenID is enabled", func(t *testing.T) {
-		spec := DummyAPI()
-		defaultScopeName := "scope"
-		spec.JWTScopeClaimName = defaultScopeName
-		scopeToPolicyMap := map[string]string{
-			"user:read": "pID1",
-		}
-		spec.UseOpenID = true
-		spec.JWTScopeToPolicyMapping = scopeToPolicyMap
-		spec.DecodeFromDB()
-		expectedOICScope := ScopeClaim{
-			ScopeClaimName: defaultScopeName,
-			ScopeToPolicy:  scopeToPolicyMap,
-		}
-		assert.Equal(t, expectedOICScope, spec.Scopes.OIDC)
-	})
-
 	t.Run("json schema validation middleware", func(t *testing.T) {
 		apiDef := DummyAPI()
 		var (
@@ -185,3 +121,184 @@ func TestAPIDefinition_GenerateAPIID(t *testing.T) {
 	a.GenerateAPIID()
 	assert.NotEmpty(t, a.APIID)
 }
+
+func TestAPIDefinition_GetScopeClaimName(t *testing.T) {
+	var (
+		scopeName        = "scope"
+		oidcScopeName    = "oidc_scope"
+		newScopeName     = "new_scope"
+		newOIDCScopeName = "new_oidc_scope"
+	)
+
+	getAPIDef := func(deprecatedScopeName, jwtScopeName, oidcScopeName string, useOIDC bool) APIDefinition {
+		return APIDefinition{
+			UseOpenID:         useOIDC,
+			JWTScopeClaimName: deprecatedScopeName,
+			Scopes: Scopes{
+				JWT: ScopeClaim{
+					ScopeClaimName: jwtScopeName,
+				},
+				OIDC: ScopeClaim{
+					ScopeClaimName: oidcScopeName,
+				},
+			},
+		}
+	}
+
+	testCases := []struct {
+		name                string
+		deprecatedScopeName string
+		jwtScopeName        string
+		oidcScopeName       string
+		useOIDC             bool
+		expectedScopeName   string
+	}{
+		{
+			name:                "jwt: only deprecated fields",
+			deprecatedScopeName: scopeName,
+			expectedScopeName:   scopeName,
+		},
+		{
+			name:              "jwt: only scopes.jwt",
+			jwtScopeName:      newScopeName,
+			expectedScopeName: newScopeName,
+		},
+		{
+			name:              "jwt: both scopes.jwt and scopes.oidc",
+			jwtScopeName:      newScopeName,
+			oidcScopeName:     newOIDCScopeName,
+			expectedScopeName: newScopeName,
+		},
+		{
+			name:                "jwt: deprecated field and jwt.scopes",
+			deprecatedScopeName: scopeName,
+			jwtScopeName:        newScopeName,
+			expectedScopeName:   newScopeName,
+		},
+
+		{
+			name:                "oidc: only deprecated fields",
+			deprecatedScopeName: oidcScopeName,
+			expectedScopeName:   oidcScopeName,
+			useOIDC:             true,
+		},
+		{
+			name:              "oidc: only scopes.oidc",
+			oidcScopeName:     newOIDCScopeName,
+			expectedScopeName: newOIDCScopeName,
+			useOIDC:           true,
+		},
+		{
+			name:              "oidc: both scopes.jwt and scopes.oidc",
+			jwtScopeName:      newScopeName,
+			oidcScopeName:     newOIDCScopeName,
+			expectedScopeName: newOIDCScopeName,
+			useOIDC:           true,
+		},
+		{
+			name:                "oidc: deprecated field and oidc.scopes",
+			deprecatedScopeName: oidcScopeName,
+			oidcScopeName:       newOIDCScopeName,
+			expectedScopeName:   newOIDCScopeName,
+			useOIDC:             true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			apiDef := getAPIDef(tc.deprecatedScopeName, tc.jwtScopeName, tc.oidcScopeName, tc.useOIDC)
+			assert.Equal(t, tc.expectedScopeName, apiDef.GetScopeClaimName())
+		})
+	}
+}
+
+func TestAPIDefinition_GetScopeToPolicyMapping(t *testing.T) {
+	var (
+		scopeToPolicyMapping        = map[string]string{"jwtClaim": "pol1"}
+		newScopeToPolicyMapping     = map[string]string{"jwtClaim1": "pol1"}
+		oidcScopeToPolicyMapping    = map[string]string{"oidcClaim": "pol1"}
+		newOIDCScopeToPolicyMapping = map[string]string{"oidcClaim1": "pol1"}
+	)
+
+	getAPIDef := func(deprecatedScopeToPolicy, jwtScopeToPolicy, oidcScopeToPolicy map[string]string, useOIDC bool) APIDefinition {
+		return APIDefinition{
+			UseOpenID:               useOIDC,
+			JWTScopeToPolicyMapping: deprecatedScopeToPolicy,
+			Scopes: Scopes{
+				JWT: ScopeClaim{
+					ScopeToPolicy: jwtScopeToPolicy,
+				},
+				OIDC: ScopeClaim{
+					ScopeToPolicy: oidcScopeToPolicy,
+				},
+			},
+		}
+	}
+
+	testCases := []struct {
+		name                    string
+		deprecatedScopeToPolicy map[string]string
+		jwtScopeToPolicy        map[string]string
+		oidcScopeToPolicy       map[string]string
+		useOIDC                 bool
+		expectedScopeToPolicy   map[string]string
+	}{
+		{
+			name:                    "jwt: only deprecated fields",
+			deprecatedScopeToPolicy: scopeToPolicyMapping,
+			expectedScopeToPolicy:   scopeToPolicyMapping,
+		},
+		{
+			name:                  "jwt: only scopes.jwt",
+			jwtScopeToPolicy:      scopeToPolicyMapping,
+			expectedScopeToPolicy: scopeToPolicyMapping,
+		},
+		{
+			name:                  "jwt: both scopes.jwt and scopes.oidc",
+			jwtScopeToPolicy:      scopeToPolicyMapping,
+			oidcScopeToPolicy:     oidcScopeToPolicyMapping,
+			expectedScopeToPolicy: scopeToPolicyMapping,
+		},
+		{
+			name:                    "jwt: deprecated field and jwt.scopes",
+			deprecatedScopeToPolicy: scopeToPolicyMapping,
+			jwtScopeToPolicy:        newScopeToPolicyMapping,
+			expectedScopeToPolicy:   newScopeToPolicyMapping,
+		},
+
+		{
+			name:                    "oidc: only deprecated fields",
+			deprecatedScopeToPolicy: oidcScopeToPolicyMapping,
+			expectedScopeToPolicy:   oidcScopeToPolicyMapping,
+			useOIDC:                 true,
+		},
+		{
+			name:                  "oidc: only scopes.oidc",
+			oidcScopeToPolicy:     newOIDCScopeToPolicyMapping,
+			expectedScopeToPolicy: newOIDCScopeToPolicyMapping,
+			useOIDC:               true,
+		},
+		{
+			name:                  "oidc: both scopes.jwt and scopes.oidc",
+			jwtScopeToPolicy:      scopeToPolicyMapping,
+			oidcScopeToPolicy:     oidcScopeToPolicyMapping,
+			expectedScopeToPolicy: oidcScopeToPolicyMapping,
+			useOIDC:               true,
+		},
+		{
+			name:                    "oidc: deprecated field and oidc.scopes",
+			deprecatedScopeToPolicy: oidcScopeToPolicyMapping,
+			oidcScopeToPolicy:       newOIDCScopeToPolicyMapping,
+			expectedScopeToPolicy:   newOIDCScopeToPolicyMapping,
+			useOIDC:                 true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			apiDef := getAPIDef(tc.deprecatedScopeToPolicy, tc.jwtScopeToPolicy, tc.oidcScopeToPolicy, tc.useOIDC)
+			assert.Equal(t, tc.expectedScopeToPolicy, apiDef.GetScopeToPolicyMapping())
+		})
+	}
+
+}

apidef/api_definitions.go

@@ -15,6 +15,8 @@ import (
 	"github.com/lonelycode/osin"
 	"gopkg.in/mgo.v2/bson"
 
+	"github.com/TykTechnologies/tyk/internal/reflect"
+
 	"github.com/TykTechnologies/graphql-go-tools/pkg/engine/datasource/kafka_datasource"
 	"github.com/TykTechnologies/graphql-go-tools/pkg/execution/datasource"
 
@@ -965,14 +967,6 @@ func (a *APIDefinition) EncodeForDB() {
 	if a.Auth.AuthHeaderName == "" {
 		a.Auth = a.AuthConfigs["authToken"]
 	}
-	// JWTScopeToPolicyMapping and JWTScopeClaimName are deprecated and following code ensures backward compatibility
-	if !a.UseOpenID && a.Scopes.JWT.ScopeClaimName != "" {
-		a.JWTScopeToPolicyMapping = a.Scopes.JWT.ScopeToPolicy
-		a.JWTScopeClaimName = a.Scopes.JWT.ScopeClaimName
-	} else if a.UseOpenID && a.Scopes.OIDC.ScopeClaimName != "" {
-		a.JWTScopeToPolicyMapping = a.Scopes.OIDC.ScopeToPolicy
-		a.JWTScopeClaimName = a.Scopes.OIDC.ScopeClaimName
-	}
 }
 
 func (a *APIDefinition) DecodeFromDB() {
@@ -1039,14 +1033,6 @@ func (a *APIDefinition) DecodeFromDB() {
 
 	makeCompatible("authToken", a.UseStandardAuth)
 	makeCompatible("jwt", a.EnableJWT)
-	// JWTScopeToPolicyMapping and JWTScopeClaimName are deprecated and following code ensures backward compatibility
-	if !a.UseOpenID && a.JWTScopeClaimName != "" && a.Scopes.JWT.ScopeClaimName == "" {
-		a.Scopes.JWT.ScopeToPolicy = a.JWTScopeToPolicyMapping
-		a.Scopes.JWT.ScopeClaimName = a.JWTScopeClaimName
-	} else if a.UseOpenID && a.JWTScopeClaimName != "" && a.Scopes.OIDC.ScopeClaimName == "" {
-		a.Scopes.OIDC.ScopeToPolicy = a.JWTScopeToPolicyMapping
-		a.Scopes.OIDC.ScopeClaimName = a.JWTScopeClaimName
-	}
 }
 
 // Expired returns true if this Version has expired
@@ -1291,6 +1277,30 @@ func DummyAPI() APIDefinition {
 	}
 }
 
+func (a *APIDefinition) GetScopeClaimName() string {
+	if reflect.IsEmpty(a.Scopes) {
+		return a.JWTScopeClaimName
+	}
+
+	if a.UseOpenID {
+		return a.Scopes.OIDC.ScopeClaimName
+	}
+
+	return a.Scopes.JWT.ScopeClaimName
+}
+
+func (a *APIDefinition) GetScopeToPolicyMapping() map[string]string {
+	if reflect.IsEmpty(a.Scopes) {
+		return a.JWTScopeToPolicyMapping
+	}
+
+	if a.UseOpenID {
+		return a.Scopes.OIDC.ScopeToPolicy
+	}
+
+	return a.Scopes.JWT.ScopeToPolicy
+}
+
 var Template = template.New("").Funcs(map[string]interface{}{
 	"jsonMarshal": func(v interface{}) (string, error) {
 		bs, err := json.Marshal(v)

apidef/migration.go

@@ -232,6 +232,7 @@ func (a *APIDefinition) Migrate() (versions []APIDefinition, err error) {
 	a.migrateAuthenticationPlugin()
 	a.migrateIDExtractor()
 	a.migrateCustomDomain()
+	a.migrateScopeToPolicy()
 
 	versions, err = a.MigrateVersioning()
 	if err != nil {
@@ -423,3 +424,20 @@ func (a *APIDefinition) SetDisabledFlags() {
 		}
 	}
 }
+
+func (a *APIDefinition) migrateScopeToPolicy() {
+	scopeClaim := ScopeClaim{
+		ScopeClaimName: a.JWTScopeClaimName,
+		ScopeToPolicy:  a.JWTScopeToPolicyMapping,
+	}
+
+	a.JWTScopeToPolicyMapping = nil
+	a.JWTScopeClaimName = ""
+
+	if a.UseOpenID {
+		a.Scopes.OIDC = scopeClaim
+		return
+	}
+
+	a.Scopes.JWT = scopeClaim
+}