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

feat: make groups scope optional to support azure with OIDC #9773

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Conversation

salonig23
Copy link
Contributor

@salonig23 salonig23 commented Jul 31, 2024

Ticket

CM-407

Description

Add a new config exclude_groups_scope in OIDC config, to allow us to connect to OIDC providers like Azure who don't support groups scope.

Test Plan

@channolanp Has QA'd these changes and tested the following scenarios:

On Devcluster+Okta:

  • Setting exclude_groups_scope: false in my devcluster’s master config makes a request with scope=openid+profile+email+groups, and auto-manages groups as expected (user added to all relevant groups)
  • Setting exclude_groups_scope: true in my devcluster’s master config makes a request with scope=openid+profile+email , and auto-manages groups as expected (user removed from all groups since I am using groups_attribute_name: groups , which is expected as Okta requires groups scope)

On Devcluster+Keycloak:

  • Setting exclude_groups_scope: false in my devcluster’s master config makes a request with scope=openid+profile+email+groups, and auto-manages groups as expected (user added to all relevant groups, and Keycloak sends this claim with or without groups scope)
  • Setting exclude_groups_scope: true in my devcluster’s master config makes a request with scope=openid+profile+email , and auto-manages groups as expected (user added to correct groups, as Keycloak sends this claim with or without groups scope)

On Devcluster+Azure:

  • Setting exclude_groups_scope: false in my devcluster’s master config makes a request with scope=openid+profile+email+groups, this causes the auth to fail, as Azure does not allow “custom scopes”
  • Setting exclude_groups_scope: true in my devcluster’s master config makes a request with scope=openid+profile+email , which does login, bypassing the issue that started this ticket. Unfortunately, our Azure instance does not support “groups”, so I won’t get better feedback on this until working with the customer, greenlighting this as it’s still giving us the workaround

Instructions to set up Azure: https://hpe-aiatscale.atlassian.net/wiki/spaces/HELP/pages/1352466497/OIDC+With+Azure

Checklist

  • Changes have been manually QA'd
  • New features have been approved by the corresponding PM
  • User-facing API changes have the "User-facing API Change" label
  • Release notes have been added as a separate file under docs/release-notes/
    See Release Note for details.
  • Licenses have been included for new code which was copied and/or modified from any external code

@cla-bot cla-bot bot added the cla-signed label Jul 31, 2024
Copy link

netlify bot commented Jul 31, 2024

Deploy Preview for determined-ui canceled.

Name Link
🔨 Latest commit 58d6dba
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66aaaad8946a2d00082d2f86

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 54.15%. Comparing base (dab6978) to head (58d6dba).
Report is 2 commits behind head on main.

Files Patch % Lines
master/internal/plugin/oidc/service.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9773   +/-   ##
=======================================
  Coverage   54.15%   54.15%           
=======================================
  Files        1258     1258           
  Lines      154990   154992    +2     
  Branches     3500     3501    +1     
=======================================
+ Hits        83937    83940    +3     
+ Misses      70907    70906    -1     
  Partials      146      146           
Flag Coverage Δ
backend 44.88% <0.00%> (+<0.01%) ⬆️
harness 72.62% <ø> (ø)
web 53.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
master/internal/config/oidc_config.go 50.00% <ø> (ø)
master/internal/plugin/oidc/service.go 8.80% <0.00%> (-0.10%) ⬇️

... and 6 files with indirect coverage changes

@salonig23 salonig23 marked this pull request as ready for review July 31, 2024 20:07
@salonig23 salonig23 requested a review from a team as a code owner July 31, 2024 20:07
@salonig23 salonig23 requested review from ShreyaLnuHpe, corban-beaird and kkunapuli and removed request for ShreyaLnuHpe July 31, 2024 20:07
@determined-ci determined-ci added the documentation Improvements or additions to documentation label Jul 31, 2024
@determined-ci determined-ci requested a review from a team July 31, 2024 20:51
Copy link
Contributor

@kkunapuli kkunapuli left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@tara-det-ai tara-det-ai left a comment

Choose a reason for hiding this comment

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

LGTM

@determined-ci determined-ci requested a review from a team July 31, 2024 21:16
Copy link
Contributor

@corban-beaird corban-beaird left a comment

Choose a reason for hiding this comment

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

LGTM!

@salonig23 salonig23 merged commit f42daca into main Jul 31, 2024
78 of 95 checks passed
@salonig23 salonig23 deleted the oidc-azure branch July 31, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants