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 65e1f65 commit 61a193c
Showing 1 changed file with 6 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,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
private def settings: PanDomainAuthSettings = panDomainSettings.settings

private implicit val ec: ExecutionContext = controllerComponents.executionContext
Expand Down Expand Up @@ -82,14 +82,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"

val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map {
lazy val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map {
new Google2FAGroupChecker(_, panDomainSettings.s3BucketLoader, applicationName)
}

Expand Down

0 comments on commit 61a193c

Please sign in to comment.