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

Migrate to Play 2.6 #614

Merged
merged 1 commit into from
Mar 17, 2018
Merged

Migrate to Play 2.6 #614

merged 1 commit into from
Mar 17, 2018

Conversation

gmethvin
Copy link
Contributor

@gmethvin gmethvin commented Mar 1, 2018

This attempts to update SecureSocial to work on Play 2.6, Scala 2.12 and sbt 1.

Fixes #611 #612

At the moment it's still a work in progress. It compiles, but I had to remove some tests and remove some deprecated code. It still requires global state because some of the "constants" use the configuration from the currently running application.

Feedback is appreciated.

@gmethvin gmethvin mentioned this pull request Mar 1, 2018
@jaliss
Copy link
Owner

jaliss commented Mar 1, 2018

@gmethvin thanks for much for this. I'm sorry I have not been able to keep up. I'll review the changes and see how we can fix the remaining issues to have a version compatible with 2.6.

@@ -58,13 +59,12 @@ object IdentityProvider {
// todo: do I want this here?
val sslEnabled: Boolean = {
import Play.current
Copy link
Contributor Author

@gmethvin gmethvin Mar 1, 2018

Choose a reason for hiding this comment

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

Most of what's left to do is to remove these usages of Play.current from the static objects.

This is also problematic because, being a val, it is not going to reload when you stop an app and start a new one with different configuration, which is something you'd often do in tests. If we changed it to a def we have to incur the initialization cost on each call, or have a cache keyed on the Application.

My suggestion would be to have traits/classes like IdentityProviderConfig that we pass around, similar to what we have in Play: https://github.com/playframework/playframework/blob/2.6.11/framework/src/play-filters-helpers/src/main/scala/play/filters/headers/SecurityHeadersFilter.scala#L75

I don't think it should affect end users that much because these constants are primarily used internally to configure SecureSocial.

@gmethvin gmethvin force-pushed the play26 branch 2 times, most recently from dca3bf5 to 7ecd5a9 Compare March 2, 2018 21:25
@gmethvin
Copy link
Contributor Author

gmethvin commented Mar 2, 2018

Updated to inject the configs

@@ -0,0 +1,81 @@
securesocial {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reference.conf documents and provides defaults for all our configuration

ws,
filters,
specs2 % "test",
"com.typesafe.play" %% "play-mailer" % "5.0.0",
"com.typesafe.play" %% "play-mailer" % "6.0.1",
"io.methvin.play" %% "autoconfig-macros" % "0.2.0" % "provided",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this macro library to generate configs from case classes.

@gmethvin gmethvin changed the title [WIP] migrate to Play 2.6 Migrate to Play 2.6 Mar 10, 2018
Copy link
Owner

@jaliss jaliss left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you @gmethvin

@jaliss jaliss merged commit f4c2189 into jaliss:master Mar 17, 2018
@normenmueller
Copy link
Contributor

@gmethvin I haven't looked at it yet, but I still ask: Is there an example project for SecureSocial in combination with Play 2.6?

@xfishernet
Copy link

How connect last secure social for Play 2.6?

libraryDependencies += "ws.securesocial" %% "securesocial" % "master"

does worked :-/ which version need write in build.sbt?

@JackHopkins
Copy link

Any news with a deployed sbt module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants