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

UsernamePasswordAuthenticationToken factory methods #10901

Closed
wants to merge 1 commit into from
Closed

UsernamePasswordAuthenticationToken factory methods #10901

wants to merge 1 commit into from

Conversation

nor-ek
Copy link
Contributor

@nor-ek nor-ek commented Feb 22, 2022

This PR does the following:

  • static factory methods for unauthenticated and authenticated token
  • test these methods

Let me know if commit messages, test names, or something else can be wrong.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 22, 2022
@sjohnr sjohnr added in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Feb 23, 2022
@nor-ek nor-ek mentioned this pull request Feb 24, 2022
Closed
jzheaux
jzheaux requested changes Feb 24, 2022
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, @nor-ek! I've left some feedback inline.

When you are ready, please also squash your commits so that there is just one commit in the PR.

...in/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java Show resolved Hide resolved
...in/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java Show resolved Hide resolved
@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 24, 2022
@nor-ek nor-ek force-pushed the gh-10790-clean branch 2 times, most recently from bee556e to 962d5ed Compare March 1, 2022 20:24
jzheaux
jzheaux requested changes Mar 1, 2022
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, @nor-ek! I've left some feedback inline.

It appears that some other commits got mixed in with your commit. I've made comments on the files that I noticed, but I'm not sure that I've caught them all. You might try rebasing with 5.7.x to remove them.

...ingframework/security/config/annotation/web/reactive/ReactiveOAuth2ClientImportSelector.java Outdated Show resolved Hide resolved
...ramework/security/config/annotation/web/reactive/ReactiveOAuth2ClientImportSelectorTest.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java Outdated Show resolved Hide resolved
...in/java/org/springframework/security/authentication/UsernamePasswordAuthenticationToken.java Outdated Show resolved Hide resolved
docs/modules/ROOT/pages/features/integrations/cryptography.adoc Outdated Show resolved Hide resolved
...g/springframework/security/saml2/provider/service/registration/RelyingPartyRegistration.java Outdated Show resolved Hide resolved
web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java Outdated Show resolved Hide resolved
jzheaux
jzheaux requested changes Mar 3, 2022
View reviewed changes
Copy link
Contributor

@jzheaux jzheaux left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the updates, @nor-ek! Will you please do one more maintenance for me?

Please change the copyright year for any files whose copyright looks like:

Copyright 2002-{year} the original author or authors

to 2022.

Additionally, I haven't seen #10901 (comment) addressed yet. Let me know if it's unclear. Basically, the class should look like this:

/**
  * ... java doc ...
  *
  * @author Ben Alex
  * @author Norbert Nowak
  **/
public class UsernamePasswordAuthenticationToken extends AbstractAuthenticationToken {
    // ...

    /**
      * ... java doc ...
      * 
      * @since 5.7
      **/
    public static UsernamePasswordAuthenticationToken unauthenticated(...) {
        ...
    }
}

Your @author tag goes at the class level. Your @since tags go on your new public methods.

@nor-ek
Copy link
Contributor Author

nor-ek commented Mar 6, 2022 •
edited
Loading

@jzheaux Yes, no problem. Should I only change 2002-{current} or also other years f.g 2006-{current}, 2009-{current}. I'm going to do this in separate commit ok?

Author tag should be corrected.

@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 Mar 6, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Mar 7, 2022

Hi, @nor-ek. Thanks for all the updating. Sorry, what I meant was, please change the copyright headers for the files that you modified in the PR. You can read more about it in the contributing guidelines.

Since they are only the files related to this PR, I think it's best to include them in the same commit as it makes forward-porting to main easier.

@nor-ek
Copy link
Contributor Author

nor-ek commented Mar 8, 2022 •
edited
Loading

Hi @jzheaux I understand now. Sorry, I thought that its separate task. Going to correct that ASAP.
Another question is how to deal with Copyright 2004, 2005, 2006 Acegi Technology Pty Limited ?

@nor-ek
Prepare UsernamePasswordAuthenticationTokenTests factory authenticate…
12fa741 
…d and unauthenticated methods

 - unauthenticated factory method
 - authenticated factory method
 - test for unauthenticated factory method
 - test for authenticated factory method
 - make existing constructor protected
 - use newly factory methods in rest of the project
 - update copyright dates

Issue spring-projectsgh-10799
@jzheaux
Copy link
Contributor

jzheaux commented Mar 9, 2022

@nor-ek, the copyrights originally assigned to Acegi we leave for now, thanks for asking.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 9, 2022

Thanks, @nor-ek. This is merged now into 5.7.x in ac9c29b. It's also been forward-ported to main.

@jzheaux jzheaux closed this Mar 9, 2022
@jzheaux jzheaux removed the status: feedback-provided Feedback has been provided label Mar 9, 2022
@jzheaux jzheaux added this to the 5.7.0-M3 milestone Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jzheaux jzheaux jzheaux requested changes

Assignees

@jzheaux jzheaux

Labels
in: core An issue in spring-security-core status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.7.0-M3
Development

Successfully merging this pull request may close these issues.
4 participants
@nor-ek @jzheaux @sjohnr @spring-projects-issues