Skip to content
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.

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

feat(IT Wallet): [SIW-1972] Remote presentation same device #6689

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

RiccardoMolinari95
Copy link
Collaborator

@RiccardoMolinari95 RiccardoMolinari95 commented Feb 5, 2025

Short description

This PR introduces the ability to start the remote presentation flow through a deep link in a same-device scenario.

List of changes proposed in this pull request

  • Updated useItwLinkingOptions to include linking options for remote deep link
  • Added ItwRemoteRequestValidationScreen, which receives the payload and validates it. If the payload is invalid, an error screen, for deeplink, will be displayed (TODO). If valid, it sends the start event to the remote presentation machine
  • Modified decodeItwRemoteBarcode to validate that the URL starts with https://continua.io.pagopa.it/itw/auth, checks the content of the QR code against the ItwRemoteRequestPayload type, and finally sends the URL

How to test

Use the following link as a deep link and to generate a QR code:
https://continua.io.pagopa.it/itw/auth?client_id=abc123xy&request_uri=https%3A%2F%2Fexample.com%2Fcallback&state=hyqizm592

Copy link
Contributor

github-actions bot commented Feb 5, 2025

PR Title Validation for conventional commit type

All good! PR title follows the conventional commit type.

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Jira Pull Request Link

This Pull Request refers to Jira issues:

Comment on lines 89 to 155

describe("test ITW_REMOTE barcode type", () => {
it("should return O.some on valid QRCode content", () => {
const value =
"https://continua.io.pagopa.it/itw/auth?client_id=abc123xy&request_uri=https%3A%2F%2Fexample.com%2Fcallback&state=hyqizm592";

const output = decodeIOBarcode(value);

expect(output).toStrictEqual(
O.some({
type: "ITW_REMOTE",
itwRemoteRequestPayload: {
clientId: "abc123xy",
requestUri: "https://example.com/callback",
state: "hyqizm592",
requestUriMethod: "GET"
}
})
);
});

it("should decode request_uri_method if provided", () => {
const value =
"https://continua.io.pagopa.it/itw/auth?client_id=abc123xy&request_uri=https%3A%2F%2Fexample.com%2Fcallback&state=hyqizm592&request_uri_method=POST";

const output = decodeIOBarcode(value);

expect(output).toStrictEqual(
O.some({
type: "ITW_REMOTE",
itwRemoteRequestPayload: {
clientId: "abc123xy",
requestUri: "https://example.com/callback",
state: "hyqizm592",
requestUriMethod: "POST"
}
})
);
});

it("should return O.none if request_uri is missing", () => {
const value =
"https://continua.io.pagopa.it/itw/auth?client_id=abc123xy&state=hyqizm592";

const output = decodeIOBarcode(value);

expect(output).toStrictEqual(O.none);
});

it("should return O.none if client_id is missing", () => {
const value =
"https://continua.io.pagopa.it/itw/auth?request_uri=https%3A%2F%2Fexample.com%2Fcallback&state=hyqizm592";

const output = decodeIOBarcode(value);

expect(output).toStrictEqual(O.none);
});

it("should return O.none if both client_id and request_uri are missing", () => {
const value =
"https://continua.io.pagopa.it/itw/auth?state=hyqizm592&request_uri_method=POST";

const output = decodeIOBarcode(value);

expect(output).toStrictEqual(O.none);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need tests for barcode decoding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. I forgot to reintroduce this file. Thanks

Comment on lines 158 to 163
navigation.navigate(ITW_REMOTE_ROUTES.MAIN, {
screen: ITW_REMOTE_ROUTES.CLAIMS_DISCLOSURE,
params: {
itwRemoteRequestPayload: barcode.itwRemoteRequestPayload
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are already extracting data from the url, we could pass the payload direclty to the screen. What do you think?

Comment on lines 121 to 130
O.chain(params =>
pipe(
ItwRemoteRequestPayload.decode({
client_id: params.get("client_id"),
request_uri: params.get("request_uri"),
state: params.get("state")
}),
E.fold(
() => O.none,
() => O.some({ type: "ITW_REMOTE", authUrl: data })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified, also request_uri_method is missing.
I'd suggest to return the payload since we have already parsed it.

Suggested change
O.chain(params =>
pipe(
ItwRemoteRequestPayload.decode({
client_id: params.get("client_id"),
request_uri: params.get("request_uri"),
state: params.get("state")
}),
E.fold(
() => O.none,
() => O.some({ type: "ITW_REMOTE", authUrl: data })
O.chainEitherK(params =>
ItwRemoteRequestPayload.decode({
client_id: params.get("client_id"),
request_uri: params.get("request_uri"),
state: params.get("state"),
request_uri_method: params.get("request_uri_method")
})
),
O.map(payload => ({ type: "ITW_REMOTE", payload }))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point of view, and you are right; once we parse the payload, it would be better to use it directly. However, we decided to pass the URL for consistency with the deep link case.
This way, we can handle the payloads coming from both the QR code and the deep link differently when navigating to ItwRemoteRequestValidationScreen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we agreed together about it, but:

  • by design, we need to show an error message whenever we have an invalid QR code
  • a double parse is not optimal and is more prone to errors.
    For these reasons I think we should pass through a normal navigation

Copy link
Collaborator Author

@RiccardoMolinari95 RiccardoMolinari95 Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the corrections in edc51dd

@@ -32,6 +32,12 @@ export const createRemoteActionsImplementation = (
});
},

navigateToClaimsDisclosureScreen: () => {
navigation.navigate(ITW_REMOTE_ROUTES.MAIN, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous screen is temporary, we can safely replace it.

Suggested change
navigation.navigate(ITW_REMOTE_ROUTES.MAIN, {
navigation.replace(ITW_REMOTE_ROUTES.MAIN, {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants