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: SCIM list provider users #3405

Merged
merged 6 commits into from
Oct 12, 2022
Merged

feat: SCIM list provider users #3405

merged 6 commits into from
Oct 12, 2022

Conversation

BruceMacD
Copy link
Collaborator

Restore #3379

  • add SCIM list users endpoint

Summary

This change adds the necessary logic for a SCIM identity provider to list the users it has associated with it.

It does not include the changes necessary to create an access key for an identity provider (which this functionality will rely on) so the code path can't be accessed yet.

Checklist

  • Wrote appropriate unit tests
  • Considered security implications of the change
  • Updated associated docs where necessary
  • Updated associated configuration where necessary
  • Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
  • Nothing sensitive logged
  • Considered data migrations for smooth upgrades

Related Issues

Part of #3378

@BruceMacD BruceMacD mentioned this pull request Oct 6, 2022
7 tasks
@BruceMacD BruceMacD marked this pull request as ready for review October 11, 2022 15:36
- add SCIM list users endpoint
- scan provider user scim fiekds
- start index begins at 1
- default names not null
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Looks good, but I think we might be missing an authorization check

Comment on lines 13 to 14
// this can only be run by an access key issued for an identity provider
ctx := GetRequestContext(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this missing a call to RequireInfraRole (or IsAuthorized ) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally that was part of this change, but after building out the rest of the SCIM functionality I realized that the SCIM role wasn't needed. This endpoint can only be called with an access key issued for a provider, and it can only modify the provider that the access key was issued for. In this way is acts similarly to our other isSelf endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, because we query with AccessKey.IssusedFor, which will have a providerID in the case of these SCIM access keys, and there should never be an overlap between userIDs and providerIDs, got it!

internal/server/data/provideruser.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Nephin <dnephin@gmail.com>
internal/access/scim.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Nephin <dnephin@gmail.com>
@BruceMacD BruceMacD enabled auto-merge (squash) October 12, 2022 20:18
@BruceMacD BruceMacD merged commit 52d3d7e into main Oct 12, 2022
@BruceMacD BruceMacD deleted the brucemacd/scim_users_list branch October 12, 2022 20:25
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