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

Auth: Add OrgRoleMapper service #86973

Merged
merged 16 commits into from May 6, 2024
Merged

Conversation

mgyongyosi
Copy link
Contributor

@mgyongyosi mgyongyosi commented Apr 26, 2024

What is this feature?
This PR adds an OrgRoleMapper that will be used by the OAuth connectors to implement OrgRole mapping. This mapper meets all of the OrgRole mapping requirements that is implemented by SAML.

Why do we need this feature?
Prerequisite for implementing OrgRole mapping for OAuth providers. It's extracted from #77683.

Who is this feature for?
#77683

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Copy link
Contributor

@gamab gamab left a comment

Choose a reason for hiding this comment

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

Code looks good 🙂
I left a couple of questions for us to think through, wdyt?

pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
@mgyongyosi mgyongyosi added the no-changelog Skip including change in changelog/release notes label Apr 26, 2024
@mgyongyosi mgyongyosi force-pushed the mgyongyosi/add-org-role-mapper branch from 4d497cc to dc77536 Compare April 26, 2024 13:08
@mgyongyosi mgyongyosi requested a review from a team as a code owner April 26, 2024 13:08
@mgyongyosi mgyongyosi force-pushed the mgyongyosi/add-org-role-mapper branch from dc77536 to 242d74c Compare April 26, 2024 13:09
@mgyongyosi mgyongyosi force-pushed the mgyongyosi/add-org-role-mapper branch from 242d74c to 2423f46 Compare April 26, 2024 13:13
@mgyongyosi mgyongyosi requested a review from gamab April 28, 2024 17:54
kalleep
kalleep previously approved these changes Apr 30, 2024
@kalleep kalleep self-requested a review April 30, 2024 09:58
@mgyongyosi mgyongyosi dismissed kalleep’s stale review May 2, 2024 10:49

Found other issues, removing the approval to make sure nobody merge this accidentally

@mgyongyosi mgyongyosi force-pushed the mgyongyosi/add-org-role-mapper branch from 0438541 to 03498fe Compare May 2, 2024 16:30
@mgyongyosi mgyongyosi force-pushed the mgyongyosi/add-org-role-mapper branch from 66f50fb to de1d94b Compare May 3, 2024 09:59
Copy link
Contributor

@gamab gamab left a comment

Choose a reason for hiding this comment

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

Great job Misi! And very well tested!

pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
pkg/login/social/connectors/org_role_mapper.go Outdated Show resolved Hide resolved
expected: map[int64]org.RoleType{2: org.RoleEditor},
},
{
name: "should return nil if the org mapping contains at least one invalid setting and directly mapped role is empty and strict role mapping is enabled",
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting case as well 😮

pkg/login/social/connectors/org_role_mapper_test.go Outdated Show resolved Hide resolved
pkg/login/social/connectors/org_role_mapper_test.go Outdated Show resolved Hide resolved
mgyongyosi and others added 7 commits May 3, 2024 17:10
Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
…ettings

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
@mgyongyosi mgyongyosi merged commit 9236c5a into main May 6, 2024
12 checks passed
@mgyongyosi mgyongyosi deleted the mgyongyosi/add-org-role-mapper branch May 6, 2024 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth/oauth area/auth area/backend no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants