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

Add support for rpcUrl with basic auth when retrieving chainId on net… #9815

Merged
merged 7 commits into from
Nov 9, 2020

Conversation

jimthematrix
Copy link
Contributor

…work creation

Fixes: #9791

Explanation: This new function to retrieve chainId on network creation does not take into account that rpcUrl may contain basic auth: https://<username>:<password>@permissioned.network. Fix adds checking for the basic auth in the URL and adds basic auth to the header and adjust the rpcUrl passed to window.fetch()

Manual testing steps:

  • Create a new network
  • in the RPC field, provide the url of the form https://<username>:<password>@permissioned.network (exact value dependent on the permissioned network)
  • provide the permissioned network's chainId
  • click "Save"
  • Error: Could not fetch chain ID. Is your RPC URL correct?

@jimthematrix jimthematrix requested a review from a team as a code owner November 6, 2020 06:31
@jimthematrix jimthematrix requested a review from rekmarks November 6, 2020 06:31
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@jimthematrix
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Thank you for creating this PR.

Can you please confirm that my suggestion works and merge it? It will fix the lint failure.

It would also be helpful if you allowed edits from maintainers.

@jimthematrix
Copy link
Contributor Author

thanks @rekmarks for the review and suggested updates, I merged the changes and made a small update after testing the function inside browser console.

ui/app/helpers/utils/util.js Outdated Show resolved Hide resolved
@jimthematrix
Copy link
Contributor Author

@rekmarks for some reason I couldn't find the "Allow edits by maintainers" checkbox anymore which used to be under the "Participants" section 🤔

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@Gudahtt Gudahtt merged commit e72f943 into MetaMask:develop Nov 9, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2020
@jimthematrix jimthematrix deleted the basic-auth-url branch November 9, 2020 15:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to add custom network with basic auth in url
3 participants