-
Notifications
You must be signed in to change notification settings - Fork 15
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: authenticated screens and pid receiving #130
Conversation
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
@berendsliedrecht still need to wait for the other PRs for it to work, but the code is complete. Please note that this is really an intermediate state as I will still be doing a lot of refactoring. So this is just to get the base working flow in. So feel free to comment on whatever, but I may push some things to upcoming PRs if that's easier |
Signed-off-by: Timo Glastra <[email protected]>
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.
Some comments, but due to the nature of the PR I assume it will be fixed in the future.
@@ -6,23 +6,14 @@ const variants = { | |||
development: { | |||
bundle: '.dev', | |||
name: ' (Dev)', | |||
trustedCertificates: [ |
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.
Ill likely encounter it later, but why was this removed?
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 just couldn't get it to work. The value was not availalbe in the app. So after spending an hour on it, I just moved it to constants file
const secureUnlock = useSecureUnlock() | ||
|
||
// Wallet is not configured yet. Redirect to onboarding | ||
if (secureUnlock.state === 'not-configured') { |
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.
enum would be nicer, can be done in the future as in improvement.
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.
same as with the href routing.
apps/funke/app/onboarding/index.tsx
Outdated
void SplashScreen.hideAsync() | ||
|
||
const onboarding = () => { | ||
secureUnlock.setup('123456', { |
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.
Same here re change to user input pin
export const FUNKE_WALLET_SEED_CREDENTIAL_RECORD_ID = 'FUNKE_WALLET_SEED_CREDENTIAL_RECORD_ID ' | ||
export const FUNKE_WALLET_INSTANCE_LONG_TERM_AES_KEY_ID = 'FUNKE_WALLET_INSTANCE_LONG_TERM_AES_KEY_ID' | ||
|
||
const TRUSTED_CERTIFICATES = ExpoConstants.expoConfig?.extra?.trustedCertificates as [string, ...string[]] | undefined | ||
// https://demo.pid-issuer.bundesdruckerei.de | ||
const bdrPidIssuerCertificate = `-----BEGIN CERTIFICATE----- |
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.
My preference would be to get it from the app config, but not too opinionated about it.
"compilerOptions": { | ||
"rootDir": "./", | ||
"paths": { | ||
"@/*": ["./apps/funke/*"] |
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 am not really fan of the @/
style. Can we just do @app/*
? Makes way more sense to me as @/
is quite vague.
return this.currentState | ||
} | ||
|
||
private static SD_JWT_VC_OFFER = |
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.
Are these static values permament or just for testing?
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.
permanent.
const { rawHash } = await argon2(pin, salt, { | ||
hashLength: 32, | ||
mode: 'argon2id', | ||
parallelism: 4, | ||
iterations: 1, | ||
memory: 21, | ||
iterations: 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.
Why did the values change? Did we not receive these form a spec?
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.
react-native-argon2 would crash. Did you test it with the previous values? This now takes around 1 sec on my phone. Wanted to do another check later to make it work also on android / older devices
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
Signed-off-by: Timo Glastra <[email protected]>
This PR adds support for receiving an SD-JWT VC from the Funke PID issuer, as well as adds the authenticated screens structure (wallet, onboarding, authentication).
It won't work until the following PRs have been released and merged: