Skip to content

Commit 7686a55

Browse files
senthalansenthalan
authored
Merge pull request #2717 from ThaminduDilshan/thamindu-fed-fix
Add federated user consistency validation
2 parents 9f2e49e + fb7608d commit 7686a55

6 files changed

Lines changed: 552 additions & 0 deletions

File tree

‎backend/internal/flow/executor/oauth_executor.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,17 @@ func (o *oAuthExecutor) ProcessAuthFlowResponse(ctx *core.NodeContext,
256256
return errors.New("federated authentication failed")
257257
}
258258

259+
if basicResult == nil {
260+
logger.Error("authnProvider.AuthenticateUser returned nil result")
261+
return errors.New("OAuth authentication failed")
262+
}
263+
264+
if !validateFederatedIdentifierConsistency(ctx, basicResult) {
265+
execResp.Status = common.ExecFailure
266+
execResp.FailureReason = "Invalid federated user"
267+
return nil
268+
}
269+
259270
sub := basicResult.ExternalSub
260271

261272
if basicResult.IsAmbiguousUser {

‎backend/internal/flow/executor/oauth_executor_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,82 @@ func (suite *OAuthExecutorTestSuite) TestBuildAuthorizeFlow_IDPNotConfigured() {
193193
assert.Contains(suite.T(), err.Error(), "idpId is not configured")
194194
}
195195

196+
func (suite *OAuthExecutorTestSuite) TestProcessAuthFlowResponse_EmailMismatch_Fails() { //nolint:dupl
197+
ctx := &core.NodeContext{
198+
ExecutionID: "flow-123",
199+
FlowType: common.FlowTypeRegistration,
200+
UserInputs: map[string]string{
201+
"code": "auth_code_123",
202+
"email": "invited@example.com",
203+
},
204+
NodeProperties: map[string]interface{}{
205+
"idpId": "idp-123",
206+
},
207+
}
208+
209+
execResp := &common.ExecutorResponse{
210+
AdditionalData: make(map[string]string),
211+
RuntimeData: make(map[string]string),
212+
}
213+
214+
suite.mockAuthnProvider.On("AuthenticateUser", mock.Anything, mock.Anything, mock.Anything,
215+
mock.Anything, mock.Anything, mock.Anything).
216+
Return(authnprovidermgr.AuthUser{}, &authnprovidermgr.AuthnBasicResult{
217+
ExternalSub: "user-sub-123",
218+
ExternalClaims: map[string]interface{}{
219+
"sub": "user-sub-123",
220+
"email": "authenticated@example.com",
221+
},
222+
IsExistingUser: false,
223+
}, (*serviceerror.ServiceError)(nil))
224+
225+
err := suite.executor.ProcessAuthFlowResponse(ctx, execResp)
226+
227+
assert.NoError(suite.T(), err)
228+
assert.Equal(suite.T(), common.ExecFailure, execResp.Status)
229+
assert.Equal(suite.T(), "Invalid federated user", execResp.FailureReason)
230+
suite.mockAuthnProvider.AssertExpectations(suite.T())
231+
}
232+
233+
func (suite *OAuthExecutorTestSuite) TestProcessAuthFlowResponse_SubMismatch_Fails() { //nolint:dupl
234+
ctx := &core.NodeContext{
235+
ExecutionID: "flow-123",
236+
FlowType: common.FlowTypeRegistration,
237+
UserInputs: map[string]string{
238+
"code": "auth_code_123",
239+
},
240+
RuntimeData: map[string]string{
241+
"sub": "stored-sub-123",
242+
},
243+
NodeProperties: map[string]interface{}{
244+
"idpId": "idp-123",
245+
},
246+
}
247+
248+
execResp := &common.ExecutorResponse{
249+
AdditionalData: make(map[string]string),
250+
RuntimeData: make(map[string]string),
251+
}
252+
253+
suite.mockAuthnProvider.On("AuthenticateUser", mock.Anything, mock.Anything, mock.Anything,
254+
mock.Anything, mock.Anything, mock.Anything).
255+
Return(authnprovidermgr.AuthUser{}, &authnprovidermgr.AuthnBasicResult{
256+
ExternalSub: "authenticated-sub-456",
257+
ExternalClaims: map[string]interface{}{
258+
"sub": "authenticated-sub-456",
259+
"email": "user@example.com",
260+
},
261+
IsExistingUser: false,
262+
}, (*serviceerror.ServiceError)(nil))
263+
264+
err := suite.executor.ProcessAuthFlowResponse(ctx, execResp)
265+
266+
assert.NoError(suite.T(), err)
267+
assert.Equal(suite.T(), common.ExecFailure, execResp.Status)
268+
assert.Equal(suite.T(), "Invalid federated user", execResp.FailureReason)
269+
suite.mockAuthnProvider.AssertExpectations(suite.T())
270+
}
271+
196272
func (suite *OAuthExecutorTestSuite) TestBuildAuthorizeFlow_BuildURLClientError() {
197273
ctx := &core.NodeContext{
198274
ExecutionID: "flow-123",

‎backend/internal/flow/executor/oidc_auth_executor.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,12 @@ func (o *oidcAuthExecutor) ProcessAuthFlowResponse(ctx *core.NodeContext,
192192
}
193193
}
194194

195+
if !validateFederatedIdentifierConsistency(ctx, basicResult) {
196+
execResp.Status = common.ExecFailure
197+
execResp.FailureReason = "Invalid federated user"
198+
return nil
199+
}
200+
195201
sub := basicResult.ExternalSub
196202

197203
if basicResult.IsAmbiguousUser {

‎backend/internal/flow/executor/oidc_auth_executor_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,82 @@ func (suite *OIDCAuthExecutorTestSuite) TestProcessAuthFlowResponse_InvalidNonce
220220
suite.mockAuthnProvider.AssertExpectations(suite.T())
221221
}
222222

223+
func (suite *OIDCAuthExecutorTestSuite) TestProcessAuthFlowResponse_EmailMismatch_Fails() { //nolint:dupl
224+
ctx := &core.NodeContext{
225+
ExecutionID: "flow-123",
226+
FlowType: common.FlowTypeRegistration,
227+
UserInputs: map[string]string{
228+
"code": "auth_code_123",
229+
"email": "invited@example.com",
230+
},
231+
NodeProperties: map[string]interface{}{
232+
"idpId": "idp-123",
233+
},
234+
}
235+
236+
execResp := &common.ExecutorResponse{
237+
AdditionalData: make(map[string]string),
238+
RuntimeData: make(map[string]string),
239+
}
240+
241+
suite.mockAuthnProvider.On("AuthenticateUser", mock.Anything, mock.Anything, mock.Anything,
242+
mock.Anything, mock.Anything, mock.Anything).
243+
Return(authnprovidermgr.AuthUser{}, &authnprovidermgr.AuthnBasicResult{
244+
ExternalSub: "user-sub-123",
245+
ExternalClaims: map[string]interface{}{
246+
"sub": "user-sub-123",
247+
"email": "authenticated@example.com",
248+
},
249+
IsExistingUser: false,
250+
}, (*serviceerror.ServiceError)(nil))
251+
252+
err := suite.executor.ProcessAuthFlowResponse(ctx, execResp)
253+
254+
assert.NoError(suite.T(), err)
255+
assert.Equal(suite.T(), common.ExecFailure, execResp.Status)
256+
assert.Equal(suite.T(), "Invalid federated user", execResp.FailureReason)
257+
suite.mockAuthnProvider.AssertExpectations(suite.T())
258+
}
259+
260+
func (suite *OIDCAuthExecutorTestSuite) TestProcessAuthFlowResponse_SubMismatch_Fails() { //nolint:dupl
261+
ctx := &core.NodeContext{
262+
ExecutionID: "flow-123",
263+
FlowType: common.FlowTypeRegistration,
264+
UserInputs: map[string]string{
265+
"code": "auth_code_123",
266+
},
267+
RuntimeData: map[string]string{
268+
"sub": "stored-sub-123",
269+
},
270+
NodeProperties: map[string]interface{}{
271+
"idpId": "idp-123",
272+
},
273+
}
274+
275+
execResp := &common.ExecutorResponse{
276+
AdditionalData: make(map[string]string),
277+
RuntimeData: make(map[string]string),
278+
}
279+
280+
suite.mockAuthnProvider.On("AuthenticateUser", mock.Anything, mock.Anything, mock.Anything,
281+
mock.Anything, mock.Anything, mock.Anything).
282+
Return(authnprovidermgr.AuthUser{}, &authnprovidermgr.AuthnBasicResult{
283+
ExternalSub: "authenticated-sub-456",
284+
ExternalClaims: map[string]interface{}{
285+
"sub": "authenticated-sub-456",
286+
"email": "user@example.com",
287+
},
288+
IsExistingUser: false,
289+
}, (*serviceerror.ServiceError)(nil))
290+
291+
err := suite.executor.ProcessAuthFlowResponse(ctx, execResp)
292+
293+
assert.NoError(suite.T(), err)
294+
assert.Equal(suite.T(), common.ExecFailure, execResp.Status)
295+
assert.Equal(suite.T(), "Invalid federated user", execResp.FailureReason)
296+
suite.mockAuthnProvider.AssertExpectations(suite.T())
297+
}
298+
223299
func (suite *OIDCAuthExecutorTestSuite) TestProcessAuthFlowResponse_ProviderClientError() { //nolint:dupl
224300
ctx := &core.NodeContext{
225301
ExecutionID: "flow-123",

‎backend/internal/flow/executor/utils.go‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,11 @@ import (
2424
"fmt"
2525

2626
authncm "github.com/thunder-id/thunderid/internal/authn/common"
27+
authnprovidermgr "github.com/thunder-id/thunderid/internal/authnprovider/manager"
2728
"github.com/thunder-id/thunderid/internal/entityprovider"
2829
"github.com/thunder-id/thunderid/internal/flow/common"
2930
"github.com/thunder-id/thunderid/internal/flow/core"
31+
systemutils "github.com/thunder-id/thunderid/internal/system/utils"
3032
)
3133

3234
// getAuthnServiceName returns the authn service name for an executor.
@@ -105,3 +107,55 @@ func isCrossOUProvisioningAllowed(ctx *core.NodeContext) bool {
105107
}
106108
return false
107109
}
110+
111+
// validateFederatedIdentifierConsistency checks if the federated identifiers from the authentication result
112+
// are consistent with any existing identifiers in the context (runtime data, user inputs, authenticated
113+
// user attributes).
114+
func validateFederatedIdentifierConsistency(ctx *core.NodeContext,
115+
basicResult *authnprovidermgr.AuthnBasicResult) bool {
116+
if basicResult == nil {
117+
return true
118+
}
119+
120+
federatedIdentifiers := map[string]string{
121+
userAttributeSub: basicResult.ExternalSub,
122+
}
123+
if email, ok := basicResult.ExternalClaims[userAttributeEmail]; ok {
124+
federatedIdentifiers[userAttributeEmail] = systemutils.ConvertInterfaceValueToString(email)
125+
}
126+
127+
// TODO: Refine this well-known-key comparison when IDP-to-local attribute mapping is supported
128+
fedIdfConsistencyKeys := []string{userAttributeEmail, userAttributeSub}
129+
for _, key := range fedIdfConsistencyKeys {
130+
federatedValue := federatedIdentifiers[key]
131+
if federatedValue == "" {
132+
continue
133+
}
134+
135+
if value, ok := ctx.RuntimeData[key]; ok && value != "" && value != federatedValue {
136+
return false
137+
}
138+
if value, ok := ctx.UserInputs[key]; ok && value != "" && value != federatedValue {
139+
return false
140+
}
141+
if value := getAuthenticatedIdentifierValue(ctx, key); value != "" && value != federatedValue {
142+
return false
143+
}
144+
}
145+
146+
return true
147+
}
148+
149+
// getAuthenticatedIdentifierValue retrieves the value of a specific identifier key from the
150+
// authenticated user's attributes in the context.
151+
func getAuthenticatedIdentifierValue(ctx *core.NodeContext, key string) string {
152+
if ctx.AuthenticatedUser.Attributes == nil {
153+
return ""
154+
}
155+
value, ok := ctx.AuthenticatedUser.Attributes[key]
156+
if !ok {
157+
return ""
158+
}
159+
160+
return systemutils.ConvertInterfaceValueToString(value)
161+
}

0 commit comments

Comments
 (0)