From 05023992cdc5bd3d8f3fb0fd00233764492fa3b3 Mon Sep 17 00:00:00 2001 From: Sabine Maennel <5292683+sabinem@users.noreply.github.com> Date: Tue, 25 Aug 2026 06:50:31 +0200 Subject: [PATCH 1/2] Three defects found while planning the frontend against the merged feature. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Signup deadlocked. Join validates the mandatory answers, answering needs the questions, and ListQuestions required `hackathon:read` — which a non-member does not hold, since AllowPublicHackathonAccess is defined in rbac.go but never called. So a hackathon asking anything mandatory could not be joined by anyone. ListQuestions now serves a public hackathon to any caller, the same rule List already applies to the hackathons themselves; a private event still requires the grant. It also returns NotFound for an unknown id instead of an empty list. 2. ListParticipantAnswers leaked. `hasWrite` was computed but only consulted on the "no user_id" path, so naming any user id skipped the check entirely and returned what that person wrote about themselves. Reading someone else's answers is now organizer-only; reading your own still is not. 3. Bool answers could not round-trip. answerEntryFromEnt always emitted text_value, so a BOOL question read back as text_value "true" — and SubmitAnswers refuses a text answer to a bool question. Loading a form and saving it unchanged therefore failed validation on every bool question, which is what an edit form does on every save. The oneof arm now follows the question's type, which the query eager-loads. One existing test changed meaning: `ListQuestions requires Read permission` asserted the pre-fix behaviour, so it is retargeted at a private hackathon and renamed accordingly. That is the only intentional behaviour change. --- .../internal/service/hackathon_service.go | 35 ++++- .../service/hackathon_service_test.go | 137 +++++++++++++++++- .../backend/internal/service/mappers.go | 18 ++- 3 files changed, 183 insertions(+), 7 deletions(-) diff --git a/components/backend/internal/service/hackathon_service.go b/components/backend/internal/service/hackathon_service.go index 4445bc69..7a79d743 100644 --- a/components/backend/internal/service/hackathon_service.go +++ b/components/backend/internal/service/hackathon_service.go @@ -1424,8 +1424,28 @@ func (s *HackathonService) ListQuestions( return nil, status.Errorf(codes.InvalidArgument, "invalid hackathon_id: %v", err) } - if err := s.enforcer.RequirePermission(ctx, id.String(), mw.Hackathon, mw.Read); err != nil { - return nil, err + // The questions have to be readable BEFORE joining. Join validates the + // mandatory answers, and answering needs the questions — so requiring + // `hackathon:read` here deadlocked signup outright: a non-member holds no + // such grant (AllowPublicHackathonAccess exists but is never called), which + // made a hackathon with any mandatory question impossible to join. + // + // Public hackathons are therefore readable by anyone, which is the rule List + // already applies to the hackathons themselves. Private ones still require + // the grant, so an uninvited caller cannot enumerate what a closed event asks. + h, err := s.dbClient.Hackathon.Query().Where(enthackathon.IDEQ(id)).Only(ctx) + if err != nil { + if ent.IsNotFound(err) { + return nil, status.Errorf(codes.NotFound, "hackathon %s not found", id) + } + slog.Error("query hackathon", "err", err) + + return nil, status.Error(codes.Internal, "couldn't query database") + } + if h.Visibility != enthackathon.VisibilityPublic { + if err := s.enforcer.RequirePermission(ctx, id.String(), mw.Hackathon, mw.Read); err != nil { + return nil, err + } } questions, err := s.dbClient.Question.Query().Where( @@ -1558,17 +1578,26 @@ func (s *HackathonService) ListParticipantAnswers( } // Build query + // WithQuestion because the answer's stored value is always a string, and only + // the QUESTION knows which oneof arm it belongs in on the way out. q := s.dbClient.Answer.Query().Where( entanswer.HasQuestionWith( entquestion.HasHackathonWith(enthackathon.IDEQ(hackID)), ), - ) + ).WithQuestion() if req.GetUserId() != "" { uidParsed, err := uuid.Parse(req.GetUserId()) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "invalid user_id: %v", err) } + // Naming someone else is organizer-only. Without this check the branch + // below was reachable by anyone: `hasWrite` was computed but only + // consulted on the "no user_id" path, so any authenticated caller could + // pass any user id and read what that person wrote about themselves. + if uidParsed != user.ID && !hasWrite { + return nil, status.Error(codes.PermissionDenied, "permission denied") + } q = q.Where(entanswer.HasUserWith( entuser.ID(uidParsed), )) diff --git a/components/backend/internal/service/hackathon_service_test.go b/components/backend/internal/service/hackathon_service_test.go index 3e1fb300..8540d84a 100644 --- a/components/backend/internal/service/hackathon_service_test.go +++ b/components/backend/internal/service/hackathon_service_test.go @@ -2525,6 +2525,31 @@ var _ = Describe("HackathonService", func() { // --- ListQuestions --- Describe("ListQuestions", func() { + // Regression: this required `hackathon:read`, which a non-member does + // not hold. Join validates the mandatory answers and answering needs + // the questions, so a hackathon asking anything mandatory could not be + // joined by anyone at all. + It("serves a non-member, so signup is not deadlocked", func() { + _, err := dbClient.User.Create(). + SetKeycloakID("lq-outsider"). + SetUsername("lq-outsider"). + Save(context.Background()) + Expect(err).NotTo(HaveOccurred()) + + outsiderCtx := metadata.NewOutgoingContext( + context.Background(), + metadata.Pairs( + "authorization", + "Bearer "+testutils.CreateTestJWTToken("lq-outsider"), + ), + ) + resp, err := client.ListQuestions(outsiderCtx, &msgs.ListQuestionsRequest{ + HackathonId: hackathonID, + }) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.GetQuestions()).NotTo(BeEmpty()) + }) + BeforeEach(func() { token := testutils.CreateTestJWTToken(testAdmin) ctx := metadata.NewOutgoingContext( @@ -2633,9 +2658,26 @@ var _ = Describe("HackathonService", func() { Expect(resp.GetQuestions()).To(BeEmpty()) }) - It("requires Read permission", func() { + It("requires Read permission on a private hackathon", func() { + // Narrowed from "requires Read" outright: a public hackathon's + // questions must be answerable before Join, and a non-member holds + // no read grant — so requiring one deadlocked signup. A private + // event still refuses an uninvited caller. + adminCtx := metadata.NewOutgoingContext( + context.Background(), + metadata.Pairs( + "authorization", + "Bearer "+testutils.CreateTestJWTToken(testAdmin), + ), + ) + priv, err := client.Create(adminCtx, &msgs.CreateRequest{ + Name: "QA Private Questions Hackathon", + Visibility: entities.Visibility_VISIBILITY_PRIVATE, + }) + Expect(err).NotTo(HaveOccurred()) + nonOwnerKeycloakID := "non-owner-list-q" - _, err := dbClient.User.Create(). + _, err = dbClient.User.Create(). SetKeycloakID(nonOwnerKeycloakID). SetUsername("non-owner-list-q-username"). Save(context.Background()) @@ -2648,7 +2690,7 @@ var _ = Describe("HackathonService", func() { ) _, err = client.ListQuestions(ctx, &msgs.ListQuestionsRequest{ - HackathonId: hackathonID, + HackathonId: priv.GetHackathonId(), }) Expect(err).To(HaveOccurred()) st := status.Convert(err) @@ -3457,6 +3499,95 @@ var _ = Describe("HackathonService", func() { Expect(err).NotTo(HaveOccurred()) }) + // Regression: `hasWrite` was computed but only consulted on the + // "no user_id" path, so naming any user id skipped the check entirely + // and returned what that person wrote about themselves. + It("refuses a non-organizer naming someone else", func() { + _, err := dbClient.User.Create(). + SetKeycloakID("lpa-outsider"). + SetUsername("lpa-outsider"). + Save(context.Background()) + Expect(err).NotTo(HaveOccurred()) + + outsiderCtx := metadata.NewOutgoingContext( + context.Background(), + metadata.Pairs( + "authorization", + "Bearer "+testutils.CreateTestJWTToken("lpa-outsider"), + ), + ) + _, err = client.ListParticipantAnswers( + outsiderCtx, + &msgs.ListParticipantAnswersRequest{ + HackathonId: hackathonID, + UserId: testutils.StringPtr(participantUser.ID.String()), + }) + Expect(status.Convert(err).Code()).To(Equal(codes.PermissionDenied)) + }) + + // Regression: the mapper always emitted text_value, so a BOOL answer + // read back as text — and SubmitAnswers refuses a text answer to a + // bool question. Loading a form and saving it unchanged therefore + // failed on every bool question, which is what an edit does on save. + It("reads a bool answer back as a bool, so a round trip re-submits", func() { + adminCtx := metadata.NewOutgoingContext( + context.Background(), + metadata.Pairs( + "authorization", + "Bearer "+testutils.CreateTestJWTToken(testAdmin), + ), + ) + boolQ, err := client.CreateQuestion(adminCtx, &msgs.CreateQuestionRequest{ + HackathonId: hackathonID, + Key: "conduct", + Label: "I accept the Code of Conduct", + Type: entities.QuestionType_QUESTION_TYPE_BOOL, + Order: 2, + }) + Expect(err).NotTo(HaveOccurred()) + + _, err = dbClient.Answer.Create(). + SetQuestionID(uuid.MustParse(boolQ.GetQuestionId())). + SetUserID(participantUser.ID). + SetValue("true"). + Save(context.Background()) + Expect(err).NotTo(HaveOccurred()) + + // The fixture writes the Participant row directly, so unlike a real + // Join it grants no casbin Member role — and SubmitAnswers checks + // hackathon:read. + _, err = enf.AddRole("list-answers-user", middleware.Member, hackathonID) + Expect(err).NotTo(HaveOccurred()) + + participantCtx := metadata.NewOutgoingContext( + context.Background(), + metadata.Pairs( + "authorization", + "Bearer "+testutils.CreateTestJWTToken("list-answers-user"), + ), + ) + resp, err := client.ListParticipantAnswers( + participantCtx, + &msgs.ListParticipantAnswersRequest{HackathonId: hackathonID}) + Expect(err).NotTo(HaveOccurred()) + + var readBack *entities.Answer + for _, a := range resp.GetAnswers() { + if a.GetQuestionId() == boolQ.GetQuestionId() { + readBack = a + } + } + Expect(readBack).NotTo(BeNil()) + Expect(readBack.GetBoolValue()).To(BeTrue()) + + // Hand straight back what was read: this is the save an edit form + // performs, and it used to fail with InvalidArgument. + _, err = client.SubmitAnswers(participantCtx, &msgs.SubmitAnswersRequest{ + HackathonId: hackathonID, + Answers: resp.GetAnswers(), + }) + Expect(err).NotTo(HaveOccurred()) + }) It("returns answers for requester's own answers (no write access)", func() { token := testutils.CreateTestJWTToken("list-answers-user") ctx := metadata.NewOutgoingContext( diff --git a/components/backend/internal/service/mappers.go b/components/backend/internal/service/mappers.go index 78d4a4de..2d2ecb30 100644 --- a/components/backend/internal/service/mappers.go +++ b/components/backend/internal/service/mappers.go @@ -529,12 +529,28 @@ func questionEntryFromEnt(q *ent.Question) *hackEnts.Question { } } +// answerEntryFromEnt maps a stored answer back onto the wire. +// +// The value is stored as a string whatever the question's type, so which arm of +// the `value` oneof it belongs in is a fact about the QUESTION, not the answer. +// Emitting text_value unconditionally meant a BOOL question read back as +// text_value "true" — and SubmitAnswers refuses a text answer to a bool +// question, so loading a form and saving it unchanged failed validation every +// time. That is exactly what an edit form does on every save. +// +// Requires the question edge; without it the type is unknowable and text is the +// only safe guess. func answerEntryFromEnt(a *ent.Answer) *hackEnts.Answer { - return &hackEnts.Answer{ + entry := &hackEnts.Answer{ QuestionId: a.QuestionID.String(), ParticipantId: a.UserID.String(), Value: &hackEnts.Answer_TextValue{TextValue: a.Value}, } + if a.Edges.Question != nil && a.Edges.Question.DataType == entquestion.DataTypeBool { + entry.Value = &hackEnts.Answer_BoolValue{BoolValue: a.Value == "true"} + } + + return entry } // protoAnswerValueToDB extracts the string representation from a proto Answer. From 5ae8038a9b0ba7508056064a92fc51ec8c755ddf Mon Sep 17 00:00:00 2001 From: Ralf Grubenmann Date: Tue, 25 Aug 2026 08:58:55 +0200 Subject: [PATCH 2/2] modify llm changes --- .../internal/service/hackathon_service.go | 68 ++++++++++--------- .../service/hackathon_service_test.go | 25 ++----- .../backend/internal/service/mappers.go | 11 --- 3 files changed, 42 insertions(+), 62 deletions(-) diff --git a/components/backend/internal/service/hackathon_service.go b/components/backend/internal/service/hackathon_service.go index 7a79d743..52e3b8cf 100644 --- a/components/backend/internal/service/hackathon_service.go +++ b/components/backend/internal/service/hackathon_service.go @@ -88,6 +88,15 @@ func (s *HackathonService) Create( return nil, status.Errorf(codes.Internal, "couldn't create hackathon in database") } + // set permission based on visibility + if visibility == enthackathon.VisibilityPublic { + _, err = s.enforcer.AllowPublicHackathonAccess(h.ID.String()) + if err != nil { + slog.Error("create hackathon", "err", err) + return nil, status.Error(codes.Internal, "couldn't set hackathon permission") + } + } + // Create default state (all capabilities disabled). _, err = s.dbClient.HackathonState.Create(). SetHackathonID(h.ID). @@ -600,6 +609,23 @@ func (s *HackathonService) Edit( return nil, status.Error(codes.Internal, "couldn't query updated hackathon") } + //nolint:nestif // Complexity is ok here. + if req.Visibility != nil { + if updated.Visibility == enthackathon.VisibilityPublic { + _, err = s.enforcer.AllowPublicHackathonAccess(h.ID.String()) + if err != nil { + slog.Error("edit hackathon", "err", err) + return nil, status.Error(codes.Internal, "couldn't change hackathon permission") + } + } else { + _, err = s.enforcer.RemovePublicHackathonAccess(h.ID.String()) + if err != nil { + slog.Error("edit hackathon", "err", err) + return nil, status.Error(codes.Internal, "couldn't change hackathon permission") + } + } + } + entry := hackathonEntryFromEnt(updated, time.Now()) entry.Creator = userEntryFromEnt(updated.Edges.Creator) entry.Modifier = userEntryFromEnt(updated.Edges.Modifier) @@ -1423,29 +1449,8 @@ func (s *HackathonService) ListQuestions( if err != nil { return nil, status.Errorf(codes.InvalidArgument, "invalid hackathon_id: %v", err) } - - // The questions have to be readable BEFORE joining. Join validates the - // mandatory answers, and answering needs the questions — so requiring - // `hackathon:read` here deadlocked signup outright: a non-member holds no - // such grant (AllowPublicHackathonAccess exists but is never called), which - // made a hackathon with any mandatory question impossible to join. - // - // Public hackathons are therefore readable by anyone, which is the rule List - // already applies to the hackathons themselves. Private ones still require - // the grant, so an uninvited caller cannot enumerate what a closed event asks. - h, err := s.dbClient.Hackathon.Query().Where(enthackathon.IDEQ(id)).Only(ctx) - if err != nil { - if ent.IsNotFound(err) { - return nil, status.Errorf(codes.NotFound, "hackathon %s not found", id) - } - slog.Error("query hackathon", "err", err) - - return nil, status.Error(codes.Internal, "couldn't query database") - } - if h.Visibility != enthackathon.VisibilityPublic { - if err := s.enforcer.RequirePermission(ctx, id.String(), mw.Hackathon, mw.Read); err != nil { - return nil, err - } + if err := s.enforcer.RequirePermission(ctx, id.String(), mw.Hackathon, mw.Read); err != nil { + return nil, err } questions, err := s.dbClient.Question.Query().Where( @@ -1516,7 +1521,10 @@ func (s *HackathonService) SubmitAnswers( ).Only(ctx) if err != nil { if ent.IsNotFound(err) { - return nil, status.Errorf(codes.NotFound, "user is not a participant in this hackathon") + return nil, status.Errorf( + codes.PermissionDenied, + "user is not a participant in this hackathon", + ) } slog.Error("query participant", "err", err) return nil, status.Error(codes.Internal, "couldn't query participant") @@ -1578,8 +1586,6 @@ func (s *HackathonService) ListParticipantAnswers( } // Build query - // WithQuestion because the answer's stored value is always a string, and only - // the QUESTION knows which oneof arm it belongs in on the way out. q := s.dbClient.Answer.Query().Where( entanswer.HasQuestionWith( entquestion.HasHackathonWith(enthackathon.IDEQ(hackID)), @@ -1591,18 +1597,16 @@ func (s *HackathonService) ListParticipantAnswers( if err != nil { return nil, status.Errorf(codes.InvalidArgument, "invalid user_id: %v", err) } - // Naming someone else is organizer-only. Without this check the branch - // below was reachable by anyone: `hasWrite` was computed but only - // consulted on the "no user_id" path, so any authenticated caller could - // pass any user id and read what that person wrote about themselves. + if uidParsed != user.ID && !hasWrite { return nil, status.Error(codes.PermissionDenied, "permission denied") } q = q.Where(entanswer.HasUserWith( entuser.ID(uidParsed), )) - } else if !hasWrite { - // No write access: filter by requester's own answers + } + if !hasWrite { + // only return own user if user does not have write access q = q.Where(entanswer.HasUserWith( entuser.ID(user.ID), )) diff --git a/components/backend/internal/service/hackathon_service_test.go b/components/backend/internal/service/hackathon_service_test.go index 8540d84a..c267bf6b 100644 --- a/components/backend/internal/service/hackathon_service_test.go +++ b/components/backend/internal/service/hackathon_service_test.go @@ -2067,12 +2067,12 @@ var _ = Describe("HackathonService", func() { Expect(err).NotTo(HaveOccurred()) Expect(h.Edges.Owners).To(HaveLen(1)) // only creator remains - // Verify casbin role was revoked (owner can no longer read hackathon) + // Verify casbin role was revoked (owner can no longer write hackathon) ok, err := enf.CheckPermission( ownerToRemove.KeycloakID, createdHackathonID, middleware.Hackathon, - middleware.Read, + middleware.Write, ) Expect(err).NotTo(HaveOccurred()) Expect(ok).To(BeFalse()) @@ -3232,10 +3232,10 @@ var _ = Describe("HackathonService", func() { Expect(err).NotTo(HaveOccurred()) // Grant owner role so they have Read permission - _, err = dbClient.Hackathon.Update(). - Where(enthackathon.IDEQ(uuid.MustParse(hackathonID))). - AddOwners(user). - Save(context.Background()) + _, err = client.AddOwner(ctx, &msgs.AddOwnerRequest{ + HackathonId: hackathonID, + UserId: user.ID.String(), + }) Expect(err).NotTo(HaveOccurred()) // Also create a participant record for the admin user so admin token works @@ -3422,19 +3422,6 @@ var _ = Describe("HackathonService", func() { metadata.Pairs("authorization", "Bearer "+token), ) - // Create participant for this user - user, err := dbClient.User.Query(). - Where(entuser.KeycloakIDEQ(nonOwnerKeycloakID)). - Only(context.Background()) - Expect(err).NotTo(HaveOccurred()) - - _, err = dbClient.Participant.Create(). - SetHackathonID(uuid.MustParse(hackathonID)). - SetUserID(user.ID). - SetIsWaiting(false). - Save(context.Background()) - Expect(err).NotTo(HaveOccurred()) - _, err = client.SubmitAnswers(ctx, &msgs.SubmitAnswersRequest{ HackathonId: hackathonID, Answers: []*entities.Answer{ diff --git a/components/backend/internal/service/mappers.go b/components/backend/internal/service/mappers.go index 2d2ecb30..31ad08c9 100644 --- a/components/backend/internal/service/mappers.go +++ b/components/backend/internal/service/mappers.go @@ -529,17 +529,6 @@ func questionEntryFromEnt(q *ent.Question) *hackEnts.Question { } } -// answerEntryFromEnt maps a stored answer back onto the wire. -// -// The value is stored as a string whatever the question's type, so which arm of -// the `value` oneof it belongs in is a fact about the QUESTION, not the answer. -// Emitting text_value unconditionally meant a BOOL question read back as -// text_value "true" — and SubmitAnswers refuses a text answer to a bool -// question, so loading a form and saving it unchanged failed validation every -// time. That is exactly what an edit form does on every save. -// -// Requires the question edge; without it the type is unknowable and text is the -// only safe guess. func answerEntryFromEnt(a *ent.Answer) *hackEnts.Answer { entry := &hackEnts.Answer{ QuestionId: a.QuestionID.String(),