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.

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

Merging to release-5.7: Revert "[TT-13422] Do not allow empty string in upstream auth configuration strings" (#6702) #6703

Conversation

buger
Copy link
Member

@buger buger commented Nov 14, 2024

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


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 📝

Relevant files
Enhancement
x-tyk-api-gateway.json
Revert non-empty string enforcement and update auth source references

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

  • Reverted the use of X-Tyk-NonEmptyString for certain fields.
  • Introduced a new definition X-Tyk-UpstreamAuthSource.
  • Updated references from X-Tyk-AuthSource to X-Tyk-UpstreamAuthSource.
  • +15/-4   

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

    …ration strings" (#6702)
    
    Reverts #6699
    temporary revert with common change for AuthSource
    
    (cherry picked from commit f0fcb3f)
    @buger
    Copy link
    Member Author

    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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    6699 - Fully compliant

    Fully compliant requirements:

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

    Schema Inconsistency
    The PR introduces a new definition 'X-Tyk-UpstreamAuthSource' but does not remove or deprecate the old 'X-Tyk-AuthSource'. This might lead to inconsistencies or confusion in the schema usage.

    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a validation pattern to the "name" property to prevent empty strings

    Consider adding a validation pattern to the "name" property under
    "X-Tyk-UpstreamAuthSource" to ensure it does not accept empty strings or unintended
    characters.

    apidef/oas/schema/x-tyk-api-gateway.json [2153-2154]

     "name": {
    -  "type": "string"
    +  "type": "string",
    +  "pattern": "\\S+"
     }
    Suggestion importance[1-10]: 7

    Why: Adding a validation pattern to ensure non-empty strings in the "name" property under "X-Tyk-UpstreamAuthSource" enhances data integrity and prevents potential errors from empty or invalid inputs.

    7
    Possible issue
    Review and enhance the "X-Tyk-UpstreamAuthSource" definition to ensure it meets all necessary constraints

    Ensure that the "X-Tyk-UpstreamAuthSource" definition includes all necessary
    properties and constraints to match the intended use cases, as it replaces
    "X-Tyk-AuthSource" which might have had additional constraints.

    apidef/oas/schema/x-tyk-api-gateway.json [2147-2154]

     "X-Tyk-UpstreamAuthSource": {
       "type": "object",
       "properties": {
         "enabled": {
           "type": "boolean"
         },
         "name": {
    -      "type": "string"
    +      "type": "string",
    +      "pattern": "\\S+"
         }
       }
     }
    Suggestion importance[1-10]: 6

    Why: The suggestion to review and potentially add more constraints to the "X-Tyk-UpstreamAuthSource" is valid as it replaces "X-Tyk-AuthSource" which might have had different constraints. This ensures the new definition aligns with intended use cases and maintains consistency.

    6

    Copy link

    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 ceeb46e into release-5.7 Nov 14, 2024
    26 of 38 checks passed
    @jeffy-mathew jeffy-mathew deleted the merge/release-5.7/f0fcb3f59104ce571d24f1e4354c8bbfc8eba9f3 branch November 14, 2024 16:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants