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

[WIP] GitHub integration #7

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

[WIP] GitHub integration #7

wants to merge 14 commits into from

Conversation

saundefined
Copy link
Member

@saundefined saundefined commented Apr 6, 2021

Changed

  • GitHub requests use a header instead of access_token

Added

  • Possibility to link GitHub profile

SQL migration

ALTER TABLE `users` ADD `github` VARCHAR(39) NULL AFTER `username`;
ALTER TABLE `users` ADD UNIQUE(`github`);

@saundefined saundefined requested a review from nikic April 6, 2021 14:16
manage/github.php Outdated Show resolved Hide resolved
users.sql Outdated Show resolved Hide resolved
manage/users.php Outdated Show resolved Hide resolved
@johannes
Copy link
Member

johannes commented Apr 6, 2021

I haven't tested it, but looking over it seems fine.when applying this needs to be coordinated with schema changes and should be tested once master synced.

@nikic
Copy link
Member

nikic commented Apr 7, 2021

This is currently using scope=repo. That makes sense to repo administration, but is way too much just to link accounts.

@saundefined
Copy link
Member Author

@nikic,

This is currently using scope=repo. That makes sense to repo administration, but is way too much just to link accounts.

I don't remove the repo administration section, if a user is in GITHUB_PHP_OWNER_TEAM_ID, it still can do it.
Also, token saved in the session, only if the user in the owners' team.

# Conflicts:
#	users.sql
Signed-off-by: Sergey Panteleev <[email protected]>
@nikic
Copy link
Member

nikic commented Apr 7, 2021

@nikic,

This is currently using scope=repo. That makes sense to repo administration, but is way too much just to link accounts.

I don't remove the repo administration section, if a user is in GITHUB_PHP_OWNER_TEAM_ID, it still can do it.
Also, token saved in the session, only if the user in the owners' team.

What I meant here is that the user shouldn't need to grant PHP write access to all their public and private repos just to associate their GitHub account. We should only request minimal permissions for this purpose.

@saundefined
Copy link
Member Author

We should only request minimal permissions for this purpose.

replaced with read:org, without it I can't check membership

Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Having looked at what this code did before ... I think we can just drop the repo management part entirely. What it currently implements will not work right now (because the team it assigns doesn't exist anymore), and this doesn't really seem like particularly useful functionality in the first place. I wasn't aware this tool existed and have always created repositories without it...

public/manage/github.php Outdated Show resolved Hide resolved
@saundefined saundefined changed the title GitHub integration [WIP] GitHub integration Apr 9, 2021
@saundefined saundefined marked this pull request as draft April 9, 2021 09:08
# Conflicts:
#	public/manage/github.php
- link account anyway
@saundefined
Copy link
Member Author

@nikic, thank you!

Updated the logic:

  • link accounts in any case;
  • don't request additional scopes;
  • removed creation of repositories.

If it's ok, after #12, I'll update the changed functions and ready for final review.

@Aaron-Junker
Copy link

Aaron-Junker commented May 27, 2022

Hi @saundefined. As you know I'm currently working on a RFC to create a global login system, that likely will use GitHub.

I just saw this PR and now I'm not sure if I should include this in the RFC and how, because it looks pretty finished, but it has no update since over a year.

@saundefined
Copy link
Member Author

@Aaron-Junker, this PR proposes simply linking GitHub profiles to a PHP account.
It should not affect global auth :)

@Aaron-Junker
Copy link

@Aaron-Junker, this PR proposes simply linking GitHub profiles to a PHP account. It should not affect global auth :)

Ok.

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

Successfully merging this pull request may close these issues.

4 participants