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

Update Documentation to point to spring-security-samples #9826

Merged
merged 4 commits into from
Jun 21, 2021

Conversation

marcusdacoregio
Copy link
Contributor

@marcusdacoregio marcusdacoregio commented May 27, 2021

Update the docs to point to the new Spring Security Samples project.
Fix some broken links not related to the samples as well.
Add a link checker that can be used by running the htmlSanityCheck gradle task

Closes gh-9815, gh-9816, gh-9817, gh-9818, gh-9869

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 27, 2021
@marcusdacoregio marcusdacoregio force-pushed the gh-9784 branch 9 times, most recently from 21ae8d3 to 60f2da4 Compare May 31, 2021 14:12
@jgrandja jgrandja requested a review from jzheaux May 31, 2021 18:45
@marcusdacoregio marcusdacoregio force-pushed the gh-9784 branch 2 times, most recently from e6e7d20 to 9d0aa38 Compare June 4, 2021 11:49
@marcusdacoregio marcusdacoregio marked this pull request as ready for review June 4, 2021 12:16
@marcusdacoregio marcusdacoregio added in: docs An issue in Documentation or samples status: duplicate A duplicate of another issue type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 4, 2021
@marcusdacoregio marcusdacoregio force-pushed the gh-9784 branch 2 times, most recently from 2362d96 to 1412a2e Compare June 4, 2021 19:54
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @marcusdacoregio! I've left some initial feedback inline.

docs/guides/spring-security-docs-guides.gradle Outdated Show resolved Hide resolved
docs/manual/spring-security-docs-manual.gradle Outdated Show resolved Hide resolved
docs/manual/src/docs/asciidoc/index.adoc Outdated Show resolved Hide resolved
@marcusdacoregio marcusdacoregio force-pushed the gh-9784 branch 5 times, most recently from 54cb855 to c07a0a3 Compare June 7, 2021 17:05
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @marcusdacoregio. I've noted a few more items inline. Please also double-check the list from the last review -- sometimes GitHub will collapse review items, making them easy to miss.

@@ -51,7 +51,7 @@ If you have more than one in your application context, you need to specify which

[[remember-me-persistent-token]]
=== Persistent Token Approach
This approach is based on the article http://jaspan.com/improved_persistent_login_cookie_best_practice[http://jaspan.com/improved_persistent_login_cookie_best_practice] with some minor modifications footnote:[Essentially, the username is not included in the cookie, to prevent exposing a valid login name unecessarily.
This approach is based on the article https://web.archive.org/web/20180819014446/http://jaspan.com/improved_persistent_login_cookie_best_practice[http://jaspan.com/improved_persistent_login_cookie_best_practice] with some minor modifications footnote:[Essentially, the username is not included in the cookie, to prevent exposing a valid login name unecessarily.
Copy link
Contributor Author

@marcusdacoregio marcusdacoregio Jun 15, 2021

Choose a reason for hiding this comment

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

This link should be updated in the PersistentTokenBasedRememberMeServices class' javadoc

Choose a reason for hiding this comment

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

It might be worth while to document the algorithm implemented by spring security in the docs, rather than pointing to web.archive.org as that link might disappear at some point in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asaikali This is a good suggestion, but I think in this PR we should stick to fixing the broken links. Could you please open an issue specifically to document it in the docs instead of point out to the web.archive.org domain?

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Looking good, @marcusdacoregio. I've left some additional feedback inline.

@marcusdacoregio
Copy link
Contributor Author

@jzheaux could you please review the PR? I've done some changes based on your comments. Thank you.

@marcusdacoregio marcusdacoregio merged commit bbf5614 into spring-projects:main Jun 21, 2021
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 status: duplicate A duplicate of another issue type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add messaging to documentation about sample migration
4 participants