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

[ISSUE #12719] refresh the client's access token #12765

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

lucky8987
Copy link
Contributor

What is the purpose of the change

issues#12719

Brief changelog

Modify the login logic in the NacosClientAuthServiceImpl code to record the refresh time of the token in the 'LoginIdentity Context'.

When the response code is determined to be 403 on the client side, set the parameter 'NextRefreshTime' in 'Login Identity Context' to 0.

After waiting for the timer task for 5 seconds, enter the login process and refresh the token.


  1. 修改代码NacosClientAuthServiceImpl中的登录逻辑,将Token下一次的刷新时间记录到 LoginIdentityContext 中。
  2. 在 client 端判断response code 为 403 时将 LoginIdentityContext 里面的参数 nextRefreshTime(下一次刷新时间) 设置为 0。
  3. 等待定时器任务5秒后进入登录流程,进行Token刷新。

Verifying this change

Modify the nacos.cre.auth.plugin.nacos.token.secreet.key parameter in the application and observe whether the client performs a re login operation

Copy link

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io


感谢您提交的PR。 🙏
请再次查看您的PR内容,确认是否包含任何使用方式/API/配置参数的变更,如:新增API新增配置参数修改默认配置等操作。
如果是,请确保在提交之前,在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档(markdown格式)。

@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2024

Codecov Report

Attention: Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.

Project coverage is 72.16%. Comparing base (f425ef5) to head (b7b587e).
Report is 10 commits behind head on develop.

Files with missing lines Patch % Lines
...m/alibaba/nacos/client/security/SecurityProxy.java 58.33% 3 Missing and 2 partials ⚠️
...ba/nacos/plugin/auth/api/LoginIdentityContext.java 0.00% 2 Missing ⚠️
...s/client/auth/impl/NacosClientAuthServiceImpl.java 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             develop   #12765      +/-   ##
=============================================
+ Coverage      72.14%   72.16%   +0.02%     
- Complexity      9782     9793      +11     
=============================================
  Files           1283     1283              
  Lines          41349    41379      +30     
  Branches        4366     4374       +8     
=============================================
+ Hits           29833    29863      +30     
+ Misses          9414     9411       -3     
- Partials        2102     2105       +3     
Files with missing lines Coverage Δ
...pi/config/remote/response/ConfigQueryResponse.java 100.00% <ø> (ø)
...nacos/client/auth/impl/NacosAuthLoginConstant.java 0.00% <ø> (ø)
...alibaba/nacos/client/config/impl/ClientWorker.java 76.89% <100.00%> (+0.59%) ⬆️
...acos/client/config/impl/ConfigTransportClient.java 86.84% <100.00%> (+0.73%) ⬆️
...lient/naming/remote/AbstractNamingClientProxy.java 100.00% <100.00%> (ø)
...ient/naming/remote/gprc/NamingGrpcClientProxy.java 98.34% <100.00%> (+0.01%) ⬆️
...ient/naming/remote/http/NamingHttpClientProxy.java 90.95% <100.00%> (+0.09%) ⬆️
...s/client/auth/impl/NacosClientAuthServiceImpl.java 97.22% <87.50%> (-2.78%) ⬇️
...ba/nacos/plugin/auth/api/LoginIdentityContext.java 80.00% <0.00%> (-20.00%) ⬇️
...m/alibaba/nacos/client/security/SecurityProxy.java 84.37% <58.33%> (-15.63%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f425ef5...b7b587e. Read the comment docs.

@lucky8987 lucky8987 requested a review from KomachiSion October 21, 2024 10:01
Copy link
Collaborator

@KomachiSion KomachiSion left a comment

Choose a reason for hiding this comment

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

  1. No implementation about step more interval to re-login which to invalid the real username and password error so that login API called so frequently.

  2. It seems only naming module active, how about config?

@lucky8987
Copy link
Contributor Author

  1. No implementation about step more interval to re-login which to invalid the real username and password error so that login API called so frequently.
  2. It seems only naming module active, how about config?

关于账号或密码错误导致的re-login频繁确实是一个困难的问题,因为最理想的方式就是auth模块针对账号密码错误与token不合法的错误通过code区分开,但是这样需要重新制定auth spi对应的标准,改动范围不可控,关于第2点没太明白具体内容,能否详细说明一下?

@KomachiSion
Copy link
Collaborator

  1. No implementation about step more interval to re-login which to invalid the real username and password error so that login API called so frequently.
  2. It seems only naming module active, how about config?

关于账号或密码错误导致的re-login频繁确实是一个困难的问题,因为最理想的方式就是auth模块针对账号密码错误与token不合法的错误通过code区分开,但是这样需要重新制定auth spi对应的标准,改动范围不可控,关于第2点没太明白具体内容,能否详细说明一下?

  1. 频繁调用login接口会导致服务端的CPU飙高, 因为密码的加密和match计算是很强的CPU操作,因此如果暂时无法区分, 也最好在此处加上错误后的延迟,避免频繁调用把服务端打挂;尤其是加入403主动调用login的机制之后。

  2. 我看代码里只写了naming模块的, config(配置)的好像不生效啊。

@lucky8987 lucky8987 force-pushed the develop-issue#12719 branch 2 times, most recently from eadc546 to dbe6d86 Compare November 8, 2024 10:47
@lucky8987 lucky8987 requested a review from KomachiSion November 8, 2024 15:05
lucky8987

This comment was marked as outdated.

@lucky8987
Copy link
Contributor Author

lucky8987 commented Nov 13, 2024

  1. No implementation about step more interval to re-login which to invalid the real username and password error so that login API called so frequently.
  2. It seems only naming module active, how about config?

关于账号或密码错误导致的re-login频繁确实是一个困难的问题,因为最理想的方式就是auth模块针对账号密码错误与token不合法的错误通过code区分开,但是这样需要重新制定auth spi对应的标准,改动范围不可控,关于第2点没太明白具体内容,能否详细说明一下?

  1. 频繁调用login接口会导致服务端的CPU飙高, 因为密码的加密和match计算是很强的CPU操作,因此如果暂时无法区分, 也最好在此处加上错误后的延迟,避免频繁调用把服务端打挂;尤其是加入403主动调用login的机制之后。
  2. 我看代码里只写了naming模块的, config(配置)的好像不生效啊。

1.针对频繁调用login接口的问题,增加了时间窗口限制。
2.之前遗漏了 config 模块,现已经补充在requestProxy增加403错误码判断触发relogin逻辑。

@wuyfee
Copy link

wuyfee commented Nov 14, 2024

$\color{green}{SUCCESS}$
DETAILS
✅ - docker: success
✅ - deploy (standalone & cluster & standalone_auth): success
✅ - e2e-java-test (standalone & cluster & standalone_auth): success
✅ - e2e-go-test (standalone & cluster): success
✅ - e2e-cpp-test (standalone & cluster): success
✅ - e2e-csharp-test (standalone & cluster): success
✅ - e2e-nodejs-test (standalone & cluster): success
✅ - e2e-python-test (standalone & cluster): success
✅ - clean (standalone & cluster & standalone_auth): success

@KomachiSion KomachiSion merged commit fb68b7e into alibaba:develop Nov 15, 2024
7 checks passed
@KomachiSion KomachiSion added this to the 2.5.0 milestone Nov 15, 2024
@KomachiSion KomachiSion added area/Client Related to Nacos Client SDK kind/enhancement Category issues or prs related to enhancement. labels Nov 15, 2024
@wuyfee
Copy link

wuyfee commented Nov 15, 2024

$\color{green}{SUCCESS}$
DETAILS
✅ - docker: success
✅ - deploy (standalone & cluster & standalone_auth): success
✅ - e2e-java-test (standalone & cluster & standalone_auth): success
✅ - e2e-go-test (standalone & cluster): success
✅ - e2e-cpp-test (standalone & cluster): success
✅ - e2e-csharp-test (standalone & cluster): success
✅ - e2e-nodejs-test (standalone & cluster): success
✅ - e2e-python-test (standalone & cluster): success
✅ - clean (standalone & cluster & standalone_auth): success

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/Client Related to Nacos Client SDK kind/enhancement Category issues or prs related to enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants