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

feat: Add expiration time setting for API key #7584

Merged
merged 1 commit into from
Dec 30, 2024
Merged

Conversation

lan-yonghui
Copy link
Member

No description provided.

Copy link

f2c-ci-robot bot commented Dec 27, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ErrApiConfigKeyInvalid = "ErrApiConfigKeyInvalid"
ErrApiConfigIPInvalid = "ErrApiConfigIPInvalid"
ErrApiConfigDisable = "ErrApiConfigDisable"
ErrApiConfigKeyTimeInvalid = "ErrApiConfigKeyTimeInvalid"
)

// app
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code snippet you provided does not contain any major irregularities, potential issues, or significant optimizations to suggest. There are just minor formatting updates made between the third and fourth line of code comments. The changes include adding an ellipsis (...) at the end of each comment to align with the previous comments' format. These changes do not affect the functionality of the code and are considered stylistic adjustments rather than functional improvements.

If there is anything else specific you would like checked regarding this or other code snippets, please let me know!

}
return apiTime == 0 || nowTime-panelTime <= int64(apiTime*60)
}

func isValid1PanelToken(panelToken string, panelTimestamp string) bool {
system1PanelToken := global.CONF.System.ApiKey
if panelToken == GenerateMD5("1panel"+system1PanelToken+panelTimestamp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Differences Analysis:

Changes Identified:

  1. Imports Moved to Top: The import statements at the top have been moved from one section to another without any issues.

  2. Function Definition:

    • SessionAuth() is defined as a gin.HandlerFunc, which is correct and matches other similar functions in the file.
    • However, the logic after checking the timestamp looks incomplete. It should validate that the timestamp is within an allowed window considering apiKeyValidityTime.
  3. Comment Update: In the second comment, there’s no change compared to the previous version but is grammatically clear.

  4. New Function isValid1PanelTimestamp:

    • Added this new function immediately after defining SessionAuth(). This is useful for validating if the API key timestamp is still valid given the configured apiKeyValidityTime.
    • Checks if apiKeyValidityTime can be converted successfully to an integer and then checks if the current time minus the parsed timestamp is less than or equal to the validity duration (in minutes).
  5. Code Optimization Suggestions:

    • Consider using constants where appropriate, like for the default timeout duration of zero if apiKeyValidityTime equals zero.
    • Improve error handling: Ensure that all imported packages' methods handle errors appropriately when converting strings to integers and parsing times.
    • Add comments explaining the purpose of each part of the timestamp validation.

Conclusion:

The code overall maintains functionality with minor adjustments made to enhance readability and maintainability. The new isValid1PanelTimestamp function provides a simple way to ensure that API keys used do not exceed their expiration period based on the server configuration.

@@ -179,6 +194,7 @@ const onSave = async (formEl: FormInstance | undefined) => {
apiKey: form.apiKey,
ipWhiteList: form.ipWhiteList,
apiInterfaceStatus: form.apiInterfaceStatus,
apiKeyValidityTime: form.apiKeyValidityTime,
};
loading.value = true;
await updateApiConfig(param)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code has a few minor improvements and clarifications, but there are no obvious irregularities or issues. Here is a brief review:

  1. Form Item Labels: The label for apiKeyValidityTime is now in the correct position within the el-form.

  2. Template Append Unit: The unit "minute" correctly appended to the el-input for apiKeyValidityTime. This helps users understand that time durations should be expressed in minutes.

  3. Initial Values: The initial value of apiKeyValidityTime is set to 120, which makes sense given its default setting in many systems. However, it would be beneficial if you could verify this with appropriate system requirements.

Overall, the changes make the form more user-friendly by providing clear instructions on how to input data and ensuring consistency across similar fields. If there are specific performance optimizations needed based on your application's use case, feel free to ask!

Copy link

@ssongliu
Copy link
Member

/lgtm

@wanghe-fit2cloud
Copy link
Member

/approve

Copy link

f2c-ci-robot bot commented Dec 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot merged commit b467dfa into dev Dec 30, 2024
7 checks passed
@f2c-ci-robot f2c-ci-robot bot deleted the pr@dev@feat_api_time branch December 30, 2024 05:33
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.

4 participants