-
Notifications
You must be signed in to change notification settings - Fork 90
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(backend): redirect to webpage when querying payment pointer in browser #3298
base: main
Are you sure you want to change the base?
feat(backend): redirect to webpage when querying payment pointer in browser #3298
Conversation
✅ Deploy Preview for brilliant-pasca-3e80ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -52,6 +52,16 @@ export async function getWalletAddress( | |||
) | |||
} | |||
|
|||
// Redirect to the wallet if the client accepts HTML | |||
if (ctx.request.header['accept']?.includes('text/html')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the env should be optional, we should include checking for that in this condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
rafiki/packages/backend/src/open_payments/wallet_address/routes.ts
Lines 56 to 64 in 4005273
if ( | |
deps.config.walletAddressRedirectHtmlPage && | |
ctx.request.header['accept']?.includes('text/html') | |
) { | |
ctx.set( | |
'Location', | |
`${deps.config.walletAddressRedirectHtmlPage}/${ctx.request.header['host']}${ctx.request.url}` | |
) | |
ctx.status = 302 |
packages/backend/src/config/app.ts
Outdated
@@ -192,6 +192,7 @@ export const Config = { | |||
'MAX_OUTGOING_PAYMENT_RETRY_ATTEMPTS', | |||
5 | |||
), | |||
ilpWalletUrl: envString('ILP_WALLET_URL', 'https://interledger.app/me'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like WALLET_ADDRESS_REDIRECT_HTML_PAGE? Should be optional as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
rafiki/packages/backend/src/config/app.ts
Line 195 in 4005273
walletAddressRedirectHtmlPage: process.env.WALLET_ADDRESS_REDIRECT_HTML_PAGE, |
) { | ||
ctx.set( | ||
'Location', | ||
`${deps.config.walletAddressRedirectHtmlPage}/${ctx.request.header['host']}${ctx.request.url}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be good to have as a separate middleware for the wallet address route (and should be the first one registered, since we should redirect as soon as possible, without needing to do other operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a separate middleware redirectIfBrowserAcceptsHtml
which I put below getWalletAddressUrlFromPath
in order to get the processed wallet address instead of redundantly composing it
rafiki/packages/backend/src/app.ts
Lines 674 to 677 in 8c95179
router.get( | |
WALLET_ADDRESS_PATH, | |
getWalletAddressUrlFromPath, | |
redirectIfBrowserAcceptsHtml, |
) { | ||
ctx.set( | ||
'Location', | ||
`${deps.config.walletAddressRedirectHtmlPage}/${ctx.request.header['host']}${ctx.request.url}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`${deps.config.walletAddressRedirectHtmlPage}/${ctx.request.header['host']}${ctx.request.url}` | |
`${deps.config.walletAddressRedirectHtmlPage}/${ctx.params.walletAddressPath}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using walletAddressPath and moved it to the new middleware, but still needed some processing to remove http://
rafiki/packages/backend/src/open_payments/wallet_address/middleware.ts
Lines 126 to 135 in 8c95179
const walletAddressPath = ctx.walletAddressUrl.replace('https://', '') | |
if ( | |
config.walletAddressRedirectHtmlPage && | |
ctx.request.header['accept']?.includes('text/html') | |
) { | |
ctx.set( | |
'Location', | |
`${config.walletAddressRedirectHtmlPage}/${walletAddressPath}` | |
) |
) { | ||
ctx.set( | ||
'Location', | ||
`${config.walletAddressRedirectHtmlPage}/${walletAddressPath}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure if the integrator enters a walletAddressRedirectHtmlPage
with a trailing slash that we don't add the extra /
. Something that would happen for sure 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
rafiki/packages/backend/src/open_payments/wallet_address/middleware.ts
Lines 132 to 135 in 4abf0ff
const redirectHtmlPage = config.walletAddressRedirectHtmlPage.replace( | |
/\/+$/, | |
'' | |
) |
Changes proposed in this pull request
Accept
withtext/html
) on the payment pointer url, we redirect to wallet websiteContext
fixes #3268
Checklist
fixes #number
user-docs
label (if necessary)