Skip to content

Deprecate OpenID 2.0 support #8450

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
Merged
merged 4 commits into from
May 12, 2020
Merged

Deprecate OpenID 2.0 support #8450

merged 4 commits into from
May 12, 2020

Conversation

dadikovi
Copy link
Contributor

@dadikovi dadikovi commented May 1, 2020

This commit puts deprecation notice on docs, sample applications and configurations (java and xml).

Fixes gh-7153

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 1, 2020
rwinch
rwinch requested changes May 1, 2020
View reviewed changes
Copy link
Member

@rwinch rwinch 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 PR @dadikovi! Could you please update it to also point to the respective OIDC related Spring Security code? In many cases the respective class won't be on the classpath, so you will likely need to use code rather than link to it.

In respect to #7153 (comment)

  • Let's combine this as a single PR with multiple commits. Please preserve the original author's commit.
  • Adding the deprecation is fine on the XML documentation. We should ensure that all the parsers related to the configuration are deprecated as well (this will trigger tooling to give deprecations)

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue in: openid An issue in spring-security-openid type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels May 1, 2020
@rwinch rwinch mentioned this pull request May 1, 2020
Closed
@dadikovi
Deprecate openID 2.0 support
339d44b 
This commit puts deprecation notice on docs, sample applications and configurations (java and xml)

Fixes spring-projectsgh-7153
ThomasVitale and others added 2 commits May 2, 2020 10:22
@ThomasVitale @dadikovi
Deprecate OpenID 2.0 Support
84f45e4 
Add deprecation notice to all files in the spring-security-openid module

Fixes spring-projectsgh-7263
@dadikovi
Deprecate openID 2.0 support
2d9a6ac 
This commit adds link to spring code on the top of ThomasVitale's changes.

Fixes spring-projectsgh-7153
@dadikovi
Copy link
Contributor Author

dadikovi commented May 2, 2020

Thanks for the feedback @rwinch!

  • Modified the deprecation note, so it points to spring-security-oauth2 now, which contains the OIDC support.
  • Cherry-picked ThomasVitale's changes, and then in a separate commit added the oauth2 note to them as well.

We should ensure that all the parsers related to the configuration are deprecated as well (this will trigger tooling to give deprecations)

I couldn't understand this part. I should modify the rnc files?

@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 May 2, 2020
rwinch
rwinch requested changes May 5, 2020
View reviewed changes
Copy link
Member

@rwinch rwinch 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 fast update. I responded to the FIXME comment inline.

You are right that only the rnc files need modified directly. The .xsd files are generated using ./gradlew rncToXsd What I was referring to is that the code that parses the OpenID attributes should include deprecation notifications. We should extract the code that parses openid xml into a separate method that is deprecated (only invoke the method if the openid xml is available). Tooling will notice if that code is entered and emit deprecation notifications.

docs/manual/src/docs/asciidoc/_includes/servlet/authentication/index.adoc Outdated
@@ -33,6 +33,7 @@ This also gives a good idea of the high level flow of authentication and how pie
* <<servlet-rememberme, Remember Me>> - How to remember a user past session expiration
* <<servlet-jaas, JAAS Authentication>> - Authenticate with JAAS
* <<servlet-openid,OpenID>> - OpenID Authentication (not to be confused with OpenID Connect)
// FIXME: The one above is deprecated. Should it be removed from here as well?
Copy link
Member

Choose a reason for hiding this comment

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

We do not need to remove it here. We still want users to find it and figure out it has been deprecated.

Suggested change
// FIXME: The one above is deprecated. Should it be removed from here as well?

@dadikovi
Deprecate openID 2.0 support
f2a2b46 
This commit adds deprecation notice to xml schema, parser of the schema and removes fixme comments.

Fixes spring-projectsgh-7153
@dadikovi
Copy link
Contributor Author

dadikovi commented May 9, 2020

Thanks @rwinch for your response, I sent another commit with the requested changes:

  • modified rnc file, then generated the xsd-s, so my earlier manual modifications could be overwritten.
  • removed FIXME comment
  • extracted parser method which is called only if there is related tag in the xml configuration, and which has deprecation notice.

@rwinch rwinch changed the title Deprecate openID 2.0 support Deprecate OpenID 2.0 support May 12, 2020
@rwinch rwinch self-assigned this May 12, 2020
@rwinch rwinch added this to the 5.4.0-M2 milestone May 12, 2020
@rwinch rwinch merged commit e5d2aaf into spring-projects:master May 12, 2020
@rwinch
Copy link
Member

rwinch commented May 12, 2020

Thanks for the updated Pull Request! This is now merged into master 😄

@ThomasVitale ThomasVitale mentioned this pull request Jul 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwinch rwinch rwinch requested changes

Assignees

@rwinch rwinch

Labels
in: openid An issue in spring-security-openid status: feedback-provided Feedback has been provided type: task A general task
Projects
None yet
Milestone
5.4.0-M2
Development

Successfully merging this pull request may close these issues.

Deprecate OpenID 2.0 Support
4 participants
@dadikovi @rwinch @spring-projects-issues @ThomasVitale