Created attachment 246700 [details] [review]
Implementation to support SSO capabilities to GDM

The above implementation can be used by any other application which requires SSO
Not only oVirt but also for example Virtual Box

Review of attachment 246700 [details] [review]:

I think having oVirt integration is a good idea.  It's not a lot of code and it's pretty cool.  I saw a gnome-boxes demo a couple months back showing oVirt integration there.  cool stuff.  Your commit message needs a little work, since not everyone knows what oVirt is. how about:

gdm: support pre-authenticated logins from oVirt

oVirt is software for managing medium-to-large scale deployments of
virtual machine guests across multiple hosts. It supports a feature
where users can authenticate with a central server and get
transparently connected to a guest system and then automatically get logged
into that guest to an associated user session.

Guests using old versions of GDM support this single-sign-on capability
by means of a greeter plugin, using the old greeter's extension
API.

This commit adds similar support to the gnome-shell based login screen.

How it works:

* The login screen listens for the

  org.ovirt.vdsm.Credentials.UserAuthenticated

  D-Bus signal on the system bus from the

  org.ovirt.vdsm.Credentials

  bus name. The service that provides that bus name is called
  the oVirt guest agent. It is also responsible for interacting
  with the the central server to get user credentials.

* This UserAuthenticated signal passes, as a parameter, the name
  of a PAM service that is specifically set up to integrate with
  the oVirt authentication architecture.

* When the signal is emitted, the login screen tells GDM to
  start user verification using the PAM service.  The
  authentication stack of the service includes a PAM module
  provided by oVirt that securely retrieves user credentials
  from the oVirt guest agent. The PAM module then forwards those
  credentials on to other modules in the stack so, e.g.,
  the user's gnome keyring can be automatically unlocked.

::: js/gdm/sso.js
@@ +1,1 @@
+// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*-

I think i'd call this ovirt.js.  SSO is sort of a loaded acronym.

@@ +3,3 @@
+const Gio = imports.gi.Gio;
+
+const SSOIface = <interface name='org.virt.vdsm.Credentials'>

OVirtCredentialsIface

@@ +9,3 @@
+</interface>;
+
+const SSOInfo = Gio.DBusInterfaceInfo.new_for_xml(SSOIface);

OVirtCredentialsInfo

