-
Notifications
You must be signed in to change notification settings - Fork 19
Feature/pre authorization implementation #1002
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
base: master
Are you sure you want to change the base?
Feature/pre authorization implementation #1002
Conversation
| import SessionContext from '@/context/SessionContext'; | ||
|
|
||
| const PrivateRoute = ({ children }: { children?: React.ReactNode }): React.ReactElement => { | ||
| const PrivateRoute = ({ children }) => { |
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 would not remove typing
| </ul> | ||
| {error && <div className="text-lm-red dark:text-dm-red pt-2">{error}</div>} | ||
| </> | ||
| <div className='flex flex-row gap-4 justify-center'> |
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.
you removed quite a bit of styling/layout here, why?
| return <Navigate to={`/${window.location.search}`} replace />; | ||
| const user = queryParams.get('user'); | ||
| const state = queryParams.get('state'); | ||
| let filteredUser = null; |
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.
move this back into a function, but add early returns to make the logical flow easier to follow.
| <h1 className="pt-4 text-xl font-bold leading-tight tracking-tight text-dm-gray-900 md:text-2xl text-center dark:text-white"> | ||
| <LoginPageLayout heading={<Trans i18nKey="loginState.welcomeBackMessage" components={{ highlight: <span className="text-primary" /> }} />}> | ||
| <div className="relative p-8 bg-white dark:bg-dm-gray-800 rounded-lg shadow text-center space-y-6"> | ||
| <h1 className="text-xl font-bold dark:text-white"> |
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.
why more styling changes and how is this relevant to the pre-authorized_code flow implementation? For clarity, please make a separate PR with the styling, layout and code formatting changes.
| {error} | ||
| </p> | ||
| <Button onClick={() => window.location.reload()} variant="primary" additionalClassName="w-full"> | ||
| Try Again |
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.
move hardcoded strings to translations
| body: new URLSearchParams({ | ||
| "response_type": "code", "client_id": client_id, "redirect_uri": REDIRECT_URI, | ||
| "scope": "pid:sd_jwt_dc", "issuer_state": issuerState, | ||
| "code_challenge": "n4bQgYhMfWWaL-qgxVrQFaO_TxsrC4Is0V1sFbDwCgg", "code_challenge_method": "S256" |
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.
should the code_challenge be hardcoded here?
| }; | ||
|
|
||
| // 1. Pushed Authorization Request | ||
| const parData = await safeFetchJson(`${BASE_URL}/pushed-authorization-request`, { |
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.
parData 🏌️ is confusing. There is no shame in pushedAuthorizationRequestData. That reduces the need for the comment above to clarify the code.
| import PopupLayout from '../../components/Popups/PopupLayout'; | ||
| import Button from '../../components/Buttons/Button'; | ||
| import { H1 } from '../../components/Shared/Heading'; | ||
| const BASE_URL = import.meta.env.VITE_WALLET_BACKEND_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.
why does this file have so little formatting? Like, a new line after the imports would be nice.
| navigate(`?${params.toString()}`, { replace: true }); | ||
|
|
||
| const queryParams = new URLSearchParams(location.search); | ||
| const targetPath = queryParams.has('qrcodeurl') ? '/pre-auth' : '/'; |
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.
handling a credential offer (uri) does not require its own page. It can be done on the homepage where the credentials are listed. Also, introducing a query parameter qrcodeurl seems incorrect. The wallet can receive a credential offer or credential offer uri, right? In the full desktop flow there is no qrcode at play at all, so the naming is invalid.
| }, [api, keystore, navigate, preAuthCode, issuerState]); | ||
|
|
||
| return ( | ||
| <PopupLayout |
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.
If you handle everything in a popup, you don't need a separate /preauth page, but you can process the credential on any page, right?
| import Button from '../../components/Buttons/Button'; | ||
| import { H1 } from '../../components/Shared/Heading'; | ||
| const BASE_URL = import.meta.env.VITE_WALLET_BACKEND_URL; | ||
| const REDIRECT_URI = "http://localhost:3000"; |
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.
this will not work everywhere
| import { H1 } from '../../components/Shared/Heading'; | ||
| const BASE_URL = import.meta.env.VITE_WALLET_BACKEND_URL; | ||
| const REDIRECT_URI = "http://localhost:3000"; | ||
| const client_id = "CLIENT123"; |
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.
this will not work everywhere
| const BASE_URL = import.meta.env.VITE_WALLET_BACKEND_URL; | ||
| const REDIRECT_URI = "http://localhost:3000"; | ||
| const client_id = "CLIENT123"; | ||
| const b64UrlEncode = (data) => { |
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.
These and the next functions are generic and some may already exist in the project. They belong in a lib for better reusability.
| const qrcodeurl = searchParams.get('qrcodeurl') || ""; | ||
| const decodedOffer = decodeURIComponent(qrcodeurl); | ||
| const stateMatch = decodedOffer.match(/issuer_state["%22]*[:%3A]*\s*["%22]*([^"%&]+)/); | ||
| const codeMatch = decodedOffer.match(/pre-authorized_code["%22]*[:%3A]*\s*["%22]*([^"%&]+)/); | ||
| return { | ||
| preAuthCode: searchParams.get('pre_auth_code') || (codeMatch ? codeMatch[1] : "N/A"), | ||
| issuerState: stateMatch ? stateMatch[1] : null | ||
| }; |
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.
this code is difficult to read and looks brittle
| {isSuccess ? ( | ||
| <div className="animate-in fade-in zoom-in duration-500"> | ||
| <div className="flex items-center justify-center w-16 h-16 rounded-full bg-green-500/20 text-green-500 mb-4 mx-auto"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" strokeWidth={3} stroke="currentColor" className="w-8 h-8"> |
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 put raw svg content in an icon component instead of straight in a page layout.
| // 1. Pushed Authorization Request | ||
| const parData = await safeFetchJson(`${BASE_URL}/pushed-authorization-request`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, |
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.
a page component should not be concerned with http request content type headers. you could abstract these details away into a client.
| credentialIssuerIdentifier: BASE_URL, | ||
| batchId: Date.now(), | ||
| instanceId: 0, | ||
| claims: { |
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.
this looks unfinished
| }, [searchParams]); | ||
|
|
||
| useEffect(() => { | ||
| const handleClaim = async () => { |
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.
this function is very long, which is error prone. It also needs to be split up, because somewhere along the way you may need to prompt the user to enter a pin.
Summary
This PR implements pre authorization work flow in the wallet side. This feature allows wallet holders to get a credential from a QR offered by the issuer with out authenticating them self to the issuer.
The flow is implemented as per https://openid.net/specs/openid-4-verifiable-credential-issuance-1_0.html#name-pre-authorized-code-flow
Type of change
Related issues / tickets
Changes
Architecture decisions
Screenshots / recordings (if UI changes)
How to test
Checklist
Notes for reviewers