-
Notifications
You must be signed in to change notification settings - Fork 87
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: whoami granular #494
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,8 +13,6 @@ import { | |
import { Static, Type } from '@sinclair/typebox'; | ||
import { AbstractController } from './AbstractController'; | ||
import { TokenType, isGranularToken } from '../../core/entity/Token'; | ||
import { TokenService } from '../../../app/core/service/TokenService'; | ||
import { getFullname } from '../../../app/common/PackageUtil'; | ||
|
||
// Creating and viewing access tokens | ||
// https://docs.npmjs.com/creating-and-viewing-access-tokens#viewing-access-tokens | ||
|
@@ -44,8 +42,6 @@ type GranularTokenOptions = Static<typeof GranularTokenOptionsRule>; | |
export class TokenController extends AbstractController { | ||
@Inject() | ||
private readonly authAdapter: AuthAdapter; | ||
@Inject() | ||
private readonly tokenService: TokenService; | ||
// https://github.com/npm/npm-profile/blob/main/lib/index.js#L233 | ||
@HTTPMethod({ | ||
path: '/-/npm/v1/tokens', | ||
|
@@ -198,12 +194,6 @@ export class TokenController extends AbstractController { | |
const tokens = await this.userRepository.listTokens(user.userId); | ||
const granularTokens = tokens.filter(token => isGranularToken(token)); | ||
|
||
for (const token of granularTokens) { | ||
const packages = await this.tokenService.listTokenPackages(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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这段代码对TokenController进行了修改,删除了TokenService和getFullname的调用。可能存在一些潜在问题和可优化空间。建议将以下问题考虑在内:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import { Static, Type } from '@sinclair/typebox'; | |
import { AbstractController } from './AbstractController'; | ||
import { LoginResultCode } from '../../common/enum/User'; | ||
import { sha512 } from '../../common/UserUtil'; | ||
import { isGranularToken } from '../../core/entity/Token'; | ||
|
||
// body: { | ||
// _id: 'org.couchdb.user:dddd', | ||
|
@@ -157,10 +158,34 @@ export class UserController extends AbstractController { | |
method: HTTPMethodEnum.GET, | ||
}) | ||
async whoami(@Context() ctx: EggContext) { | ||
const authorizedUser = await this.userRoleManager.requiredAuthorizedUser(ctx, 'read'); | ||
await this.userRoleManager.requiredAuthorizedUser(ctx, 'read'); | ||
const authorizedRes = await this.userRoleManager.getAuthorizedUserAndToken(ctx); | ||
const { token, user } = authorizedRes!; | ||
|
||
if (isGranularToken(token)) { | ||
const { name, description, expiredAt, allowedPackages, allowedScopes, lastUsedAt, type } = token; | ||
return { | ||
username: user.displayName, | ||
name, | ||
description, | ||
allowedPackages, | ||
allowedScopes, | ||
lastUsedAt, | ||
expiredAt, | ||
// do not return token value | ||
// token: token.token, | ||
key: token.tokenKey, | ||
cidr_whitelist: token.cidrWhitelist, | ||
readonly: token.isReadonly, | ||
created: token.createdAt, | ||
updated: token.updatedAt, | ||
type, | ||
}; | ||
} | ||
return { | ||
username: authorizedUser.displayName, | ||
username: user.displayName, | ||
}; | ||
|
||
} | ||
|
||
// https://github.com/cnpm/cnpmcore/issues/64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这段代码主要是一个用户控制器,包含了一些处理用户登陆、权限等的方法。对于提供的代码补丁,以下是我的小型代码审查:
建议:
总的来说,这段代码看起来良好且易于理解,但需要专注于特殊的对象和方法,并仔细验证所有新添加的内容。 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,15 @@ | ||
import { AccessLevel, SingletonProto, Inject } from '@eggjs/tegg'; | ||
import { ModelConvertor } from './util/ModelConvertor'; | ||
import type { User as UserModel } from './model/User'; | ||
import type { Package as PackageModel } from './model/Package'; | ||
import type { Token as TokenModel } from './model/Token'; | ||
import type { WebauthnCredential as WebauthnCredentialModel } from './model/WebauthnCredential'; | ||
import { User as UserEntity } from '../core/entity/User'; | ||
import { Token as TokenEntity, isGranularToken } from '../core/entity/Token'; | ||
import { WebauthnCredential as WebauthnCredentialEntity } from '../core/entity/WebauthnCredential'; | ||
import { AbstractRepository } from './AbstractRepository'; | ||
import { TokenPackage as TokenPackageModel } from './model/TokenPackage'; | ||
import { getScopeAndName } from '../common/PackageUtil'; | ||
import { getFullname, getScopeAndName } from '../common/PackageUtil'; | ||
import { PackageRepository } from './PackageRepository'; | ||
|
||
@SingletonProto({ | ||
|
@@ -24,6 +25,9 @@ export class UserRepository extends AbstractRepository { | |
@Inject() | ||
private readonly TokenPackage: typeof TokenPackageModel; | ||
|
||
@Inject() | ||
private readonly Package: typeof PackageModel; | ||
|
||
@Inject() | ||
private readonly packageRepository: PackageRepository; | ||
|
||
|
@@ -58,6 +62,7 @@ export class UserRepository extends AbstractRepository { | |
if (!token) return null; | ||
const userModel = await this.User.findOne({ userId: token.userId }); | ||
if (!userModel) return null; | ||
|
||
return { | ||
token, | ||
user: ModelConvertor.convertModelToEntity(userModel, UserEntity), | ||
|
@@ -67,7 +72,19 @@ export class UserRepository extends AbstractRepository { | |
async findTokenByTokenKey(tokenKey: string) { | ||
const model = await this.Token.findOne({ tokenKey }); | ||
if (!model) return null; | ||
return ModelConvertor.convertModelToEntity(model, TokenEntity); | ||
const token = ModelConvertor.convertModelToEntity(model, TokenEntity); | ||
await this._injectTokenPackages(token); | ||
return token; | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 希望未来别一个 token 有几十万个包 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/cnpm/cnpmcore/blob/master/app/port/controller/TokenController.ts#L36 现在做了最大 50 个的限制,等 npm 后面调整了我们再跟进 😄 |
||
if (Array.isArray(packages)) { | ||
token.allowedPackages = packages.map(p => getFullname(p.scope, p.name)); | ||
} | ||
} | ||
} | ||
|
||
async saveToken(token: TokenEntity): Promise<void> { | ||
|
@@ -111,7 +128,11 @@ export class UserRepository extends AbstractRepository { | |
|
||
async listTokens(userId: string): Promise<TokenEntity[]> { | ||
const models = await this.Token.find({ userId }); | ||
return models.map(model => ModelConvertor.convertModelToEntity(model, TokenEntity)); | ||
const tokens = models.map(model => ModelConvertor.convertModelToEntity(model, TokenEntity)); | ||
for (const token of tokens) { | ||
await this._injectTokenPackages(token); | ||
} | ||
return tokens; | ||
} | ||
|
||
async saveCredential(credential: WebauthnCredentialEntity): Promise<void> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这个代码补丁主要是对一个用户仓库 回顾代码,以下问题可能需要注意:
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { AuthAdapter } from 'app/infra/AuthAdapter'; | ||
import assert from 'assert'; | ||
import { app } from 'egg-mock/bootstrap'; | ||
import dayjs from 'dayjs'; | ||
import { app, mock } from 'egg-mock/bootstrap'; | ||
import { TestUtil } from 'test/TestUtil'; | ||
|
||
describe('test/port/controller/UserController/whoami.test.ts', () => { | ||
|
@@ -10,7 +12,7 @@ describe('test/port/controller/UserController/whoami.test.ts', () => { | |
.get('/-/whoami') | ||
.set('authorization', authorization) | ||
.expect(200); | ||
assert.equal(res.body.username, name); | ||
assert.deepStrictEqual(res.body, { username: name }); | ||
}); | ||
|
||
it('should unauthorized', async () => { | ||
|
@@ -25,5 +27,38 @@ describe('test/port/controller/UserController/whoami.test.ts', () => { | |
.expect(401); | ||
assert.equal(res.body.error, '[UNAUTHORIZED] Invalid token'); | ||
}); | ||
|
||
it('should return granular token info', async () => { | ||
const { name, email } = await TestUtil.createUser({ name: 'banana' }); | ||
await TestUtil.createPackage({ name: '@cnpm/banana' }); | ||
mock(AuthAdapter.prototype, 'ensureCurrentUser', async () => { | ||
return { | ||
name, | ||
email, | ||
}; | ||
}); | ||
let res = await app.httpRequest() | ||
.post('/-/npm/v1/tokens/gat') | ||
.send({ | ||
name: 'apple', | ||
description: 'lets play', | ||
allowedPackages: [ '@cnpm/banana' ], | ||
allowedScopes: [ '@banana' ], | ||
expires: 30, | ||
}) | ||
.expect(200); | ||
|
||
res = await app.httpRequest() | ||
.get('/-/whoami') | ||
.set('authorization', `Bearer ${res.body.token}`) | ||
.expect(200); | ||
|
||
assert.equal(res.body.username, name); | ||
assert.equal(res.body.name, 'apple'); | ||
assert.equal(res.body.description, 'lets play'); | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 此代码段主要是针对一个Node.js应用中UserController的whoami方法编写的测试代码。这里引入了AuthAdapter库,新加入了dayjs和mock库。 建议对新增内容进行以下改进:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这段代码补丁看起来没有明显的错误风险,但是以下是一些可以改进的建议:
interface BaseTokenData
中添加 API 文档注释。data.type
的类型不是TokenType
时,将其默认值更改为null
或引发错误可能会更好,并将实例化Token
对象的逻辑清晰化,在构造函数中分别处理默认值、可选值和必需值将有助于代码可读性上的改进。