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

Clarify Username/Password Authentication Docs #15806

Merged
merged 2 commits into from
Sep 30, 2024

Conversation

nimakarimiank
Copy link
Contributor

@nimakarimiank nimakarimiank commented Sep 13, 2024

FormLogin docs change

UsernamePasswordAuthenticationFilter Definition raises confusion

as written in the docs : "When the username and password are submitted, the UsernamePasswordAuthenticationFilter authenticates the username and password.", It states that UsernamePasswordAuthenticationFilter is responsible for Authenticating users and populating the SecurityContext. instead as stated later we read : "UsernamePasswordAuthenticationFilter creates a UsernamePasswordAuthenticationToken, which is a type of Authentication, by extracting the username and password from the HttpServletRequest instance."
I think we should change the docs so it wouldn't raise any confusions.

Wrong Class reference

SecurityContextPersistenceRepository is not a recognized class, it's a typo for SecurityContextRepository

@pivotal-cla
Copy link

@nimakarimiank Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@nimakarimiank Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 13, 2024
@nimakarimiank nimakarimiank changed the title Docs -> Servlet Applications Authentication Username/Password Reading Username/Password Form -> Form Login Docs -> Servlet Applications Authentication -> Form Login & Persistence Sep 13, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Sep 16, 2024

Thanks, @nimakarimiank! Will you please update the PR so each commit atomically describes what is being changed? For example instead of docs modification, you could do:

Fix SecurityContextPersistenceRepository Typo

and

Clarify `UsernamePasswordAuthenticationFilter` Workflow

etc.

Please also squash the two commits that are for the same line in the documentation.

@jzheaux jzheaux self-assigned this Sep 16, 2024
@jzheaux jzheaux added in: docs An issue in Documentation or samples type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 16, 2024
@nimakarimiank
Copy link
Contributor Author

hey @jzheaux,
please take a look and let me know if it's ok.
Thanks!

@jzheaux
Copy link
Contributor

jzheaux commented Sep 23, 2024

Thanks for the updates, @nimakarimiank. Will you please take a look at the commits again? Now there are 7, and we want to have 3. It may help for you to rebase with main.

@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Sep 24, 2024
@nimakarimiank
Copy link
Contributor Author

@jzheaux This should fix the commit clarity issue
we have 2 commits, 1 for each change that I proposed to the docs.
1- Clarify UsernamePasswordAuthenticationFilter Workflow

for Form.adoc

2- Fix SecurityContextPersistenceRepository Typo

for Persistence.adoc

Does it look good to you ?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 26, 2024
@jzheaux jzheaux merged commit 8a5a603 into spring-projects:main Sep 30, 2024
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Sep 30, 2024

Thanks, @nimakarimiank! I went ahead and made the final adjustment to rebase with Spring Security's main branch.

In the future, I recommend creating a branch and pushing from there instead:

git checkout -b my-fixes
// add the commits you want
git push origin my-fixes

Then your PR is from my-fixes to main. This will help your main stay in line with Spring Security's main and also make it simpler to rebase a PR when that's needed.

This is now merged into main, nice job!

@jzheaux jzheaux added this to the 6.4.0-RC1 milestone Sep 30, 2024
@jzheaux jzheaux removed the status: feedback-provided Feedback has been provided label Sep 30, 2024
@jzheaux jzheaux changed the title Docs -> Servlet Applications Authentication -> Form Login & Persistence Clarify Username/Password Authentication Docs Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants