Skip to content

Merging to release-5-lts: [TT-10518] Tests/address some racy tests (#5771)#5788

Merged
buger merged 1 commit into
release-5-ltsfrom
merge/release-5-lts/0aa67fd80bca0e92e59d8f932bc35498d8b4b2e5
Nov 21, 2023
Merged

Merging to release-5-lts: [TT-10518] Tests/address some racy tests (#5771)#5788
buger merged 1 commit into
release-5-ltsfrom
merge/release-5-lts/0aa67fd80bca0e92e59d8f932bc35498d8b4b2e5

Conversation

@buger

@buger buger commented Nov 21, 2023

Copy link
Copy Markdown
Member

TT-10518 Tests/address some racy tests (#5771)

  • Adds unstable build tag to tests with !race (so we can run them
    with -race -tags=unstable)
  • Fixes a race over base middleware Logger(), SetLogger() and Base()
  • looping tests marked as flaky or racy respectively (jira tickets in
    comments)

https://tyktech.atlassian.net/browse/TT-10518


Co-authored-by: Tit Petric [email protected]

- Adds `unstable` build tag to tests with `!race` (so we can run them
with `-race -tags=unstable`)
- Fixes a race over base middleware Logger(), SetLogger() and Base()
- looping tests marked as flaky or racy respectively (jira tickets in
comments)

https://tyktech.atlassian.net/browse/TT-10518

---------

Co-authored-by: Tit Petric <[email protected]>
(cherry picked from commit 0aa67fd)
@buger
buger enabled auto-merge (squash) November 21, 2023 09:33
@github-actions

Copy link
Copy Markdown
Contributor

API Changes

--- prev.txt	2023-11-21 09:34:10.028610362 +0000
+++ current.txt	2023-11-21 09:34:07.012614314 +0000
@@ -6572,8 +6572,8 @@
 type BaseMiddleware struct {
 	Spec  *APISpec
 	Proxy ReturningHttpHandler
+	Gw    *Gateway `json:"-"`
 
-	Gw *Gateway `json:"-"`
 	// Has unexported fields.
 }
     BaseMiddleware wraps up the ApiSpec and Proxy objects to be included in a
@@ -6583,7 +6583,7 @@
     ApplyPolicies will check if any policies are loaded. If any are, it will
     overwrite the session state to use the policy values.
 
-func (t BaseMiddleware) Base() *BaseMiddleware
+func (t *BaseMiddleware) Base() *BaseMiddleware
 
 func (t BaseMiddleware) CheckSessionAndIdentityForValidKey(originalKey string, r *http.Request) (user.SessionState, bool)
     CheckSessionAndIdentityForValidKey will check first the Session store for a
@@ -6600,7 +6600,7 @@
 
 func (t BaseMiddleware) Init()
 
-func (t BaseMiddleware) Logger() (logger *logrus.Entry)
+func (t *BaseMiddleware) Logger() (logger *logrus.Entry)
 
 func (t BaseMiddleware) OrgSession(orgID string) (user.SessionState, bool)
 

@sweep-ai-deprecated

Copy link
Copy Markdown

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

@github-actions

Copy link
Copy Markdown
Contributor

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

@buger

buger commented Nov 21, 2023

Copy link
Copy Markdown
Member Author

API tests result: success
Branch used: refs/pull/5788/merge
Commit:
Triggered by: pull_request (@buger)
Execution page

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

86.3% 86.3% Coverage
0.0% 0.0% Duplication

@buger
buger merged commit 942cf18 into release-5-lts Nov 21, 2023
@buger
buger deleted the merge/release-5-lts/0aa67fd80bca0e92e59d8f932bc35498d8b4b2e5 branch November 21, 2023 10:15
mativm02 added a commit that referenced this pull request Jun 9, 2026
## Description

A registry-bound IdP can declare a non-default **`scope_claim_name`**
(e.g. `scp` for Entra/Azure AD, `roles`). Until now the gateway resolved
the scope claim name only from the API def / OAS, defaulting to
`"scope"`. So a registry-only API whose IdP emits scopes under a
non-`scope` claim — with no manual scope config — would `403` (`no
matching policy found`) because the gateway read the wrong claim.

This wires the dashboard's new `scope_claim_name` field
(TykTechnologies/tyk-analytics#5788) through the gateway registry so
it's actually consumed.

## Changes

- **`IdP`** gains `ScopeClaimName` (`json:"scope_claim_name"`),
mirroring the dashboard `model.ClientIdP` and the MDCB model
(tyk-sink#794). Picked up automatically on both feeds (Dashboard
envelope + RPC `unmarshalIdPs`) — no transport change.
- **`Binding`** denormalises it from the IdP at `rebuild` time, so the
JWT hot path needs no second registry lookup (same approach already used
for `ScopeToPolicy`).
- **`scopeClaimNameForRequest`** resolves with **manual-wins
precedence**: Classic `GetScopeClaimName` / OAS → registry binding →
`"scope"`. The fallback only fires when the API def sets nothing, so
manual-config APIs stay byte-identical.

## Cross-repo

| Repo | PR | Field |
|------|----|----|
| tyk-analytics (dashboard) | #5788 | `model.ClientIdP.ScopeClaimName` |
| tyk-sink (MDCB) | #794 | `models.ClientIdP.ScopeClaimName` |
| tyk (gateway) | this PR | `IdP.ScopeClaimName` |

Verified end-to-end: the MDCB RPC read path (`LoadClientIdPs` →
`filterAndPruneClientIdPs` → `pruneIdP`) uses full-document loads and
shallow struct copies, so the field is carried through with no
projection dropping it.

## How this has been tested

New tests (TDD, race-clean):
- `TestIdPRegistry_RebuildCarriesScopeClaimName` — propagation into the
binding.
- `TestScopeClaimNameForRequest` — the three-tier resolution order
(manual wins → binding → default).
- `TestIdPRegistry_JWT_ResolvesScopeClaimNameFromRegistry` — end-to-end:
token with scopes under `scp`, no manual scope claim → 200.
- `TestIdPRegistry_JWT_NoScopeClaimNameRejects` — counterpart proving
the same setup with no IdP `scope_claim_name` falls through to `"scope"`
and 403s, pinning down that the registry fallback is what flips the
result.

## Types of changes

- [x] New feature (non-breaking change which adds functionality)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant