Implement active flag in SCIM #1158

arianvp · 2020-07-03T17:15:21Z

When the active flag is false, suspend the user

Fixes https://github.com/zinfra/backend-issues/issues/1471

TODOs that I couldn't get done:

Format with ormolu (The tools/ormolu.sh script refuses to run on my computer, there's a bash bug somewhere, or yq on nixos is something different than what you are using)
Test against Azure, to see if this fixes the soft delete problems

almost compiles. Need to add the Intra calls still

Involved exposing some types in brig that were internal before.

arianvp · 2020-07-03T17:16:00Z

This is the error the bash script is giving me by the way:

[nix-shell:~/Projects/wire/wire-server]$ ./tools/ormolu.sh 
usage: yq [-h] [--yaml-output] [--width WIDTH] [--indentless-lists]
          [--version]
          jq_filter [files [files ...]]
yq: error: argument files: can't open 'extra-deps[*]': [Errno 2] No such file or directory: 'extra-deps[*]'
Usage: grep [OPTION]... PATTERNS [FILE]...
Try 'grep --help' for more information.
please install ormolu  (eg., run 'stack install ormolu' and ensure ormolu is on your PATH.)

[nix-shell:~/Projects/wire/wire-server]$ which ormolu 
/home/arian/.local/bin/ormolu

[nix-shell:~/Projects/wire/wire-server]$ ormolu --version
ormolu 0.0.5.0 UNKNOWN UNKNOWN
using ghc-lib-parser 8.10.1.20200412

https://github.com/wireapp/wire-server/blob/b9a84f9b654a69c9a296761b36c042dc993236d3/services/brig/src/Brig/Team/API.hs#L306

services/brig/src/Brig/API/User.hs

services/spar/src/Spar/Scim/User.hs

fisx · 2020-07-03T18:09:33Z

services/spar/src/Spar/Scim/User.hs

-      let neededInfo = NeededInfo handle name externalId richInfo
+      -- NOTE: A user can be 'Active | Deleted | Ephemeral | Suspended'. We
+      -- only consider them Active when they're 'Active'
+      let neededInfo = NeededInfo handle name externalId richInfo (Just (status == Active))


what about ephemeral users? can we even create them here? but even if not, we're moving towards accessing brig data directly from both brig and scim, and this will be an interesting case.

e86c8c1 94168e7

I think this is much clearer, and it handles deleted and ephemeral users more robustly.

fisx · 2020-07-03T18:26:35Z

services/spar/test-integration/Test/Spar/Scim/UserSpec.hs

+    -- We make the assumption that when the `active` field is not there, that the user is
+    -- active by default; such that the IdP will de-activate it when it disagrees with the IdP
+    -- state.   Now if the IdP wants to remove the "active" field altogether, we dont "know"
+    -- anything again, and this should cause the user to be "active" again. This


"This" is not a complete sentence :-) It's also not clear how an IdP comes into play here. Do you mean the SCIM peer that patches the user? What does it mean if "it" (the IdP, presumably) disagrees with the (same?) IdP state?

Anyway I think it makes no sense to change the activation state if the SCIM peer explicitly does NOT patch it. I think we should leave it at whatever it is in that case. I suspect the reason why you came up with the current semantics is due to the confusion about the different representations of the user in brig and spar. See https://github.com/zinfra/backend-issues/issues/1006

4a1fe43 28ae745

fisx · 2020-07-03T18:42:22Z

Looks good, but I think we need to revisit #1158 (comment) and #1158 (comment), and probably make some more changes to resolve them.

both more correct (does not fail on valid requests) and more readable, maintainable.

fisx

thanks @mheinzel !

fisx · 2020-07-08T13:45:54Z

services/spar/src/Spar/Scim/Types.hs

+  Deleted -> False
+  Ephemeral -> True -- do not treat ephemeral users any different from active ones.
+
+scimActiveFlagToAccountStatus :: AccountStatus -> Maybe Bool -> AccountStatus


add a comment here!

fisx · 2020-07-08T13:46:48Z

services/spar/src/Spar/Scim/Types.hs

+scimActiveFlagToAccountStatus :: AccountStatus -> Maybe Bool -> AccountStatus
+scimActiveFlagToAccountStatus oldstatus = \case
+  Nothing -> if oldstatus == Ephemeral then Ephemeral else Active
+  Just True -> Active


Suggested change

Just True -> Active

Just True -> if oldstatus == Ephemeral then Ephemeral else Active

fisx · 2020-07-09T07:26:17Z

concourse says:

+ helm-wrapper upgrade --install --namespace test-euvg01uiguzu test-euvg01uiguzu-fake-aws wire-develop/fake-aws -f /tmp/build/06eff510/cailleach/bin/../helm_vars/integration/eu-central-1/fake-aws/values.yaml --set brig.image.tag=0.0.1-pr.2601 --set cannon.image.tag=0.0.1-pr.2601 --set gundeck.image.tag=0.0.1-pr.2601 --set galley.image.tag=0.0.1-pr.2601 --set spar.image.tag=0.0.1-pr.2601 --set cargohold.image.tag=0.0.1-pr.2601 --set proxy.image.tag=0.0.1-pr.2601 --set elasticsearch-index.image.tag=0.0.1-pr.2601 --set nginz.images.nginz.tag=0.0.1-pr.2601 --set cassandra-migrations.images.tag=0.0.1-pr.2601 --set brig.config.optSettings.setCookieDomain=test-euvg01uiguzu.svc.cluster.local --devel --wait


