play-silhouette-seed icon indicating copy to clipboard operation
play-silhouette-seed copied to clipboard

Add MyRequest

Open wsargent opened this issue 5 years ago • 14 comments

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 avatar Dec 30 '19 03:12 wsargent

@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 avatar Jan 15 '20 03:01 wsargent

Also see https://discourse.silhouette.rocks/t/extending-securedrequest-with-actionrefiner/167

wsargent avatar Jan 15 '20 04:01 wsargent

@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?

akkie avatar Jan 15 '20 06:01 akkie

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 avatar Jan 15 '20 17:01 wsargent

Probably should split up into IdentityProvider and AuthenticatorProvider traits really

will-sargent-eero avatar Jan 15 '20 18:01 will-sargent-eero

Okay, broken out some more.

wsargent avatar Jan 16 '20 05:01 wsargent

@wsargent Thanks :+1: 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?

akkie avatar Jan 16 '20 16:01 akkie

@akkie updated with comment fixes

wsargent avatar Jan 17 '20 04:01 wsargent

The type alias can be used by subclasses. The parametrized type can’t.

On Thu, Jan 16, 2020 at 11:07 PM Christian Kaps [email protected] wrote:

@akkie commented on this pull request.

In app/controllers/SilhouetteController.scala https://github.com/mohiva/play-silhouette-seed/pull/118#discussion_r367794773 :

def eventBus: EventBus = silhouette.env.eventBus }

-trait SilhouetteComponents { +trait SilhouetteComponents[E <: Env] {

  • type EnvType = E

Why not using E directly?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mohiva/play-silhouette-seed/pull/118?email_source=notifications&email_token=AMUFROUVASV55WUOGKOWEUTQ6FKMXA5CNFSM4KBHG472YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSDQWQY#pullrequestreview-344394563, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMUFROREMCSSXFB35AOMKVLQ6FKMXANCNFSM4KBHG47Q .

will-sargent-eero avatar Jan 17 '20 07:01 will-sargent-eero

@akkie how does it look?

wsargent avatar Jan 27 '20 02:01 wsargent

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

akkie avatar Jan 27 '20 07:01 akkie

I’m fine with rebasing and merging any conflicts

On Sun, Jan 26, 2020 at 11:15 PM Christian Kaps [email protected] wrote:

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

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mohiva/play-silhouette-seed/pull/118?email_source=notifications&email_token=AMUFROR6OU6OCOT5Q4RKFC3Q72CY7A5CNFSM4KBHG472YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ6QQLI#issuecomment-578619437, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMUFROXHMED6RARB6RMRZX3Q72CY7ANCNFSM4KBHG47Q .

will-sargent-eero avatar Jan 27 '20 18:01 will-sargent-eero

@akkie 2.8.1 is out now https://github.com/playframework/playframework/releases/tag/2.8.1

wsargent avatar Feb 03 '20 17:02 wsargent

@wsargent I've released the final 7.0.0

akkie avatar Feb 27 '20 16:02 akkie