fix(web,rpc): handle an API or web server built without merge requests or OIDC keys
The commits banner says the branch moved when there is no merge request service to summarise the push, and RotateOIDCKey answers unimplemented when the API has no OIDC key service, instead of panicking.
- commit
- 558568c800dd87c44f5a22bdd22b4b279937aa7d
- author
- brice · · 2026-10-19 19:44 UTC
- committer
- brice · · 2026-10-19 20:01 UTC
- parent
- 5344be5
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
4 files · +35 -1
M internal/adapters/rpc/server.go +6 -1
| @@ -4,6 +4,7 @@ package rpc | |||
| 4 | 4 | ||
| 5 | 5 | import ( | |
| 6 | 6 | "context" | |
| 7 | + | "errors" | |
| 7 | 8 | "slices" | |
| 8 | 9 | ||
| 9 | 10 | "connectrpc.com/connect" | |
| @@ -39,8 +40,12 @@ func (s *serverService) GetSigningKey(context.Context, *connect.Request[pb.GetSi | |||
| 39 | 40 | } | |
| 40 | 41 | ||
| 41 | 42 | // RotateOIDCKey replaces the key that signs the ID tokens of CI jobs. Only the | |
| 42 | - | // owner may; failed_precondition when the server issues no ID tokens. | |
| 43 | + | // owner may; failed_precondition when the server issues no ID tokens. An API | |
| 44 | + | // built without an OIDC key service answers unimplemented. | |
| 43 | 45 | func (s *serverService) RotateOIDCKey(ctx context.Context, _ *connect.Request[pb.RotateOIDCKeyRequest]) (*connect.Response[pb.RotateOIDCKeyResponse], error) { | |
| 46 | + | if s.oidc == nil { | |
| 47 | + | return nil, connect.NewError(connect.CodeUnimplemented, errors.New("this API serves no OIDC key service")) | |
| 48 | + | } | |
| 44 | 49 | p := caller(ctx) | |
| 45 | 50 | r, err := s.oidc.RotateKey(ctx, p) | |
| 46 | 51 | if err != nil { | |
| @@ -4,6 +4,7 @@ package rpc | |||||
| 4 | 4 | ||||
| 5 | import ( | 5 | import ( | ||
| 6 | "context" | 6 | "context" | ||
| 7 | + | "errors" | |||
| 7 | "slices" | 8 | "slices" | ||
| 8 | 9 | ||||
| 9 | "connectrpc.com/connect" | 10 | "connectrpc.com/connect" | ||
| @@ -39,8 +40,12 @@ func (s *serverService) GetSigningKey(context.Context, *connect.Request[pb.GetSi | |||||
| 39 | } | 40 | } | ||
| 40 | 41 | ||||
| 41 | // RotateOIDCKey replaces the key that signs the ID tokens of CI jobs. Only the | 42 | // RotateOIDCKey replaces the key that signs the ID tokens of CI jobs. Only the | ||
| 42 | - | // owner may; failed_precondition when the server issues no ID tokens. | 43 | + | // owner may; failed_precondition when the server issues no ID tokens. An API |
| 44 | + | // built without an OIDC key service answers unimplemented. | |||
| 43 | func (s *serverService) RotateOIDCKey(ctx context.Context, _ *connect.Request[pb.RotateOIDCKeyRequest]) (*connect.Response[pb.RotateOIDCKeyResponse], error) { | 45 | func (s *serverService) RotateOIDCKey(ctx context.Context, _ *connect.Request[pb.RotateOIDCKeyRequest]) (*connect.Response[pb.RotateOIDCKeyResponse], error) { | ||
| 46 | + | if s.oidc == nil { | |||
| 47 | + | return nil, connect.NewError(connect.CodeUnimplemented, errors.New("this API serves no OIDC key service")) | |||
| 48 | + | } | |||
| 44 | p := caller(ctx) | 49 | p := caller(ctx) | ||
| 45 | r, err := s.oidc.RotateKey(ctx, p) | 50 | r, err := s.oidc.RotateKey(ctx, p) | ||
| 46 | if err != nil { | 51 | if err != nil { | ||
M internal/adapters/rpc/server_test.go +11 -0
| @@ -49,6 +49,17 @@ func TestServerServiceGetSigningKey(t *testing.T) { | |||
| 49 | 49 | assert.Equal(t, "ermite@example.com ssh-ed25519 AAAA", resp.Msg.GetAllowedSigners()) | |
| 50 | 50 | } | |
| 51 | 51 | ||
| 52 | + | // An API built without an OIDC key service answers unimplemented instead of | |
| 53 | + | // panicking, for the owner and for anyone else. | |
| 54 | + | func TestRotateOIDCKeyWithoutService(t *testing.T) { | |
| 55 | + | t.Parallel() | |
| 56 | + | for _, p := range []domain.Principal{domain.Owner(), {}} { | |
| 57 | + | ctx := domain.ContextWithPrincipal(context.Background(), p) | |
| 58 | + | _, err := (&serverService{}).RotateOIDCKey(ctx, connect.NewRequest(&pb.RotateOIDCKeyRequest{})) | |
| 59 | + | assert.Equal(t, connect.CodeUnimplemented, connect.CodeOf(err)) | |
| 60 | + | } | |
| 61 | + | } | |
| 62 | + | ||
| 52 | 63 | // GetVersion lists "passkeys" only when the API is built with passkeys. | |
| 53 | 64 | func TestAPIListsPasskeysFeatureWhenAvailable(t *testing.T) { | |
| 54 | 65 | t.Parallel() | |
| @@ -49,6 +49,17 @@ func TestServerServiceGetSigningKey(t *testing.T) { | |||||
| 49 | assert.Equal(t, "ermite@example.com ssh-ed25519 AAAA", resp.Msg.GetAllowedSigners()) | 49 | assert.Equal(t, "ermite@example.com ssh-ed25519 AAAA", resp.Msg.GetAllowedSigners()) | ||
| 50 | } | 50 | } | ||
| 51 | 51 | ||||
| 52 | + | // An API built without an OIDC key service answers unimplemented instead of | |||
| 53 | + | // panicking, for the owner and for anyone else. | |||
| 54 | + | func TestRotateOIDCKeyWithoutService(t *testing.T) { | |||
| 55 | + | t.Parallel() | |||
| 56 | + | for _, p := range []domain.Principal{domain.Owner(), {}} { | |||
| 57 | + | ctx := domain.ContextWithPrincipal(context.Background(), p) | |||
| 58 | + | _, err := (&serverService{}).RotateOIDCKey(ctx, connect.NewRequest(&pb.RotateOIDCKeyRequest{})) | |||
| 59 | + | assert.Equal(t, connect.CodeUnimplemented, connect.CodeOf(err)) | |||
| 60 | + | } | |||
| 61 | + | } | |||
| 62 | + | ||||
| 52 | // GetVersion lists "passkeys" only when the API is built with passkeys. | 63 | // GetVersion lists "passkeys" only when the API is built with passkeys. | ||
| 53 | func TestAPIListsPasskeysFeatureWhenAvailable(t *testing.T) { | 64 | func TestAPIListsPasskeysFeatureWhenAvailable(t *testing.T) { | ||
| 54 | t.Parallel() | 65 | t.Parallel() | ||
M internal/adapters/web/events_test.go +13 -0
| @@ -465,3 +465,16 @@ func TestEventsIssuesViewsAreDeniedLikeAMissingRepo(t *testing.T) { | |||
| 465 | 465 | resp, _ := open(t, ts, "topic=repo:proj&view=issues", nil) | |
| 466 | 466 | assert.Equal(t, http.StatusOK, resp.StatusCode) | |
| 467 | 467 | } | |
| 468 | + | ||
| 469 | + | // Without the merge request service the commits page still gets a banner when | |
| 470 | + | // its branch moves; it only cannot count the new commits. | |
| 471 | + | func TestCommitsBannerWithoutMergeRequests(t *testing.T) { | |
| 472 | + | t.Parallel() | |
| 473 | + | srv := newTestServer(t, Config{Repos: mocks.NewRepoService(t), Browse: mocks.NewBrowseService(t)}) | |
| 474 | + | old, tip := strings.Repeat("a", 40), strings.Repeat("b", 40) | |
| 475 | + | ls := &liveStream{s: srv, repo: "x", view: "commits", ref: "main", head: old} | |
| 476 | + | got, err := ls.banner(t.Context(), domain.PushEvent{Updates: []domain.RefUpdate{{Ref: "refs/heads/main", Old: old, New: tip}}}) | |
| 477 | + | require.NoError(t, err) | |
| 478 | + | require.Len(t, got, 1) | |
| 479 | + | assert.Contains(t, got[0].html, "the branch moved — reload") | |
| 480 | + | } | |
| @@ -465,3 +465,16 @@ func TestEventsIssuesViewsAreDeniedLikeAMissingRepo(t *testing.T) { | |||||
| 465 | resp, _ := open(t, ts, "topic=repo:proj&view=issues", nil) | 465 | resp, _ := open(t, ts, "topic=repo:proj&view=issues", nil) | ||
| 466 | assert.Equal(t, http.StatusOK, resp.StatusCode) | 466 | assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| 467 | } | 467 | } | ||
| 468 | + | ||||
| 469 | + | // Without the merge request service the commits page still gets a banner when | |||
| 470 | + | // its branch moves; it only cannot count the new commits. | |||
| 471 | + | func TestCommitsBannerWithoutMergeRequests(t *testing.T) { | |||
| 472 | + | t.Parallel() | |||
| 473 | + | srv := newTestServer(t, Config{Repos: mocks.NewRepoService(t), Browse: mocks.NewBrowseService(t)}) | |||
| 474 | + | old, tip := strings.Repeat("a", 40), strings.Repeat("b", 40) | |||
| 475 | + | ls := &liveStream{s: srv, repo: "x", view: "commits", ref: "main", head: old} | |||
| 476 | + | got, err := ls.banner(t.Context(), domain.PushEvent{Updates: []domain.RefUpdate{{Ref: "refs/heads/main", Old: old, New: tip}}}) | |||
| 477 | + | require.NoError(t, err) | |||
| 478 | + | require.Len(t, got, 1) | |||
| 479 | + | assert.Contains(t, got[0].html, "the branch moved — reload") | |||
| 480 | + | } | |||
M internal/adapters/web/fragments.go +5 -0
| @@ -390,6 +390,11 @@ func (l *liveStream) bannerText(ctx context.Context, tip string) (string, error) | |||
| 390 | 390 | if tip == l.head { | |
| 391 | 391 | return "", nil | |
| 392 | 392 | } | |
| 393 | + | if l.s.cfg.MergeRequests == nil { | |
| 394 | + | // Without the merge request service there is no history summary: the | |
| 395 | + | // banner only says the branch moved. | |
| 396 | + | return "the branch moved — reload", nil | |
| 397 | + | } | |
| 393 | 398 | name, err := domain.ParseRepoName(l.repo) | |
| 394 | 399 | if err != nil { | |
| 395 | 400 | return "", err | |
| @@ -390,6 +390,11 @@ func (l *liveStream) bannerText(ctx context.Context, tip string) (string, error) | |||||
| 390 | if tip == l.head { | 390 | if tip == l.head { | ||
| 391 | return "", nil | 391 | return "", nil | ||
| 392 | } | 392 | } | ||
| 393 | + | if l.s.cfg.MergeRequests == nil { | |||
| 394 | + | // Without the merge request service there is no history summary: the | |||
| 395 | + | // banner only says the branch moved. | |||
| 396 | + | return "the branch moved — reload", nil | |||
| 397 | + | } | |||
| 393 | name, err := domain.ParseRepoName(l.repo) | 398 | name, err := domain.ParseRepoName(l.repo) | ||
| 394 | if err != nil { | 399 | if err != nil { | ||
| 395 | return "", err | 400 | return "", err | ||