Skip to content

Multi project token #6373

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Multi project token #6373

wants to merge 13 commits into from

Conversation

Sterbic
Copy link
Contributor

@Sterbic Sterbic commented Aug 6, 2019

This PR adds support for multi project tokens. The backend was mostly ready for this it seems, I just had to make the forms and other UI components compatible wit it.

Thanks to @ip4368 for the help with the CSS so that the multi select doesn't look as if it were from the 90s.

Screen capture: https://www.dropbox.com/s/ukyqp5ag0ocrbd2/multi-proj-token.mov?dl=0

Closes: #6292

@Sterbic
Copy link
Contributor Author

Sterbic commented Aug 6, 2019

CI seems to have some transient issues with apt get but can't find a way of retrying it myself :/

@Sterbic Sterbic force-pushed the token-multi-project branch from e5e26c6 to 8a8332d Compare August 8, 2019 20:26
@Sterbic Sterbic force-pushed the token-multi-project branch from 8a8332d to 6d184bd Compare August 8, 2019 20:41
@nlhkabu
Copy link
Contributor

nlhkabu commented Aug 9, 2019

HI @Sterbic - thank you for this PR!

I have a suggestion regarding the user interface. Instead of throwing an error when a user makes a mistake with the token scope, we can prevent it by not allowing users to select their entire account and a project at the same time.

I was thinking something like this would work well:

For accounts with fewer than 10 projects

  • No radio selected by default
  • Checkboxes disabled unless "by project" is selected.

Screenshot from 2019-08-08 08-58-56

For accounts with more than 10 projects

  • No radio selected by default
  • Select disabled unless "by project" is selected

more-than-10

@Sterbic
Copy link
Contributor Author

Sterbic commented Aug 9, 2019

Thanks @nlhkabu, that design looks pretty neat! Would you be ok with it going into a separate pull request since it would be touching only the UI? This PR has all the backend logic and tests so I'd rather merge it as is so it's easier to review the UI changes afterwards.

@nlhkabu
Copy link
Contributor

nlhkabu commented Aug 9, 2019

Hi @Sterbic - my strong preference is to include the UI improvements in this PR, rather than ship as is and fix it later. My reason: there is no guarantee we'll actually "fix it later".

Did you want some help with the HTML/CSS/JS here?

@Sterbic
Copy link
Contributor Author

Sterbic commented Aug 9, 2019

Fair enough, I'll amend this PR. Is there a similar example in the codebase for such a UI? I have zero JS experience tbh.

@di
Copy link
Member

di commented Aug 9, 2019

Agreed w/ @nlhkabu.

As an alternative to adding the commits here, someone else could create a PR against your branch.

We use Stimulus for our JS framework, this should be a pretty straightforward controller.

@woodruffw
Copy link
Member

Instead of throwing an error when a user makes a mistake with the token scope, we can prevent it by not allowing users to select their entire account and a project at the same time.

Just a nitpick: we still need to perform this validation on the backend, since a user could use curl or another client to bypass the client-side restriction on what they're allowed to select. But it'll be good to also have it on the frontend 😄

@brainwane
Copy link
Contributor

@yeraydiazdiaz do you think you could help @Sterbic here?

@@ -35,7 +35,9 @@ <h2>Token for "{{ macaroon.description }}"</h2>
{% if macaroon.caveats.permissions == "user" %}
<strong>Scope:</strong> Entire account (all projects)
{% else %}
<strong>Scope:</strong> Project "{{ macaroon.caveats.permissions.projects[0] }}"
{% for project in macaroon.caveats.permissions.projects %}
<strong>Scope:</strong> Project "{{ project }}"<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit-pick: We probably shouldn't add a "Scope:" for every project and instead have it just once, with the project names in a comma-separated-with-and-for-final-join manner.

@yeraydiazdiaz
Copy link
Contributor

@Sterbic do you want to have a go at the JavaScript for this or should I tackle it myself?

@Sterbic
Copy link
Contributor Author

Sterbic commented Sep 4, 2019

Have been a bit swamped at work since I started working on this at PyCon AU but definitely want to finish it.

@yeraydiazdiaz
Copy link
Contributor

That's fine, let me know if you need help 🙂

@nlhkabu
Copy link
Contributor

nlhkabu commented Sep 15, 2019

Note:
If / when this PR is re-reviewed, we will need to ensure that the template is appropriately translated.

@Sterbic
Copy link
Contributor Author

Sterbic commented Oct 1, 2019

@yeraydiazdiaz, if you want to take a stab at the JS part go for it. I have a fire at work and won't have any cycles to spend learning JS any time soon 😭 I can rebase this PR on master if it helps.

@yeraydiazdiaz
Copy link
Contributor

Hey @Sterbic no worries, I can rebase myself and take it from here. Thanks so much for starting this PR 🌟

@yeraydiazdiaz
Copy link
Contributor

@Sterbic, I created a PR on your fork merging master onto your branch to bring this PR up to date. I was planning on creating an additional one with the JavaScripts changes as suggested by @di.

If you don't have time or don't want the noise let me know. 🙂

@yeraydiazdiaz
Copy link
Contributor

I've implemented the changes suggested by @nlhkabu, here's the capture using checkboxes:

token-multi-project-checkboxes

And another using multi-select, note this should only happen if the number of projects it greater than 10, I changed the code in the template to use it in this example:

token-multi-project-multiselect

Once the PR in @Sterbic's repo is merged this should be ready for review.

@yeraydiazdiaz
Copy link
Contributor

@Sterbic has merged Sterbic#2 so this is now ready for review.

Base automatically changed from master to main January 21, 2021 18:39
@miketheman
Copy link
Member

#Triage
This PR currently has merge conflicts that would need to be addressed before proceeding.

@Sterbic are you still interested in pursuing this effort? It's okay if not!

@miketheman miketheman added the awaiting-response PRs and issues that are awaiting author response label Jan 9, 2023
@miketheman miketheman added the tokens Issues relating to API tokens label Oct 5, 2023
@Sterbic Sterbic requested a review from a team as a code owner February 22, 2024 19:07
@ttodua
Copy link

ttodua commented Mar 20, 2025

is this still planed or not? would be very useful.

@woodruffw
Copy link
Member

is this still planed or not? would be very useful.

It's still planned in the sense that there's desire for it, but it needs an owner unless @Sterbic comes back to it.

In the mean time, you can use a Trusted Publisher to perform multi-project publishing -- that's built natively into the TP flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-response PRs and issues that are awaiting author response tokens Issues relating to API tokens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow scoped tokens with rights for multiple projects
9 participants