UPGRADE FAILED

Error: "test-euvg01uiguzu-fake-aws" has no deployed releases

Error: UPGRADE FAILED: "test-euvg01uiguzu-fake-aws" has no deployed releases

I'll try again.

fisx · 2020-07-09T10:14:18Z

concourse says:

+ helm-wrapper upgrade --install --namespace test-euvg01uiguzu test-euvg01uiguzu-fake-aws wire-develop/fake-aws -f /tmp/build/06eff510/cailleach/bin/../helm_vars/integration/eu-central-1/fake-aws/values.yaml --set brig.image.tag=0.0.1-pr.2601 --set cannon.image.tag=0.0.1-pr.2601 --set gundeck.image.tag=0.0.1-pr.2601 --set galley.image.tag=0.0.1-pr.2601 --set spar.image.tag=0.0.1-pr.2601 --set cargohold.image.tag=0.0.1-pr.2601 --set proxy.image.tag=0.0.1-pr.2601 --set elasticsearch-index.image.tag=0.0.1-pr.2601 --set nginz.images.nginz.tag=0.0.1-pr.2601 --set cassandra-migrations.images.tag=0.0.1-pr.2601 --set brig.config.optSettings.setCookieDomain=test-euvg01uiguzu.svc.cluster.local --devel --wait


UPGRADE FAILED

Error: "test-euvg01uiguzu-fake-aws" has no deployed releases

Error: UPGRADE FAILED: "test-euvg01uiguzu-fake-aws" has no deployed releases

I'll try again.

Reproducible. Here is another possibly helpful chunk from the logs:

sops 3.2.0

[warning] failed to retrieve latest version from upstream: Version information not found in upstream file


[warning] failed to compare current version with latest: Version string empty

arianvp and others added 4 commits July 3, 2020 19:12

WIP: Add active flag

9dd3675

almost compiles. Need to add the Intra calls still

Haddocks.

4ee3e6a

WIP SCIM Implement

c765d8a

Implement tests and calls to brig

a9933ad

Involved exposing some types in brig that were internal before.

fisx added 3 commits July 3, 2020 19:43

ormolu

3e61596

Remove misguided TODO.

820f410

https://github.com/wireapp/wire-server/blob/b9a84f9b654a69c9a296761b36c042dc993236d3/services/brig/src/Brig/Team/API.hs#L306

Cleanup; haddocks.

08e3262

fisx reviewed Jul 3, 2020

View reviewed changes

Cleanup tests.

abc9d8f

fisx added 4 commits July 7, 2020 10:08

hi ci

d7faaac

Promote NOTE to TODO; make it clear which line it belongs to.

5b6867d

Scim onboarding: handle all AccountStatus values reasonably.

e86c8c1

Clarifying comment.

94168e7

fisx changed the title ~~Implement active flag in SCIM~~ [WIP] Implement active flag in SCIM Jul 7, 2020

Fix test case docs.

4a1fe43

fisx changed the title ~~[WIP] Implement active flag in SCIM~~ Implement active flag in SCIM Jul 7, 2020

Better mapping: scim active flag <-> AccountStatus.

28ae745

both more correct (does not fail on valid requests) and more readable, maintainable.

fisx approved these changes Jul 7, 2020

View reviewed changes

fisx added 3 commits July 8, 2020 09:51

hi ci

fe6bde4

Cleanup.

3e798f1

Cleanup.

9821b45

fisx reviewed Jul 8, 2020

View reviewed changes

Fix account status mapping for bogus ephemeral case.

a23bfa3

hi ci

c50eb32

fisx merged commit 26f339b into develop Jul 9, 2020

fisx deleted the scim-for-real-2 branch July 9, 2020 11:02

fisx mentioned this pull request Jul 13, 2020

Release 2020/07/13 #1164

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement active flag in SCIM #1158

Implement active flag in SCIM #1158

arianvp commented Jul 3, 2020 •

edited by fisx

Loading

arianvp commented Jul 3, 2020

fisx Jul 3, 2020

fisx Jul 7, 2020

fisx Jul 3, 2020

fisx Jul 7, 2020

fisx commented Jul 3, 2020

fisx left a comment

fisx Jul 8, 2020

fisx Jul 8, 2020

fisx commented Jul 9, 2020

fisx commented Jul 9, 2020

	Just True -> Active
	Just True -> if oldstatus == Ephemeral then Ephemeral else Active

Implement active flag in SCIM #1158

Implement active flag in SCIM #1158

Conversation

arianvp commented Jul 3, 2020 • edited by fisx Loading

arianvp commented Jul 3, 2020

fisx Jul 3, 2020

Choose a reason for hiding this comment

fisx Jul 7, 2020

Choose a reason for hiding this comment

fisx Jul 3, 2020

Choose a reason for hiding this comment

fisx Jul 7, 2020

Choose a reason for hiding this comment

fisx commented Jul 3, 2020

fisx left a comment

Choose a reason for hiding this comment

fisx Jul 8, 2020

Choose a reason for hiding this comment

fisx Jul 8, 2020

Choose a reason for hiding this comment

fisx commented Jul 9, 2020

fisx commented Jul 9, 2020

arianvp commented Jul 3, 2020 •

edited by fisx

Loading