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.

Sign up for GitHub

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

Jump to bottom

Revert "[TT-13422] Do not allow empty string in upstream auth configuration strings" #6702

Merged
merged 3 commits into from
Nov 14, 2024
Merged

Revert "[TT-13422] Do not allow empty string in upstream auth configuration strings" #6702

merged 3 commits into from
Nov 14, 2024

Conversation

jeffy-mathew
Copy link
Contributor

@jeffy-mathew jeffy-mathew commented Nov 14, 2024 •
edited
Loading

Reverts #6699
temporary revert with common change for AuthSource

@buger
Copy link
Member

buger commented Nov 14, 2024

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status DoD Check ❌
Allowed Statuses In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review ✔️

Please ensure your jira story is in one of the allowed statuses

@github-actions GitHub Actions
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

6699 - Not compliant

Fully compliant requirements:

  • None

Not compliant requirements:

  • Add validation rules on backend to ensure that configuration strings are not empty.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Validation Removal
The PR removes validation that ensures configuration strings are not empty. This is contrary to the ticket requirements.

Validation Removal
The PR removes validation that ensures configuration strings are not empty. This is contrary to the ticket requirements.

Validation Removal
The PR removes validation that ensures configuration strings are not empty. This is contrary to the ticket requirements.

Validation Removal
The PR removes validation that ensures configuration strings are not empty. This is contrary to the ticket requirements.

Validation Removal
The PR removes validation that ensures configuration strings are not empty. This is contrary to the ticket requirements.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 14, 2024 •
edited
Loading

API Changes

no api changes detected

@github-actions GitHub Actions
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Security
Reinstate stricter validation for critical fields to prevent empty values

Reinstate the use of the X-Tyk-NonEmptyString definition for critical fields like
username, password, clientId, clientSecret, and tokenUrl to ensure these fields are
not empty, enhancing validation and security.

apidef/oas/schema/x-tyk-api-gateway.json [2029-2030]

 "username": {
-  "type": "string"
+  "$ref": "#/definitions/X-Tyk-NonEmptyString"
 }
Suggestion importance[1-10]: 9

Why: Reverting to the 'X-Tyk-NonEmptyString' definition for fields like 'username' ensures that these critical fields are not left empty, thereby enhancing security and data integrity. This is a significant improvement in validation practices.

9

@buger
Copy link
Member

buger commented Nov 14, 2024

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status DoD Check ❌
Allowed Statuses In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review ✔️

Please ensure your jira story is in one of the allowed statuses

@buger
Copy link
Member

buger commented Nov 14, 2024

💔 The detected issue is not in one of the allowed statuses 💔

Detected Status DoD Check ❌
Allowed Statuses In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review ✔️

Please ensure your jira story is in one of the allowed statuses

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@jeffy-mathew jeffy-mathew merged commit f0fcb3f into master Nov 14, 2024
28 of 40 checks passed
@jeffy-mathew jeffy-mathew deleted the revert-6699-fix/TT-13422/do-not-allow-empty-values branch November 14, 2024 12:40
@jeffy-mathew
Copy link
Contributor Author

/release to release-5.7

@tykbot tykbot
Copy link

tykbot bot commented Nov 14, 2024

Working on it! Note that it can take a few minutes.

@jeffy-mathew
Copy link
Contributor Author

/release to release-5.7.0

@tykbot tykbot
Copy link

tykbot bot commented Nov 14, 2024

Working on it! Note that it can take a few minutes.

tykbot bot pushed a commit that referenced this pull request Nov 14, 2024
@jeffy-mathew
Revert "[TT-13422] Do not allow empty string in upstream auth configu…
4e664bc 
…ration strings" (#6702)

Reverts #6699
temporary revert with common change for AuthSource

(cherry picked from commit f0fcb3f)
@buger buger mentioned this pull request Nov 14, 2024
Merged
@tykbot tykbot
Copy link

tykbot bot commented Nov 14, 2024

@jeffy-mathew Seems like there is conflict and it require manual merge.

tykbot bot pushed a commit that referenced this pull request Nov 14, 2024
@jeffy-mathew
Revert "[TT-13422] Do not allow empty string in upstream auth configu…
4b4f01a 
…ration strings" (#6702)