@@ +11,3 @@
+const SSOInfo = Gio.DBusInterfaceInfo.new_for_xml(SSOIface);
+
+function SSOInterface() {

OVirtCredentials

::: js/gdm/util.js
@@ +121,3 @@
+                                         Lang.bind(this,
+                                                   function(proxy, senderName, [authService]) {
+            this._authService = authService;

this._oVirtAuthService

@@ +134,3 @@
 
+    _ssoBegin: function() {
+        this._cancellable = new Gio.Cancellable();

hmm, so this will get overwritten by in the begin() function when a user clicks on a user in the user list, which isn't right.  Also the cancel() method cancels this._cancellable. I don't think you want cancel() to cancel this do you?  If so, we probably need to update the UI to provide a cancel button while the stack is chugging through the stack. If not, you should just be passing null to the user verifier for its cancellable.

@@ +135,3 @@
+    _ssoBegin: function() {
+        this._cancellable = new Gio.Cancellable();
+        this._hold = new Batch.Hold();

I don't think you are using this hold for anything?  Holds are for when you have a sequence of things tod do that take a while (run asynchronously) and you don't want to start one thing in the sequence until the previous thing finishes. The sequence stalls until the hold is released.

@@ +141,3 @@
+    _ssoUserVerifierGot: function(client, result) {
+        try {
+            this._userVerifier = client.get_user_verifier_finish(result);

you're overwriting the _userVerifier used for password and smartcard requests here. It's actually a singleton so overwriting 'works', but it's a little sloppy.

@@ +156,3 @@
+        this._userVerifier.call_begin_verification(self._authService,
+                                                   this._cancellable,
+                                                   Lang.bind(this, function(obj, result) {

so what happens if some module in the pam stack needs to ask the user a question? For instance, say the user's password is expired and they need to change it. I think maybe it makes sense to funnel into the existing user verifier codepath instead of doing things on the side.  Also, we use this same class for the unlock screen. Is that okay? or are you handling unlock using logind? It might be better to do unlock using this mechanism than logind since kerberos credentials will get refreshed this way, but if you're using logind for unlock you probably need to make sure UserAuthenticated only gets listened for when at the login screen and not at the unlock dialog.

so the code in latest git is somewhat different than what this patch was against. 

It's going to change more for bug 683437 .  That bug should make it easier to see how to merge in oVirt support, since the oVirt support and the smartcard support could work in basically the same way.

(In reply to comment #3)
> so the code in latest git is somewhat different than what this patch was
> against. 
> 
> It's going to change more for bug 683437 .  That bug should make it easier to
> see how to merge in oVirt support, since the oVirt support and the smartcard
> support could work in basically the same way.

Ok will do. I am just a bit pre-occupied at the moment, but I will take care of this as soon as there's a little bit space for it. I will try to get it done by next week.

is this still on your radar?

Yes, sorry for no update on this. I am in the process of preparing a new patch. I had just to focus on some other things first for an upcoming release.

no worries, was just checking in.

Created attachment 256885 [details] [review]
Implementation to support SSO capabilities to GDM

Created attachment 256919 [details] [review]
Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

Review of attachment 256919 [details] [review]:

looking good.  There's just a few minor things.

::: js/gdm/authPrompt.js
@@ +222,3 @@
 
+    _onOvirtCredUserAuthenticated: function() {
+        if (this._userVerifier.serviceIsDefault(GdmUtil.OVIRTCRED_SERVICE_NAME) &&

OVIRTCRED_SERVICE_NAME can never be a default service right?

@@ +460,2 @@
             // We don't need to know the username if the user preempted the login screen
+            // with a smartcard or oVirt cred

maybe "preauthenticated oVirt credentials" or something less terse than oVirt cred

::: js/gdm/ovirt.js
@@ +53,3 @@
+Signals.addSignalMethods(OVirtCredentialsManager.prototype);
+
+let _ovirtManager = null;

the variable should be called _oVirtManager not _ovirtManager.  Also typically these kinds of declarations happen up top.

@@ +55,3 @@
+let _ovirtManager = null;
+
+function getOVirtManager() {

this function should be called getOVirtCredentialsManager() or the class should be called OVirtManager, so there's parity between the two.

::: js/gdm/util.js
@@ +466,3 @@
+                this.answerQuery(serviceName, this._ovirtManager.getToken());
+            }
+            else {

put else on the same line as the close bracket.

@@ +470,3 @@
+                // answers as these information are answered by the pam
+                // service itself
+                this.answerQuery(serviceName, "");

hmm not sure about this.  the pam module shouldn't be asking other questions to begin with right?

@@ +483,3 @@
             return;
 
+        if (this._ovirtCredInfoQueryHook(serviceName, question)) {

The pam module shouldn't be asking non-secret questions to begin with right? I think i'd just drop this.  Then you won't have duplicated comments in the code.

@@ +499,3 @@
+            return;
+        }
+

though, i think maybe you could just put

// oVirt service should be handled internally, without user interaction
if (serviceName == OVIRTCRED_SERVICE_NAME) {
    this.answerQuery(serviceName, this._ovirtManager.getToken());
    return;
}

An argument could be made that there should be a more generic mechanism for credential providers to be able to register themselves to answer pam queries, but since we've only got password, fingerprint, smartcard, and ovirt (with the first three all wanting the user to answer queries) at the moment, i think avoiding the abstraction is fine.  When PIN support lands we could potentially come up with something more sophisticated, since we'll then have two things needing to answer questions en lieu of the user.

::: js/ui/screenShield.js
@@ +20,3 @@
 const Hash = imports.misc.hash;
 const Layout = imports.ui.layout;
+const Ovirt = imports.gdm.ovirt;

this file should be called oVirt.js and the constant should be OVirt

@@ +552,3 @@
+                                          if (this._isLocked) {
+                                              this._liftShield(true, 0);
+                                          }

don't need braces around one line if statements.

Created attachment 256969 [details] [review]
Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

Created attachment 256991 [details] [review]
Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

The patch has been verified with an up-to-date Fedora 20 x86_64 Alpha, where the modified gnome-shell js files have been replaced with these here.

I have tested the following scenarios:

* Single Sign On on GDM Login Screen
* Unlock screen with Shield down
* Unlock screen with Shield already lifted

Review of attachment 256991 [details] [review]:

couple more things..

::: js/gdm/oVirt.js
@@ +48,3 @@
+
+    resetToken: function() {
+        this._token = null;

i think you need to call this when a token expires, otherwise...

::: js/gdm/util.js
@@ +158,3 @@
+
+        if (this._oVirtCredentialsManager.hasToken())
+            this._ovirtCredUserAuthenticated(this._oVirtCredentialsManager.getToken());

this is going to fail-loop for expired tokens.

::: js/ui/screenShield.js
@@ +20,3 @@
 const Hash = imports.misc.hash;
 const Layout = imports.ui.layout;
+const OVirt = imports.gdm.ovirt;

imports.gdm.oVirt;

@@ +549,3 @@
+        this._oVirtCredentialsManager = OVirt.getOVirtCredentialsManager();
+        this._oVirtCredentialsManager.connect('user-authenticated',
+                                      Lang.bind(this, function() {

either put the Lang.bind on the line above and slide everything else back to start below the "." in this._oVirtCredentialsManager or slide everthing forward to line up with 'user-authenticated'

Created attachment 256998 [details] [review]
 Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

Includes now the handling for invalid credentials (including timed out tokens)

Applied the style changes to ui/screenShield

Review of attachment 256998 [details] [review]:

some more issues (I think maybe you forgot to git commit --amend some of your changes?)

::: js/gdm/oVirt.js
@@ +1,1 @@
+// -*- mode: js; js-indent-level: 4; indent-tabs-mode: nil -*-

This file never gets added to the build system. Please make sure it builds and that "make distcheck" passes.

@@ +47,3 @@
+    },
+
+    resetToken: function() {

You still don't call this anywhere

@@ +54,3 @@
+
+function getOVirtCredentialsManager() {
+    if (!_oVirtCredentialsManager) {

don't need braces

::: js/gdm/util.js
@@ +161,3 @@
+
+        this._oVirtCredentialsManager.connect('user-authenticated',
+                                    Lang.bind(this, this._ovirtCredUserAuthenticated));

indentation is wrong.

@@ +288,3 @@
     },
 
+    _ovirtCredUserAuthenticated: function(token) {

oVirt, not ovirt.  Though, I would call this function _oVirtUserAuthenticated without the Cred (we don't usually abbreviate like that, and having the word "credentials" in the middle doesn't read right to me)

@@ +472,3 @@
 
+        if (serviceName == OVIRTCRED_SERVICE_NAME) {
+            // The only question asked by this service is "Token?

missing end quote

::: js/ui/screenShield.js
@@ +552,3 @@
+                                          if (this._isLocked)
+                                              this._liftShield(true, 0);
+                                      }));

still didn't indent this correctly.

Created attachment 257085 [details] [review]
 Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

Indeed that was the case, I forgot to commit it :-( Sorry.

This is the version with the commit.

Comment on attachment 257085 [details] [review]
 Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

still not updated

Created attachment 257232 [details] [review]
 Implementation to support SSO capabilities to GDM - With oVirt Manager as singleton

Attached the wrong file - I made the previous patches from a different folder :(

Sorry

Created attachment 257263 [details] [review]
Implementation to support SSO capabilities to GDM

Added to js/Makefile.am

make distcheck went through cleanly

Pushed with minor style changes