-
Notifications
You must be signed in to change notification settings - Fork 71
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
Tighten eligibility vpn check #2150
Conversation
data: z | ||
.object({ | ||
result: z.boolean(), | ||
confidence: z.enum(['low', 'medium', 'high']), |
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.
Taken from their documentation: https://dev.fingerprint.com/docs/smart-signals-reference#confidence
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.
Nit: could we extract this to an enum
, as it's used in across several places?
(as an example):
export enum FingerprintConfidenceLevels {...}
data: z | ||
.object({ | ||
result: z.boolean(), | ||
confidence: z.enum(['low', 'medium', 'high']), |
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.
Nit: we should add an unknown
value that we fallback to as we do elsewhere to ensure we cover unknowns. We should also add test coverage for this.
@@ -39,7 +39,9 @@ export class FingerprintApiService implements IIdentityApi { | |||
return { | |||
requestId, | |||
isAllowed: this.isAllowed(unsealedData), | |||
isVpn: unsealedData.products.vpn?.data?.result === true, | |||
isVpn: |
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.
Nit: maybe we could extract this to a new private isVpn(unsealedData: FingerprintUnsealedData): boolean
function?
data: z | ||
.object({ | ||
result: z.boolean(), | ||
confidence: z.enum(['low', 'medium', 'high']), |
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.
Nit: could we extract this to an enum
, as it's used in across several places?
(as an example):
export enum FingerprintConfidenceLevels {...}
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.
🚀
Summary
Follow up on #2103
Through testing we noticed that Fingerprint sometimes mistakenly marks a request as using a VPN but with a low confidence. This change tightens the condition that decides whether vpn is true or false by also checking the confidence score.
Changes