Skip to content

Comments

[SQSERVICE-253] Support provisioning role information with SCIM#2851

Merged
battermann merged 26 commits intodevelopfrom
SQSERVICES-253-feature-support-provisioning-role-information-with-scim
Nov 23, 2022
Merged

[SQSERVICE-253] Support provisioning role information with SCIM#2851
battermann merged 26 commits intodevelopfrom
SQSERVICES-253-feature-support-provisioning-role-information-with-scim

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented Nov 17, 2022

https://wearezeta.atlassian.net/browse/SQSERVICES-253

  • POST and PUTare supported
  • PATCH is not supported, yet, see subsequent PR
  • SCIM will override changes from team management
  • an empty roles list will always be interpreted as member and will override any previously set role

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix November 17, 2022 11:20 Inactive
@battermann battermann temporarily deployed to cachix November 17, 2022 11:20 Inactive
@battermann battermann changed the title Sqservices 253 feature support provisioning role information with SCIM [SQSERVICE-253] feature support provisioning role information with SCIM Nov 17, 2022
@battermann battermann changed the title [SQSERVICE-253] feature support provisioning role information with SCIM [SQSERVICE-253] Support provisioning role information with SCIM Nov 17, 2022
@battermann battermann temporarily deployed to cachix November 17, 2022 15:20 Inactive
@battermann battermann temporarily deployed to cachix November 17, 2022 15:20 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 17, 2022
@battermann battermann temporarily deployed to cachix November 18, 2022 12:31 Inactive
@battermann battermann temporarily deployed to cachix November 18, 2022 12:31 Inactive
@battermann battermann temporarily deployed to cachix November 18, 2022 13:04 Inactive
@battermann battermann temporarily deployed to cachix November 18, 2022 13:04 Inactive
@battermann battermann force-pushed the SQSERVICES-253-feature-support-provisioning-role-information-with-scim branch from 575b36e to 3580346 Compare November 21, 2022 10:34
@battermann battermann temporarily deployed to cachix November 21, 2022 10:34 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 10:34 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 10:50 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 10:50 Inactive
@battermann battermann temporarily deployed to cachix November 22, 2022 09:16 Inactive
@battermann battermann temporarily deployed to cachix November 22, 2022 09:16 Inactive
@battermann battermann temporarily deployed to cachix November 22, 2022 09:18 Inactive
@battermann battermann temporarily deployed to cachix November 22, 2022 09:18 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

still need to read brig and galley, otherwise looks good. a few comments.

_ <- ScimT.updateUser tok userId scimUserWithRole
ScimT.checkTeamMembersRole tid owner userId role
mapM_ testUpdateUserWithRole [minBound .. maxBound]
it "update user - default to member if no role given" $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be the previous value instead of the default? not sure, though. i think we have an issue here with not being able to distinguish [] from null or missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it is not surprising for the SCIM admin when they change the roles field from the previous SCIM provider setting (e.g. [owner]) to [] or null that this leads to the role being reset to the default. What would you expect as a user?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, makes sense. maybe we can add something to that end somewhere under docs.wire.com? (separate PR though.)

TeamId ->
Role ->
m ()
updateTeamMember u tid role = do
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're only passing in a role here, you shouldn't construct the entire member from scratch and write that. you should only update the role. otherwise you may overwrite current or yet-to-be-introduced attributes of that member with default values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT this endpoint only exists to update a team member's permissions (equivalent to role). Are you concerned about not yet existing future functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you concerned about not yet existing future functionality?

yes. that, and i didn't actually check the types. i was expecting there to be something else, too.

@battermann battermann temporarily deployed to cachix November 23, 2022 09:12 Inactive
@battermann battermann temporarily deployed to cachix November 23, 2022 09:12 Inactive
@battermann battermann temporarily deployed to cachix November 23, 2022 09:50 Inactive
@battermann battermann temporarily deployed to cachix November 23, 2022 09:50 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants