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

[TT-11909]: Added Session Lifetime to OAS #6835

Merged
merged 9 commits into from
Jan 17, 2025
Merged

[TT-11909]: Added Session Lifetime to OAS #6835

merged 9 commits into from
Jan 17, 2025

Conversation

kofoworola
Copy link
Contributor

@kofoworola kofoworola commented Jan 16, 2025

User description

TT-11909
Summary [OAS] Session lifetime
Type Story Story
Status In Dev
Points N/A
Labels -

Description

TT-1909

Related Issue

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, Tests


Description

  • Added session_lifetime_disabled field to API definition.

  • Introduced KeyRetentionPeriod struct for token TTL management.

  • Updated OpenAPI schema to include keyRetentionPeriod.

  • Enhanced migration and linter tests to cover new fields.


Changes walkthrough 📝

Relevant files
Enhancement
api_definitions.go
Introduced `session_lifetime_disabled` field                         

apidef/api_definitions.go

  • Added session_lifetime_disabled field to API definition.
  • Enhanced session lifetime management capabilities.
  • +1/-0     
    migration.go
    Updated migration logic for session lifetime                         

    apidef/migration.go

    • Set SessionLifetimeDisabled flag in migration logic.
    +1/-0     
    authentication.go
    Introduced `KeyRetentionPeriod` for token TTL                       

    apidef/oas/authentication.go

  • Added KeyRetentionPeriod struct for token TTL management.
  • Implemented Fill and ExtractTo methods for KeyRetentionPeriod.
  • +35/-0   
    x-tyk-api-gateway.json
    Updated OpenAPI schema for `keyRetentionPeriod`                   

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

  • Extended OpenAPI schema with keyRetentionPeriod definition.
  • Defined properties and validation for keyRetentionPeriod.
  • +16/-0   
    schema.json
    Updated schema with `session_lifetime_disabled`                   

    apidef/schema.json

    • Added session_lifetime_disabled field to schema.
    +3/-0     
    Tests
    migration_test.go
    Enhanced migration tests for session lifetime                       

    apidef/migration_test.go

    • Added test coverage for SessionLifetimeDisabled flag.
    +1/-0     
    linter_test.go
    Added linter test for `KeyRetentionPeriod`                             

    apidef/oas/linter_test.go

    • Updated linter tests to include KeyRetentionPeriod.
    +1/-0     

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

    @buger
    Copy link
    Member

    buger commented Jan 16, 2025

    I'm a bot and I 👍 this PR title. 🤖

    Copy link
    Contributor

    github-actions bot commented Jan 16, 2025

    API Changes

    --- prev.txt	2025-01-17 04:28:44.729677054 +0000
    +++ current.txt	2025-01-17 04:28:40.248664381 +0000
    @@ -246,6 +246,7 @@
     	CacheOptions                         CacheOptions           `bson:"cache_options" json:"cache_options"`
     	SessionLifetimeRespectsKeyExpiration bool                   `bson:"session_lifetime_respects_key_expiration" json:"session_lifetime_respects_key_expiration,omitempty"`
     	SessionLifetime                      int64                  `bson:"session_lifetime" json:"session_lifetime"`
    +	SessionLifetimeDisabled              bool                   `bson:"session_lifetime_disabled" json:"session_lifetime_disabled"`
     	Active                               bool                   `bson:"active" json:"active"`
     	Internal                             bool                   `bson:"internal" json:"internal"`
     	AuthProvider                         AuthProviderMeta       `bson:"auth_provider" json:"auth_provider"`
    @@ -2137,6 +2138,9 @@
     
     	// SecuritySchemes contains security schemes definitions.
     	SecuritySchemes SecuritySchemes `bson:"securitySchemes,omitempty" json:"securitySchemes,omitempty"`
    +
    +	// KeyRetentionPeriod contains configuration for key retention.
    +	KeyRetentionPeriod *KeyRetentionPeriod `bson:"keyRetentionPeriod,omitempty" json:"keyRetentionPeriod,omitempty"`
     }
         Authentication contains configuration about the authentication methods and
         security policies applied to requests.
    @@ -3058,6 +3062,36 @@
     
     func (j *JWTValidation) Fill(jwt apidef.JWTValidation)
     
    +type KeyRetentionPeriod struct {
    +	// Enabled enables Key retention for the API
    +	//
    +	// Tyk classic API definition: `disable_expire_analytics`.
    +	Enabled bool `bson:"enabled,omitempty" json:"enabled,omitempty"`
    +	// Value configures the expiry interval for a Key.
    +	// The value is a string that specifies the interval in a compact form,
    +	// where hours, minutes and seconds are denoted by 'h', 'm' and 's' respectively.
    +	// Multiple units can be combined to represent the duration.
    +	//
    +	// Examples of valid shorthand notations:
    +	// - "1h"   : one hour
    +	// - "20m"  : twenty minutes
    +	// - "30s"  : thirty seconds
    +	// - "1m29s": one minute and twenty-nine seconds
    +	// - "1h30m" : one hour and thirty minutes
    +	//
    +	// An empty value is interpreted as "0s"
    +	//
    +	// Tyk classic API definition: `expire_analytics_after`.
    +	Value ReadableDuration `bson:"value" json:"value"`
    +}
    +    KeyRetentionPeriod contains configuration for key retention.
    +
    +func (k *KeyRetentionPeriod) ExtractTo(api *apidef.APIDefinition)
    +    ExtractTo extracts *Authentication into *apidef.APIDefinition.
    +
    +func (k *KeyRetentionPeriod) Fill(api apidef.APIDefinition)
    +    Fill fills *KeyRetentionPeriod from apidef.APIDefinition.
    +
     type Kind = event.Kind
         Kind is an alias maintained to be used in imports.
     

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Possible Misconfiguration

    The addition of the SessionLifetimeDisabled field should be carefully reviewed to ensure it does not conflict with existing session lifetime logic or introduce unintended behavior.

    SessionLifetimeDisabled              bool                   `bson:"session_lifetime_disabled" json:"session_lifetime_disabled"`
    Logic Validation

    The Fill and ExtractTo methods for KeyRetentionPeriod should be validated to ensure they correctly handle the conversion between SessionLifetimeDisabled and SessionLifetime.

    func (k *KeyRetentionPeriod) Fill(api apidef.APIDefinition) {
    	k.Enabled = !api.SessionLifetimeDisabled
    	k.Value = ReadableDuration(time.Duration(api.ExpireAnalyticsAfter) * time.Second)
    }
    
    func (k *KeyRetentionPeriod) ExtractTo(api *apidef.APIDefinition) {
    	api.SessionLifetimeDisabled = !k.Enabled
    	api.SessionLifetime = int64(k.Value.Seconds())
    Schema Consistency

    The new keyRetentionPeriod schema addition should be reviewed for consistency with the rest of the schema and to ensure it aligns with the intended functionality.

    "keyRetentionPeriod": {
      "type": "object",
      "properties": {
        "enabled": {
          "type": "boolean"
        },
        "value": {
          "type": "string",
          "pattern": "^(\\d+h)?(\\d+m)?(\\d+s)?$"
        }
      },
      "required": [
        "enabled",
        "value"
      ]

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add nil checks to prevent runtime errors when accessing potentially nil fields

    Ensure that the Fill and ExtractTo methods of the KeyRetentionPeriod struct handle
    potential nil values for api.ExpireAnalyticsAfter and api.SessionLifetime to avoid
    runtime panics.

    apidef/oas/authentication.go [73]

    -k.Value = ReadableDuration(time.Duration(api.ExpireAnalyticsAfter) * time.Second)
    +if api.ExpireAnalyticsAfter != nil {
    +    k.Value = ReadableDuration(time.Duration(*api.ExpireAnalyticsAfter) * time.Second)
    +} else {
    +    k.Value = ReadableDuration(0)
    +}
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential runtime panic by adding nil checks for api.ExpireAnalyticsAfter. It is highly relevant and improves the robustness of the code, ensuring it handles edge cases where the field might be nil.

    9
    General
    Add a default value for the SessionLifetimeDisabled field to ensure consistent initialization

    Ensure that the SessionLifetimeDisabled field is properly initialized with a default
    value to avoid unintended behavior during its usage.

    apidef/api_definitions.go [741]

    -SessionLifetimeDisabled bool                   `bson:"session_lifetime_disabled" json:"session_lifetime_disabled"`
    +SessionLifetimeDisabled bool                   `bson:"session_lifetime_disabled" json:"session_lifetime_disabled" default:"false"`
    Suggestion importance[1-10]: 7

    Why: Adding a default value for SessionLifetimeDisabled ensures consistent behavior and avoids unintended issues during initialization. This is a useful enhancement, though its impact is moderate as the field is likely to be explicitly set in most cases.

    7
    Add test cases to validate handling of edge-case or invalid durations in KeyRetentionPeriod.Value

    Include a test case to verify that KeyRetentionPeriod.Value correctly handles
    invalid or edge-case durations to ensure robustness.

    apidef/oas/linter_test.go [86]

     settings.Server.Authentication.KeyRetentionPeriod.Value = settings.Upstream.RateLimit.Per
    +// Add test cases for invalid or edge-case durations
    Suggestion importance[1-10]: 6

    Why: Including test cases for edge-case or invalid durations would improve the robustness of the code. However, the suggestion is vague and does not provide specific test cases, reducing its immediate utility.

    6

    @kofoworola kofoworola requested review from titpetric and a team January 16, 2025 14:27
    apidef/oas/authentication.go Outdated Show resolved Hide resolved
    apidef/oas/authentication.go Outdated Show resolved Hide resolved
    apidef/oas/linter_test.go Outdated Show resolved Hide resolved
    apidef/oas/oas_test.go Outdated Show resolved Hide resolved
    @kofoworola kofoworola enabled auto-merge (squash) January 17, 2025 04:27
    @kofoworola kofoworola merged commit 4070fe0 into master Jan 17, 2025
    26 of 39 checks passed
    @kofoworola kofoworola deleted the feat/TT-11909 branch January 17, 2025 09:52
    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.

    3 participants