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

fix: lowered user permissions on registration #48

Merged

Conversation

gentlementlegen
Copy link
Member

@gentlementlegen gentlementlegen commented May 10, 2024

Resolves #41
Resolves #47

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented May 10, 2024

@gentlementlegen
Copy link
Member Author

This is a proposal to fix the permissions requested for newcomers, as requesting full access on public / private repos seems excessive and scary for new users.

It's a bit complicated to solve, since knowing the data related to a user requires that user to be logged in, where scopes have already been requested. So here is a draft proposal on how to handle it:

  1. A new user logs in for the first time at work.ubq.fi and gets requested the following
image

Everything is read-only, and the Org will help us later on determine if that user if part of Ubiquity's Org.

  1. We check post login if that user is part of Ubiquity's team. If so, we display a "lock" button next to the user name, like so
image

When clicked, we get redirected to the GitHub OAuth permission page requesting higher privileges

image

If allowed, the "lock" button is gone.

If that workflow is satisfactory, I can spend some time on the CSS to make it look better.

@0x4007
Copy link
Member

0x4007 commented May 10, 2024

I think it's a great proposal thanks for figuring this out. I am unsure that a lock is the best icon but until I have time to look through icon libraries I am unsure which is better.

Try using Google material design icons I normally stick with those for all of our UIs.

@0x4007
Copy link
Member

0x4007 commented May 10, 2024

I think you should also set a time estimate.

@0x4007
Copy link
Member

0x4007 commented May 10, 2024

Oh I just realized this is a pull sorry!

@gentlementlegen
Copy link
Member Author

@0x4007 Thanks for the feedback. Seems that Ubiquibot still doesn't like me: #41 (comment)

For the lock icon I actually pulled it out from Material UI icons. Maybe an opened lock would make more sense anyway.
By CSS issue I was referring that the placement of the button is inappropriate right now as it takes too much left and right space with the user name.

@gentlementlegen gentlementlegen marked this pull request as ready for review May 11, 2024 11:13
@gentlementlegen
Copy link
Member Author

Current looks of the button in the navbar:
image

@gitcoindev
Copy link

Looks good! I also reviewed from my side.

@molecula451 molecula451 merged commit fed9bac into ubiquity:development May 13, 2024
1 check passed
@0x4007
Copy link
Member

0x4007 commented May 13, 2024

Resolves #41

Resolves #47

We discourage multiple associated issues in a single pull.

This is so that we can audit changes (git blame) more efficiently if problems are discovered later.

In the future I would branch off and open two pulls separately. This can be done retroactively after all the code is complete as long as you use the git "cherry pick" feature @gentlementlegen

@gentlementlegen
Copy link
Member Author

@0x4007 Yep makes sense, very accidentally fixed two by doing this one. Thought it would not make much sense to have another PR with the same exact code as this one, will keep this in mind and open two different PRs next time.

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.

Empty directory page after logged in for the first time Lower requested permissions on user registration
5 participants