Skip to content

Commit

Permalink
Fix creation of superfluous PanDomainAuthSettingsRefresher instances
Browse files Browse the repository at this point in the history
Having `def panDomainSettings: PanDomainAuthSettingsRefresher` in the `AuthActions`
trait as a `def` means that implementors of `AuthActions` are able to make the
mistake of also defining their implementation of that field as a `def`, meaning that
every reference to it can potentially create _another_ instance of the refresher,
as seen here:

guardian/atom-workshop#361 (comment)

Surprisingly, this seems to have been a problem since at least as far back as
#41 in February 2018.

Changing the field type to `val` forces implementers to also use `val` for that
field, effectively making it a singleton, as we want.

Changing the abstract field of a trait to be `val` does open up another danger due
the initialization order of vals - the field could end up being evaluated as
`null` if the trait immediately evaluates the field:

See: https://docs.scala-lang.org/tutorials/FAQ/initialization-order.html

...consequently, I've made all `val` declarations in the `AuthActions` trait (that
evaluate `panDomainSettings` in some way) into `lazy val`s. This hopefully should
fix the problem.
  • Loading branch information
rtyley committed Sep 12, 2024
1 parent a6ae5e8 commit 6e11a68
Showing 1 changed file with 6 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ trait AuthActions {
*/
def wsClient: WSClient
def controllerComponents: ControllerComponents
def panDomainSettings: PanDomainAuthSettingsRefresher
val panDomainSettings: PanDomainAuthSettingsRefresher

private def system: String = panDomainSettings.system
private def domain: String = panDomainSettings.domain
private lazy val system: String = panDomainSettings.system
private lazy val domain: String = panDomainSettings.domain

This comment has been minimized.

Copy link
@rtyley

rtyley Sep 12, 2024

Author Member

Although system & domain could have remained as defs, the truth is that they are constant val members of PanDomainAuthSettingsRefresher, so they can not change - consequently, they might as well be lazy vals (but they must be lazy, as otherwise that would immediately evaluate panDomainSettings at construction time, which we want to avoid).

private def settings: PanDomainAuthSettings = panDomainSettings.settings

private implicit val ec: ExecutionContext = controllerComponents.executionContext
Expand Down Expand Up @@ -81,14 +81,14 @@ trait AuthActions {
*/
def authCallbackUrl: String

val OAuth = new OAuth(settings.oAuthSettings, system, authCallbackUrl)(ec, wsClient)
lazy val OAuth = new OAuth(settings.oAuthSettings, system, authCallbackUrl)(ec, wsClient)

/**
* Application name used for initialising Google API clients for directory group checking
*/
val applicationName: String = s"pan-domain-authentication-$system"
lazy val applicationName: String = s"pan-domain-authentication-$system"

This comment has been minimized.

Copy link
@rtyley

rtyley Sep 12, 2024

Author Member

The fields OAuth and applicationName are both made lazy because they both reference system - and to evaluate system, they need to evaluate panDomainSettings, which we want to delay until necessary, due to abstract vals in traits evaluating to null during the construction.


val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map {
lazy val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map {

This comment has been minimized.

Copy link
@rtyley

rtyley Sep 12, 2024

Author Member

The field multifactorChecker is made lazy because it references panDomainSettings and there is again the risk that it will be null if evaluated at the point of construction.

new Google2FAGroupChecker(_, panDomainSettings.s3BucketLoader, applicationName)
}

Expand Down

0 comments on commit 6e11a68

Please sign in to comment.