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.

Sign up for GitHub

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

Jump to bottom

Support A Well-Known URL for Changing Passwords #8688

Merged
merged 1 commit into from Jul 1, 2021
Merged

Support A Well-Known URL for Changing Passwords #8688

merged 1 commit into from Jul 1, 2021

Conversation

evgeniycheban
Copy link
Contributor

@evgeniycheban evgeniycheban commented Jun 12, 2020 •
edited

Closes gh-8657

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 12, 2020
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 2 times, most recently from d913059 to 2933f45 Compare June 13, 2020 01:07
@evgeniycheban evgeniycheban marked this pull request as draft June 13, 2020 01:31
@evgeniycheban evgeniycheban marked this pull request as ready for review June 13, 2020 10:50
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 2 times, most recently from d95c351 to 4d7a1b6 Compare June 14, 2020 15:35
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 15, 2020
jzheaux
jzheaux requested changes Jun 16, 2020
View reviewed changes
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, @evgeniycheban! A special thanks for thinking about board support for the feature.

I'd like to focus on the Servlet DSL first, so I've only got the one comment thus far. Once we are aligned on that, I think the others will follow.

Also, with features like these, it's nice to have a sample to play with -- would you be able to add a minimal Spring Security sample application as part of the PR? It would go into /samples/boot/. The change password page could be an MVC endpoint that returns a message like "Change Password Page".

.../src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java Outdated Show resolved Hide resolved
@jzheaux jzheaux assigned jzheaux and unassigned rwinch Jun 16, 2020
@jzheaux jzheaux added the status: duplicate A duplicate of another issue label Jun 16, 2020
@jzheaux jzheaux added this to the 5.4.0-M2 milestone Jun 16, 2020
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 2 times, most recently from 361d5ed to 430cf01 Compare June 20, 2020 20:10
jzheaux
jzheaux reviewed Jun 22, 2020
View reviewed changes
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, @evgeniycheban! I've some additional feedback inline.

.../main/java/org/springframework/security/config/annotation/web/builders/FilterComparator.java Outdated Show resolved Hide resolved
...n/java/org/springframework/security/web/wellknown/WellKnownChangePasswordRedirectFilter.java Outdated Show resolved Hide resolved
...n/java/org/springframework/security/web/wellknown/WellKnownChangePasswordRedirectFilter.java Outdated Show resolved Hide resolved
...n/java/org/springframework/security/web/wellknown/WellKnownChangePasswordRedirectFilter.java Outdated Show resolved Hide resolved
web/src/main/java/org/springframework/security/web/wellknown/package-info.java Outdated Show resolved Hide resolved
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 2 times, most recently from 19daf8b to 255e41a Compare June 23, 2020 20:13
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 2 times, most recently from 91d4bf2 to 74a02c2 Compare June 23, 2020 22:55
@jzheaux jzheaux modified the milestones: 5.4.0-M2, 5.4.0-RC1 Jul 1, 2020
@evgeniycheban
Copy link
Contributor Author

evgeniycheban commented Jul 14, 2020 •
edited

@jzheaux Did you see the latest changes?
Is there anything that I could improve on this PR?

jzheaux
jzheaux requested changes Jul 15, 2020
View reviewed changes
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 your patience, @evgeniycheban - I've left one additional piece of feedback inline.

...in/java/org/springframework/security/web/password/WellKnownChangePasswordRedirectFilter.java Outdated Show resolved Hide resolved
jzheaux
jzheaux requested changes Jul 21, 2020
View reviewed changes
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.

@evgeniycheban I've left just one more feedback comment. I believe we're ready to go after that.

.../src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java Outdated Show resolved Hide resolved
@jzheaux
Copy link
Contributor

jzheaux commented Jul 27, 2020

@evgeniycheban, thanks again for the PR.

Fyrd/caniuse#5485 does a good job of summarizing where adoption of this spec is at currently. At the time of my post here, it shows that browser support is still in the early stages

It would be nice to have a little more adoption before committing to supporting it. Even nicer would be to see the specification finalized. Also, once multiple browsers are using it in their password management tools, then it will be much easier to confirm that the feature works with those browsers.

It's also quite simple for an app to do this themselves while they wait for this to be merged.

So, I'd say let's wait on merging until the spec has matured. It'll be much easier to make changes to the PR than an already-GA'd feature.

@jgrandja jgrandja modified the milestones: 5.4.0-RC1, 5.4.0 Aug 5, 2020
@jzheaux jzheaux modified the milestones: 5.4.0, 5.5.x Sep 8, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Oct 6, 2020

This appears to have recently been implemented by Chrome in https://bugs.chromium.org/p/chromium/issues/detail?id=927473. Once that makes it out to Chrome's stable channel, the PR can be tested against more than one browser to confirm the feature works as expected.

@jzheaux jzheaux modified the milestones: 5.5.x, 5.5.0-RC1 Mar 12, 2021
@rwinch rwinch modified the milestones: 5.5.0-RC1, 5.6.0-M1 Apr 12, 2021
@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 3 times, most recently from 8c4d8bc to c9a3602 Compare June 27, 2021 00:38
@evgeniycheban
Copy link
Contributor Author

@jzheaux I tested this feature in Chrome 91, it works as expected.

@evgeniycheban evgeniycheban force-pushed the gh-8657 branch 5 times, most recently from 41cb208 to 5030fb2 Compare June 27, 2021 03:23
@jzheaux jzheaux merged commit d121ab9 into spring-projects:main Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux Awaiting requested review from jzheaux

Assignees

@jzheaux jzheaux

Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.6.0-M1
Development

Successfully merging this pull request may close these issues.

Support A Well-Known URL for Changing Passwords
5 participants
@evgeniycheban @jzheaux @rwinch @jgrandja @spring-projects-issues