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

WebUI: Login for AD Users #639

Closed
wants to merge 3 commits into from
Closed

WebUI: Login for AD Users #639

wants to merge 3 commits into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Mar 22, 2017

Allows login as AD user. AD Users has its own menu specification as there is visible only its profile and list of active IPA users.

https://pagure.io/freeipa/issue/3242

Copy link
Member

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

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

Comments inline

this.app_widget.set('fullname', fullname);
},

on_profile: function() {
routing.navigate(['entity', 'user', 'details', [IPA.whoami.uid[0]]]);
var entity = IPA.whoami.metadata.object;
Copy link
Member

Choose a reason for hiding this comment

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

This whole method can be replaced by line:

routing.navigate(['entity', IPA.whoami.metadata.object, 'details',
                    IPA.whoami.metadata.arguments]);

var fullname = '';
var entity = whoami.metadata.object;

if (entity === 'user') fullname = whoami.data.krbprincipalname[0];
Copy link
Member

Choose a reason for hiding this comment

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

Previously, it was cn[0],, e.g. "John Doe" but now with krbprincipalname which displays e.g. jdoe@REALM.TEST. Unfortunately user-show login doesn't return cn nor displayname without --all. Now I'm torn if this is a good change or not.

Copy link
Author

Choose a reason for hiding this comment

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

I can call user-show login with all and change the behavior to try to use cn and in case that there is no cn I can use krbprincipalname. Would it be better?

method: data.details || data.command,
args: data.arguments,
options: function() {
$.extend(data.options, {all: true});
Copy link
Member

Choose a reason for hiding this comment

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

data.options is undefined at least in IPA user case so it won't add the all:true option

});
reset_dialog.succeeded.attach(function() {
var command = IPA.get_whoami_command();
var orig_on_success = command.on_success;
command.on_success = function(data, text_status, xhr) {
orig_on_success.call(this, data, text_status, xhr);
orig_on_success.call(this, data.result, text_status, xhr);
IPA.update_password_expiration();
Copy link
Member

Choose a reason for hiding this comment

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

IPA.update_password_expiration() method wasn't update to new IPA.whoami object content

Pavel Vomacka added 3 commits March 23, 2017 15:52
WebUI checks whether principal name of logged user and principal name
in each command is equal. As KDC for our principals is case insensitive
- it does make sense to switch this check also into case insensitive.
So both principals are reformated to lower case and then
compared.

Part of: https://pagure.io/freeipa/issue/3242
AD user can do only several things. One of those which are not
allowed is to reset password to itself. Therefore we need to be
able to turn of a item in dropdown menu. In our case
'Password reset' item. Function which disable menu item and detach
the listener on click from the item specified by its name was added.

Part of: https://pagure.io/freeipa/issue/3242
After login, method user-find --whoami was called which cannot be
called for AD users. That method was replaced by ipa whoami command
and sequential command according to result of ipa whoami. AD user
can now be logged in.

AD users have new menu definition which contains only list of IPA
users and profile page of AD user - "User ID Override".

This commit also fixes several places where IPA.whoami object was
used, because its structure was also changed. It now contains two
objects. First one is stored in 'metadata' property and stores
result from ipa whoami (type of object, command which should be
called for showing detailed data about currently logged entity, etc).
The second one is stored in 'data' property which stores result of
_show command for currently logged entity.

https://pagure.io/freeipa/issue/3242
@pvomacka
Copy link
Author

I implemented all comments which you proposed and I also changed menu of AD user selfservice - I removed User tab and renamed User ID override to Profile.

@abbra
Copy link
Contributor

abbra commented Mar 24, 2017

LGTM and works just fine:

@pvoborni
Copy link
Member

The code changes looks good to me. ACK given that it works fine (@abbra 's comment).

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Mar 24, 2017
@martbab martbab added the pushed Pull Request has already been pushed label Mar 27, 2017
@martbab
Copy link
Contributor

martbab commented Mar 27, 2017

master:

  • 1dcdcd1 WebUI: check principals in lowercase

  • 2992e3c WebUI: add method for disabling item in user dropdown menu

  • ceedc3f WebUI: Add support for login for AD users
    ipa-4-5:

  • bee9c9f WebUI: check principals in lowercase

  • 01a0a38 WebUI: add method for disabling item in user dropdown menu

  • 228e039 WebUI: Add support for login for AD users

@martbab martbab closed this Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants