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 #3379

Merged
merged 3 commits into from
Oct 4, 2022
Merged

feat: SCIM list provider users #3379

merged 3 commits into from
Oct 4, 2022

Conversation

BruceMacD
Copy link
Collaborator

  • add SCIM list users endpoint
  • add SCIM role

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

- add SCIM list users endpoint
- add SCIM role
@@ -64,7 +64,7 @@ func AssignIdentityToGroups(tx GormTxn, user *models.Identity, provider *models.
}
addIDs = append(addIDs, item)
}
if rows.Err() != nil {
if err := rows.Err(); err != nil {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like this would have returned nil previously, which doesnt seem intentional.


if p != nil {
// apply scim parameters
if p.Count != 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are similar to our pagination, but slightly more complicated. Our pagination only allows set page sizes, but SCIM requires a "start index" which offsets the results.

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.

LGTM! Mostly questions, some of which I suspect will be answered when the create/update endpoints are added.

@@ -105,7 +105,7 @@ func AssignIdentityToGroups(tx GormTxn, user *models.Identity, provider *models.
}
ids = append(ids, item)
}
if rows.Err() != nil {
if err := rows.Err(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing these!

internal/server/data/provideruser.go Show resolved Hide resolved
internal/server/data/provideruser.go Outdated Show resolved Hide resolved
internal/server/routes.go Outdated Show resolved Hide resolved
internal/server/scim.go Outdated Show resolved Hide resolved
Comment on lines +706 to +708
ADD COLUMN IF NOT EXISTS given_name text,
ADD COLUMN IF NOT EXISTS family_name text,
ADD COLUMN IF NOT EXISTS active boolean DEFAULT true;
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't being used yet, but I guess the next PR will include an update endpoint where these will be set, is that right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, control of these will be set through the SCIM create user endpoint, I'll make some updates to the just-in-time logic too. These are required fields for the SCIM spec.

internal/server/data/provideruser.go Show resolved Hide resolved
err := CreateProvider(tx, provider)
assert.NilError(t, err)

pu := createTestProviderUser(t, tx, provider, "david@example.com")
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but I've been trying to include a few records that I expect not to match the query in most of the tests for List operations. Something like same email different provider.

internal/server/data/provideruser_test.go Show resolved Hide resolved
internal/server/models/provideruser.go Show resolved Hide resolved
@BruceMacD BruceMacD enabled auto-merge (squash) October 4, 2022 20:51
@BruceMacD BruceMacD merged commit 50721ec into main Oct 4, 2022
@BruceMacD BruceMacD deleted the brucemacd/scim-users-get branch October 4, 2022 20:52
BruceMacD added a commit that referenced this pull request Oct 5, 2022
BruceMacD added a commit that referenced this pull request Oct 5, 2022
BruceMacD added a commit that referenced this pull request Oct 5, 2022
BruceMacD added a commit that referenced this pull request Oct 6, 2022
- add SCIM list users endpoint
- add SCIM role
@BruceMacD BruceMacD mentioned this pull request Oct 6, 2022
7 tasks
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