Skip to content

Make Saml2Authentication serializable #7683

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
eleftherias merged 1 commit into from
Dec 12, 2019
Merged

Make Saml2Authentication serializable #7683

eleftherias merged 1 commit into from
Dec 12, 2019

Conversation

@clemstoquart
Copy link
Contributor

@clemstoquart clemstoquart commented Nov 28, 2019

Fix #7681

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 28, 2019
eleftherias
eleftherias suggested changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias 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 PR @clemstoquart! I left some comments inline.

@eleftherias eleftherias self-assigned this Nov 29, 2019
@clemstoquart clemstoquart force-pushed the fix-saml2authentication-serializable branch from 77e68bf to 7a7cdae Compare November 29, 2019 12:16
@clemstoquart
Copy link
Contributor Author

clemstoquart commented Nov 29, 2019

Some integration tests are failing on my computer.

eleftherias
eleftherias suggested changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks @clemstoquart, I noticed a few more things and I've left comments inline.

@eleftherias
Copy link
Contributor

eleftherias commented Nov 29, 2019

Some integration tests are failing on my computer.

Are you using Java 11+? If so the LDAP integrations tests will fail.
You can either switch to Java 8 or run the command ./gradlew clean test --refresh-dependencies --no-daemon --stacktrace instead.

@clemstoquart clemstoquart force-pushed the fix-saml2authentication-serializable branch from 7a7cdae to b101ab6 Compare November 29, 2019 13:28
@clemstoquart
Copy link
Contributor Author

clemstoquart commented Nov 29, 2019

Some integration tests are failing on my computer.

Are you using Java 11+? If so the LDAP integrations tests will fail.
You can either switch to Java 8 or run the command ./gradlew clean test --refresh-dependencies --no-daemon --stacktrace instead.

I'm using Java 8. It was because of the return null getter.

@eleftherias eleftherias added in: saml2 An issue in SAML2 modules and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 2, 2019
@fhanik
Copy link
Contributor

fhanik commented Dec 2, 2019

Could the Saml2AuthenticatedPrincipal be an interface and have a basic implementation (package private)?

There are features that would benefit from this later, such as #7465

@fhanik fhanik mentioned this pull request Dec 2, 2019
Closed
@clemstoquart clemstoquart force-pushed the fix-saml2authentication-serializable branch from b101ab6 to 462f183 Compare December 3, 2019 09:10
@clemstoquart
Copy link
Contributor Author

clemstoquart commented Dec 3, 2019

@fhanik Sure. It's done.

eleftherias
eleftherias suggested changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@eleftherias eleftherias 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 all the work you've been doing @clemstoquart!
I have left a few small comments.

@clemstoquart clemstoquart force-pushed the fix-saml2authentication-serializable branch from 462f183 to d152c80 Compare December 12, 2019 14:38
@eleftherias eleftherias merged commit 31b999e into spring-projects:master Dec 12, 2019
@eleftherias
Copy link
Contributor

eleftherias commented Dec 12, 2019

Thanks for the PR @clemstoquart! This is now merged into master.

@eleftherias eleftherias added the type: bug A general bug label Dec 12, 2019
@eleftherias eleftherias changed the title fix: make Saml2Authentication serializable Make Saml2Authentication serializable Dec 12, 2019
@clemstoquart
Copy link
Contributor Author

clemstoquart commented Dec 12, 2019

@eleftherias Thanks for the reviews. Have a nice day :)

@clemstoquart clemstoquart deleted the fix-saml2authentication-serializable branch December 12, 2019 18:22
@eleftherias eleftherias modified the milestones: 5.2.2, 5.3.0.M1 Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@eleftherias eleftherias eleftherias requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@eleftherias eleftherias

Labels

in: saml2 An issue in SAML2 modules status: duplicate A duplicate of another issue type: bug A general bug

Projects

None yet

Milestone

5.3.0.M1

Development

Successfully merging this pull request may close these issues.

Saml2Authentication isn't serializable

4 participants

@clemstoquart @eleftherias @fhanik @spring-projects-issues