Skip to content

Commit

Permalink
feat(#94): remove admin header check
Browse files Browse the repository at this point in the history
the room service does not handle highly sensitive data, so we can remove
this crutch here even though the idp does not yet enforce 2FA.
  • Loading branch information
Jumpy-Squirrel committed Nov 9, 2024
1 parent 940f297 commit 7799bce
Show file tree
Hide file tree
Showing 4 changed files with 1 addition and 126 deletions.
13 changes: 0 additions & 13 deletions internal/application/middleware/security.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,6 @@ func fromApiTokenHeader(r *http.Request) string {
return r.Header.Get(common.ApiKeyHeader)
}

// TODO Remove after legacy system was replaced with 2FA
// See reference https://github.com/eurofurence/reg-room-service/issues/57
func storeAdminRequestHeaderIfAvailable(ctx context.Context, r *http.Request) context.Context {
adminHeader := r.Header.Get(adminRequestHeader)

if adminHeader == "" {
return ctx
}

return context.WithValue(ctx, common.CtxKeyAdminHeader{}, adminHeader)
}

// --- validating the individual pieces ---

// important - if any of these return an error, you must abort processing via "return" and log the error message
Expand Down Expand Up @@ -263,7 +251,6 @@ func CheckRequestAuthorization(conf *config.SecurityConfig) func(http.Handler) h
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()

ctx = storeAdminRequestHeaderIfAvailable(ctx, r)
apiTokenHeaderValue := fromApiTokenHeader(r)
authHeaderValue := fromAuthHeader(r)
idTokenCookieValue := parseAuthCookie(r, conf.Oidc.IDTokenCookieName)
Expand Down
42 changes: 0 additions & 42 deletions internal/application/middleware/security_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,45 +275,3 @@ func TestCookiesExpiredJwt(t *testing.T) {
require.Nil(t, ctx.Value(common.CtxKeyClaims{}))
tstRequireNoAuthServiceCall(t)
}

// TODO Remove after legacy system was replaced with 2FA
// See reference https://github.com/eurofurence/reg-room-service/issues/57
func TestStoreAdminHeaderInContext(t *testing.T) {
docs.Description("stores the header value for legacy system admin calls")

tests := []struct {
name string
shouldStore bool
}{
{
name: "should not store value in context",
shouldStore: false,
},
{
name: "should not store value in context",
shouldStore: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()

r, err := http.NewRequest(http.MethodGet, "http://test.local", nil)
require.NoError(t, err)
if tt.shouldStore {
r.Header.Add(adminRequestHeader, "available")
}

ctx = storeAdminRequestHeaderIfAvailable(ctx, r)
val, ok := ctx.Value(common.CtxKeyAdminHeader{}).(string)
if tt.shouldStore {
require.True(t, ok)
require.NotEmpty(t, val)
} else {
require.False(t, ok)
require.Empty(t, val)
}
})
}
}
14 changes: 1 addition & 13 deletions internal/service/rbac/rbac.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func NewValidator(ctx context.Context) (Validator, error) {
manager.isUser = true

for _, group := range claims.Groups {
if group == conf.Security.Oidc.AdminGroup && hasValidAdminHeader(ctx) {
if group == conf.Security.Oidc.AdminGroup {
manager.isUser = false
manager.isAdmin = true
break
Expand All @@ -90,15 +90,3 @@ func NewValidator(ctx context.Context) (Validator, error) {

return manager, nil
}

// TODO remove after 2FA is available
// See reference https://github.com/eurofurence/reg-payment-service/issues/57
func hasValidAdminHeader(ctx context.Context) bool {
adminHeaderValue, ok := ctx.Value(common.CtxKeyAdminHeader{}).(string)
if !ok {
return false
}

// legacy system implementation requires check against constant value "available"
return adminHeaderValue == "available"
}
58 changes: 0 additions & 58 deletions internal/service/rbac/rbac_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ func TestNewRBACValidator(t *testing.T) {
inputJWT string
inputAPIKey string
inputClaims *common.AllClaims
includeAdminHeader bool
customAdminHeaderValue string
}

Expand Down Expand Up @@ -72,67 +71,13 @@ func TestNewRBACValidator(t *testing.T) {
EMail: "[email protected]",
},
},
includeAdminHeader: true,
},
expected: expected{
isAdmin: true,
subject: "123456",
roles: []string{"admin", "test"},
},
},
// TODO remove test case after 2FA is available
// See reference https://github.com/eurofurence/reg-payment-service/issues/57
{
name: "Should not create manager with admin role when no admin header is set",
args: args{
inputJWT: "valid",
inputAPIKey: "",
inputClaims: &common.AllClaims{
RegisteredClaims: jwt.RegisteredClaims{
Subject: "123456",
},
CustomClaims: common.CustomClaims{
Groups: []string{"admin", "test"},
Name: "Peter",
EMail: "[email protected]",
},
},
includeAdminHeader: false,
},
expected: expected{
isAdmin: false,
isRegisteredUser: true,
subject: "123456",
roles: []string{"admin", "test"},
},
},
// TODO remove test case after 2FA is available
// See reference https://github.com/eurofurence/reg-payment-service/issues/57
{
name: "Should not create manager with admin role when no valid admin header is set",
args: args{
inputJWT: "valid",
inputAPIKey: "",
inputClaims: &common.AllClaims{
RegisteredClaims: jwt.RegisteredClaims{
Subject: "123456",
},
CustomClaims: common.CustomClaims{
Groups: []string{"admin", "test"},
Name: "Peter",
EMail: "[email protected]",
},
},
includeAdminHeader: true,
customAdminHeaderValue: "test-12345",
},
expected: expected{
isAdmin: false,
isRegisteredUser: true,
subject: "123456",
roles: []string{"admin", "test"},
},
},
{
name: "Should create manager with registered user role",
args: args{
Expand Down Expand Up @@ -218,9 +163,6 @@ func TestNewRBACValidator(t *testing.T) {

if tt.args.inputClaims != nil {
ctx = context.WithValue(ctx, common.CtxKeyClaims{}, tt.args.inputClaims)
if tt.args.includeAdminHeader {
ctx = context.WithValue(ctx, common.CtxKeyAdminHeader{}, coalesce(tt.args.customAdminHeaderValue, "available"))
}
}

mgr, err := NewValidator(ctx)
Expand Down

0 comments on commit 7799bce

Please sign in to comment.