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

Implement #2099,添加apollo-client的身份认证功能及accesskey管理功能 #2828

Merged
merged 2 commits into from
Jan 1, 2020

Conversation

nisiyong
Copy link
Member

@nisiyong nisiyong commented Dec 10, 2019

What's the purpose of this PR

  • 添加apollo-configservice对apollo-client的身份认证
  • 添加apollo-portal的accesskey管理功能

Which issue(s) this PR fixes:

Implement #2099

Brief changelog

apollo-client

  1. 在客户端添加配置apollo.accesskey.secret,用来做访问apollo对密钥
  2. 用secret对请求签名,在客户端访问configservice对请求添加请求头Authorization,Timestamp

apollo-configservice

  1. 添加filter过滤器,解析header,对客户端请求进行身份认证,如果非法请求则返回401
  2. 对于旧版项目没有配置secret对,默认跳过。等添加了secret才会进行身份认证
  3. ApolloConfigDB添加新表AccessKey
CREATE TABLE `AccessKey` (
  `Id` int(10) unsigned NOT NULL AUTO_INCREMENT COMMENT '自增主键',
  `AppId` varchar(500) NOT NULL DEFAULT 'default' COMMENT 'AppID',
  `Secret` varchar(128) NOT NULL DEFAULT '' COMMENT 'Secret',
  `IsEnabled` bit(1) NOT NULL DEFAULT b'0' COMMENT '1: enabled, 0: disabled',
  `IsDeleted` bit(1) NOT NULL DEFAULT b'0' COMMENT '1: deleted, 0: normal',
  `DataChange_CreatedBy` varchar(32) NOT NULL DEFAULT 'default' COMMENT '创建人邮箱前缀',
  `DataChange_CreatedTime` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP COMMENT '创建时间',
  `DataChange_LastModifiedBy` varchar(32) NOT NULL DEFAULT '' COMMENT '最后修改人邮箱前缀',
  `DataChange_LastTime` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP COMMENT '最后修改时间',
  PRIMARY KEY (`Id`),
  KEY `AppId` (`AppId`(191)),
  KEY `DataChange_LastTime` (`DataChange_LastTime`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8mb4 COMMENT='访问密钥';

apollo-portal & apollo-adminservice

感谢 @huangchenhai-666 提供Portal页面支持

  1. 添加密钥管理功能
  2. 支持多个密钥(最多5个),用于密钥更替时兼容
  3. 已创建的项目需要添加权限。可以用insert select清洗数据
INSERT INTO `Permission` (`PermissionType`, `TargetId`, `IsDeleted`, `DataChange_CreatedBy`, `DataChange_LastModifiedBy`) VALUES ( 'ManageAppMaster', 'demo', 0, 'apollo', 'apollo');

Featrue Preview

项目管理员可以看到管理密钥对菜单
image

为每个项目配置密钥,每个环境密钥不同。选择环境添加后自动生成,默认是禁用的
image

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.

@codecov-io
Copy link

codecov-io commented Dec 10, 2019

Codecov Report

Merging #2828 into master will increase coverage by 8.53%.
The diff coverage is 52.23%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2828      +/-   ##
============================================
+ Coverage      50.4%   58.94%   +8.53%     
+ Complexity     2083     1219     -864     
============================================
  Files           414      201     -213     
  Lines         12672     6084    -6588     
  Branches       1297      649     -648     
============================================
- Hits           6387     3586    -2801     
+ Misses         5831     2227    -3604     
+ Partials        454      271     -183
Impacted Files Coverage Δ Complexity Δ
...ring/boot/ApolloApplicationContextInitializer.java 93.61% <ø> (ø) 15 <0> (ø) ⬇️
...m/ctrip/framework/apollo/biz/config/BizConfig.java 27.77% <0%> (-3.48%) 8 <0> (ø)
...trip/framework/apollo/common/dto/AccessKeyDTO.java 0% <0%> (ø) 0 <0> (?)
...rk/foundation/internals/provider/NullProvider.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../apollo/internals/RemoteConfigLongPollService.java 80.72% <100%> (+0.47%) 29 <0> (+1) ⬆️
...ework/apollo/internals/RemoteConfigRepository.java 87.42% <100%> (+0.32%) 23 <0> (+1) ⬆️
...va/com/ctrip/framework/apollo/util/ConfigUtil.java 94.61% <100%> (+0.04%) 50 <1> (+1) ⬆️
.../ctrip/framework/apollo/util/http/HttpRequest.java 86.66% <100%> (+3.33%) 7 <2> (+2) ⬆️
...com/ctrip/framework/apollo/util/http/HttpUtil.java 75% <20%> (-5%) 8 <0> (ø)
...framework/apollo/biz/service/AccessKeyService.java 42.42% <42.42%> (ø) 3 <3> (?)
... and 225 more

Continue to review full report at Codecov.

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

@coveralls
Copy link

coveralls commented Dec 10, 2019

Coverage Status

Coverage increased (+0.3%) to 54.24% when pulling e5c479e on 360jinrong:feature/accesskey into 759d364 on ctripcorp:master.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

This feature looks great! I left some comments above, please take a look.

Besides, we need to at least provide some unit tests for core modules like apollo-core, apollo-client and apollo-configservice, so please take some time to add ut for changes in RemoteConfigLongPollService, RemoteConfigRepository, DefaultApplicationProvider, HmacSha1Utils, Signature, HttpUtil, AccessFilter, AccessKeyUtil, AccessKeyServiceWithCache, etc so that we could ensure their logic correctness in the future.

@nisiyong
Copy link
Member Author

@nobodyiam Hi, Song. Thank you for your review and for some advice. I have resolved some problems that you mentioned above, and this PR has some changes as follows:

  1. Use isAppAdmin replace hasManageAccessKeyPermission, then we just need to create AccessKey table with the upgrade, no more DML to fix historical data.
  2. Add icon and some tips for AccessKey Management in the portal, it is more beautiful than before.
  3. RenameAccessFilter toClientAuthenticationFilter.
  4. Add some unit tests for core modules.

Please take a look.

Preview

image
image
image

@nisiyong nisiyong requested a review from nobodyiam December 24, 2019 12:49
@nisiyong nisiyong force-pushed the feature/accesskey branch 2 times, most recently from 184b1d0 to 3574bc4 Compare December 25, 2019 07:16
@nisiyong
Copy link
Member Author

@nobodyiam I have fixed the problems above, the changes are as follow:

  1. Revert some files that have nothing changes.
  2. Add new unit test case about AccessKeySecret in RemoteConfigRepositoryTest and RemoteConfigLongPollServiceTest
  3. Squash git commits. There are 2 commits in this PR.

@nisiyong nisiyong requested a review from nobodyiam December 25, 2019 07:37
@nobodyiam
Copy link
Member

@nisiyong

I have tested in my local environment and it looked great!

However, it seems it will take up to 1 minute to take effect after I enable/disable the access key. It may be a problem in some situations.

Can we change the implementation of com.ctrip.framework.apollo.configservice.service.AccessKeyServiceWithCache#scanNewAccessKeys to make it faster?

@nisiyong
Copy link
Member Author

However, it seems it will take up to 1 minute to take effect after I enable/disable the access key. It may be a problem in some situations.

Yes, the cache is refreshed every minute by default. In my opinion, I don't think it is a problem because we don't change the secret often. Maybe you can tell me more details about this in some situations.

Can we change the implementation of com.ctrip.framework.apollo.configservice.service.AccessKeyServiceWithCache#scanNewAccessKeys to make it faster?

Do you mean to change the default rebuildInterval? I think I can set it 10 seconds by default.

@nobodyiam
Copy link
Member

@nisiyong

Suppose the user enable the secret and just to find the applications printing error logs (because of no secret), so he decides to disable the secret temporarily. In this situation, he has to wait for up to 1 minute to see the effect, which I think will confuse the user.

What I suggest is to change the implementation, e.g. find the new/updated access key by DataChange_LastTime, I think this can solve the problem.

@nisiyong
Copy link
Member Author

What I suggest is to change the implementation, e.g. find the new/updated access key by DataChange_LastTime, I think this can solve the problem.

@nobodyiam

I have considered this implementation before, but the deletion of the access key will still take effect one minute by default. It is indeed better to use DataChange_LastTime to find the new/updated access key every second by default.

Okay, I will change the implementation this week.

@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #2828 into master will increase coverage by 0.28%.
The diff coverage is 60.94%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2828      +/-   ##
============================================
+ Coverage     50.36%   50.64%   +0.28%     
- Complexity     2083     2147      +64     
============================================
  Files           414      425      +11     
  Lines         12682    13055     +373     
  Branches       1297     1337      +40     
============================================
+ Hits           6387     6612     +225     
- Misses         5841     5974     +133     
- Partials        454      469      +15
Impacted Files Coverage Δ Complexity Δ
...ring/boot/ApolloApplicationContextInitializer.java 93.61% <ø> (ø) 15 <0> (ø) ⬇️
...m/ctrip/framework/apollo/biz/config/BizConfig.java 27.77% <0%> (-3.48%) 8 <0> (ø)
...trip/framework/apollo/common/dto/AccessKeyDTO.java 0% <0%> (ø) 0 <0> (?)
...rk/foundation/internals/provider/NullProvider.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../apollo/internals/RemoteConfigLongPollService.java 80.12% <100%> (-0.13%) 28 <0> (ø)
.../configservice/ConfigServiceAutoConfiguration.java 93.33% <100%> (+1.66%) 5 <1> (+1) ⬆️
...ework/apollo/internals/RemoteConfigRepository.java 87.42% <100%> (+0.32%) 23 <0> (+1) ⬆️
...va/com/ctrip/framework/apollo/util/ConfigUtil.java 94.61% <100%> (+0.04%) 50 <1> (+1) ⬆️
.../ctrip/framework/apollo/util/http/HttpRequest.java 86.66% <100%> (+3.33%) 7 <2> (+2) ⬆️
...o/adminservice/controller/AccessKeyController.java 13.63% <13.63%> (ø) 1 <1> (?)
... and 24 more

Continue to review full report at Codecov.

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

@nisiyong
Copy link
Member Author

nisiyong commented Dec 31, 2019

@nobodyiam

I have already solved all the problems you mentioned. If there are other problems, I will be happy to solve them.🙂

Happy New Year! 🎉

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants