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

Store the actual domain username in the user model #795

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbaerike
Copy link

This change includes 2 things:

  • in UpdateUsers(), the previous code converted UserPrincipal objects into username strings, then back into UserPrincipal objects ... losing the original UserPrincipal.Context on the way, and with that any way to determine the actual user domain.
  • the UserPrincipal.UserPrincipalName property does not always contain the correct domain name after the @ character (e.g. in environments where an internal "company.local" domain was connected with Office 365, and all users received the external email address as their UPN. Using the SamAccountName and user.Context.Name instead fixes this.

@cbaerike
Copy link
Author

Just saw issue #738 - this PR will fix it.

@gabriel-alecu
Copy link

Hello!

Any reason why this hasn't been merged yet?
I just installed Bonobo and am facing the same issue.

@willdean
Copy link
Collaborator

Unfortunately this is the kind of change seems almost certain to break something in the very complex matrix of different auth permutations, many of which I'm not actually able to test here.

For example, I am somewhat sceptical that user.SamAccountName + "@" + user.Context.Name is a universally better way to get the name of a principal than user.UserPrincipalName, even though I do recognise that it probably works better for some people in some situations.

Something more adaptable is probably needed here, though I'm not really sure what.

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

Successfully merging this pull request may close these issues.

3 participants