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

Fix broken cookie auth and allow app-based TOTP #5

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

macintoshhelper
Copy link
Contributor

Fixes issues #2 #3 and an issue around rate limiting when Cookie and Tsid headers are missing.

The issue in cookie parsing in #2 appears to be because of global fetch changes in Node.js, or a change in the HTTP response

Requires documentation on how to retrieve and set the new environment variables:

  • FIGMA_COOKIE
  • FIGMA_TSID

@NiklasBuchfink
Copy link
Member

@macintoshhelper Thank you for your contribution. We are happy to merge your changes.

Can you please add the documentation for the two environment variables?

@NiklasBuchfink
Copy link
Member

NiklasBuchfink commented Jul 16, 2024

@macintoshhelper Main was out of date with the released npm version because I didn't have write access to the repo, but I created an npm version with my local version (however that was possible).
You can see the changes here: #6

It contains the recent_user_data that I referred to in our Discord chat (https://discord.com/channels/897438559458430986/1261213783360274472/1261338567901319178)
You can extract it from the cookie in the same way as you did with the authn token.

@NiklasBuchfink
Copy link
Member

@macintoshhelper I checked out your branch and tried to run the commands like the following:

  1. node bin/cli.js get-cookies -> get the FIGMA_COOKIE='***'; FIGMA_TSID=***
  2. node bin/cli.js auth --cookie '***' --tsid '***' -> I always get the TypeError: Cannot read properties of undefined (reading 'phone_number').

What do I have to change so that it is working?

@NiklasBuchfink
Copy link
Member

@macintoshhelper Ok, I think it worked. I didn't have 2-factor authentication until now. Now an authentication token has been returned.

@macintoshhelper
Copy link
Contributor Author

Performing node bin/cli.js get-cookies doesn't appear to be necessary, the auth command will fetch it automatically if it isn't passed as an argument, but I can add documentation to the README incase the cookie/TSID need to be saved for the future.

@NiklasBuchfink
Copy link
Member

@macintoshhelper Here are the three possible responsive for a valid account.

  1. Results without 2FA:
  error: false,
  status: 200,
  meta: {
    id: '1234567890',
    img_url: 'https://s3.figma.com/ba43e35d3f321b31a12ced90',
    handle: 'therealjohndoe',
    email: '[email protected]',
    name: 'John Doe'
  },
  i18n: null
}
  1. With SMS 2FA:
  error: true,
  status: 400,
  message: 'Please enter your authenticator code',
  i18n: {
    id: 'auth.error.enter-two-factor-code',
    fallback_text: 'Please enter your authenticator code',
    params: {}
  },
  reason: { missing: 'two_factor', sms: true, phone_number: '7957' }
}
  1. With Auth App 2FA:
  error: true,
  status: 400,
  message: 'Please enter your authenticator code',
  i18n: {
    id: 'auth.error.enter-two-factor-code',
    fallback_text: 'Please enter your authenticator code',
    params: {}
  },
  reason: { missing: 'two_factor', sms: false }
}

Can you please update this PR one last time to:

  1. just request and return authn-token, if status 200 is returned (same request with empty totp_key)
  2. prompt for the SMS code if status === 400 && reason?.sms === true -> this check will remove the Type error
  3. prompt for the TOTP code if status === 400 && reason?.sms === false

Then we have all cases covered 👍

Copy link
Member

@NiklasBuchfink NiklasBuchfink left a comment

Choose a reason for hiding this comment

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

These are my last remarks before merging and releasing

src/auth-helper.js Outdated Show resolved Hide resolved
src/auth-helper.js Outdated Show resolved Hide resolved
@NiklasBuchfink NiklasBuchfink merged commit f4c5589 into opral:main Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants