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

Add MyRequest #118

Open
wants to merge 6 commits into
base: master
from
Open

Add MyRequest #118

wants to merge 6 commits into from

Conversation

@wsargent
Copy link
Contributor

wsargent commented Dec 30, 2019

This is a sample PR to show extending a custom request type on top of SecuredRequest.

Note that since MyRequest extends MessagesRequest it is automatically a MessagesProvider so that's one fewer implicit to pass around

@wsargent wsargent force-pushed the wsargent:use-my-request branch from ce7d651 to bd78888 Jan 15, 2020
@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Jan 15, 2020

@akkie I think this is a good practice because people will want to customize the request, also see the XXX bits for where traits would be used.

@wsargent

This comment has been minimized.

@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jan 15, 2020

@wsargent What's the advantage over using the action refiner? For me it looks like a lot of boilerplate. Could you assemble a really small example that can then be used in the documentation?

@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Jan 15, 2020

I did some refactoring and switched to ActionTransformer. Does this work?

abstract class SomeAbstractController @Inject() (controllerComponents: SilhouetteControllerComponents) {
  type SecuredEnvRequest[A] = SecuredRequest[DefaultEnv, A]
  
  protected abstract class AbstractActionTransformer[-R[_], +P[_]](cc: SilhouetteControllerComponents) extends ActionTransformer[R, P] {
    override protected def executionContext: ExecutionContext =
      cc.executionContext
  }

  class MySecuredActionTransformer(cc: SilhouetteControllerComponents) extends AbstractActionTransformer[SecuredEnvRequest, MySecuredRequest](cc) {
    override protected def transform[A](request: SecuredEnvRequest[A]): Future[MySecuredRequest[A]] = {
      Future.successful( new MySecuredRequest[A](
        messagesApi = cc.messagesApi,
        identity = request.identity,
        authenticator = request.authenticator,
        request = request
      ))
    }
  }

  private val mySecuredActionTransformer = new MySecuredActionTransformer(controllerComponents)

  def SecuredAction: ActionBuilder[MySecuredRequest, AnyContent] = {
    controllerComponents.silhouette.SecuredAction.andThen(mySecuredActionTransformer)
  }
}
@wsargent wsargent force-pushed the wsargent:use-my-request branch from bd78888 to cef7458 Jan 15, 2020
@wsargent wsargent force-pushed the wsargent:use-my-request branch from 07e4d94 to 2a25d60 Jan 15, 2020
@will-sargent-eero

This comment has been minimized.

Copy link

will-sargent-eero commented Jan 15, 2020

Probably should split up into IdentityProvider and AuthenticatorProvider traits really

@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Jan 16, 2020

Okay, broken out some more.

@wsargent wsargent force-pushed the wsargent:use-my-request branch from 69137ee to 3b1633f Jan 16, 2020
@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jan 16, 2020

@wsargent Thanks 👍 It looks much cleaner and more understandable now.

I've some notes to your implementation. Some are global some I will comment inline.

  • You have hardcoded the type of the environment. There are a lot of users which are use different environments. Maybe the environment could be a type parameter of the SilhouetteComponents trait. This makes it easier to switch the environment without rewriting a lot of code.

  • Could you please move the auth related traits to the utils.auth package?

  • I don't like the name MyRequest. MAybe we find an mane that is more descriptive? Maybe AppRequest?

@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Jan 17, 2020

@akkie updated with comment fixes

@will-sargent-eero

This comment has been minimized.

Copy link

will-sargent-eero commented Jan 17, 2020

@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Jan 27, 2020

@akkie how does it look?

@akkie

This comment has been minimized.

Copy link
Member

akkie commented Jan 27, 2020

I thought we should wait for the next Silhouette release, so that you can depend on mohiva/play-silhouette#574. But the next Silhouette release depends on the new Play 2.8.1 release because of mohiva/play-silhouette#570

@will-sargent-eero

This comment has been minimized.

Copy link

will-sargent-eero commented Jan 27, 2020

@wsargent

This comment has been minimized.

Copy link
Contributor Author

wsargent commented Feb 3, 2020

@wsargent wsargent mentioned this pull request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.