8000 [#IOCIT-118] make prevalidate errors more readable by arcogabbo · Pull Request #119 · pagopa/io-spid-commons · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[#IOCIT-118] make prevalidate errors more readable #119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 30, 2022
Merged

[#IOCIT-118] make prevalidate errors more readable #119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30917e9
[#IOCIT-118] made prevalidate errors more readable
arcogabbo Sep 23, 2022
76ca1f1
linter fix
arcogabbo Sep 23, 2022
b6d1608
variable renaming and some explanation
arcogabbo Sep 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
10000
Diff view
31 changes: 22 additions & 9 deletions src/utils/__tests__/saml.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ describe("preValidateResponse", () => {
await asyncExpectOnCallback(mockCallback, expectedError);
expect(mockEventTracker).toBeCalledWith({
data: {
message: expectedError.message
message: expectedError.message,
requestId: expect.any(String),
idpIssuer: expect.any(String)
},
name: expectedGenericEventName,
type: "ERROR"
Expand All @@ -151,7 +153,7 @@ describe("preValidateResponse", () => {
mockTestIdpIssuer: true
};
getPreValidateResponse(strictValidationOption, mockEventTracker)(
{ ...samlConfig, acceptedClockSkewMs: 0 },
{ ...samlConfig, acceptedClockSkewMs: 2000 },
mockBody,
mockRedisCacheProvider,
undefined,
Expand All @@ -162,7 +164,9 @@ describe("preValidateResponse", () => {
await asyncExpectOnCallback(mockCallback, expectedError);
expect(mockEventTracker).toBeCalledWith({
data: {
message: expectedError.message
message: expectedError.message,
requestId: expect.any(String),
idpIssuer: expect.any(String)
},
name: expectedGenericEventName,
type: "ERROR"
Expand Down Expand Up @@ -192,7 +196,9 @@ describe("preValidateResponse", () => {
await asyncExpectOnCallback(mockCallback, expectedError);
expect(mockEventTracker).toBeCalledWith({
data: {
message: expectedError.message
message: expectedError.message,
requestId: expect.any(String),
idpIssuer: expect.any(String)
},
name: expectedGenericEventName,
type: "ERROR"
Expand Down Expand Up @@ -234,7 +240,8 @@ describe("preValidateResponse", () => {
data: {
idpIssuer: expectedError.idpIssuer,
message: expectedError.message,
numberOfTransforms: String(expectedError.numberOfTransforms)
numberOfTransforms: String(expectedError.numberOfTransforms),
requestId: expect.any(String)
},
name: expectedTransformEventName,
type: "ERROR"
Expand Down Expand Up @@ -269,7 +276,8 @@ describe("preValidateResponse", () => {
data: {
idpIssuer: expectedError.idpIssuer,
message: expectedError.message,
numberOfTransforms: String(expectedError.numberOfTransforms)
numberOfTransforms: String(expectedError.numberOfTransforms),
requestId: expect.any(String)
},
name: expectedTransformEventName,
type: "ERROR"
Expand Down Expand Up @@ -317,7 +325,8 @@ describe("preValidateResponse", () => {
expect(mockEventTracker).toBeCalledWith({
data: {
idpIssuer: mockTestIdpIssuer,
message: expect.any(String)
message: expect.any(String),
requestId: expect.any(String)
},
name: expectedSignatureErrorName,
type: "INFO"
Expand Down Expand Up @@ -422,7 +431,9 @@ describe("preValidateResponse", () => {
await asyncExpectOnCallback(mockCallback, expectedError);
expect(mockEventTracker).toBeCalledWith({
data: {
message: expectedError.message
message: expectedError.message,
requestId: expect.any(String),
idpIssuer: expect.any(String)
},
name: expectedGenericEventName,
type: "ERROR"
Expand Down Expand Up @@ -486,7 +497,9 @@ describe("preValidateResponse", () => {
await asyncExpectOnCallback(mockCallback, expectedError);
expect(mockEventTracker).toBeCalledWith({
data: {
message: expectedError.message
message: expectedError.message,
requestId: expect.any(String),
idpIssuer: expect.any(String)
},
name: expectedGenericEventName,
type: "ERROR"
Expand Down
209 changes: 133 additions & 76 deletions src/utils/saml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,59 @@ export {

export type SamlAttributeT = keyof typeof SPID_USER_ATTRIBUTES;

export interface ISAMLError extends Error {
readonly idpIssuer: string;
readonly requestId: string;
}

interface IBaseOutput {
readonly InResponseTo: NonEmptyString;
readonly Assertion: Element;
readonly IssueInstant: Date;
readonly Response: Element;
readonly AssertionIssueInstant: Date;
}

interface ISamlCacheType {
readonly RequestXML: string;
readonly createdAt: Date;
readonly idpIssuer: string;
}

type IRequestAndResponseStep = IBaseOutput & {
readonly SAMLRequestCache: ISamlCacheType;
};

type ISAMLRequest = IRequestAndResponseStep & { readonly Request: Document };

type IIssueInstant = ISAMLRequest & {
readonly RequestIssueInstant: Date;
readonly RequestAuthnRequest: Element;
};

type IIssueInstantWithAuthnContextCR = IIssueInstant & {
readonly RequestAuthnContextClassRef: NonEmptyString;
};

interface ITransformValidation {
readonly idpIssuer: string;
readonly message: string;
readonly numberOfTransforms: number;
}

export const errorToSamlError = (
e: Error,
info?: { readonly requestId?: string; readonly idpIssuer?: string }
): ISAMLError => ({
// the nullish coalescing operator and optional chaining will cover all cases here
// eslint-disable-next-line sonarjs/no-duplicate-string
idpIssuer: info?.idpIssuer ?? "not available",
message: e.message,
name: e.name,
requestId: info?.requestId ?? "not available",
stack: e.stack
});

// eslint-disable-next-line max-lines-per-function
export const getPreValidateResponse = (
strictValidationOptions?: StrictResponseValidationOptions,
Expand All @@ -68,7 +121,7 @@ export const getPreValidateResponse = (
doneCb,
callback
// eslint-disable-next-line sonarjs/cognitive-complexity
): Promise<void | E.Left<void> | E.Right<void>> => {
): void => {
const maybeDoc = getXmlFromSamlResponse(body);

if (O.isNone(maybeDoc)) {
Expand All @@ -81,72 +134,73 @@ export const getPreValidateResponse = (
"Response"
);

const maybeIdpIssuer = getSamlIssuer(doc);

const hasStrictValidation = pipe(
O.fromNullable(strictValidationOptions),
O.chain(_ =>
O.chain(validationOptions =>
pipe(
getSamlIssuer(doc),
O.chainNullableK(issuer => _[issuer])
maybeIdpIssuer,
O.chainNullableK(issuer => validationOptions[issuer])
)
),
O.getOrElse(() => false)
);

interface IBaseOutput {
readonly InResponseTo: NonEmptyString;
readonly Assertion: Element;
readonly IssueInstant: Date;
readonly Response: Element;
readonly AssertionIssueInstant: Date;
}

interface ISamlCacheType {
readonly RequestXML: string;
readonly createdAt: Date;
readonly idpIssuer: string;
}

type IRequestAndResponseStep = IBaseOutput & {
readonly SAMLRequestCache: ISamlCacheType;
};

type ISAMLRequest = IRequestAndResponseStep & { readonly Request: Document };
const idpIssuer: string = pipe(
maybeIdpIssuer,
O.getOrElse(() => "not available")
);

type IIssueInstant = ISAMLRequest & {
readonly RequestIssueInstant: Date;
readonly RequestAuthnRequest: Element;
};
// here we are partially validating the response just to obtain a requestId (InResponseTo) before doing any more step.
// this is needed if we want to have to log the requestId at any step further
const errorOrPartiallyValidatedResponse: E.Either<
Error,
{ readonly InResponseTo: NonEmptyString; readonly Response: Element }
> = pipe(
responsesCollection,
E.fromPredicate(
coll => coll.length < 2,
_ => new Error("SAML Response must have only one Response element")
5DA8
),
E.map(coll => coll.item(0)),
// this check is bound to the previous one, because we can receive no Response based on the official validator guidelines
E.chain(
E.fromNullable(new Error("Missing Response element inside SAML Response"))
),
E.chain(Response =>
pipe(
NonEmptyString.decode(Response.getAttribute("InResponseTo")),
E.mapLeft(
() => new Error("InResponseTo must contain a non empty string")
),
E.map(InResponseTo => ({ InResponseTo, Response }))
)
)
);

type IIssueInstantWithAuthnContextCR = IIssueInstant & {
readonly RequestAuthnContextClassRef: NonEmptyString;
};
const requestId: string = pipe(
errorOrPartiallyValidatedResponse,
E.map(({ InResponseTo }) => InResponseTo),
E.getOrElse(() => "not available")
);

interface ITransformValidation {
readonly idpIssuer: string;
readonly message: string;
readonly numberOfTransforms: number;
}
// this method make idpIssuer and requestId available on all the scope
// this is useful while logging errors
const toSamlError = (message: string): ISAMLError =>
errorToSamlError(new Error(message), { idpIssuer, requestId });

const responseElementValidationStep: TaskEither<
Error,
IBaseOutput
> = TE.fromEither(
pipe(
responsesCollection,
E.fromPredicate(
_ => _.length < 2,
_ => new Error("SAML Response must have only one Response element")
),
E.map(_ => _.item(0)),
E.chain(Response =>
E.fromOption(
() => new Error("Missing Reponse element inside SAML Response")
)(O.fromNullable(Response))
),
E.chain(Response =>
errorOrPartiallyValidatedResponse,
E.chainW(({ Response, InResponseTo }) =>
pipe(
mainAttributeValidation(Response, samlConfig.acceptedClockSkewMs),
E.map(IssueInstant => ({
InResponseTo,
IssueInstant,
Response
}))
Expand Down Expand Up @@ -263,15 +317,6 @@ export const getPreValidateResponse = (
E.map(assertion => ({ ..._, Assertion: assertion }))
)
),
E.chain(_ =>
pipe(
NonEmptyString.decode(_.Response.getAttribute("InResponseTo")),
E.mapLeft(
() => new Error("InResponseTo must contain a non empty string")
),
E.map(inResponseTo => ({ ..._, InResponseTo: inResponseTo }))
)
),
E.chain(_ =>
pipe(
mainAttributeValidation(_.Assertion, samlConfig.acceptedClockSkewMs),
Expand Down Expand Up @@ -564,26 +609,37 @@ export const getPreValidateResponse = (
TE.map(() => _)
);

/* LOGGING INFOS:
having the idpIssuer and requestId as data here we leverage multiple advantages:
1. we can query based on the idp and display graphs about errors/usage
2. we know what idp is causing the error
3. having the requestId it's possible to analyze further the problem encountered
*/
const validationFailure = (error: Error | ITransformValidation): void => {
if (eventHandler) {
// eslint-disable-next-line @typescript-eslint/no-unused-expressions
TransformError.is(error)
? eventHandler({
data: {
idpIssuer: error.idpIssuer,
message: error.message,
numberOfTransforms: String(error.numberOfTransforms)
},
name: "spid.error.transformOccurenceOverflow",
type: "ERROR"
})
: eventHandler({
data: {
message: error.message
},
name: "spid.error.generic",
type: "ERROR"
});
if (TransformError.is(error)) {
eventHandler({
data: {
idpIssuer: error.idpIssuer,
message: error.message,
numberOfTransforms: String(error.numberOfTransforms),
requestId
},
name: "spid.error.transformOccurenceOverflow",
type: "ERROR"
});
} else {
const samlError: ISAMLError = toSamlError(error.message);
eventHandler({
data: {
idpIssuer: samlError.idpIssuer,
message: samlError.message,
requestId: samlError.requestId
},
name: "spid.error.generic",
type: "ERROR"
});
}
}
return callback(E.toError(error.message));
};
Expand All @@ -604,7 +660,8 @@ export const getPreValidateResponse = (
eventHandler({
data: {
idpIssuer: _.SAMLRequestCache.idpIssuer,
message: "Missing Request signature"
message: "Missing Request signature",
requestId: _.InResponseTo
},
name: "spid.error.signature",
type: "INFO"
Expand All @@ -613,7 +670,7 @@ export const getPreValidateResponse = (
return callback(null, true, _.InResponseTo);
};

return pipe(
pipe(
responseElementValidationStep,
TE.chain(returnRequestAndResponseStep),
TE.chain(parseSAMLRequestStep),
Expand Down
0