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.

Sign up for GitHub

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

Jump to bottom

ZEPPELIN-3664: ActiveDirectoryGroupRealm returns "cn" instead of "userPrincipalName" for note permission auto completion #3098

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

ZEPPELIN-3664: ActiveDirectoryGroupRealm returns "cn" instead of "userPrincipalName" for note permission auto completion #3098

wants to merge 1 commit into from

Conversation

florian-trimble
Copy link

  • Return "userPrincipalName" from ActiveDirectoryGroupRealm when searching for users.
  • Added unit test for search functionality.

What is this PR for?

Fix for https://issues.apache.org/jira/browse/ZEPPELIN-3664
Return the "userPrincipalName" instead of the "cn" (common name) from ActiveDirectoryGroupRealm when searching users to set note permissions.

What type of PR is it?

[Bug Fix]

Todos

What is the Jira issue?

How should this be tested?

Steps to reproduce

  1. Configure connection to AD with org.apache.zeppelin.realm.ActiveDirectoryGroupRealm
  2. Login to Zeppelin
  3. Create new notebook
  4. Edit the permissions and start typing the name of a AD user to one of the permission fields
  5. Select one of the suggestions and save
  6. Check the content of "notebook-authorization.json"

Expected result

  • The "userPrincipalName" of the user is shown in the suggestion box and written to "notebook-authorization.json" (see screenshot attached).

Actual result

  • The "cn" (common name) of the user is shown in the suggestion box and written to "notebook-authorization.json".

Screenshots

notepermission

Questions:

  • Does the licenses files need update? - No
  • Is there breaking changes for older versions? - No
  • Does this needs documentation? - No

ZEPPELIN-3664:
e909e88 
* Return "userPrincipalName" from ActiveDirectoryGroupRealm when searching for users.
* Added unit test for search functionality.
@mebelousov
Copy link
Contributor

@FSteinle our team use authorization with parameter
activeDirectoryRealm.principalSuffix = @company.com
Thus email is not necessary. Could it possible to use mailNickName, for example?

BTW, userPrincipalName is good.

@florian-trimble
Copy link
Author

@mebelousov: Basically this is not related to emails but to the userPrincipalName. The principalSuffix parameter makes sense, but doesn't address this issue. In our case we wanna keep the suffix to be sure it's really unique. Not all users of the organization are in the same AD branch.

@zjffdu
Copy link
Contributor

zjffdu commented Aug 1, 2018

\cc @prabhjyotsingh

prabhjyotsingh
prabhjyotsingh reviewed Aug 1, 2018
View reviewed changes
zeppelin-server/src/main/java/org/apache/zeppelin/realm/ActiveDirectoryGroupRealm.java
@@ -265,7 +265,7 @@ protected AuthorizationInfo buildAuthorizationInfo(Set<String> roleNames) {
NamingEnumeration ae = attrs.getAll();
while (ae.hasMore()) {
Attribute attr = (Attribute) ae.next();
if (attr.getID().toLowerCase().equals("cn")) {
if (attr.getID().toLowerCase().equals("userprincipalname")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this (cn/userprincipalname/or any other parameter) configurable?

Copy link
Author

Choose a reason for hiding this comment

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

From my point of view "cn" just doesn't work. So I don't see a need having this option.
When adding a new notebook also the upn is set as owner (or also for readers/writers/runners depending on the config). So why should one use the "cn" here when searching?
It also actually is the upn we're searching for in this method ("userPrincipalName=" + containString + "").

But maybe I do not have the full picture...

prabhjyotsingh
prabhjyotsingh approved these changes Aug 2, 2018
View reviewed changes
Copy link
Contributor

@prabhjyotsingh prabhjyotsingh left a comment

Choose a reason for hiding this comment

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

LGTM

@zjffdu
Copy link
Contributor

zjffdu commented Aug 2, 2018

@FSteinle Could you update the PR title ?

@florian-trimble florian-trimble changed the title ZEPPELIN-3664 ZEPPELIN-3664: ActiveDirectoryGroupRealm returns "cn" instead of "userPrincipalName" for note permission auto completion Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prabhjyotsingh prabhjyotsingh prabhjyotsingh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@florian-trimble @mebelousov @zjffdu @prabhjyotsingh