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

feat: Optimize Location Information Display in System Login Logs #7459

Merged
merged 1 commit into from
Dec 20, 2024
Merged

feat: Optimize Location Information Display in System Login Logs #7459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions backend/app/api/v1/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import (
"github.com/1Panel-dev/1Panel/backend/constant"
"github.com/1Panel-dev/1Panel/backend/global"
"github.com/1Panel-dev/1Panel/backend/utils/captcha"
"github.com/1Panel-dev/1Panel/backend/utils/common"
"github.com/1Panel-dev/1Panel/backend/utils/geo"
"github.com/gin-gonic/gin"
)

Expand Down Expand Up @@ -157,11 +155,6 @@ func saveLoginLogs(c *gin.Context, err error) {
logs.Status = constant.StatusSuccess
}
logs.IP = c.ClientIP()
address, err := geo.GetIPLocation(logs.IP, common.GetLang(c))
if err != nil {
global.LOG.Errorf("get ip location failed: %s", err)
}
logs.Agent = c.GetHeader("User-Agent")
logs.Address = address
_ = logService.CreateLoginLog(logs)
}
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 provided has some minor adjustments for readability and maintenance:

  1. Unused Imports: The imports of common and geo from backend/utils are not used elsewhere in the file, so they can be removed to clean up the file.

  2. Redundant Code Block:

     address, err := geo.GetIPLocation(logs.IP, common.GetLang(c))
 if err != nil {
 	global.LOG.Errorf("get ip location failed: %s", err)
 }
 logs.Address = address

    This redundant block should be removed because it doesn't add value to the function when only the IP is needed for logging purposes without additional geolocation information.

  3. Minor Styling Improvements: Minor style fixes such as removing trailing spaces at the end of lines or using consistent indentation might improve code readability.

Here's the updated version of the code after applying these changes:

package routes

import (
    "github.com/gin-gonic/gin"
)

// No significant changes made beyond minor formatting improvements

Summary of Changes:

  • Removed unused imports: common and geo.
  • Simplified the code by removing the extraneous retrieval of geographical location data since only the IP is required for logging.

These adjustments make the code cleaner and more maintainable while preserving its functionality.

2 changes: 1 addition & 1 deletion backend/app/api/v1/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func (b *BaseApi) GetLoginLogs(c *gin.Context) {
return
}

total, list, err := logService.PageLoginLog(req)
total, list, err := logService.PageLoginLog(c, req)
if err != nil {
helper.ErrorWithDetail(c, constant.CodeErrInternalServer, constant.ErrTypeInternalServer, err)
return
Expand Down
8 changes: 6 additions & 2 deletions backend/app/service/logs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ package service

import (
"fmt"
"github.com/1Panel-dev/1Panel/backend/utils/common"
"github.com/1Panel-dev/1Panel/backend/utils/geo"
"github.com/gin-gonic/gin"
"os"
"path"
"path/filepath"
Expand All @@ -26,7 +29,7 @@ const logs = "https://resource.fit2cloud.com/installation-log.sh"
type ILogService interface {
ListSystemLogFile() ([]string, error)
CreateLoginLog(operation model.LoginLog) error
PageLoginLog(search dto.SearchLgLogWithPage) (int64, interface{}, error)
PageLoginLog(c *gin.Context, search dto.SearchLgLogWithPage) (int64, interface{}, error)

CreateOperationLog(operation model.OperationLog) error
PageOperationLog(search dto.SearchOpLogWithPage) (int64, interface{}, error)
Expand Down Expand Up @@ -77,7 +80,7 @@ func (u *LogService) ListSystemLogFile() ([]string, error) {
return files, nil
}

func (u *LogService) PageLoginLog(req dto.SearchLgLogWithPage) (int64, interface{}, error) {
func (u *LogService) PageLoginLog(c *gin.Context, req dto.SearchLgLogWithPage) (int64, interface{}, error) {
total, ops, err := logRepo.PageLoginLog(
req.Page,
req.PageSize,
Expand All @@ -91,6 +94,7 @@ func (u *LogService) PageLoginLog(req dto.SearchLgLogWithPage) (int64, interface
if err := copier.Copy(&item, &op); err != nil {
return 0, nil, errors.WithMessage(constant.ErrStructTransform, err.Error())
}
item.Address, _ = geo.GetIPLocation(item.IP, common.GetLang(c))
dtoOps = append(dtoOps, item)
}
return total, dtoOps, err
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 changes look mostly correct for the intended functionality. However, a few improvements could be made:

  1. Use of Interfaces: Ensure that all dependencies on model, dto, and other packages are properly imported at the beginning of each file where they are used to avoid unnecessary imports.

  2. Error Handling: Consider adding more detailed logging or user-facing errors when encountering specific types of failures during operation execution.

  3. Performance and Resource Management: Since the code processes operations from the database, ensure there is appropriate handling for pagination, caching if applicable, and resource management techniques like rate limiting or connection pooling.

Here's an optimized version with some considerations:

package service

import (
	"errors"
	"fmt"
	"github.com/1Panel-dev/1Panel/backend/utils/common"
	"github.com/1Panel-dev/1Panel/backend/utils/geo"
	"github.com/gin-gonic/gin"
)

var (
	logs        = "https://resource.fit2cloud.com/installation-log.sh"
	defaultLimit int64 = 50 // Default page size
)

type LogService struct{}

func NewLogService() *LogService {
	return &LogService{}
}

// ListSystemLogFile returns system log files.
func (u *LogService) ListSystemLogFile() ([]string, error) {
	files := make([]string, 0)
	err := filepath.Walk(logDir, func(path string, info os.FileInfo, err error) error {
		if !info.IsDir() && path == filepath.Join(logDir, fileName) {
			files = append(files, path)
		}
		return nil
	})
	if err != nil {
		return nil, fmt.Errorf("error reading logs directory: %w", err)
	}
	return files, nil
}

// PageLoginLog retrieves paginated login logs based on filters and pagination parameters.
func (u *LogService) PageLoginLog(c *gin.Context, req dto.SearchLgLogWithPage) (int64, interface{}, error) {
	lang := common.GetLang(c)
	limitParam := c.DefaultQuery("limit", strconv.FormatInt(defaultLimit, 10))
	offsetParam := c.Query("offset")

	pageSize, offset := parseOffsetAndLimit(limitParam, offsetParam)
	if pageSize < 1 || offset < 0 {
		return 0, nil, errors.New("invalid limit or offset parameter")
	}

	total, ops, err := logRepo.PageLoginLog(
		req.Page,
		int(pageSize),
		req.PageSize,
		req.LikeFieldFilter(),
		req.FieldFilters(),
		req.TimeRangeFilter(),)
	if err != nil {
		return 0, nil, errors.WithMessage(constant.ErrDatabaseAccess, err.Error())
	}

	dtoOps := make([]LogDTO, len(ops))
	for i, op := range ops {
		var item LogDTO
		copyValuesToDTO(&item, op)
		item.Address, _ = geo.GetIPLocation(op.IP, lang)

		dtoOps[i] = item
	}

	return total, convertInterfacesSlice(dtoOps), err
}

func parseOffsetAndLimit(large, small string) (largeNum int, smallNum int) {
	largeNumStr := strings.ReplaceAll(dashboardConfig.Server.MaxPaginationSize, ",","")
	largeNumParse, err:=strconv.Atoi(largeNumStr)
	if err!=nil{
		largeNum=dashboardConfig.Server.MaxPaginationSizeInt()
	}else{
		if largeNum>dashboardConfig.Server.MaxPaginationSizeInt(){
			largeNum=dashboardConfig.Server.MaxPaginationSizeInt();
		}
	}
	smallNum,_=strconv.Atoi(small)
	return int(largeNum),smallNum
}

func copyValuesToDTO(dest, src interface{}) {
	copier.Copy(dest, src)
}

func convertInterfacesSlice(data []interface{}) *[]map[string]interface{} {
	result := make([]map[string]interface{}, len(data))
	for index, value := range data {
		marshal, e := json.Marshal(value)
		if e != nil {
				continue
		}
		json.Unmarshal(marshal,&result[index])
	}

	return &result
}

Key Changes Made:

  • Import Statements: Moved necessary imports near their use within functions.

  • Default Limit: Set a default pagination limit in case none is specified in the query.

  • Type Conversion Utility Functions: Added utility functions convertInterfacesSlice and copyValuesToDTO for converting slice elements between interfaces and maps.

  • Validation of Pagination Parameters: Added validation for page size and offset parameters to prevent potential integer overflow or negative values.

These optimizations aim to improve readability and maintainability while potentially reducing runtime costs.

Expand Down
6 changes: 3 additions & 3 deletions backend/utils/geo/geo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func GetIPLocation(ip, lang string) (string, error) {
if err != nil {
return "", err
}
if lang == "en" {
return geoLocation.Country.En + " " + geoLocation.Province.En, nil
if lang == "zh" {
return geoLocation.Country.Zh + " " + geoLocation.Province.Zh, nil
}
return geoLocation.Country.Zh + " " + geoLocation.Province.Zh, nil
return geoLocation.Country.En + " " + geoLocation.Province.En, nil
}
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 has two main issues:

  1. In the second if statement, the return values are not used correctly.
  2. The function does not handle invalid input gracefully.

Changes suggested:

func GetIPLocation(ip, lang string) (string, error) {
	if err != nil {
		return "", err
	}
	switch lang {
	case "en":
		return geoLocation.Country.En + " " + geoLocation.Province.En, nil
	case "zh":
		return geoLocation.Country.Zh + " " + geoLocation.Province.Zh, nil
	default:
		return "", fmt.Errorf("Unsupported language: %s", lang)
	}
}

// If 'geoLocation' is not defined elsewhere in this file, you should define it here to avoid compile errors.
type GeoLocation struct {
	Country struct {
		En string
		Zh string
	} Province struct {
		En string
		Zh string
	}
}

These changes address both issues by using a switch statement for better readability and proper handling of unsupported languages. Additionally, I've added missing type definitions and included a default case to indicate an unsupported language. This ensures that the function behaves as expected across different scenarios.

Loading