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

【新增】yudao-spring-boot-starter-signature接口签名模块 #526

Closed

Conversation

craftsman4j
Copy link
Contributor

No description provided.

@YunaiV
Copy link
Owner

YunaiV commented May 16, 2024

不错的 pr,我可能要周末在细看,非常感谢哈。

@@ -23,6 +23,10 @@ public class CacheRequestBodyWrapper extends HttpServletRequestWrapper {
*/
private final byte[] body;

public byte[] getBody() {
Copy link
Owner

Choose a reason for hiding this comment

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

建议加一个 @VisableForTest 的注解,表示它是临时可见

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是Guava包中的@VisibleForTesting注解吗?搜索了一下,并未找到@VisableForTest注解相关资料

@@ -25,6 +25,7 @@ public interface GlobalErrorCodeConstants {
ErrorCode METHOD_NOT_ALLOWED = new ErrorCode(405, "请求方法不正确");
ErrorCode LOCKED = new ErrorCode(423, "请求失败,请稍后重试"); // 并发请求,不允许
ErrorCode TOO_MANY_REQUESTS = new ErrorCode(429, "请求过于频繁,请稍后重试");
ErrorCode SIGNATURE_REQUESTS = new ErrorCode(430, "签名失败");
Copy link
Owner

Choose a reason for hiding this comment

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

建议使用 400 Bad Request,提示可以使用,签名不正确

<packaging>jar</packaging>

<name>${project.artifactId}</name>
<description>提供接口验签功能</description>
Copy link
Owner

Choose a reason for hiding this comment

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

建议放到 protection 模块下

/**
* url客户端不需要传递,但是可以用来加签(如:/{id}带有动态参数的url,如果没有动态参数可设置为false,不进行加签)
*/
boolean urlEnable() default true;
Copy link
Owner

Choose a reason for hiding this comment

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

一般带 url 的多么?
我瞅了几个平台,貌似带的不多。

所以在想,是不是默认不要这个属性哈?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

项目中基本没有带动态参数的,不过出于扩展性,或者以后考虑把framework做为组件发布至中央仓库供其他项目使用时,是有必要的

Class<? extends SignatureApi> signatureApi() default DefaultSignatureApiImpl.class;

/**
* 同一个请求多长时间内有效 默认10分钟
Copy link
Owner

Choose a reason for hiding this comment

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

同一个请求多长时间内有效 默认 10 分钟

改成这个。中文写作习惯,中英文之间要有空格

}
SignatureApi signatureApi = SpringUtil.getBean(signature.signatureApi());
Assert.notNull(signatureApi, "找不到对应的 SignatureApi 实现");
// 校验appId是否能获取到对应的appSecret
Copy link
Owner

Choose a reason for hiding this comment

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

注释,注意下中英文之间的空行

String serverSign = signatureApi.digestEncoder(signContent.toString());
// 客户端签名
String clientSign = request.getHeader(signature.sign());
if (!StrUtil.equals(clientSign, serverSign)) {
Copy link
Owner

Choose a reason for hiding this comment

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

StrUtil.notEquals

建议上,逻辑尽量避免 if (!xxx)

人对取反要多一层思考;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StrUtil 中没有 notEquals 方法

signContent.append(appSecret);

// 生成服务端签名
String serverSign = signatureApi.digestEncoder(signContent.toString());
Copy link
Owner

Choose a reason for hiding this comment

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

这个签名,抽一个小方法在这个逻辑里。

第一期,就不要搞太复杂的哈。HMAC-SHA256 之类的就 ok 啦

sortedMap.put("param", JsonUtils.toJsonString(paramMap));
}

CacheRequestBodyWrapper requestWrapper = new CacheRequestBodyWrapper(request);
Copy link
Owner

Choose a reason for hiding this comment

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

body 的话,建议是简单点;直接拿整个 requestBody;里面不需要再排序了;

可以在调研下其他做开放平台的,他们是怎么考虑的哈。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,这个之前确实有考虑不需要排序。

* @param args args
* @throws UnsupportedEncodingException
*/
public static void main(String[] args) throws UnsupportedEncodingException {
Copy link
Owner

Choose a reason for hiding this comment

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

可以学习下单元测试的写法哈

@YunaiV
Copy link
Owner

YunaiV commented May 18, 2024

第一轮 review 结束。整体写的蛮好的,实现思路没问题。

code review 给的建议,更多是希望怎么更简洁,满足绝大多数场景。

ps:有没可能基于 master-jdk17 改下,发起一次 pr 哈;因为项目是基于 master-jdk17,然后可以自动适配同步到 master 分支的噢

@craftsman4j
Copy link
Contributor Author

好的,感谢大佬采纳并给出建议,我明天按规范改一版

@craftsman4j craftsman4j deleted the dev-0515-framework-signature branch May 20, 2024 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants