WPB-20203 backend updating user attributes via SCIM should be possible when E2EID is enabled #4772
Conversation
zebot
added
the
ok-to-test
libs/wire-subsystems/test/unit/Wire/UserSubsystem/InterpreterSpec.hs
Outdated
Show resolved
Hide resolved
supersven
left a comment
supersven
left a comment
There was a problem hiding this comment.
LGTM 👍 (I especially liked the well-written test.)
|
IMHO we can merge this once @emil-wire finished testing the backported edition of this PR (#4775). |
emil-wire
left a comment
emil-wire
left a comment
There was a problem hiding this comment.
lgtm, but I'd like some small changes/questions answered
|
|
||
| prop | ||
| "if e2e identity is activated, the user name cannot be updated" | ||
| "if e2e identity is activated, the user name cannot be updated by a wire client" |
There was a problem hiding this comment.
what happens if you throw SSO into the mix and the users data are different from the ones in scim? Do we have a way of testing/hedging against this scenario?
There was a problem hiding this comment.
you mean the saml attributes and scim data disagree? i don't remember us reading any saml attributes, so that might be fine, but this needs to be double-checked.
There was a problem hiding this comment.
I don't understand what this question has to do with the problem that this PR solves? But maybe I don't understand the question correctly. SSO/SCIM scenarios are excessively tested in other places.
|
|
||
| prop | ||
| "if e2e identity is activated, the user name cannot be updated" | ||
| "if e2e identity is activated, the user name cannot be updated by a wire client" |
There was a problem hiding this comment.
you mean the saml attributes and scim data disagree? i don't remember us reading any saml attributes, so that might be fine, but this needs to be double-checked.
| idempLocale = isNothing locale || locale == user.locale | ||
| scim = updateOrigin == UpdateOriginWireClient && user.managedBy == Just ManagedByScim | ||
| scimConflict = updateOrigin == UpdateOriginWireClient && user.managedBy == Just ManagedByScim | ||
| updateByScim = updateOrigin == UpdateOriginScim && user.managedBy == Just ManagedByScim |
There was a problem hiding this comment.
it is always true that scimConflict == not updateByScim, so you could replace not updateByScim below by scimConflict, then you could simplify line 533 below. and maybe rename scimConflict to updateFromWireClientForScimUser, or something.
or maybe you could shuffle the code long enough to end up with something involving this:
case updateOrigin of
Wire... -> _
Scim... -> _
that may be more readable, and would help with refactoring UpdateOriginType.
(don't take any of this too seriously, doing this review as a test how my virus-infested brain is recovering.)
There was a problem hiding this comment.
I gave it a try, but was not happy with the result. I suggest leaving it as is, as I think the readability is good, or you make a suggestion?
There was a problem hiding this comment.
nah, if you've tried and it didn't improve things i say it's good enough.
|
@emil-wire I tried to address all of your comments and re-requested a review. |
…pec.hs Co-authored-by: Sven Tennie <[email protected]>
battermann
force-pushed
the
WPB-20203-backend-updating-user-attributes-via-scim-should-be-possible-when-e-2-ei-is-enabled
branch
from
e898360 to
dc3bff4
Compare
stefanwire
requested review from
emil-wire
and removed request for
emil-wire
|
@emil-wire Requesting changes comes with the obligation to follow up. Dismissed your review for the moment. |
battermann
deleted the
WPB-20203-backend-updating-user-attributes-via-scim-should-be-possible-when-e-2-ei-is-enabled
branch
…e when E2EID is enabled (#4772)
Checklist
changelog.d