Skip to content

Commit 0de2ff9

Browse files
sangalo20sangalo20
committed
Refactor: address remaining technical debt with senior architectural patterns
Resolves #30 by implementing a gRPC Interceptor. Resolves #27 by using a Config struct to avoid constructor over-injection. Resolves #21 by leveraging Go's encoding/json UnmarshalJSON for token parsing.
1 parent 2630acf commit 0de2ff9

7 files changed

Lines changed: 157 additions & 60 deletions

File tree

‎nexus-broker/cmd/nexus-broker/main.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,21 @@ func main() {
9999
redirectPath = "/auth/callback"
100100
}
101101
providersHandler := handlers.NewProvidersHandler(store)
102-
consentHandler := handlers.NewConsentHandler(db, baseURL, redirectPath, stateKey, cachingClient)
103-
callbackHandler := handlers.NewCallbackHandler(db, baseURL, redirectPath, encryptionKey, stateKey, cachingClient)
102+
consentHandler := handlers.NewConsentHandler(handlers.ConsentHandlerConfig{
103+
DB: db,
104+
BaseURL: baseURL,
105+
RedirectPath: redirectPath,
106+
StateKey: stateKey,
107+
HTTPClient: cachingClient,
108+
})
109+
callbackHandler := handlers.NewCallbackHandler(handlers.CallbackHandlerConfig{
110+
DB: db,
111+
BaseURL: baseURL,
112+
RedirectPath: redirectPath,
113+
EncryptionKey: encryptionKey,
114+
StateKey: stateKey,
115+
HTTPClient: cachingClient,
116+
})
104117

105118
// Setup routes
106119
router := srv.Router()

‎nexus-broker/pkg/handlers/callback.go‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,18 @@ type CallbackHandler struct {
4040
metricTokenGet *prometheus.CounterVec
4141
}
4242

