Skip to content

Commit 8e346da

Browse files
BK1031BK1031
committed
fix(core): enforce join request integrity
1 parent db8e71e commit 8e346da

8 files changed

Lines changed: 300 additions & 90 deletions

File tree

‎core/api/api.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,14 @@ func RequestTokenHasUserID(c *gin.Context, userID string) bool {
293293
return GetRequestTokenUserID(c) == userID
294294
}
295295

296+
func RequestTokenHasInternalAccess(c *gin.Context) bool {
297+
claims := GetRequestTokenClaims(c)
298+
if claims == nil || claims["type"] != "service_account" {
299+
return false
300+
}
301+
return RequestTokenHasScope(c, "sentinel:all")
302+
}
303+
296304
// RequestUserIsAdmin reports whether the bearer's subject entity is a
297305
// member of the Admins group. Used to grant admin-only write access
298306
// without requiring per-resource ownership. Returns false for unauth'd
@@ -324,7 +332,7 @@ func RequestUserIsGroupOwner(c *gin.Context, groupID string) bool {
324332
// the caller continues.
325333
func requireGroupOwnerOrAdmin(c *gin.Context, groupID string) bool {
326334
if Any(
327-
RequestTokenHasScope(c, "sentinel:all"),
335+
RequestTokenHasInternalAccess(c),
328336
RequestUserIsGroupOwner(c, groupID),
329337
RequestUserIsAdmin(c),
330338
) {

‎core/api/group.go‎

Lines changed: 110 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func CreateOrUpdateGroup(c *gin.Context) {
141141
// on any group, including the Admins group.
142142
if existing.ID == "" {
143143
Require(c, Any(
144-
RequestTokenHasScope(c, "sentinel:all"),
144+
RequestTokenHasInternalAccess(c),
145145
RequestTokenHasScope(c, "groups:write"),
146146
))
147147
} else if !requireGroupOwnerOrAdmin(c, existing.ID) {
@@ -262,7 +262,7 @@ type addGroupMemberRequest struct {
262262
}
263263

264264
func requestAddedBy(c *gin.Context, claimed string) string {
265-
if RequestTokenHasScope(c, "sentinel:all") && claimed != "" {
265+
if RequestTokenHasInternalAccess(c) && claimed != "" {
266266
return claimed
267267
}
268268
return GetRequestTokenEntityID(c)
@@ -303,13 +303,13 @@ func AddGroupMember(c *gin.Context) {
303303
if source == "" {
304304
source = string(model.GroupMemberSourceDirect)
305305
}
306-
if source != string(model.GroupMemberSourceDirect) && !RequestTokenHasScope(c, "sentinel:all") {
306+
if source != string(model.GroupMemberSourceDirect) && !RequestTokenHasInternalAccess(c) {
307307
c.JSON(http.StatusForbidden, gin.H{"error": "only internal services can add synced group members"})
308308
return
309309
}
310310
if source == string(model.GroupMemberSourceDirect) &&
311311
!containsSource(group.AllowedSources, model.GroupMemberSourceDirect) &&
312-
!RequestTokenHasScope(c, "sentinel:all") {
312+
!RequestTokenHasInternalAccess(c) {
313313
c.JSON(http.StatusBadRequest, gin.H{"error": "direct memberships are not enabled for this group"})
314314
return
315315
}
@@ -460,8 +460,9 @@ func GetGroupJoinRequests(c *gin.Context) {
460460
}
461461

462462
func GetGroupJoinRequest(c *gin.Context) {
463+
id := c.Param("id")
463464
requestID := c.Param("requestID")
464-
request, err := service.GetJoinRequestByID(requestID)
465+
request, err := service.GetJoinRequestForGroup(id, requestID)
465466
if err != nil {
466467
if err == gorm.ErrRecordNotFound {
467468
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
@@ -473,7 +474,7 @@ func GetGroupJoinRequest(c *gin.Context) {
473474
// Applicants can read their own request; otherwise the group's
474475
// owner roster, admins, and internal services can see it.
475476
Require(c, Any(
476-
RequestTokenHasScope(c, "sentinel:all"),
477+
RequestTokenHasInternalAccess(c),
477478
RequestTokenHasEntityID(c, request.EntityID),
478479
RequestUserIsGroupOwner(c, request.GroupID),
479480
RequestUserIsAdmin(c),
@@ -499,7 +500,7 @@ func CreateGroupJoinRequest(c *gin.Context) {
499500
// that is admin or internal; group owners can't backdoor people in
500501
// via this endpoint (they'd use AddGroupMember directly).
501502
Require(c, Any(
502-
RequestTokenHasScope(c, "sentinel:all"),
503+
RequestTokenHasInternalAccess(c),
503504
RequestTokenHasEntityID(c, req.EntityID),
504505
RequestUserIsAdmin(c),
505506
))
@@ -527,7 +528,6 @@ func CreateGroupJoinRequest(c *gin.Context) {
527528
}
528529

529530
type reviewJoinRequestRequest struct {
530-
ReviewedBy string `json:"reviewed_by" binding:"required"`
531531
// Optional approval-time overrides. When provided, they replace the
532532
// expiration that the requester originally chose — used by reviewers
533533
// who want to grant a shorter/longer membership than what was asked
@@ -539,25 +539,24 @@ type reviewJoinRequestRequest struct {
539539

540540
func ApproveGroupJoinRequest(c *gin.Context) {
541541
id := c.Param("id")
542-
if !requireGroupOwnerOrAdmin(c, id) {
543-
return
544-
}
545542
requestID := c.Param("requestID")
546-
var req reviewJoinRequestRequest
547-
if err := c.ShouldBindJSON(&req); err != nil {
548-
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
549-
return
550-
}
551-
request, err := service.GetJoinRequestByID(requestID)
543+
request, err := service.GetJoinRequestForGroup(id, requestID)
552544
if err != nil {
553-
if err == gorm.ErrRecordNotFound {
545+
if errors.Is(err, gorm.ErrRecordNotFound) {
554546
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
555547
return
556548
}
557549
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
558550
return
559551
}
560-
552+
if !requireGroupOwnerOrAdmin(c, id) {
553+
return
554+
}
555+
var req reviewJoinRequestRequest
556+
if err := c.ShouldBindJSON(&req); err != nil {
557+
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
558+
return
559+
}
561560
hasExpiration := request.HasExpiration
562561
expiresAt := request.ExpiresAt
563562
if req.HasExpiration != nil {
@@ -571,63 +570,74 @@ func ApproveGroupJoinRequest(c *gin.Context) {
571570
return
572571
}
573572

574-
request.Status = string(model.GroupJoinRequestStatusApproved)
575-
request.ReviewedBy = req.ReviewedBy
576-
request.ReviewedAt = time.Now()
577-
request.HasExpiration = hasExpiration
578-
request.ExpiresAt = expiresAt
579-
request, err = service.UpdateJoinRequest(request)
580-
if err != nil {
581-
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
582-
return
583-
}
584-
_, err = service.CreateGroupMember(model.GroupMember{
585-
GroupID: request.GroupID,
586-
EntityID: request.EntityID,
587-
Source: string(model.GroupMemberSourceDirect),
588-
AddedBy: req.ReviewedBy,
589-
HasExpiration: hasExpiration,
590-
ExpiresAt: expiresAt,
591-
})
573+
request, err = service.ReviewJoinRequest(
574+
id,
575+
requestID,
576+
GetRequestTokenEntityID(c),
577+
model.GroupJoinRequestStatusApproved,
578+
hasExpiration,
579+
expiresAt,
580+
)
592581
if err != nil {
582+
if errors.Is(err, service.ErrJoinRequestNotPending) || errors.Is(err, service.ErrGroupMemberExists) {
583+
c.JSON(http.StatusConflict, gin.H{"error": err.Error()})
584+
return
585+
}
586+
if errors.Is(err, gorm.ErrRecordNotFound) {
587+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
588+
return
589+
}
593590
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
594591
return
595592
}
596593
recordAudit(c, model.AuditActionJoinRequestApproved, "join_request", requestID, model.JSONMap{
597594
"group_id": request.GroupID,
598595
"entity_id": request.EntityID,
599596
})
597+
service.ReconcileConditionalForEntity(request.EntityID)
600598
c.JSON(http.StatusOK, request)
601599
}
602600

603601
func RejectGroupJoinRequest(c *gin.Context) {
604602
id := c.Param("id")
603+
requestID := c.Param("requestID")
604+
request, err := service.GetJoinRequestForGroup(id, requestID)
605+
if err != nil {
606+
if errors.Is(err, gorm.ErrRecordNotFound) {
607+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
608+
return
609+
}
610+
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
611+
return
612+
}
605613
if !requireGroupOwnerOrAdmin(c, id) {
606614
return
607615
}
608-
requestID := c.Param("requestID")
609616
var req reviewJoinRequestRequest
610617
if err := c.ShouldBindJSON(&req); err != nil {
611618
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
612619
return
613620
}
614-
request, err := service.GetJoinRequestByID(requestID)
621+
request, err = service.ReviewJoinRequest(
622+
id,
623+
requestID,
624+
GetRequestTokenEntityID(c),
625+
model.GroupJoinRequestStatusRejected,
626+
request.HasExpiration,
627+
request.ExpiresAt,
628+
)
615629
if err != nil {
616-
if err == gorm.ErrRecordNotFound {
630+
if errors.Is(err, service.ErrJoinRequestNotPending) {
631+
c.JSON(http.StatusConflict, gin.H{"error": err.Error()})
632+
return
633+
}
634+
if errors.Is(err, gorm.ErrRecordNotFound) {
617635
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
618636
return
619637
}
620638
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
621639
return
622640
}
623-
request.Status = string(model.GroupJoinRequestStatusRejected)
624-
request.ReviewedBy = req.ReviewedBy
625-
request.ReviewedAt = time.Now()
626-
request, err = service.UpdateJoinRequest(request)
627-
if err != nil {
628-
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
629-
return
630-
}
631641
recordAudit(c, model.AuditActionJoinRequestRejected, "join_request", requestID, model.JSONMap{
632642
"group_id": request.GroupID,
633643
"entity_id": request.EntityID,
@@ -637,11 +647,30 @@ func RejectGroupJoinRequest(c *gin.Context) {
637647

638648
func DeleteGroupJoinRequest(c *gin.Context) {
639649
id := c.Param("id")
640-
if !requireGroupOwnerOrAdmin(c, id) {
650+
requestID := c.Param("requestID")
651+
request, err := service.GetJoinRequestForGroup(id, requestID)
652+
if err != nil {
653+
if errors.Is(err, gorm.ErrRecordNotFound) {
654+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
655+
return
656+
}
657+
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
641658
return
642659
}
643-
requestID := c.Param("requestID")
644-
if err := service.DeleteJoinRequest(requestID); err != nil {
660+
if !Any(
661+
RequestTokenHasInternalAccess(c),
662+
RequestTokenHasEntityID(c, request.EntityID) && request.Status == string(model.GroupJoinRequestStatusPending),
663+
RequestUserIsGroupOwner(c, id),
664+
RequestUserIsAdmin(c),
665+
) {
666+
c.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "you are not authorized to delete this join request"})
667+
return
668+
}
669+
if err := service.DeleteJoinRequestForGroup(id, requestID); err != nil {
670+
if errors.Is(err, gorm.ErrRecordNotFound) {
671+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
672+
return
673+
}
645674
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
646675
return
647676
}
@@ -651,8 +680,7 @@ func DeleteGroupJoinRequest(c *gin.Context) {
651680
// Join Request Comments
652681

653682
type createJoinRequestCommentRequest struct {
654-
EntityID string `json:"entity_id" binding:"required"`
655-
Comment string `json:"comment" binding:"required"`
683+
Comment string `json:"comment" binding:"required"`
656684
}
657685

658686
func CreateJoinRequestComment(c *gin.Context) {
@@ -663,20 +691,25 @@ func CreateJoinRequestComment(c *gin.Context) {
663691
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
664692
return
665693
}
666-
// Comments are scoped to the join-request thread: the requester
667-
// (commenting on their own request) and the group's owners /
668-
// admins (reviewing the request) are the legitimate posters.
669-
// Bearer must match the comment's claimed entity_id; the owner/
670-
// admin path bypasses the self check.
694+
request, err := service.GetJoinRequestForGroup(id, requestID)
695+
if err != nil {
696+
if errors.Is(err, gorm.ErrRecordNotFound) {
697+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
698+
return
699+
}
700+
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
701+
return
702+
}
703+
actorID := GetRequestTokenEntityID(c)
671704
Require(c, Any(
672-
RequestTokenHasScope(c, "sentinel:all"),
673-
RequestTokenHasEntityID(c, req.EntityID),
705+
RequestTokenHasInternalAccess(c),
706+
actorID == request.EntityID,
674707
RequestUserIsGroupOwner(c, id),
675708
RequestUserIsAdmin(c),
676709
))
677710
comment, err := service.CreateJoinRequestComment(model.GroupJoinRequestComment{
678711
RequestID: requestID,
679-
EntityID: req.EntityID,
712+
EntityID: actorID,
680713
Comment: req.Comment,
681714
})
682715
if err != nil {
@@ -688,11 +721,17 @@ func CreateJoinRequestComment(c *gin.Context) {
688721

689722
func DeleteJoinRequestComment(c *gin.Context) {
690723
id := c.Param("id")
724+
requestID := c.Param("requestID")
691725
commentID := c.Param("commentID")
692-
// Look up the comment first so we can authorize against its
693-
// claimed author (the entity who posted it can delete their own
694-
// comment; otherwise owner/admin/internal).
695-
comment, err := service.GetJoinRequestComment(commentID)
726+
if _, err := service.GetJoinRequestForGroup(id, requestID); err != nil {
727+
if errors.Is(err, gorm.ErrRecordNotFound) {
728+
c.JSON(http.StatusNotFound, gin.H{"error": "join request not found"})
729+
return
730+
}
731+
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
732+
return
733+
}
734+
comment, err := service.GetJoinRequestCommentForRequest(requestID, commentID)
696735
if err != nil {
697736
if err == gorm.ErrRecordNotFound {
698737
c.JSON(http.StatusNotFound, gin.H{"error": "comment not found"})
@@ -702,12 +741,16 @@ func DeleteJoinRequestComment(c *gin.Context) {
702741
return
703742
}
704743
Require(c, Any(
705-
RequestTokenHasScope(c, "sentinel:all"),
744+
RequestTokenHasInternalAccess(c),
706745
RequestTokenHasEntityID(c, comment.EntityID),
707746
RequestUserIsGroupOwner(c, id),
708747
RequestUserIsAdmin(c),
709748
))
710-
if err := service.DeleteJoinRequestComment(commentID); err != nil {
749+
if err := service.DeleteJoinRequestCommentForRequest(requestID, commentID); err != nil {
750+
if errors.Is(err, gorm.ErrRecordNotFound) {
751+
c.JSON(http.StatusNotFound, gin.H{"error": "comment not found"})
752+
return
753+
}
711754
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
712755
return
713756
}

‎core/api/internal_access_test.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
package api
2+
3+
import (
4+
"testing"
5+
6+
"github.com/gin-gonic/gin"
7+
)
8+
9+
func TestRequestTokenHasInternalAccessRequiresServiceAccount(t *testing.T) {
10+
gin.SetMode(gin.TestMode)
11+
12+
userContext, _ := gin.CreateTestContext(nil)
13+
userContext.Set("Auth-Scope", "sentinel:all")
14+
userContext.Set("Auth-Claims", map[string]interface{}{"user_id": "usr_1"})
15+
if RequestTokenHasInternalAccess(userContext) {
16+
t.Fatal("user session must not receive internal access")
17+
}
18+
19+
serviceContext, _ := gin.CreateTestContext(nil)
20+
serviceContext.Set("Auth-Scope", "sentinel:all")
21+
serviceContext.Set("Auth-Claims", map[string]interface{}{"type": "service_account"})
22+
if !RequestTokenHasInternalAccess(serviceContext) {
23+
t.Fatal("internal service account should receive internal access")
24+
}
25+
}

0 commit comments

Comments
 (0)