Skip to content
This repository has been archived by the owner on Sep 12, 2021. It is now read-only.

Make SecuredRequest and UserAwareRequest traits #574

Merged
merged 3 commits into from
Jan 22, 2020

Conversation

wsargent
Copy link
Contributor

@wsargent wsargent commented Dec 28, 2019

Purpose

This PR changes SecuredRequest from a case class to a trait, and adds a SecuredRequestHeader trait as well.

This makes it easier to change the implementation of a secured request because a wrapped request may want to implement secured request behavior, i.e. the identity and authenticator, but cannot because there is not a trait for it.

Because there is a factory method which uses the default impl behind the scenes, this should have no impact on user facing code (although I have not tried binary compatibility, etc.)

Background Context

This is the approach used for extending Play requests internally, i.e. in https://github.com/playframework/playframework/blob/2.6.x/core/play/src/main/scala/play/api/mvc/MessagesRequest.scala

Note that the request body should be covariant per playframework/playframework#8240

@wsargent wsargent force-pushed the make-secured-request-trait branch from b05c067 to 19b55b8 Compare December 28, 2019 23:04
@wsargent wsargent force-pushed the make-secured-request-trait branch from 19b55b8 to a00e861 Compare December 28, 2019 23:07
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 95.83% when pulling a00e861 on wsargent:make-secured-request-trait into 12976ca on mohiva:master.

@coveralls
Copy link

coveralls commented Dec 28, 2019

Coverage Status

Coverage decreased (-0.06%) to 95.833% when pulling 37f60dd on wsargent:make-secured-request-trait into 12976ca on mohiva:master.

@akkie
Copy link
Contributor

akkie commented Dec 29, 2019

This makes it easier to change the implementation of a secured request because a wrapped request may want to implement secured request behavior, i.e. the identity and authenticator, but cannot because there is not a trait for it.

Do you have a concrete example for this? The SecuredRequest itself cannot be changed by the user, because it will only be instantiated in Silhouette. So I currently see no reason to make this a trait. Maybe I miss something!?

@wsargent
Copy link
Contributor Author

@akkie created mohiva/play-silhouette-seed#118 as a concrete example

@wsargent
Copy link
Contributor Author

wsargent commented Jan 4, 2020

So the issue is more about being able to expose SecuredRequest as a behavior, so that MyRequestHeader can extend both MessagesRequestHeader and SecuredRequestHeader and have Twirl fragments be able to pass in the specific behavior needed:

@()(implicit provider: MessagesProvider) 
@* MessagesRequestHeader is a MessagesProvider 
from https://www.playframework.com/documentation/2.7.x/ScalaForms#Option-Two:-Use-MessagesRequest 
*@

or for a username fragment (for example in the navbar):

@()(implicit request: SecuredRequestHeader) @* render the identity etc *@

Thinking about it a bit more, this could be broken down more into IdentityProvider and AuthenticatorProvider:

trait SecuredMessagesHeader extends IdentityProvider with AuthenticatorProvider

And then the twirl code wouldn't have to be tied directly to the request:

@()(implicit identityProvider: IdentityProvider) @* render the identity etc *@

@akkie
Copy link
Contributor

akkie commented Jan 19, 2020

I think the only thing that could probably break current code is the ability to pattern match against the current implementation. So I think this pull request needs an unapply method too in the companion object.

@wsargent
Copy link
Contributor Author

wsargent commented Jan 20, 2020

So I think this pull request needs an unapply method too in the companion object.

Sorry, I'm lost. What companion object are we talking about? Both SecuredRequest and UserAwareRequest?

@akkie
Copy link
Contributor

akkie commented Jan 20, 2020

Yes, I meant both companion objects. In the old implementation both request types were case classes, which a user could pattern match on. Currently this isn't possible. I'm not sure it's really a use case to pattern match on the request types. What do you think?

@wsargent
Copy link
Contributor Author

Can't hurt, I'll add it.

@wsargent
Copy link
Contributor Author

Done. I see a failure on higher kinded types, not sure about touching the CI build system though.

@akkie akkie merged commit 1d6defb into mohiva:master Jan 22, 2020
@akkie
Copy link
Contributor

akkie commented Jan 22, 2020

Thanks Will 👍

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

Successfully merging this pull request may close these issues.

3 participants