43+
// CallbackHandlerConfig holds the dependencies for CallbackHandler
44+
type CallbackHandlerConfig struct {
45+
DB *sqlx.DB
46+
BaseURL string
47+
RedirectPath string
48+
EncryptionKey []byte
49+
StateKey []byte
50+
HTTPClient *http.Client
51+
}
52+
4353
// NewCallbackHandler creates a new callback handler
44-
func NewCallbackHandler(db *sqlx.DB, baseURL, redirectPath string, encryptionKey, stateKey []byte, httpClient *http.Client) *CallbackHandler {
54+
func NewCallbackHandler(cfg CallbackHandlerConfig) *CallbackHandler {
4555
success := prometheus.NewCounter(prometheus.CounterOpts{
4656
Name: "oauth_token_exchanges_total",
4757
Help: "Total OAuth token exchanges",
@@ -76,12 +86,12 @@ func NewCallbackHandler(db *sqlx.DB, baseURL, redirectPath string, encryptionKey
7686
}
7787

7888
return &CallbackHandler{
79-
db: db,
80-
baseURL: baseURL,
81-
redirectPath: redirectPath,
82-
encryptionKey: encryptionKey,
83-
stateKey: stateKey,
84-
httpClient: httpClient,
89+
db: cfg.DB,
90+
baseURL: cfg.BaseURL,
91+
redirectPath: cfg.RedirectPath,
92+
encryptionKey: cfg.EncryptionKey,
93+
stateKey: cfg.StateKey,
94+
httpClient: cfg.HTTPClient,
8595
metricExchangeSuccess: success,
8696
metricExchangeError: failure,
8797
histogramExchangeDur: hist,

‎nexus-broker/pkg/handlers/callback_test.go‎

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,14 @@ func TestRefresh_StaticKeyProvider(t *testing.T) {
2626
defer db.Close()
2727

2828
sqlxDB := sqlx.NewDb(db, "sqlmock")
29-
handler := NewCallbackHandler(sqlxDB, "http://localhost:8080", "/auth/callback", []byte("test-key"), []byte("test-key"), http.DefaultClient)
29+
handler := NewCallbackHandler(CallbackHandlerConfig{
30+
DB: sqlxDB,
31+
BaseURL: "http://localhost:8080",
32+
RedirectPath: "/auth/callback",
33+
EncryptionKey: []byte("test-key"),
34+
StateKey: []byte("test-key"),
35+
HTTPClient: http.DefaultClient,
36+
})
3037

3138
// Mock the initial query to find the connection
3239

@@ -66,7 +73,14 @@ func TestRefresh_OAuth2Provider(t *testing.T) {
6673
}))
6774
defer mockProviderServer.Close()
6875

69-
handler := NewCallbackHandler(sqlxDB, "http://localhost:8080", "/auth/callback", []byte("01234567890123456789012345678901"), []byte("01234567890123456789012345678901"), mockProviderServer.Client())
76+
handler := NewCallbackHandler(CallbackHandlerConfig{
77+
DB: sqlxDB,
78+
BaseURL: "http://localhost:8080",
79+
RedirectPath: "/auth/callback",
80+
EncryptionKey: []byte("01234567890123456789012345678901"),
81+
StateKey: []byte("01234567890123456789012345678901"),
82+
HTTPClient: mockProviderServer.Client(),
83+
})
7084

7185
// Mock the initial query to find the connection
7286

@@ -117,7 +131,14 @@ func TestGetCaptureSchema(t *testing.T) {
117131
sqlxDB := sqlx.NewDb(db, "sqlmock")
118132
// Use a real key for signing/verifying state
119133
stateKey := []byte("01234567890123456789012345678901")
120-
handler := NewCallbackHandler(sqlxDB, "http://localhost:8080", "/auth/callback", nil, stateKey, http.DefaultClient)
134+
handler := NewCallbackHandler(CallbackHandlerConfig{
135+
DB: sqlxDB,
136+
BaseURL: "http://localhost:8080",
137+
RedirectPath: "/auth/callback",
138+
EncryptionKey: nil,
139+
StateKey: stateKey,
140+
HTTPClient: http.DefaultClient,
141+
})
121142

122143
providerID := uuid.New()
123144
stateData := auth.StateData{
@@ -170,7 +191,14 @@ func TestSaveCredential_ValidState(t *testing.T) {
170191
sqlxDB := sqlx.NewDb(db, "sqlmock")
171192
stateKey := []byte("01234567890123456789012345678901")
172193
encryptionKey := []byte("01234567890123456789012345678901")
173-
handler := NewCallbackHandler(sqlxDB, "http://localhost:8080", "/auth/callback", encryptionKey, stateKey, http.DefaultClient)
194+
handler := NewCallbackHandler(CallbackHandlerConfig{
195+
DB: sqlxDB,
196+
BaseURL: "http://localhost:8080",
197+
RedirectPath: "/auth/callback",
198+
EncryptionKey: encryptionKey,
199+
StateKey: stateKey,
200+
HTTPClient: http.DefaultClient,
201+
})
174202

175203
connectionID := uuid.New()
176204
stateData := auth.StateData{
@@ -224,7 +252,14 @@ func TestSaveCredential_ValidState(t *testing.T) {
224252
}
225253

226254
func TestSaveCredential_InvalidState(t *testing.T) {
227-
handler := NewCallbackHandler(nil, "http://localhost:8080", "/auth/callback", nil, []byte("test-key"), http.DefaultClient)
255+
handler := NewCallbackHandler(CallbackHandlerConfig{
256+
DB: nil,
257+
BaseURL: "http://localhost:8080",
258+
RedirectPath: "/auth/callback",
259+
EncryptionKey: nil,
260+
StateKey: []byte("test-key"),
261+
HTTPClient: http.DefaultClient,
262+
})
228263

229264
creds := map[string]interface{}{"api_key": "test-key"}
230265
body := map[string]interface{}{
@@ -245,7 +280,14 @@ func TestSaveCredential_InvalidState(t *testing.T) {
245280
}
246281

247282
func TestSaveCredential_InvalidJSON(t *testing.T) {
248-
handler := NewCallbackHandler(nil, "http://localhost:8080", "/auth/callback", nil, nil, http.DefaultClient)
283+
handler := NewCallbackHandler(CallbackHandlerConfig{
284+
DB: nil,
285+
BaseURL: "http://localhost:8080",
286+
RedirectPath: "/auth/callback",
287+
EncryptionKey: nil,
288+
StateKey: nil,
289+
HTTPClient: http.DefaultClient,
290+
})
249291

250292
req, err := http.NewRequest("POST", "/auth/capture-credential", bytes.NewBuffer([]byte("not-json")))
251293
assert.NoError(t, err)

‎nexus-broker/pkg/handlers/consent.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,17 @@ type ConsentHandler struct {
4040
consentsOpenID prometheus.Counter
4141
}
4242

43+
// ConsentHandlerConfig holds the dependencies for ConsentHandler
44+
type ConsentHandlerConfig struct {
45+
DB *sqlx.DB
46+
BaseURL string
47+
RedirectPath string
48+
StateKey []byte
49+
HTTPClient *http.Client
50+
}
51+
4352
// NewConsentHandler creates a new consent handler
44-
func NewConsentHandler(db *sqlx.DB, baseURL, redirectPath string, stateKey []byte, httpClient *http.Client) *ConsentHandler {
53+
func NewConsentHandler(cfg ConsentHandlerConfig) *ConsentHandler {
4554
metric := prometheus.NewCounter(prometheus.CounterOpts{
4655
Name: "oauth_consents_created_total",
4756
Help: "Total OAuth consents created",
@@ -61,11 +70,11 @@ func NewConsentHandler(db *sqlx.DB, baseURL, redirectPath string, stateKey []byt
6170
}
6271

6372
return &ConsentHandler{
64-
db: db,
65-
baseURL: baseURL,
66-
redirectPath: redirectPath,
67-
stateKey: stateKey,
68-
httpClient: httpClient,
73+
db: cfg.DB,
74+
baseURL: cfg.BaseURL,
75+
redirectPath: cfg.RedirectPath,
76+
stateKey: cfg.StateKey,
77+
httpClient: cfg.HTTPClient,
6978
consentsMetric: metric,
7079
consentsOpenID: metricOpenID,
7180
}

‎nexus-broker/pkg/handlers/consent_test.go‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,13 @@ func TestGetSpec_OAuth2(t *testing.T) {
3232
defer mockProviderServer.Close()
3333

3434
// Pass the test server's client to the handler
35-
handler := NewConsentHandler(sqlxDB, "http://localhost:8080", "/auth/callback", []byte("test-key"), mockProviderServer.Client())
35+
handler := NewConsentHandler(ConsentHandlerConfig{
36+
DB: sqlxDB,
37+
BaseURL: "http://localhost:8080",
38+
RedirectPath: "/auth/callback",
39+
StateKey: []byte("test-key"),
40+
HTTPClient: mockProviderServer.Client(),
41+
})
3642

3743
paramsJSON := []byte(`{"access_type": "offline", "prompt": "consent"}`)
3844

@@ -81,7 +87,13 @@ func TestGetSpec_StaticKey(t *testing.T) {
8187

8288
sqlxDB := sqlx.NewDb(db, "sqlmock")
8389
// For static key tests, we can pass a default client as no external calls are made.
84-
handler := NewConsentHandler(sqlxDB, "http://localhost:8080", "/auth/callback", []byte("test-key"), http.DefaultClient)
90+
handler := NewConsentHandler(ConsentHandlerConfig{
91+
DB: sqlxDB,
92+
BaseURL: "http://localhost:8080",
93+
RedirectPath: "/auth/callback",
94+
StateKey: []byte("test-key"),
95+
HTTPClient: http.DefaultClient,
96+
})
8597

8698
rows := sqlmock.NewRows([]string{"id", "name", "auth_type", "auth_url", "client_id", "scopes", "params"}).
8799
AddRow("b1b1b1b1-b1b1-b1b1-b1b1-b1b1b1b1b1b1", "Test API", "api_key", nil, nil, "{}", []byte("{}"))
@@ -141,7 +153,13 @@ func TestGetSpec_MixedOAuth2_Discovery(t *testing.T) {
141153
defer ts.Close()
142154

143155
// Handler under test
144-
handler := NewConsentHandler(sqlxDB, "http://localhost:8080", "/auth/callback", []byte("test-key"), ts.Client())
156+
handler := NewConsentHandler(ConsentHandlerConfig{
157+
DB: sqlxDB,
158+
BaseURL: "http://localhost:8080",
159+
RedirectPath: "/auth/callback",
160+
StateKey: []byte("test-key"),
161+
HTTPClient: ts.Client(),
162+
})
145163

146164
// Define the configured (legacy) auth URL
147165
configuredAuthURL := ts.URL + "/oauth/v2/authorize"

‎nexus-gateway/pkg/grpc/server_grpc.go‎

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,26 @@ func NewService(handler *usecase.Handler) *Service {
3131
return &Service{usecaseHandler: handler}
3232
}
3333

34-
func mapUsecaseError(err error, msg string) error {
35-
switch {
36-
case errors.Is(err, usecase.ErrProviderNotFound):
37-
return status.Errorf(codes.NotFound, "%s: %v", msg, err)
38-
case errors.Is(err, usecase.ErrInvalidState):
39-
return status.Errorf(codes.InvalidArgument, "%s: %v", msg, err)
40-
case errors.Is(err, usecase.ErrProviderAmbiguous):
41-
return status.Errorf(codes.FailedPrecondition, "%s: %v", msg, err)
42-
case errors.Is(err, usecase.ErrBrokerUnavailable):
43-
return status.Errorf(codes.Unavailable, "%s: %v", msg, err)
44-
default:
45-
return status.Errorf(codes.Internal, "%s: %v", msg, err)
34+
func usecaseErrorInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
35+
resp, err := handler(ctx, req)
36+
if err != nil {
37+
switch {
38+
case errors.Is(err, usecase.ErrProviderNotFound):
39+
return nil, status.Errorf(codes.NotFound, "%v", err)
40+
case errors.Is(err, usecase.ErrInvalidState):
41+
return nil, status.Errorf(codes.InvalidArgument, "%v", err)
42+
case errors.Is(err, usecase.ErrProviderAmbiguous):
43+
return nil, status.Errorf(codes.FailedPrecondition, "%v", err)
44+
case errors.Is(err, usecase.ErrBrokerUnavailable):
45+
return nil, status.Errorf(codes.Unavailable, "%v", err)
46+
default:
47+
if _, ok := status.FromError(err); ok {
48+
return nil, err
49+
}
50+
return nil, status.Errorf(codes.Internal, "%v", err)
51+
}
4652
}
53+
return resp, nil
4754
}
4855

4956
// RequestConnection implements NexusServiceServer.RequestConnection.
@@ -60,7 +67,7 @@ func (s *Service) RequestConnection(ctx context.Context, req *nexuspb.RequestCon
6067
Action: req.GetAction(),
6168
})
6269
if err != nil {
63-
return nil, mapUsecaseError(err, "request connection failed")
70+
return nil, err
6471
}
6572
return &nexuspb.RequestConnectionResponse{
6673
AuthUrl: out.AuthURL,
@@ -79,7 +86,7 @@ func (s *Service) CheckConnection(ctx context.Context, req *nexuspb.CheckConnect
7986
}
8087
statusStr, err := s.usecaseHandler.CheckConnectionCore(ctx, req.GetConnectionId())
8188
if err != nil {
82-
return nil, mapUsecaseError(err, "check connection failed")
89+
return nil, err
8390
}
8491
return &nexuspb.CheckConnectionResponse{Status: statusStr}, nil
8592
}
@@ -92,7 +99,7 @@ func (s *Service) GetToken(ctx context.Context, req *nexuspb.GetTokenRequest) (*
9299
data, code, err := s.usecaseHandler.GetTokenCore(ctx, req.GetConnectionId())
93100
if err != nil {
94101
_ = code // keep the HTTP status for potential mapping if needed later
95-
return nil, mapUsecaseError(err, "get token failed")
102+
return nil, err
96103
}
97104
st, err := structpb.NewStruct(data)
98105
if err != nil {
@@ -109,7 +116,7 @@ func (s *Service) RefreshConnection(ctx context.Context, req *nexuspb.RefreshCon
109116
data, code, err := s.usecaseHandler.RefreshConnectionCore(ctx, req.GetConnectionId())
110117
if err != nil {
111118
_ = code // unused
112-
return nil, mapUsecaseError(err, "refresh connection failed")
119+
return nil, err
113120
}
114121
st, err := structpb.NewStruct(data)
115122
if err != nil {
@@ -144,7 +151,7 @@ func NewServer(opts Options) (*Server, error) {
144151
opts.HTTPAddress = ":8090"
145152
}
146153
service := NewService(opts.Handler)
147-
grpcSrv := grpc.NewServer()
154+
grpcSrv := grpc.NewServer(grpc.UnaryInterceptor(usecaseErrorInterceptor))
148155
nexuspb.RegisterNexusServiceServer(grpcSrv, service)
149156
return &Server{
150157
grpcAddress: opts.GRPCAddress,

‎nexus-sdk/client.go‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -147,9 +147,9 @@ func (c *Client) GetToken(ctx context.Context, connectionID string) (*TokenRespo
147147
resp, err := c.do(ctx, http.MethodGet, c.GatewayBaseURL+"/v1/token/"+url.PathEscape(connectionID), nil, nil)
148148
if err != nil { return nil, err }
149149
defer resp.Body.Close()
150-
var raw map[string]any
151-
if err := json.NewDecoder(resp.Body).Decode(&raw); err != nil { return nil, err }
152-
return parseTokenResponse(raw), nil
150+
var out TokenResponse
151+
if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { return nil, err }
152+
return &out, nil
153153
}
154154

155155
// RefreshConnection calls the Gateway to force a token refresh.
@@ -158,24 +158,22 @@ func (c *Client) RefreshConnection(ctx context.Context, connectionID string) (*T
158158
resp, err := c.do(ctx, http.MethodPost, c.GatewayBaseURL+"/v1/refresh/"+url.PathEscape(connectionID), nil, nil)
159159
if err != nil { return nil, err }
160160
defer resp.Body.Close()
161-
var raw map[string]any
162-
if err := json.NewDecoder(resp.Body).Decode(&raw); err != nil { return nil, err }
163-
return parseTokenResponse(raw), nil
161+
var out TokenResponse
162+
if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { return nil, err }
163+
return &out, nil
164164
}
165165

166-
func parseTokenResponse(raw map[string]any) *TokenResponse {
167-
tr := &TokenResponse{Raw: raw}
168-
if v, ok := raw["access_token"].(string); ok { tr.AccessToken = v }
169-
if v, ok := raw["token_type"].(string); ok { tr.TokenType = &v }
170-
if v, ok := raw["expires_in"].(float64); ok { vv := int64(v); tr.ExpiresIn = &vv }
171-
if v, ok := raw["expires_at"]; ok { tr.ExpiresAt = v }
172-
if v, ok := raw["scope"].(string); ok { tr.Scope = &v }
173-
if v, ok := raw["id_token"].(string); ok { tr.IDToken = &v }
174-
if v, ok := raw["refresh_token"].(string); ok { tr.RefreshToken = &v }
175-
if v, ok := raw["provider"].(string); ok { tr.Provider = &v }
176-
if v, ok := raw["strategy"].(map[string]interface{}); ok { tr.Strategy = v }
177-
if v, ok := raw["credentials"].(map[string]interface{}); ok { tr.Credentials = v }
178-
return tr
166+
func (t *TokenResponse) UnmarshalJSON(data []byte) error {
167+
type Alias TokenResponse
168+
var aux Alias
169+
if err := json.Unmarshal(data, &aux); err != nil {
170+
return err
171+
}
172+
*t = TokenResponse(aux)
173+
if err := json.Unmarshal(data, &t.Raw); err != nil {
174+
return err
175+
}
176+
return nil
179177
}
180178

181179
// RefreshViaBroker calls RefreshConnection (Gateway Proxy).

0 commit comments

Comments
 (0)