Skip to content

Commit 026cde4

Browse files
authored
Fix that auth tokens would sometimes be overwritten (#8738)
Quoting from Silhouette’s BearerTokenAuthenticator docstring for `update`: ``` * Updates the authenticator with the new last used date in the backing store. * * We needn't embed the token in the response here because the token itself will not be changed. * Only the authenticator in the backing store will be changed. ``` Our implementation of `update` did something else. It fetched *any* authenticator of the same user and changed not only the last used date time but also the value. So if update was called by silhouette on a datastore token, it could happen that an Authentication token was changed. This bug has been in place ever since #2149 but to trigger it, a request has to be sent to `/api/` with a valid *datastore* token. We don’t usually do that, but the libs tests seem to do so now. That is also a libs bug. This also caused the webknossos-libs tests to get 401 responses when using X-Auth-Tokens that were lost in this way. ### Steps to test: - Requests with X-Auth-Token should work, token should stay fixed. - Normal datastore/tracingstore operation should also still work ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md)
1 parent 0c9bb64 commit 026cde4

File tree

3 files changed

+7
-24
lines changed

3 files changed

+7
-24
lines changed

app/security/BearerTokenAuthenticatorRepository.scala

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package security
33
import play.silhouette.api.LoginInfo
44
import play.silhouette.api.repositories.AuthenticatorRepository
55
import play.silhouette.impl.authenticators.BearerTokenAuthenticator
6-
import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext}
76
import com.scalableminds.util.time.Instant
87
import com.scalableminds.util.tools.Fox
98
import TokenType.TokenType
@@ -19,23 +18,15 @@ class BearerTokenAuthenticatorRepository(tokenDAO: TokenDAO)(implicit ec: Execut
1918
override def add(authenticator: BearerTokenAuthenticator): Future[BearerTokenAuthenticator] =
2019
add(authenticator, TokenType.Authentication)
2120

22-
override def update(newAuthenticator: BearerTokenAuthenticator): Future[BearerTokenAuthenticator] = {
23-
implicit val ctx: DBAccessContext = GlobalAccessContext
21+
override def update(newAuthenticator: BearerTokenAuthenticator): Future[BearerTokenAuthenticator] =
2422
(for {
25-
oldAuthenticatorSQL <- tokenDAO.findOneByLoginInfo(newAuthenticator.loginInfo.providerID,
26-
newAuthenticator.loginInfo.providerKey,
27-
TokenType.Authentication)
28-
_ <- tokenDAO.updateValues(
29-
oldAuthenticatorSQL._id,
23+
_ <- tokenDAO.updateLastUsedDateTime(
3024
newAuthenticator.id,
3125
Instant.fromZonedDateTime(newAuthenticator.lastUsedDateTime),
32-
Instant.fromZonedDateTime(newAuthenticator.expirationDateTime),
33-
newAuthenticator.idleTimeout
3426
)
3527
updated <- findOneByValue(newAuthenticator.id)
3628
} yield updated).toFutureOrThrowException(
3729
"Could not update Token. Throwing exception because update cannot return a box, as defined by Silhouette trait AuthenticatorDAO")
38-
}
3930

4031
override def remove(value: String): Future[Unit] =
4132
for {

app/security/Token.scala

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package security
22

33
import play.silhouette.api.LoginInfo
44
import play.silhouette.impl.authenticators.BearerTokenAuthenticator
5-
import com.scalableminds.util.accesscontext.DBAccessContext
65
import com.scalableminds.util.enumeration.ExtendedEnumeration
76
import com.scalableminds.util.time.Instant
87
import com.scalableminds.util.tools.Fox
@@ -116,20 +115,11 @@ class TokenDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContext)
116115
${t.tokenType}, ${t.created}, ${t.isDeleted})""".asUpdate)
117116
} yield ()
118117

119-
def updateValues(id: ObjectId,
120-
value: String,
121-
lastUsedDateTime: Instant,
122-
expirationDateTime: Instant,
123-
idleTimeout: Option[FiniteDuration])(implicit ctx: DBAccessContext): Fox[Unit] =
118+
def updateLastUsedDateTime(value: String, lastUsedDateTime: Instant): Fox[Unit] =
124119
for {
125-
_ <- assertUpdateAccess(id)
126120
_ <- run(q"""UPDATE webknossos.tokens
127-
SET
128-
value = $value,
129-
lastUsedDateTime = $lastUsedDateTime,
130-
expirationDateTime = $expirationDateTime,
131-
idleTimeout = ${idleTimeout.map(_.toMillis)}
132-
WHERE _id = $id""".asUpdate)
121+
SET lastUsedDateTime = $lastUsedDateTime
122+
WHERE value = $value""".asUpdate)
133123
} yield ()
134124

135125
def deleteOneByValue(value: String): Fox[Unit] = {

unreleased_changes/8738.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
### Fixed
2+
- Fixed a bug where authentication tokens would sometimes expire too soon.

0 commit comments

Comments
 (0)