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

Configurable identity mapper strategies #5060

Merged
merged 1 commit into from
Oct 15, 2015
Merged

Configurable identity mapper strategies #5060

merged 1 commit into from
Oct 15, 2015

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Oct 11, 2015

https://trello.com/c/cSa9BKUL/512-3-configurable-identity-user-provisioning-strategies

On login, we currently auto-provision a unique username for new identities. Additional strategies should exist.

This PR adds a mappingMethod option to all identity providers:

oauthConfig:
  identityProviders:
  - challenge: false
    login: false
    mappingMethod: "..."
    ...

mappingMethod can be set to the following values:

  • lookup
    • Administrator wants to manually configure identity/user mappings. Login process should only look up existing mappings, not create them
  • generate (prior default behavior)
    • Provision with generation of unique usernames (e.g. "bob", "bob2")
    • This strategy should not be used in combination with LDAP group sync
  • claim (new default)
    • Provision with deterministic username, fail if username is already mapped to another identity
  • add
    • Provision with deterministic username, add to existing user if username is already mapped to another identity
    • Needed when multiple identity providers are configured which identity the same set of users and map to the same usernames

TODO:

Followups:

Fixes #5009

@liggitt
Copy link
Contributor Author

liggitt commented Oct 11, 2015

@stevekuznetsov @deads2k

@liggitt
Copy link
Contributor Author

liggitt commented Oct 11, 2015

[test]

@liggitt liggitt changed the title WIP - Configurable identity mapper strategies Configurable identity mapper strategies Oct 12, 2015
userregistry "github.com/openshift/origin/pkg/user/registry/user"
)

type StrategyClaim struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

GoDoc here might be helpful, before reading further I thought this would be stomp strategy.

@liggitt
Copy link
Contributor Author

liggitt commented Oct 12, 2015

@liggitt
Copy link
Contributor Author

liggitt commented Oct 12, 2015

comments addressed

@liggitt
Copy link
Contributor Author

liggitt commented Oct 13, 2015

The current default (by virtue of being the only option) is "generate", which automatically generates a unique username in case of preferredUsername conflicts between identity providers.

I would like to change the default for new configs and existing configs which don't specify a strategy to use "claim". This would have the following impact on existing configurations:

  • Users that had already logged in and been assigned a uniquified username would be unaffected
  • New logins that encountered a conflict with an existing username would fail.
    • If a cluster admin really wanted the deconflict behavior, they could update their config to specify "generate"
    • If a cluster admin wanted both identity providers to map to the same user, they could update their config to specify "add"

@smarterclayton, this is technically a behavior change, but I haven't encountered anyone using multiple identity providers who wanted the generate behavior... ruling on changing this?

@smarterclayton
Copy link
Contributor

+1 do it

On Tue, Oct 13, 2015 at 2:50 PM, Jordan Liggitt [email protected]
wrote:

The current default (by virtue of being the only option) is "generate",
which automatically generates a unique username in case of
preferredUsername conflicts between identity providers.

I would like to change the default for new configs and existing configs
which don't specify a strategy to use "claim". This would have the
following impact on existing configurations:

  • Users that had already logged in and been assigned a uniquified
    username would be unaffected
  • New logins that encountered a conflict with an existing username
    would fail.
    • If a cluster admin really wanted the deconflict behavior, they
      could update their config to specify "generate"
    • If a cluster admin wanted both identity providers to map to the
      same user, they could update their config to specify "add"

@smarterclayton https://github.com/smarterclayton, this is technically
a behavior change, but I haven't encountered anyone using multiple identity
providers who wanted the deconflict behavior... ruling on changing this?


Reply to this email directly or view it on GitHub
#5060 (comment).

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5826/)

@liggitt
Copy link
Contributor Author

liggitt commented Oct 13, 2015

Tagging with compatibility, deserves a mention in release notes

@liggitt
Copy link
Contributor Author

liggitt commented Oct 14, 2015

@deads2k PTAL


// StrategyTypeReplace associates a new identity with a user with the identity's preferred username,
// replacing any existing identities associated with the user
StrategyTypeReplace UserStrategyType = "replace"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would anyone ever choose this? It seems crazy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@liggitt
Copy link
Contributor Author

liggitt commented Oct 15, 2015

comments addressed, still need to split up tests

@liggitt
Copy link
Contributor Author

liggitt commented Oct 15, 2015

comments addressed

@liggitt
Copy link
Contributor Author

liggitt commented Oct 15, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3635/) (Image: devenv-fedora_2472)

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 75710d5

@liggitt
Copy link
Contributor Author

liggitt commented Oct 15, 2015

@danmcp, in https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3634/:

remote: error: unable to create temporary file: File exists        
remote: fatal: failed to write object        
error: unpack failed: unpack-objects abnormal exit
To verifier:/data/src/github.com/openshift/source-to-image-bare
 ! [remote rejected] master -> master (unpacker error)
error: failed to push some refs to 'verifier:/data/src/github.com/openshift/source-to-image-bare'

@liggitt
Copy link
Contributor Author

liggitt commented Oct 15, 2015

re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 75710d5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants