Skip to content

Modifications required to implement GitHub SSO in commercial #7183

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

Merged
merged 8 commits into from
Jun 30, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 11, 2020

Changes on this PR:

  • user is part of an organization if there is at least one Project imported under that Organization that it's linked to a RemoteRepository the user owns and the organization has SSOIntegration enabled

  • Organization.members and Organization.users methods were moved to AdminPermission.members

    Note that .members and .users return exactly the same users

  • delete RemoteRepository instances when syncing GitHub (only those instances that are not tied to a Project) (Fixes, partially, Deleted repository not detected #2412)

@humitos humitos added the PR: work in progress Pull request is not ready for full review label Jun 11, 2020
humitos added 2 commits June 22, 2020 11:21
If a user delete a repository or if their permissions were removed from that
repository, we delete our local RemoteRepository instance tied to that User.
@humitos humitos force-pushed the humitos/db-user-github-sso branch from 33ece5f to c001f07 Compare June 22, 2020 09:22
@humitos humitos marked this pull request as ready for review June 22, 2020 15:02
@humitos humitos requested a review from a team June 22, 2020 15:02
@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Jun 22, 2020
humitos added 2 commits June 23, 2020 12:36
These methods came from corporate and we are not really using them in community.
Now that corporate is implementing SSO, we need to make some modification on
them and we can't include them here.

We could re-add them once SSO is brought to community or we can implement them
in a different class that we can override with our SettingsOverrideObject method.
@humitos humitos force-pushed the humitos/db-user-github-sso branch from 9ae0f1a to abdc017 Compare June 23, 2020 11:21
With this pattern we can override them from Corporate using the same method we
already have, without confusions.

Note that .users and .members are exactly the same logic.
@humitos humitos force-pushed the humitos/db-user-github-sso branch from abdc017 to 83c42f4 Compare June 23, 2020 11:25
@humitos humitos requested a review from ericholscher June 23, 2020 16:05
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This is a cleaner solution. I'd love to get rid of the model methods entirely, but baby steps 👍

if isinstance(obj, Organization):
return User.objects.filter(
Q(teams__organization=obj) | Q(owner_organizations=obj),
).distinct()
Copy link
Member

Choose a reason for hiding this comment

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

I like seeing this logic backported into the .org 👍

@humitos humitos merged commit 041edb3 into master Jun 30, 2020
@humitos humitos deleted the humitos/db-user-github-sso branch June 30, 2020 11:13
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.

2 participants