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: Add verified badge to team member card #216

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft

feat: Add verified badge to team member card #216

wants to merge 4 commits into from

Conversation

oSumAtrIX
Copy link
Member

@oSumAtrIX oSumAtrIX commented Jan 8, 2024

About

This PR builds on ReVanced/revanced-api#150 to display GPG keys for members:

image

Todo

@revanced-bot
Copy link

Deployed at https://5911eb71.revanced.pages.dev.

@oSumAtrIX oSumAtrIX marked this pull request as draft January 8, 2024 21:58
@PalmDevs
Copy link
Member

PalmDevs commented Jan 8, 2024

Design wise, it looks pretty okay. Although, I think it'd be better to put the keys into a <ul> or <ol> and align all text to the left.

Usability isn't that good right now but you've already noticed the issue, so I won't repeat the same thing.

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 8, 2024

GPG keys can be very long, this has to be dealt with as well, usually they are displayed in armored representation of 16 hex chars length, but I am not sure how do enforce that on the website or API side as the GitHub, which is used as the source for the keys returns them in plain text

Copy link
Contributor

@KTibow KTibow left a comment

Choose a reason for hiding this comment

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

This seems like a bad place to put this info.

If I was looking for info about the team, I would probably open "contributors". not the donate page. If I was looking to donate, I wouldn't care about your GPG keys.

I would say more (eg suggest linking to https://github.com/user.gpg) but I won't because I don't understand the underlying motivation for
a. showing that a user is verified
b. using gpg keys to show that
c. showing who the team is on the donate page

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 10, 2024

Moving the badge to the contributors tab leaves no proper place for the badge to be shown and it gets lost between all the other contributors or you'd have to display it for all of them which deviates from what we want and loses the the purpose of adding the badge.

The donation page was chosen as it is currently the only place the website hits the members endpoint which returns information about the team. It is done on the donation page to see who you are donating too. Look up GitHub Sponsors for a native implementation of that done by GitHub.

To see the motivation to why GPG keys are added to members, check on this: ReVanced/revanced-api#150 (comment)

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 10, 2024

I am not sure what the exact difference is between the key visible in your suggested URL and the one the API will return. Users can have a list of keys which is why I don't know how that would work on your URL. Apart from that, it seems like your URL returns the full public key whereas the API will return some short form of it. If there is a reason to use your URL over what is returned by the API. Please give it a comment with a reason on the issue I sent in my previous comment. In the case we return that, we either may want to separate the endpoint as it is gonna take up most of the requests size but usually not be necessary. Additionally, the website would have to rethink how to display it die to its length

@KTibow
Copy link
Contributor

KTibow commented Jan 10, 2024

Okay, well that lets me update my questions.

a. why are we showing that a user is verified? probably to give the feeling of security
b. why are gpg keys shown? probably because they're used to sign commits
c. why is the team shown? to show who you're donating to

But I'm still not sure why I would want a GPG key when GitHub already tells me if a commit is signed ("verified") using my uploaded GPG key. I guess if this is just security theater it would work fine showing the short form of the key though.

@oSumAtrIX
Copy link
Member Author

oSumAtrIX commented Jan 10, 2024

a. As referenced in one my previous comment, it is a common security practice. If you trust the domain revanced.app, you can transitively trust keys published there. If someone signs something with one of these keys, you can transitively trust them. An example scenario would be that you can prove in an arbitrary place on the web that you are who you claim to be on revanced.app. Inheritently, the trust chain starts at revanced.app and ends with your sign, which means whoever revanced.app claims this key belongs to is not verifiable without any additional means. Because it is not important who is behind the key, and instead it is only important that you are someone trusted on revanced.app, the keys are published as a verification method.
b. The GPG keys are shown to prove your relationship with revanced.app
c. ✔️

But I'm still not sure why I would want a GPG key when GitHub already tells me if a commit is signed ("verified") using my uploaded GPG key.

This question is answered here:

As GitHub is a third party platform it is not where we would want to publish the keys primarily, unlike our first party platform @ revanced.app. People would not need to visit GitHub to verify that you on Discord are who you claim to be on ReVanced, dependently of GitHub whereas the ultimate fallback and source of truth, where the user consumes from, is the revanced.app domain. It goes as far as that the user also is not dependant on GitHub in terms of the source code we write, as all source is available on git.revanced.app as well. GPG signing in itself has nothing to do with code, as to why it is not a good idea to only make it accessible in relation to that. Instead it is good to present them in a primary accessible location such as the website, as the key can be used to verify your identity for any general purpose.

@KTibow
Copy link
Contributor

KTibow commented Jan 10, 2024

Okay, fair enough. Maybe in some later day contributors/team/donate would have a different organization but I'm fine with putting them there.

Now on the topic of displaying them, I think it could work better to have a dialog triggered either by a separate button or clicking on the badge instead. What's your opinion on that?

@oSumAtrIX
Copy link
Member Author

Okay, fair enough. Maybe in some later day contributors/team/donate would have a different organization but I'm fine with putting them there.

If you want please open an issue. Open to suggestions :)

Now on the topic of displaying them, I think it could work better to have a dialog triggered either by a separate button or clicking on the badge instead. What's your opinion on that?

I think it is expected to hover on the badge to open the tooltip. If a popup were to be shown, a click should be used. On mobile, the badge can be expanded to look like this (with the icon for example):

image

@KTibow
Copy link
Contributor

KTibow commented Jan 10, 2024

Okay, sounds like a plan. I have to go but when I get back next morning I guess I can take a look at the code.
(One more thing, given I'm assigned are you expecting me to a. leave a review as is b. wait until it's finished and leave a review then c. update the code to finish it?)

@oSumAtrIX
Copy link
Member Author

It would be good if you could look into resolving the todos and push it. I have had my attempts but didn't really succeed in them

@KTibow KTibow changed the base branch from main to dev January 11, 2024 14:44
@revanced-bot
Copy link

Deployed at https://29e2d9a0.revanced.pages.dev.

@revanced-bot
Copy link

Deployed at https://89729c68.revanced.pages.dev.

@revanced-bot
Copy link

Deployed at https://0dc44863.revanced.pages.dev.

@revanced-bot
Copy link

Deployed at https://head.revanced.pages.dev.

Copy link

cloudflare-pages bot commented Mar 3, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 73c0416
Status:🚫  Build failed.

View logs

@oSumAtrIX oSumAtrIX assigned Ushie and unassigned KTibow Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Backlog
Development

Successfully merging this pull request may close these issues.

None yet

6 participants