SLING-12122 - Add unit-test creating group with rep:externalId property

[jsedding]

No description provided.

[anchela]

hi @jsedding , what exactly is the reason for creating this test? is it about illustrating that it's possible the create a user with a protected external identity?
since tests also act as a way to document the code, i would strongly recommend to also add some explanation that the externalId is a system-maintained property created upon sychronization from an external identity provider. adding external identities manually somewhat defeats the purpose and is relying on implementation details that may change depending on the configured sync-handler.

[anchela]

i would recommend to create a proper oak security setup

[jsedding]

@anchela the test was intended to validate and document that it is possible to set properties on a group/user within the same Session#save() call. I.e. that there are no intermediate saves, which might break some use cases, like e.g. setting the rep:externalId property (which is NOT recommended, because it is an implementation detail and should only be managed by Oak's external authentication code).

Instead of creating a complete security setup, which I feel is not trivial, I could implement a custom Oak Validator that has an analogous check. That way, the test also doesn't document "bad practice".

Would that work for you?

[anchela]

hi @jsedding , creating a somewhat reasonable security setup should not be too complicated. you could e.g. use the default created by SecurityProviderBuilder (an not calling with with null values) and the benefit from the test-helper class SecurityProviderHelper to inject the additional configuration.

that's probably easier than writing your own validator. what i meant about the best practices: i would add a comment to the test class mentioning that manually creating the rep:externalId is making assumptions about implementation details that may change. so, just add a word of caution :-)

the sync-mechanims defined in oak-auth-external comes with API to sync users/groups outside of the regular login flow.... that's just not supported by all IDP implementations.... so an attempt to manually sync users/groups is probably the result of an imcomplete IDP implementation. so addressing that would likely be the better option (and avoid the issue about impl-details).

[jsedding]

hi @anchela, I tried to address your comments and suggestions in my latest commit. I'm not 100% sure about the SecurityProvider setup. I am trying to rely on its defaults, like before, but now I'm doing it explicitly, without violating any nullability annotations.

Regarding the IDP implementation, I agree that would help, if the groups are transparently sync'ed on demand. I.e. if the external groups appear via Oak's APIs in the same way local groups do. Then it should be possible to just set ACLs for them via repoinit, and the groups would automatically, and transparently, be sync'ed.

This test case more in order to document current behaviour of repoinit, than it is about a solution to the sync problem. I have dismissed this approach as bad practice, and will not recommend it to anyone.

[anchela]

hi @jsedding , i might be mistaken but i believe lines 118-129 can be simplified by using the SecurityProviderHelper to inject just the ExternalPrincipalConfiguration in addition to the default.
that utility class is located in the oak-core test bundle. then you don't have to construct a new SecurityProvider again. wdyt?

[jsedding]

I'll try that!

[jsedding]

Ok, that was easy :)

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication