Skip to content

Commit 37f90d8

Browse files
JamesKaneJamesKane
committed
**feat(security): Add login rate limiter and cleanup API curator handling**
- Introduced `LoginRateLimiter` to manage failed login attempts and block excessive retries from the same IP. - Replaced `curatorId` in multiple API controllers with a hardcoded `ApiCuratorId` for audit logging, improving consistency and security. - Enhanced input validation in `AuthController` login form with stricter constraints. - Integrated `LoginRateLimiter` to throttle login attempts and provide meaningful error messages for blocked requests.
1 parent c022cf2 commit 37f90d8

5 files changed

Lines changed: 120 additions & 77 deletions

File tree

‎app/controllers/AuthController.scala‎

Lines changed: 40 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import play.api.data.Form
77
import play.api.data.Forms.*
88
import play.api.i18n.I18nSupport
99
import play.api.mvc.{Action, AnyContent, BaseController, ControllerComponents}
10-
import services.AuthService
10+
import services.{AuthService, LoginRateLimiter}
1111

1212
import scala.concurrent.{ExecutionContext, Future}
1313

@@ -17,13 +17,14 @@ case class LoginData(handle: String, appPassword: String)
1717
class AuthController @Inject()(
1818
val controllerComponents: ControllerComponents,
1919
authService: AuthService,
20-
userRoleRepository: repositories.UserRoleRepository // Injected
20+
userRoleRepository: repositories.UserRoleRepository,
21+
rateLimiter: LoginRateLimiter
2122
)(implicit ec: ExecutionContext, webJarsUtil: WebJarsUtil) extends BaseController with I18nSupport with Logging {
2223

2324
val loginForm = Form(
2425
mapping(
25-
"handle" -> nonEmptyText,
26-
"appPassword" -> nonEmptyText
26+
"handle" -> nonEmptyText(maxLength = 255),
27+
"appPassword" -> nonEmptyText(maxLength = 255)
2728
)(LoginData.apply)(data => Some((data.handle, data.appPassword)))
2829
)
2930

@@ -32,31 +33,42 @@ class AuthController @Inject()(
3233
}
3334

3435
def authenticate: Action[AnyContent] = Action.async { implicit request =>
35-
loginForm.bindFromRequest().fold(
36-
formWithErrors => Future.successful(BadRequest(views.html.auth.login(formWithErrors))),
37-
data => {
38-
authService.login(data.handle, data.appPassword).flatMap { // Changed map to flatMap
39-
case Some(user) =>
40-
// Fetch roles to store in session for UI logic
41-
userRoleRepository.getUserRoles(user.id.get).map { roles =>
42-
val roleString = roles.mkString(",")
43-
val displayName = user.displayName.orElse(user.handle).getOrElse("User")
44-
Redirect(routes.HomeController.index())
45-
.withSession(
46-
"userId" -> user.id.get.toString,
47-
"userRoles" -> roleString,
48-
"userDisplayName" -> displayName
49-
)
50-
.flashing("success" -> s"Welcome back, $displayName!")
51-
}
52-
case None =>
53-
Future.successful(
54-
Redirect(routes.AuthController.login)
55-
.flashing("error" -> "Invalid handle or password.")
56-
)
36+
val clientIp = request.headers.get("X-Real-IP").getOrElse(request.remoteAddress)
37+
38+
if (!rateLimiter.isAllowed(clientIp)) {
39+
Future.successful(
40+
Redirect(routes.AuthController.login)
41+
.flashing("error" -> "Too many login attempts. Please try again later.")
42+
)
43+
} else {
44+
loginForm.bindFromRequest().fold(
45+
formWithErrors => Future.successful(BadRequest(views.html.auth.login(formWithErrors))),
46+
data => {
47+
authService.login(data.handle, data.appPassword).flatMap {
48+
case Some(user) =>
49+
rateLimiter.recordSuccess(clientIp)
50+
// Fetch roles to store in session for UI logic
51+
userRoleRepository.getUserRoles(user.id.get).map { roles =>
52+
val roleString = roles.mkString(",")
53+
val displayName = user.displayName.orElse(user.handle).getOrElse("User")
54+
Redirect(routes.HomeController.index())
55+
.withSession(
56+
"userId" -> user.id.get.toString,
57+
"userRoles" -> roleString,
58+
"userDisplayName" -> displayName
59+
)
60+
.flashing("success" -> s"Welcome back, $displayName!")
61+
}
62+
case None =>
63+
rateLimiter.recordFailure(clientIp)
64+
Future.successful(
65+
Redirect(routes.AuthController.login)
66+
.flashing("error" -> "Invalid handle or password.")
67+
)
68+
}
5769
}
58-
}
59-
)
70+
)
71+
}
6072
}
6173

6274
def logout: Action[AnyContent] = Action { implicit request =>

‎app/controllers/DiscoveryApiController.scala‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,16 @@ class DiscoveryApiController @Inject()(
2323
treeEvolutionService: TreeEvolutionService
2424
)(implicit ec: ExecutionContext) extends BaseController with Logging {
2525

26+
// Audit identity for API-key-authenticated actions
27+
private val ApiCuratorId = "api-system"
28+
2629
// Request DTOs
27-
case class AcceptProposalRequest(curatorId: String, proposedName: String, reason: Option[String] = None)
30+
case class AcceptProposalRequest(proposedName: String, reason: Option[String] = None)
2831
object AcceptProposalRequest { implicit val format: OFormat[AcceptProposalRequest] = Json.format }
2932

30-
case class RejectProposalRequest(curatorId: String, reason: String)
33+
case class RejectProposalRequest(reason: String)
3134
object RejectProposalRequest { implicit val format: OFormat[RejectProposalRequest] = Json.format }
3235

33-
case class StartReviewRequest(curatorId: String)
34-
object StartReviewRequest { implicit val format: OFormat[StartReviewRequest] = Json.format }
35-
36-
case class PromoteProposalRequest(curatorId: String)
37-
object PromoteProposalRequest { implicit val format: OFormat[PromoteProposalRequest] = Json.format }
38-
3936
/**
4037
* List proposals with optional filters.
4138
* GET /api/v1/discovery/proposals?type=Y&status=READY_FOR_REVIEW
@@ -78,9 +75,9 @@ class DiscoveryApiController @Inject()(
7875
* Start review of a proposal.
7976
* POST /api/v1/discovery/proposals/:id/start-review
8077
*/
81-
def startReview(id: Int): Action[StartReviewRequest] =
82-
secureApi.jsonAction[StartReviewRequest].async { request =>
83-
discoveryService.startReview(id, request.body.curatorId).map { proposal =>
78+
def startReview(id: Int): Action[AnyContent] =
79+
secureApi.async { request =>
80+
discoveryService.startReview(id, ApiCuratorId).map { proposal =>
8481
Ok(Json.toJson(proposal))
8582
}.recover {
8683
case e: NoSuchElementException =>
@@ -100,7 +97,7 @@ class DiscoveryApiController @Inject()(
10097
def acceptProposal(id: Int): Action[AcceptProposalRequest] =
10198
secureApi.jsonAction[AcceptProposalRequest].async { request =>
10299
val body = request.body
103-
discoveryService.acceptProposal(id, body.curatorId, body.proposedName, body.reason).map { proposal =>
100+
discoveryService.acceptProposal(id, ApiCuratorId, body.proposedName, body.reason).map { proposal =>
104101
Ok(Json.toJson(proposal))
105102
}.recover {
106103
case e: NoSuchElementException =>
@@ -120,7 +117,7 @@ class DiscoveryApiController @Inject()(
120117
def rejectProposal(id: Int): Action[RejectProposalRequest] =
121118
secureApi.jsonAction[RejectProposalRequest].async { request =>
122119
val body = request.body
123-
discoveryService.rejectProposal(id, body.curatorId, body.reason).map { proposal =>
120+
discoveryService.rejectProposal(id, ApiCuratorId, body.reason).map { proposal =>
124121
Ok(Json.toJson(proposal))
125122
}.recover {
126123
case e: NoSuchElementException =>
@@ -137,9 +134,9 @@ class DiscoveryApiController @Inject()(
137134
* Promote an accepted proposal to the canonical haplogroup tree.
138135
* POST /api/v1/discovery/proposals/:id/promote
139136
*/
140-
def promoteProposal(id: Int): Action[PromoteProposalRequest] =
141-
secureApi.jsonAction[PromoteProposalRequest].async { request =>
142-
treeEvolutionService.promoteProposal(id, request.body.curatorId).map { result =>
137+
def promoteProposal(id: Int): Action[AnyContent] =
138+
secureApi.async { request =>
139+
treeEvolutionService.promoteProposal(id, ApiCuratorId).map { result =>
143140
Ok(Json.toJson(result))
144141
}.recover {
145142
case e: NoSuchElementException =>

‎app/controllers/InstrumentProposalController.scala‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,18 @@ class InstrumentProposalController @Inject()(
2121
)(implicit ec: ExecutionContext)
2222
extends BaseController with Logging {
2323

24+
// Audit identity for API-key-authenticated actions
25+
private val ApiCuratorId = "api-system"
26+
2427
case class AcceptProposalRequest(
25-
curatorId: String,
2628
labName: String,
2729
manufacturer: Option[String] = None,
2830
model: Option[String] = None,
2931
notes: Option[String] = None
3032
)
3133
object AcceptProposalRequest { implicit val format: OFormat[AcceptProposalRequest] = Json.format }
3234

33-
case class RejectProposalRequest(curatorId: String, reason: String)
35+
case class RejectProposalRequest(reason: String)
3436
object RejectProposalRequest { implicit val format: OFormat[RejectProposalRequest] = Json.format }
3537

3638
def listProposals(status: Option[String]): Action[AnyContent] = secureApi.async { _ =>
@@ -76,7 +78,7 @@ class InstrumentProposalController @Inject()(
7678
def acceptProposal(id: Int): Action[AcceptProposalRequest] =
7779
secureApi.jsonAction[AcceptProposalRequest].async { request =>
7880
val body = request.body
79-
proposalService.acceptProposal(id, body.curatorId, body.labName, body.manufacturer, body.model, body.notes).map {
81+
proposalService.acceptProposal(id, ApiCuratorId, body.labName, body.manufacturer, body.model, body.notes).map {
8082
case Right(proposal) => Ok(Json.toJson(proposal))
8183
case Left(error) => BadRequest(Json.obj("error" -> error))
8284
}.recover {
@@ -89,7 +91,7 @@ class InstrumentProposalController @Inject()(
8991
def rejectProposal(id: Int): Action[RejectProposalRequest] =
9092
secureApi.jsonAction[RejectProposalRequest].async { request =>
9193
val body = request.body
92-
proposalService.rejectProposal(id, body.curatorId, body.reason).map {
94+
proposalService.rejectProposal(id, ApiCuratorId, body.reason).map {
9395
case Right(proposal) => Ok(Json.toJson(proposal))
9496
case Left(error) => BadRequest(Json.obj("error" -> error))
9597
}.recover {

‎app/controllers/TreeVersioningApiController.scala‎

Lines changed: 16 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,36 +27,23 @@ class TreeVersioningApiController @Inject()(
2727
// Request/Response DTOs
2828
// ============================================================================
2929

30-
case class StartReviewRequest(curatorId: String)
31-
object StartReviewRequest {
32-
implicit val format: OFormat[StartReviewRequest] = Json.format[StartReviewRequest]
33-
}
34-
35-
case class ApplyChangeSetRequest(curatorId: String)
36-
object ApplyChangeSetRequest {
37-
implicit val format: OFormat[ApplyChangeSetRequest] = Json.format[ApplyChangeSetRequest]
38-
}
30+
// Audit identity for API-key-authenticated actions
31+
private val ApiCuratorId = "api-system"
3932

40-
case class DiscardChangeSetRequest(curatorId: String, reason: String)
33+
case class DiscardChangeSetRequest(reason: String)
4134
object DiscardChangeSetRequest {
4235
implicit val format: OFormat[DiscardChangeSetRequest] = Json.format[DiscardChangeSetRequest]
4336
}
4437

4538
case class ReviewChangeRequest(
46-
curatorId: String,
4739
action: String, // "APPLIED", "SKIPPED", "REVERTED"
4840
notes: Option[String] = None
4941
)
5042
object ReviewChangeRequest {
5143
implicit val format: OFormat[ReviewChangeRequest] = Json.format[ReviewChangeRequest]
5244
}
5345

54-
case class ApproveAllRequest(curatorId: String)
55-
object ApproveAllRequest {
56-
implicit val format: OFormat[ApproveAllRequest] = Json.format[ApproveAllRequest]
57-
}
58-
59-
case class AddCommentRequest(author: String, content: String, treeChangeId: Option[Int] = None)
46+
case class AddCommentRequest(content: String, treeChangeId: Option[Int] = None)
6047
object AddCommentRequest {
6148
implicit val format: OFormat[AddCommentRequest] = Json.format[AddCommentRequest]
6249
}
@@ -112,9 +99,9 @@ class TreeVersioningApiController @Inject()(
11299
* Start review of a change set.
113100
* POST /api/v1/manage/change-sets/:id/start-review
114101
*/
115-
def startReview(id: Int): Action[StartReviewRequest] =
116-
secureApi.jsonAction[StartReviewRequest].async { request =>
117-
treeVersioningService.startReview(id, request.body.curatorId).map { success =>
102+
def startReview(id: Int): Action[AnyContent] =
103+
secureApi.async { request =>
104+
treeVersioningService.startReview(id, ApiCuratorId).map { success =>
118105
if (success) {
119106
Ok(Json.obj("success" -> true, "message" -> s"Review started for change set $id"))
120107
} else {
@@ -135,9 +122,9 @@ class TreeVersioningApiController @Inject()(
135122
* Apply a change set to Production.
136123
* POST /api/v1/manage/change-sets/:id/apply
137124
*/
138-
def applyChangeSet(id: Int): Action[ApplyChangeSetRequest] =
139-
secureApi.jsonAction[ApplyChangeSetRequest].async { request =>
140-
treeVersioningService.applyChangeSet(id, request.body.curatorId).map { success =>
125+
def applyChangeSet(id: Int): Action[AnyContent] =
126+
secureApi.async { request =>
127+
treeVersioningService.applyChangeSet(id, ApiCuratorId).map { success =>
141128
if (success) {
142129
Ok(Json.obj("success" -> true, "message" -> s"Change set $id applied to Production"))
143130
} else {
@@ -161,7 +148,7 @@ class TreeVersioningApiController @Inject()(
161148
def discardChangeSet(id: Int): Action[DiscardChangeSetRequest] =
162149
secureApi.jsonAction[DiscardChangeSetRequest].async { request =>
163150
val req = request.body
164-
treeVersioningService.discardChangeSet(id, req.curatorId, req.reason).map { success =>
151+
treeVersioningService.discardChangeSet(id, ApiCuratorId, req.reason).map { success =>
165152
if (success) {
166153
Ok(Json.obj("success" -> true, "message" -> s"Change set $id discarded"))
167154
} else {
@@ -211,7 +198,7 @@ class TreeVersioningApiController @Inject()(
211198
case None =>
212199
Future.successful(BadRequest(Json.obj("error" -> s"Invalid action: ${req.action}")))
213200
case Some(action) =>
214-
treeVersioningService.reviewChange(changeId, req.curatorId, action, req.notes).map { success =>
201+
treeVersioningService.reviewChange(changeId, ApiCuratorId, action, req.notes).map { success =>
215202
if (success) {
216203
Ok(Json.obj("success" -> true, "message" -> s"Change $changeId reviewed as ${req.action}"))
217204
} else {
@@ -231,9 +218,9 @@ class TreeVersioningApiController @Inject()(
231218
* Approve all pending changes in a change set.
232219
* POST /api/v1/manage/change-sets/:id/approve-all
233220
*/
234-
def approveAllPending(id: Int): Action[ApproveAllRequest] =
235-
secureApi.jsonAction[ApproveAllRequest].async { request =>
236-
treeVersioningService.approveAllPending(id, request.body.curatorId).map { count =>
221+
def approveAllPending(id: Int): Action[AnyContent] =
222+
secureApi.async { request =>
223+
treeVersioningService.approveAllPending(id, ApiCuratorId).map { count =>
237224
Ok(Json.obj("success" -> true, "approvedCount" -> count))
238225
}.recover {
239226
case e: Exception =>
@@ -253,7 +240,7 @@ class TreeVersioningApiController @Inject()(
253240
def addComment(id: Int): Action[AddCommentRequest] =
254241
secureApi.jsonAction[AddCommentRequest].async { request =>
255242
val req = request.body
256-
treeVersioningService.addComment(id, req.author, req.content, req.treeChangeId).map { commentId =>
243+
treeVersioningService.addComment(id, ApiCuratorId, req.content, req.treeChangeId).map { commentId =>
257244
Created(Json.obj("success" -> true, "commentId" -> commentId))
258245
}.recover {
259246
case e: Exception =>

‎app/services/LoginRateLimiter.scala‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
package services
2+
3+
import java.time.Instant
4+
import java.util.concurrent.ConcurrentHashMap
5+
import javax.inject.Singleton
6+
7+
/**
8+
* Simple in-memory rate limiter for login attempts.
9+
* Tracks failed attempts per IP and blocks after threshold.
10+
*/
11+
@Singleton
12+
class LoginRateLimiter {
13+
14+
private val MaxAttempts = 10
15+
private val WindowSeconds = 900L // 15 minutes
16+
private val attempts = new ConcurrentHashMap[String, (Int, Instant)]()
17+
18+
/** Returns true if the IP is allowed to attempt login. */
19+
def isAllowed(ip: String): Boolean = {
20+
pruneExpired()
21+
val entry = attempts.get(ip)
22+
entry == null || entry._1 < MaxAttempts
23+
}
24+
25+
/** Record a failed login attempt for the given IP. */
26+
def recordFailure(ip: String): Unit = {
27+
attempts.compute(ip, (_, existing) => {
28+
if (existing == null || existing._2.plusSeconds(WindowSeconds).isBefore(Instant.now())) {
29+
(1, Instant.now())
30+
} else {
31+
(existing._1 + 1, existing._2)
32+
}
33+
})
34+
}
35+
36+
/** Clear attempts for an IP on successful login. */
37+
def recordSuccess(ip: String): Unit = {
38+
attempts.remove(ip)
39+
}
40+
41+
private def pruneExpired(): Unit = {
42+
val cutoff = Instant.now().minusSeconds(WindowSeconds)
43+
attempts.entrySet().removeIf(e => e.getValue._2.isBefore(cutoff))
44+
}
45+
}

0 commit comments

Comments
 (0)