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

Merging to release-5.7: [TT-13535/TT-13566] Make upstream oauth flow client secret omitempty (#6708) #6709

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Merging to release-5.7: [TT-13535/TT-13566] Make upstream oauth flow client secret omitempty (#6708) #6709

merged 1 commit into from
Nov 18, 2024

Conversation

buger
Copy link
Member

@buger buger commented Nov 18, 2024 •
edited by github-actions bot
Loading

User description

[TT-13535/TT-13566] Make upstream oauth flow client secret omitempty (#6708)

User description

TT-13566
Summary Make upstream auth oauth password client secret not required in oas schema
Type Sub-task Sub-task
Status Ready for Testing
Points N/A
Labels -

Description

Make upstream oauth flow client secret omitempty to not break when an
API is created without clientSecret and saved later.

Related Issue

Parent: https://tyktech.atlassian.net/browse/TT-13535
Subtask: https://tyktech.atlassian.net/browse/TT-13566

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • 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)
  • Refactoring or add test (improvements in base code or adds test
    coverage to functionality)

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

PR Type

enhancement

Description

  • Updated the ClientAuthData struct in apidef/api_definitions.go to
    make the ClientSecret field optional by adding the omitempty tag.
  • This change prevents errors when an API is created without a
    clientSecret and saved later.

Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Make `ClientSecret` optional in OAuth client auth data     

apidef/api_definitions.go

  • Made ClientSecret field optional by adding omitempty tag.
  • Updated JSON and BSON tags for ClientSecret to reflect optional
    status.
    		•  +1/-1     

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

    PR Type

    Enhancement, Tests

    Description

    • Made the ClientSecret field optional in multiple files by adding the omitempty tag, ensuring it is not required for OAuth password flow.
    • Updated the OpenAPI schema to remove client_secret from the list of required fields.
    • Added tests to validate the schema with the optional ClientSecret field.

    Changes walkthrough 📝

    Relevant files
    Enhancement
    api_definitions.go
    Make `ClientSecret` optional in OAuth client auth data     

    apidef/api_definitions.go

  • Made ClientSecret field optional by adding omitempty tag.
  • Updated JSON and BSON tags for ClientSecret to reflect optional
    status.
    		•  +1/-1     
    upstream.go
    Make `ClientSecret` optional in OAuth client auth data     

    apidef/oas/upstream.go

  • Made ClientSecret field optional by adding omitempty tag.
  • Updated JSON and BSON tags for ClientSecret to reflect optional
    status.
    		•  +1/-1     
    schema.go
    Update schema to make `ClientSecret` optional                       

    apidef/schema.go

    • Removed client_secret from required fields in schema.
    		 +0/-1     
    Tests
    api_definitions_test.go
    Add tests for optional `ClientSecret` in schema                   

    apidef/api_definitions_test.go

  • Added test setup for UpstreamAuth with ClientSecret.
  • Ensured schema validation with optional ClientSecret.
    		•  +22/-0   

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

    @jeffy-mathew
    [TT-13535/TT-13566] Make upstream oauth flow client secret omitempty (#…
    4f3622e 
    …6708)

### **User description**
<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-13566"
title="TT-13566" target="_blank">TT-13566</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
<td>Make upstream auth oauth password client secret not required in oas
schema</td>
    </tr>
    <tr>
      <th>Type</th>
      <td>
<img alt="Sub-task"
src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10316?size=medium"
/>
        Sub-task
      </td>
    </tr>
    <tr>
      <th>Status</th>
      <td>Ready for Testing</td>
    </tr>
    <tr>
      <th>Points</th>
      <td>N/A</td>
    </tr>
    <tr>
      <th>Labels</th>
      <td>-</td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

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

## Description

Make upstream oauth flow client secret omitempty to not break when an
API is created without `clientSecret` and saved later.

## Related Issue
Parent: https://tyktech.atlassian.net/browse/TT-13535
Subtask: https://tyktech.atlassian.net/browse/TT-13566

## 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: -->

- [ ] 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)
- [ ] 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


___

### **PR Type**
enhancement


___

### **Description**
- Updated the `ClientAuthData` struct in `apidef/api_definitions.go` to
make the `ClientSecret` field optional by adding the `omitempty` tag.
- This change prevents errors when an API is created without a
`clientSecret` and saved later.



___



### **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>api_definitions.go</strong><dd><code>Make
`ClientSecret` optional in OAuth client auth data</code>&nbsp; &nbsp;
&nbsp; </dd></summary>
<hr>

apidef/api_definitions.go

<li>Made <code>ClientSecret</code> field optional by adding
<code>omitempty</code> tag.<br> <li> Updated JSON and BSON tags for
<code>ClientSecret</code> to reflect optional <br>status.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6708/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67">+1/-1</a>&nbsp;
&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

(cherry picked from commit c8f21dc)
    @buger buger merged commit 89b122d into release-5.7 Nov 18, 2024
    @buger buger deleted the merge/release-5.7/c8f21dcd77bc48da511628e67a43a691e29407f8 branch November 18, 2024 13:58
    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    6708 - Fully compliant

    Fully compliant requirements:

    • Made the ClientSecret field optional in the ClientAuthData struct.

    Not compliant requirements:
    []

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Consistency
    Ensure that the changes made to the ClientSecret field are consistent across all relevant files and schemas.

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Replace hardcoded sensitive data in tests with dynamic values

    Replace the hardcoded 'dummy' client secret in tests with a variable or a function
    that can generate dynamic values to better simulate real-world scenarios and avoid
    potential misuse of hardcoded sensitive data in the test environment.

    apidef/api_definitions_test.go [24]

    -ClientSecret: "dummy", // workaround to fix schema error
+ClientSecret: generateTestClientSecret(), // dynamically generate client secret for testing
    Suggestion importance[1-10]: 7

    Why: Replacing hardcoded sensitive data with dynamically generated values in tests enhances security and realism, which is a good practice especially for sensitive data like client secrets.

    		7
    Possible issue
    Ensure proper handling of optional ClientSecret to avoid runtime errors

    Ensure that the optional nature of ClientSecret in ClientAuthData is handled
    appropriately in all parts of the application where ClientAuthData is used, to
    prevent runtime errors due to nil or empty values.

    apidef/api_definitions.go [852]

    -ClientSecret string `bson:"client_secret,omitempty" json:"client_secret,omitempty"` // client secret is optional for password flow
+ClientSecret string `bson:"client_secret,omitempty" json:"client_secret,omitempty"` // Ensure handling of optional ClientSecret throughout the application
    Suggestion importance[1-10]: 5

    Why: The suggestion to ensure proper handling of an optional field is valid to prevent potential runtime errors. However, it lacks specific implementation details and is more of a cautionary note than a direct code improvement.

    		5

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

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