feat: whoami granular #494
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
elrrrrrrr
commented
Jun 3, 2023
elrrrrrrr
commented
- 🧶 Add token information to the "whoami" interface.
- 🔨 Modify the query logic for allowedPackages uniformly within the repository.
- 🧶 在whoami 接口中添加 token 信息
- 🔨 修改 allowedPackages 查询逻辑，统一在 repository 中集成
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -61,7 +61,7 @@ export class Token extends Entity { | |||
| this.tokenKey = data.tokenKey; | |||
| this.cidrWhitelist = data.cidrWhitelist || []; | |||
| this.isReadonly = data.isReadonly || false; | |||
| this.type = data.type || TokenType.classic; | |||
| this.type = (data.type as TokenType) || TokenType.classic; | |||
| this.lastUsedAt = data.lastUsedAt || null; | |||
|
|
|||
| if (isGranularToken(data)) { | |||
There was a problem hiding this comment.
这段代码补丁看起来没有明显的错误风险，但是以下是一些可以改进的建议：
- 为了提高代码的可读性和易于维护性，可以向
interface BaseTokenData中添加 API 文档注释。 - 当
data.type的类型不是TokenType时，将其默认值更改为null或引发错误可能会更好，并将实例化Token对象的逻辑清晰化，在构造函数中分别处理默认值、可选值和必需值将有助于代码可读性上的改进。
| if (Array.isArray(packages)) { | ||
| token.allowedPackages = packages.map(p => getFullname(p.scope, p.name)); | ||
| } | ||
| } | ||
| const objects = granularTokens.map(token => { | ||
| const { name, description, expiredAt, allowedPackages, allowedScopes, lastUsedAt, type } = token; | ||
| return { |
There was a problem hiding this comment.
这段代码对TokenController进行了修改，删除了TokenService和getFullname的调用。可能存在一些潜在问题和可优化空间。建议将以下问题考虑在内：
-
删除TokenService和getFullname的代码段是否意味着需要在其他地方引入它们的实现？
-
granularTokens.map会产生一个新数组，如果tokens数组过大，可能会导致内存溢出，建议使用可迭代的解决方案。
-
是否应该添加一些错误处理来避免可能的异常情况？
| }; | ||
|
|
||
| } | ||
|
|
||
| // https://github.com/cnpm/cnpmcore/issues/64 |
There was a problem hiding this comment.
这段代码主要是一个用户控制器，包含了一些处理用户登陆、权限等的方法。对于提供的代码补丁，以下是我的小型代码审查：
- 在第 4 行，import 引入了新模块
isGranularToken，需要确定此模块是否存在和正确性。 - 在第 157 行，在获取 authorizedUser 后在必要时应该对其进行空值检查以避免潜在的 NullReference 错误。
- 在第 158 至 187 行中新增了
whoami方法，返回已授权用户的相关信息。其中，使用了类型为GranularToken的 token 对象，并只返回部分属性。需要确认 GranularToken 对象有否定义完整并符合预期。 - 返回数据可以加上描述信息，增强代码阅读的可理解性。
建议：
- 除非
isGranularToken已由其他模块导出或定义，否则应考虑将其放在同一文件夹下进行维护和管理。 - 设置控制器的分层结构。
- 确保所引用的外部依赖库已正确安装，并针对在执行测例时可能出现的所有异常情况进行单元测试。
总的来说，这段代码看起来良好且易于理解，但需要专注于特殊的对象和方法，并仔细验证所有新添加的内容。
| for (const token of tokens) { | ||
| await this._injectTokenPackages(token); | ||
| } | ||
| return tokens; | ||
| } | ||
|
|
||
| async saveCredential(credential: WebauthnCredentialEntity): Promise<void> { |
There was a problem hiding this comment.
这个代码补丁主要是对一个用户仓库 UserRepository 进行了调整，添加了一个新的模型依赖注入，并在私有方法 _injectTokenPackages() 中使用。这个方法会获取特定 token 的详细信息，包括它所属的 package 信息，并将这些信息添加到返回的 token 实体中。
回顾代码，以下问题可能需要注意：
- 没有看到对
AbstractRepository类的定义，因此无法判断属性和方法是否按照预期工作。 - 对于返回值的类型，可以尝试使用更明确的类型或接口而不是泛型，例如
Promise<TokenEntity | null>可以改写为Promise<{ token: TokenEntity; user: UserEntity } | null>。 - 在
findTokenByTokenKey()方法中，如果找不到相应的 model ，应该返回 Promise.resolve(null) 而不是直接返回 null 。 - 私有方法
_injectTokenPackages()只在单个方法中调用。因此，您可以删除私有方法并将其内容放到findTokenByTokenKey()和listTokens()方法之中，或者将之提取出来作为公共方法，以便其他仓库也可以使用。
Codecov Report
@@ Coverage Diff @@
## master #494 +/- ##
=======================================
Coverage 97.45% 97.45%
=======================================
Files 168 168
Lines 15691 15751 +60
Branches 2018 2024 +6
=======================================
+ Hits 15291 15350 +59
- Misses 400 401 +1
|
| assert.deepEqual(res.body.allowedPackages, [ '@cnpm/banana' ]); | ||
| assert.deepEqual(res.body.allowedScopes, [ '@banana' ]); | ||
| assert(dayjs(res.body.expires).isBefore(dayjs().add(30, 'days'))); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
此代码段主要是针对一个Node.js应用中UserController的whoami方法编写的测试代码。这里引入了AuthAdapter库，新加入了dayjs和mock库。
建议对新增内容进行以下改进：
- 需要在代码开始部分添加必要的import语句。
- 同时添加用于控制测试范围的边界条件。例如，在测试函数开头添加beforeEach和afterEach标记，以便能够在测试之间设置并清理使用的资源。
- 在进行模拟调用时，需要注意不要篡改其他方法，可以通过创建新实例或仅返回所需数据进行优化。
- 建议在测试用例说明中注明测试对象和测试点。
fengmk2
reviewed
Jun 4, 2023
| private async _injectTokenPackages(token: TokenEntity) { | ||
| if (isGranularToken(token)) { | ||
| const models = await this.TokenPackage.find({ tokenId: token.tokenId }); | ||
| const packages = await this.Package.find({ packageId: models.map(m => m.packageId) }); |
There was a problem hiding this comment.
https://github.com/cnpm/cnpmcore/blob/master/app/port/controller/TokenController.ts#L36
现在做了最大 50 个的限制，等 npm 后面调整了我们再跟进 😄
fengmk2
pushed a commit
that referenced
this pull request
Jun 4, 2023
[skip ci] ## [3.26.0](v3.25.1...v3.26.0) (2023-06-04) ### Features * whoami return granular token info ([#494](#494)) ([d0d2f78](d0d2f78))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.