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

feat(iam, app, platforms): adds idena stamps #671

Closed
wants to merge 4 commits into from

Conversation

mbidenaio
Copy link
Contributor

GenericOauthPlatform is not used since, how I understood, it is not fully universal yet (performs fetching /twitter/generateAuthUrl).
But I followed the latest changes - new procedures and copies of the new providers are placed in the platforms module.

@mbidenaio mbidenaio force-pushed the idena-provider branch 2 times, most recently from 0996312 to db4592e Compare November 3, 2022 12:01
@mbidenaio
Copy link
Contributor Author

@nutrina Could you please review this PR?

@lucianHymer
Copy link
Collaborator

Hey @mbidenaio!

Thanks for putting this together, and thanks for your patience!

We've restructured the platforms directory a bit, so here's a rebased version of your PR if you'd like to pull it in (https://github.com/gitcoinco/passport/tree/1042-idena-provider). I attempted to switch the code over to use a caching method that we've added. And I changed it to use the cache as little as possible, i.e. just for the auth part. Storing the result values is instead done using the "context", which is simpler than the cache. I believe I did all this correctly, but it's not fully tested since there aren't unit tests and I'm not able to test the stamp locally.

I also added a feature flag, so you'll want to add this line to your app/.env:
NEXT_PUBLIC_FF_IDENA_STAMP=on

Would you mind adding unit tests for Idena? You can see an example in platforms/src/GTC/__tests__/ethErc20PossessionGtc.test.ts

Also, it seems like it's not really possible to fully test this stamp in the UI locally, right? Since the Idena server is expecting to make a couple calls to a public address? Did you have a workaround for this (i.e. a testing mode in the Idena app or something?), or did you only test this out on a machine that exposed a public endpoint?

Stepping back real quick, are we sure we need all of these authentication steps? What exactly is the purpose of each step (i.e. why exactly are you requesting a nonce, signing it, and having us check the signature? What do we gain from this?) What would the trade-off be if we were to i.e. just grab a throwaway nonce and never actually check the signature, so that this stamp works locally?

Let me know if you have any questions!

@midenaio
Copy link

Hey @lucianHymer

Would you mind adding unit tests for Idena?

The unit tests were added here: #1356
Please review.

I believe I did all this correctly, but it's not fully tested since there aren't unit tests and I'm not able to test the stamp locally.

We have just tested it on the branch and it works fine. For the local testing you need to run both passport and Idena web client locally so the Idena client could call the localhost endpoint.

  1. Start idena-web locally:
git clone https://github.com/idena-network/idena-web
cd idena-web
npm i --force
next dev -p 3001

Open http://localhost:3001

  1. Configure gitcoinco/passport:
In app/.env set NEXT_PUBLIC_PASSPORT_IDENA_WEB_APP=http://localhost:3001/

To test Idena stamp you will also need a real Idena account. We can send you a private key for the account which we have recently validated specially for the testing purposes. Please email us to info@idena.io

Stepping back real quick, are we sure we need all of these authentication steps? What exactly is the purpose of each step (i.e. why exactly are you requesting a nonce, signing it, and having us check the signature? What do we gain from this?) What would the trade-off be if we were to i.e. just grab a throwaway nonce and never actually check the signature, so that this stamp works locally?

Idena sing-in protocol requires a nonce to be generated on the server side. Otherwise if protocol allows the browser to generate a throwaway nonce, the MITM attack can be performed. The attacker can steal both nonce and signature and use them for malicious authentication. Idena client enforces a secure protocol by calling the server side endpoint to obtain a new nonce for each new authentication session.

@lucianHymer
Copy link
Collaborator

@mbidenaio Fantastic! Thanks so much for putting this all together, and for providing an awesome explanation. I'll dig into this now!

@lucianHymer
Copy link
Collaborator

Sweet, this is working great for me! We're wrapping up some considerations on the future of expiration dates. We think we're going to make them optional for providers and let providers choose their own expiration like you've done here, in which case I'll merge the PR right away! I'll let you know if anything needs to be changed, but it's looking like this should be good to go as-is. Thanks!

@lucianHymer
Copy link
Collaborator

lucianHymer commented Jun 21, 2023

Release requirements: Set the following in fleek for the Passport frontend
NEXT_PUBLIC_FF_IDENA_STAMP=on
NEXT_PUBLIC_PASSPORT_IDENA_WEB_APP=https://app.idena.io/
NEXT_PUBLIC_PASSPORT_IDENA_CALLBACK=https://review.passport.gitcoin.co/

@lucianHymer
Copy link
Collaborator

This is merged! Currently working well on our review server. #1388

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.

None yet

3 participants