Reverts #6699
temporary revert with common change for AuthSource

(cherry picked from commit f0fcb3f)
@buger buger mentioned this pull request Nov 14, 2024
Merged
@tykbot tykbot
Copy link

tykbot bot commented Nov 14, 2024

@jeffy-mathew Seems like there is conflict and it require manual merge.

jeffy-mathew added a commit that referenced this pull request Nov 14, 2024
@buger @jeffy-mathew
Merging to release-5.7: Revert "[TT-13422] Do not allow empty string …
ceeb46e 
…in upstream auth configuration strings" (#6702) (#6703)

### **User description**
Revert "[TT-13422] Do not allow empty string in upstream auth
configuration strings" (#6702)

Reverts #6699
temporary revert with common change for AuthSource

[TT-13422]:
https://tyktech.atlassian.net/browse/TT-13422?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ


___

### **PR Type**
enhancement, bug fix


___

### **Description**
- Reverted the enforcement of non-empty strings for certain fields in
the OpenAPI Specification (OAS) schema.
- Introduced a new definition `X-Tyk-UpstreamAuthSource` to replace
`X-Tyk-AuthSource` in specific schema fields.
- Updated schema references to use `X-Tyk-UpstreamAuthSource` instead of
`X-Tyk-AuthSource`.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>x-tyk-api-gateway.json</strong><dd><code>Revert
non-empty string enforcement and update auth source
references</code></dd></summary>
<hr>

apidef/oas/schema/x-tyk-api-gateway.json

<li>Reverted the use of <code>X-Tyk-NonEmptyString</code> for certain
fields.<br> <li> Introduced a new definition
<code>X-Tyk-UpstreamAuthSource</code>.<br> <li> Updated references from
<code>X-Tyk-AuthSource</code> to
<code>X-Tyk-UpstreamAuthSource</code>.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6703/files#diff-78828969c0c04cc1a776dfc93a8bad3c499a8c83e6169f83e96d090bed3e7dd0">+15/-4</a>&nbsp;
&nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

Co-authored-by: Jeffy Mathew <[email protected]>
jeffy-mathew added a commit that referenced this pull request Nov 14, 2024
@buger @jeffy-mathew
Merging to release-5.7.0: Revert "[TT-13422] Do not allow empty strin…
70fc07b 
…g in upstream auth configuration strings" (#6702) (#6704)

### **User description**
Revert "[TT-13422] Do not allow empty string in upstream auth
configuration strings" (#6702)

Reverts #6699
temporary revert with common change for AuthSource

[TT-13422]:
https://tyktech.atlassian.net/browse/TT-13422?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ


___

### **PR Type**
enhancement, bug fix


___

### **Description**
- Reverted changes that enforced non-empty strings in the OpenAPI
Specification (OAS) schema, specifically for the `name` property in
`X-Tyk-AuthSource`.
- Adjusted references for `header` properties in various authentication
objects to use `X-Tyk-UpstreamAuthSource`.
- Reintroduced the `X-Tyk-UpstreamAuthSource` definition to align with
previous configurations.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug
fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>x-tyk-api-gateway.json</strong><dd><code>Revert
non-empty string enforcement and adjust AuthSource
references</code></dd></summary>
<hr>

apidef/oas/schema/x-tyk-api-gateway.json

<li>Reverted the <code>name</code> property in
<code>X-Tyk-AuthSource</code> to use <code>type: string</code>
<br>instead of <code>X-Tyk-NonEmptyString</code>.<br> <li> Changed
<code>$ref</code> for <code>header</code> in
<code>X-Tyk-UpstreamBasicAuthentication</code> and other <br>sections to
<code>X-Tyk-UpstreamAuthSource</code>.<br> <li> Reintroduced
<code>X-Tyk-UpstreamAuthSource</code> definition with <code>name</code>
as a string <br>type.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6704/files#diff-78828969c0c04cc1a776dfc93a8bad3c499a8c83e6169f83e96d090bed3e7dd0">+15/-4</a>&nbsp;
&nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

Co-authored-by: Jeffy Mathew <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lghiur lghiur lghiur approved these changes

Assignees
No one assigned
Labels
Review effort [1-5]: 2 Sub-task Tyk Product Development
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jeffy-mathew @buger @lghiur