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

Refactor CryptoService for simplifying RSA key generation #5978

Conversation

JohnNiang
Copy link
Member

What type of PR is this?

/kind cleanup
/area core
/milestone 2.16.x

What this PR does / why we need it:

This PR removes PatJwkSupplier interface, scheduled RSA key generation, and move some of them into CryptoService.

Currently, we only use pat_id_rsa as private key for authentication modules instead of id_rsa(deprecated).

Does this PR introduce a user-facing change?

None

@f2c-ci-robot f2c-ci-robot bot added the release-note-none Denotes a PR that doesn't merit a release note. label May 23, 2024
@f2c-ci-robot f2c-ci-robot bot added this to the 2.16.x milestone May 23, 2024
@f2c-ci-robot f2c-ci-robot bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 23, 2024
@f2c-ci-robot f2c-ci-robot bot requested review from guqing and wan92hen May 23, 2024 12:52
@f2c-ci-robot f2c-ci-robot bot added the area/core Issues or PRs related to the Halo Core label May 23, 2024
Copy link

codecov bot commented May 23, 2024

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 56.68%. Comparing base (5fdf6c0) to head (37094a0).
Report is 196 commits behind head on main.

Files Patch % Lines
...pp/security/authentication/impl/RsaKeyService.java 83.33% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5978      +/-   ##
============================================
- Coverage     56.91%   56.68%   -0.24%     
- Complexity     3319     3474     +155     
============================================
  Files           587      609      +22     
  Lines         18968    20476    +1508     
  Branches       1401     1418      +17     
============================================
+ Hits          10795    11606     +811     
- Misses         7594     8292     +698     
+ Partials        579      578       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@guqing guqing left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 89.23077% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 56.68%. Comparing base (5fdf6c0) to head (42ca0a3).
Report is 198 commits behind head on main.

Files Patch % Lines
...pp/security/authentication/impl/RsaKeyService.java 88.33% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5978      +/-   ##
============================================
- Coverage     56.91%   56.68%   -0.23%     
- Complexity     3319     3475     +156     
============================================
  Files           587      609      +22     
  Lines         18968    20478    +1510     
  Branches       1401     1418      +17     
============================================
+ Hits          10795    11608     +813     
- Misses         7594     8292     +698     
+ Partials        579      578       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JohnNiang JohnNiang force-pushed the refactor/disable-scheduled-rsakey-generator branch from 37094a0 to 923c007 Compare May 24, 2024 03:21
@f2c-ci-robot f2c-ci-robot bot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2024
@JohnNiang JohnNiang force-pushed the refactor/disable-scheduled-rsakey-generator branch from 923c007 to 42ca0a3 Compare May 24, 2024 04:19
Copy link

sonarcloud bot commented May 24, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.3% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@guqing guqing left a comment

Choose a reason for hiding this comment

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

/lgtm

@f2c-ci-robot f2c-ci-robot bot added the lgtm Indicates that a PR is ready to be merged. label May 24, 2024
Copy link

f2c-ci-robot bot commented May 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guqing

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot bot merged commit de85156 into halo-dev:main May 24, 2024
9 checks passed
@JohnNiang JohnNiang deleted the refactor/disable-scheduled-rsakey-generator branch May 24, 2024 04:38
@JohnNiang JohnNiang modified the milestones: 2.16.x, 2.16.0 May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/core Issues or PRs related to the Halo Core kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants