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: Alby OAuth Connector #2399

Merged
merged 83 commits into from
Jul 7, 2023
Merged

Conversation

pavanjoshi914
Copy link
Contributor

@pavanjoshi914 pavanjoshi914 commented May 6, 2023

Wallet Interface with Oauth security

Type of change

  • feat: New feature (non-breaking change which adds functionality)

Screenshots of the changes [optional]

Alby.OAUTH.CONNECTOR.mp4

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

…ialogue

using lndhub backend currently
signed-off-by:pavanj914@gmail.com
pass connector field to accountContext
use it to validate that account is alby or not
eliminate alias hack to check alby account which can be changed
signed-off-by:pavanj914@gmail.com
signed-off-by: pavanj914@gmail.com
@socket-security
Copy link

socket-security bot commented May 6, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@github-actions
Copy link

github-actions bot commented May 6, 2023

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: channel.ninja (who recently dropped 1000 sats):


Want to sponsor the next build? send some sats to ⚡️builds@getalby.com (don't forget to provide your name)

Don't forget: keep earning sats!

@reneaaron reneaaron marked this pull request as draft May 8, 2023 19:51
@rolznz rolznz changed the title prototype: launchWebAuthFlow Alby OAuth Connector (prototype: launchWebAuthFlow) Jun 5, 2023
@rolznz rolznz self-assigned this Jun 5, 2023
@rolznz rolznz changed the title Alby OAuth Connector (prototype: launchWebAuthFlow) Draft: Alby OAuth Connector Jun 5, 2023
@rolznz
Copy link
Contributor

rolznz commented Jun 5, 2023

Some issues found so far:

  • Firefox not tested yet / no auth credentials created etc. We also need to make sure we can have a static ID for development.
  • Alby-js-sdk should be used rather than a custom http client for all methods - Roland working on this
  • Connect with Alby button takes a long time to launch the modal. Why does it take so long? - seems to be caused by extension background script launching inside of the popup. Using a loading state for now - fixed by Pavan
  • Hardcoded fields for OAuth clients - Roland is fixing this
  • Client ID changes every time unpackeddev extension is loaded (Chrome) - Roland fixed
  • Does not work in regtest - Roland fixed
  • Connect with Alby button should be primary color - Pavan fixed
  • Connect with Alby is impossible to login because it launches webln lnurl-auth instead of allowing to enter username/password - Roland
  • LNURL auth - use secret key - Roland
  • LNURL auth - toggle to use secret key for non-alby accounts - Roland
  • We might need to change the connector interface since the OAuth tokens are returned in the init (OAuth login happens in the background script) and need to be saved when addAccount is called
  • Double check other supported browsers work (Opera, Brave) - do not handle for now
  • What will we do with unsupported browsers? (Safari) - do not handle for now
  • Make sure LNURL auth does not break login in new OAuth OTP flow in getalby.com (not released yet)
  • Make sure isAlbyAccount utility functions work for both legacy alby accounts (by alias) and the new alby accounts

const response = await msg.request(
"sendPayment",
{ paymentRequest: paymentRequest },
{
origin: navState.origin,
}
);
if (response.error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check if that is needed? Is that really the case for existing connectors too, that no errors would be shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are multiple places where an error is returned, and the sendPayment action is wrapped in try/catch. I think we need this check, I don't understand how it worked before.

@reneaaron reneaaron marked this pull request as ready for review July 6, 2023 14:11
@reneaaron reneaaron changed the title Draft: Alby OAuth Connector Alby OAuth Connector Jul 6, 2023
@reneaaron reneaaron changed the title Alby OAuth Connector feat: Alby OAuth Connector Jul 6, 2023
@rolznz rolznz merged commit bdcfa1e into getAlby:master Jul 7, 2023
@pavanjoshi914 pavanjoshi914 deleted the web-auth-prototype branch July 8, 2023 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants