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

Fix the problem of logging in successfully even if request not permitted #4101

Merged

Conversation

JohnNiang
Copy link
Member

What type of PR is this?

/kind bug
/area core

What this PR does / why we need it:

This is a bug introduced from #4062. I have overridden onAuthenticationSuccess to create rate limiter in advance instead of invoking securityContextRepository#save before.

See #4099 (comment) for more.

Special notes for your reviewer:

  1. Try to log in with incorrect password three times
  2. Log in with correct password and check if the response headers contain Set-Cookie

Does this PR introduce a user-facing change?

None

Signed-off-by: John Niang <johnniang@fastmail.com>
@f2c-ci-robot f2c-ci-robot bot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Jun 20, 2023
@f2c-ci-robot f2c-ci-robot bot requested review from lan-yonghui and LIlGG June 20, 2023 14:23
@f2c-ci-robot f2c-ci-robot bot added the area/core Issues or PRs related to the Halo Core label Jun 20, 2023
@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #4101 (c5ef019) into main (2fd9cbd) will increase coverage by 0.00%.
The diff coverage is 55.55%.

@@            Coverage Diff            @@
##               main    #4101   +/-   ##
=========================================
  Coverage     60.22%   60.23%           
  Complexity     2385     2385           
=========================================
  Files           357      357           
  Lines         12392    12397    +5     
  Branches        892      892           
=========================================
+ Hits           7463     7467    +4     
- Misses         4492     4493    +1     
  Partials        437      437           
Impacted Files Coverage Δ
...ntication/login/UsernamePasswordAuthenticator.java 37.89% <55.55%> (+2.33%) ⬆️

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.

nice work🎉

/approve

@f2c-ci-robot f2c-ci-robot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023
Copy link
Member

@ruibaby ruibaby 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 Jun 21, 2023
@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Jun 21, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guqing, ruibaby

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 a19f342 into halo-dev:main Jun 21, 2023
4 checks passed
@ruibaby ruibaby modified the milestones: 2.7.x, 2.7.0 Jun 21, 2023
@JohnNiang JohnNiang deleted the bug/ratelimiting-login-endpoint branch January 14, 2024 14:01
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/bug Categorizes issue or PR as related to a bug. 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.

None yet

3 participants