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: authenticated calls in Candid UI #475

Merged
merged 10 commits into from
Oct 17, 2023

Conversation

Web3NL
Copy link
Contributor

@Web3NL Web3NL commented Oct 9, 2023

See #474 and demo link

Overview
Allow authenticated calls via Internet Identity (and possibly pem files)

Requirements
New deps: auth-client and identity packages from agent-js
Modifications and additions to tools/ui

Considered Solutions
Added II to Candid UI canister

Recommended Solution
Also recommend uploading pem files (with warning to developer about security)

Considerations
Should be easy to maintain, modifications of existing files are kept to a minimum
New deps version bumps

TODO
Remove dfx.json, canister_ids.json and target_test_canister from branch (left them for demo purpose)
Browser compatibility check
Testing preferably with Docker and Selenium (for local II alternative origins testing over https)
UI enhancements
Gather feedback

@Web3NL Web3NL changed the title Feat: authenticated calls Feat: authenticated calls in Candid UI Oct 9, 2023
@rvanasa
Copy link
Contributor

rvanasa commented Oct 9, 2023

Looks good so far! Thanks for the contribution!

tools/ui/src/candid.ts Outdated Show resolved Hide resolved
v2i0s2h2

This comment was marked as spam.

maheshVishwakarma1998

This comment was marked as spam.

Web3NL and others added 3 commits October 11, 2023 17:34
Co-authored-by: Ryan Vandersmith <ryanvandersmith@gmail.com>
@Web3NL Web3NL marked this pull request as ready for review October 12, 2023 13:12
@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 12, 2023

This is only tested manually in production.
See https://m4ul7-aqaaa-aaaal-qcewq-cai.raw.icp0.io/?id=mvxad-wyaaa-aaaal-qcexa-cai

Let me know if I can improve

Leaving this for now, until further review

Copy link
Contributor

@chenyan-dfinity chenyan-dfinity left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

We don't need the target_test_canister here. I can merge once it's removed.

tools/ui/dfx.json Outdated Show resolved Hide resolved
@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 12, 2023

Thank you!

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 13, 2023

Here we go!

@chenyan-dfinity chenyan-dfinity merged commit 5be80b9 into dfinity:master Oct 17, 2023
3 checks passed
@Web3NL Web3NL deleted the feat-authenticated-calls branch October 17, 2023 19:00
@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 17, 2023

Many thnx!

Will this make it into dfx 0.15.2? I would be so proud 😄

@chenyan-dfinity
Copy link
Contributor

Should be. I will make a PR to include this in dfx soon.

@chenyan-dfinity
Copy link
Contributor

@Web3NL Actually, there is one more UI fix we need to do. We use Candid UI in the playground with small width. The II button looks big there. Is there an easy way to make it smaller with small window width? (cc @rvanasa if you have a quick fix)

Screenshot 2023-10-17 at 12 13 13 PM

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 17, 2023

Will have a look tomorrow i hope

@ggreif
Copy link
Contributor

ggreif commented Oct 17, 2023

@Web3NL Does the merged version work with ic0.app-domain anchors too?

@chenyan-dfinity
Copy link
Contributor

Yes, I think it supports both raw and icp0.io

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 17, 2023

@Web3NL Does the merged version work with ic0.app-domain anchors too?

yes, i added a form to choose between ic0.app / icp0.io. Same for raw / non-raw
Note, the obtained principal is independent of derivation origin domain, but does change if you include / exclude raw

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 17, 2023

@Web3NL Does the merged version work with ic0.app-domain anchors too?

Excuse me, I corrected my last response.

Does this answer your question, or were you refering to the default identity provider, which now is always identity.internetcomputer.org and never identity.ic0.app

The derivationOrigin (canister for which we want to obtain a Principal) may be icp0.io or ic0.app

@rvanasa
Copy link
Contributor

rvanasa commented Oct 18, 2023

Actually, there is one more UI fix we need to do. We use Candid UI in the playground with small width. The II button looks big there. Is there an easy way to make it smaller with small window width?

From a responsive UX standpoint, the following three improvements would help a lot on smaller screen sizes:

  • Using @media CSS queries to change the login form / button layout for different screen widths
  • Moving checkboxes inside of the <label> tag (which makes it possible to toggle the checkbox by clicking the label)
  • Shortening "Login with Internet Identity" to just "Login" (maybe depending on screen width)

@Web3NL, here is a partial solution in case you want to borrow any of this code: #480

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 18, 2023

I will have a look, thnx for tagging.

And I think this feature deserves some documention on the portal on how a canister should serve the ii-alternative-origins file.

I will attend to that 🙂

@ggreif
Copy link
Contributor

ggreif commented Oct 18, 2023

yes, i added a form to choose between ic0.app / icp0.io. Same for raw / non-raw
Note, the obtained principal is independent of derivation origin domain, but does change if you include / exclude raw

I am asking because my ic0.app-based anchor didn't work in the example canister.

Screenshot 2023-10-18 at 18 18 52

I did:
Screenshot 2023-10-18 at 18 37 59
and still got
Screenshot 2023-10-18 at 18 39 47
@Web3NL

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 18, 2023

Ah, I see.

Is this because identityProvider is set to identity.internetcomputer.org?

If so, then there should also be an option to choose identity.ic0.app as the provider

@chenyan-dfinity
Copy link
Contributor

Deployed to the main net!

Another possible improvement is to only show the Login button when alternativeOrigins is presented in the destination canister response.

@Web3NL
Copy link
Contributor Author

Web3NL commented Oct 18, 2023

Deployed to the main net!

Another possible improvement is to only show the Login button when alternativeOrigins is presented in the destination canister response.

Yes, or perhaps disable it and inform the user of the possibility with a link to how to set it up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants