Hi Shaddy,

any news?


Thanks,
Corinna

On Oct 15 18:00, Corinna Vinschen wrote:
> On Oct 16 02:18, Shaddy Baddah wrote:
> > On 15/10/13 23:22, Corinna Vinschen wrote:
> > >On Oct 15 21:21, Shaddy Baddah wrote:
> > >>On 15/10/13 20:08, Corinna Vinschen wrote:
> > >>>[...]
> > >thanks for letting us know and your patch.  I had a look and it looks
> > >good for a start.  You just call the CheckTokenMembership function,
> > >though.  The problem is, you won't know if the process has been started
> > >by a non-admin or by an admin without elevation.  So you always call
> > >ShellExecute if setup is started without admin rights, for non-admins
> > >and non-elevated admins alike, unless the --no-admin option is given.
> > 
> > Yes that is deliberate. Initially I coded this for how I viewed was
> > desirable, that being that it would use CheckTokenMembership to see if
> > it was elevated (or just run as Administrator on XP). If it was not, it
> > would attempt to elevate using "runas" flag. If that was rejected, it
> > would carry on. The --no-admin was still used as per the patch
> > submitted to avoid infinite loop just in-case ShellExecuteEx didn't
> > honour "runas" and just ran setup all over again with no Administrator
> > rights.
> > 
> > However I sensed from previous discussions that really it was desirable
> > to steer users towards running as Admin/elevated privilege. Unless they
> > really knew what they were doing. So much so that building a custom
> > setup.exe was being recommended without much discussion of an
> > alternative... until now.
> > 
> > So this patch tries to be as backwards compatible as possible. That is,
> > unless the user has already explicitly elevated privilege (context
> > menu -> Run as ...), setup.exe forces an attempt. And fails if it does
> > not elevate. The forced attempt can be avoided by explicitly specifying
> > --no-admin.
> 
> Yeah, that makes sense.
> 
> > The patch I provided doesn't match exactly the behaviour of setup on
> > various systems at the moment, in the following ways:
> > 
> > * I don't know how, and did not bother too much about emulating the
> >   "access denied" when the user rejects the privilege elevation.
> 
> I don't think that much of a problem.  If the user refuses the elevation,
> setup won't run.  That's as much message as one needs, I think.
> 
> > * Under x86 (32bit), where the user has performed the rename magic that
> >   normally circumvents the UAC Installer Detection Technology, setup
> >   will still behave as if they hadn't and continue to try to elevate.
> >   IMO this is minor, but if we were going for 100% compatibility, we
> >   could check the exe filename against the same list that cygports uses,
> >   and avoid the privilege elevation attempt:
> > 
> > /usr/share/cygport/lib/src_postinst.cygpart:1112:       find * -type
> > f -executable -a \( -name '*instal*.exe' -o -name '*patch*.exe' -o
> > -name '*setup*.exe' -o -name '*update*.exe' \) -print0 | \
> 
> I don't know if that's really needed anymore.  Your patch provides a
> nice workaround, the --no-admin option.
> 
> > * Under XP (32 bit... I don't know about any other archs (if they
> 
> XP 64 bit exists :)
> 
> >   exist?) or other XP like editions (server???)), for a
> >   non-Administrator the attempt to elevate privilege is also an extra
> >   behaviour that wouldn't have been normally encountered. Again, it
> >   may have been easy to detect that and do something about it too.
> 
> XP and Server 2003 don't know the concept anyway, so calling GetVersion
> and skipping the entire elevation code if the result shows we're running
> on a pre-Vista system should do it.  Something like this:
> 
>   if (LOWORD (GetVersion ()) >= _WIN32_WINNT_VISTA)
>     {
>       ...do the elevate thingy...
>     }
> 
> > Another bit of polish this patch needs is a effective way to close off
> > logging once and for all when we are about to call ShellExecuteEx.
> > Putting in the line:
> > 
> > +          l->msg.clear();
> > 
> > staved off the doubling up of logging when LogFile::exit was being
> > called. But unfortunately a bunch of newlines where being logged
> > instead. I never got around to knocking all that on the head.
> 
> Hmm, that should be fixable.  AFAICS, LogFile::log_save doesn't
> check if tstr is longer than 0:
> 
>   if (tstr[strlen (tstr) - 1] != '\n')
>     f->write ("\n", 1);
> 
> Does it help to write the NL only if there was some non-empty string?
> 
>   if (strlen (tstr) > 0 && tstr[strlen (tstr) - 1] != '\n')
>     f->write ("\n", 1);
> 
> ?
> 
> > >Is that what we want?  Or should the process only be elevated when
> > >started by a non-elevated admin as I proposed.  I'm not sure, really.
> > 
> > If I understand right, you were proposing that if the user was not an
> > Admin at all, then just let them run as they were.
> > 
> > As I wrote earlier, it seemed from discussion that it was desirable (if
> > not pseudo policy) that regular Cygwin users run with privilege
> > elevated. And knowing of various features built into setup that require
> > elevated privilege, eg. replace on boot, I can see why that is.
> 
> Yes, as I said, that makes sense.  I think your approach is fine.
> 
> > But I am happy to be guided either way. I can help out too, but as you
> > can see by the latency in my communication/implementation, I am not
> > a pillar of reliability :-(
> 
> There's no immediate pressure, I think.  Feel free to submit your
> patch when you think you're ready, or if you want some testing.
> 
> 
> Thanks,
> Corinna
> 
> -- 
> Corinna Vinschen                  Please, send mails regarding Cygwin to
> Cygwin Maintainer                 cygwin AT cygwin DOT com
> Red Hat



-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat