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 lastUsedAt for classic token #547

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

elrrrrrrr
Copy link
Member

Follow #488 , add lastUsedAt for classic tokens.

  1. 🧶 Modify the checkTokenExpired method to checkTokenStatus which update the token field internally.
  2. ♻️ No compensation will be made for existing data, and it should be updated by the consuming end.

Follow #488, 为 classic token 也添加 lastUsedAt 信息

  1. 🧶 修改 checkTokenExpired 方法为 checkTokenStatus,内部进行 token 字段更新
  2. ♻️ 存量数据不做补偿,由消费端控制

@elrrrrrrr elrrrrrrr added the enhancement New feature or request label Jul 12, 2023
@@ -113,7 +113,7 @@ export class UserRoleManager {
}

// check token expired & set lastUsedAt
await this.tokenService.checkTokenExpired(authorizedUserAndToken.token);
await this.tokenService.checkTokenStatus(authorizedUserAndToken.token);
this.currentAuthorizedToken = authorizedUserAndToken.token;
this.currentAuthorizedUser = authorizedUserAndToken.user;
ctx.userId = authorizedUserAndToken.user.userId;

Choose a reason for hiding this comment

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

这段代码是一个UserRoleManager类中的一部分。代码检查了令牌是否过期,并设置了lastUsedAt的值。

在修改的代码行中,将调用checkTokenExpired方法改为了checkTokenStatus方法。

对于代码的风险和改进建议,我目前无法提供详细信息,因为我无法看到完整的代码上下文和相关代码。但是,根据给出的部分代码,没有明显的错误或问题,而且修改似乎只是用不同的方法名替换了原有的方法名。

如果您提供更多的上下文或代码细节,我可能能够给出更具体的反馈。

const lastUsedAt = res.body.objects[0].lastUsedAt;
assert(dayjs(lastUsedAt).isAfter(now));
});

it('should 401 when readonly token access', async () => {
const { authorization } = await TestUtil.createUser({ tokenOptions: { readonly: true } });
const res = await app.httpRequest()

Choose a reason for hiding this comment

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

这段代码看起来是一个关于TokenController的测试。

潜在的问题和改进建议如下:

  1. 引入dayjs库之后没有执行npm install命令,可能会导致缺少依赖。需要确保安装了dayjs库。
  2. 在第二个测试用例中,通过调用TestUtil.createUser()创建用户并获取authorization。然而,在之后没有清理创建的用户或回滚对数据库的更改。建议在测试运行结束后进行清理操作,以确保数据库的一致性。
  3. 第二个测试用例验证了lastUsedAt属性是否在当前时间之后,但是使用了Date.now()来获取当前时间。为了准确比较时间,建议使用dayjs库提供的方法来获取当前时间,例如dayjs().valueOf()

总体上看,代码比较简洁,没有明显的错误。以上是一些建议和潜在问题,供参考。

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #547 (5aaf6cf) into master (dd3438f) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
- Coverage   97.02%   96.96%   -0.06%     
==========================================
  Files         174      174              
  Lines       16632    16627       -5     
  Branches     2186     2180       -6     
==========================================
- Hits        16137    16123      -14     
- Misses        495      504       +9     
Impacted Files Coverage Δ
app/core/service/TokenService.ts 100.00% <100.00%> (ø)
app/port/UserRoleManager.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

Copy link
Member

@fengmk2 fengmk2 left a comment

Choose a reason for hiding this comment

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

+1

@fengmk2 fengmk2 merged commit e061685 into master Jul 12, 2023
12 of 13 checks passed
@fengmk2 fengmk2 deleted the classic-token-last-used-at branch July 12, 2023 07:11
fengmk2 pushed a commit that referenced this pull request Jul 12, 2023
[skip ci]

## [3.37.0](v3.36.0...v3.37.0) (2023-07-12)

### Features

* add lastUsedAt for classic token ([#547](#547)) ([e061685](e061685))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants