Re: Review Request 117174: Fix installing and removing desktop plasma theme packages.

[Andrei Amuraritei]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117174/
---

(Updated April 1, 2014, 12:18 a.m.)


Review request for kdelibs, Albert Astals Cid, Aaron J. Seigo, David Faure, and 
Ian Monroe.


Changes
---

So I've modified it to search for the metadata.desktop file in the first 
subdirectory only.


Bugs: 149479
http://bugs.kde.org/show_bug.cgi?id=149479


Repository: kdelibs


Description
---

Even though the bug appears RESOLVED it is not.

Minor hack to packagestructure.cpp to search for the metadata.desktop file 
recursively. This helps with installing desktop themes and removing them.
I have tested this on kdelibs 4.13 compiled with kdesrc-build. When testing 
themes ignore SoftSand for example, it's metadata.desktop is not properly 
formatted. There are others too which are not formatted which I guess could be 
fixed by setting a new format standard, maybe even a check package script to 
check new uploads on kde-look.org.


Diffs
-

  plasma/packagestructure.cpp 71148e1 

Diff: https://git.reviewboard.kde.org/r/117174/diff/


Testing
---

Compiled, run systemsettings, go to Desktop Themes, install / remove away. Some 
themes are broken so they won't work (not install).


File Attachments (updated)


Limit the extra search to first subdirectory.patch
  
https://git.reviewboard.kde.org/media/uploaded/files/2014/04/01/c9b4a3ee-bd3b-498a-b18c-a4eb13b349d3__0002-Limit-the-search-to-include-the-first-directory-only.patch


Thanks,

Andrei Amuraritei

Re: Review Request 117157: Unlock session via DBus

[Ingo Klöcker]

On Sunday 30 March 2014 15:36:29 Thiago Macieira wrote:
> Em seg 31 mar 2014, às 00:01:13, Thomas Lübking escreveu:
> > > If they can gain access to a TTY login we are already screwed
> > 
> > leaving aside the present issue (/MainApplication quit being exposed
> > to dbus) and given ptrace (gdb solution) is denied: in how far?
> > (beyond killing the session, ie. being a nasty little jerk
> 
> They can already access all of the other applications and the user's
> files.

Exactly. And that's why I agree with the people who argue in favor of 
unlocking the session via DBus.

AFAIU the threat model which not providing this feature protects against 
is that some user locks his KDE session, but forgets to also lock some 
other local or remote session. IMHO this is a ridiculous threat model. 
There are so many possible attack vectors if an attacker has full access 
to the user's files that it doesn't really make any sense to try to 
protect some other session from the attacker.

(In the past, I have already locked my KDE session, but left a TTY 
session unlocked. Doh! But I also had to kill the screen locker several 
times to re-gain access to my KDE session because for some reason the 
screen locker didn't let me unlock the session anymore. So, I've been in 
both situations and I definitely prefer to have the ability to unlock 
the KDE session via DBus.)

Just my 2 cents.


Regards,
Ingo


signature.asc
Description: This is a digitally signed message part.

Re: Review Request 117157: Unlock session via DBus

[Thiago Macieira]

Em seg 31 mar 2014, às 08:55:05, Martin Gräßlin escreveu:
> Personally I have to disagree. To me the graphical login is a an asset
> which  needs to be protected in a stronger way. Access to a tty should not
> equal access to the graphical system. The fact that X is broken should not
> result in us adding further insecurities which need to be fixed up once we
> transit to Wayland. The opposite has to happen: all the small security
> issues we let in, because X was already broken need to get fixed. This
> thread turned into a nice TODO list

I'm not asking for it to be insecure. I've already authenticated by logging in 
to the virtual console. So let me unlock my session via D-Bus.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.

Re: KF5: Parsing times with timezone abbreviations

[Kevin Kofler]

David Jarvie wrote:
> They can't just be ignored for small countries, since they may contain a
> daylight savings time indication.

Except for one "evil" hour every year, a given local time with the date 
included is either necessarily DST or necessarily non-DST.

That said, sure, if we are about to look at the abbreviation anyway, we may 
just as well always do it.

Now for that BBC weather RSS issue, I figured out that it apparently always 
reports the pubDate (which is unfortunately not the real date&time the 
conditions are from, but the date&time the RSS was requested at) in the 
local time of the place, given in a format like +1100. (That's the offset 
for Sydney, at which I looked because of that Australian vs. US timezone 
ambiguity.) So we could probably work with that, and hope that the reported 
observation time is really always given in the same timezone as that pubDate 
(and that this does not change in the future, e.g. to a pubDate in UTC).

But what do we do with other sources using this (admittedly broken) type of 
timezone specification? There's even RFC-822 that specifies those 3-letter 
specifications as one of the allowed formats, but only for "UT"="GMT"=UTC 
and for the continental US time zones ([PMCE][SD]T). As a data point of what 
other implementations do with that, the BSD strptime actually tries to 
support RFC-822 completely if you pass "%z" (but does not support non-US 
three-letter abbreviations), the glibc one didn't bother at all last I 
checked (its "%z" supports only the +1100 type (ISO 8601) format).

Kevin Kofler

Re: Review Request 117174: Fix installing and removing desktop plasma theme packages.

[Sebastian Kügler]

> On March 31, 2014, 4:04 p.m., Sebastian Kügler wrote:
> > plasma/packagestructure.cpp, line 653
> > 
> >
> > { go on the next line (here and elsewhere)
> 
> Andrei Amuraritei wrote:
> Sorry don't get what you mean here?

Yeah, I meant on the same line, so

while (...) {

Same for if (...) {

Sorry for causing confusion.


> On March 31, 2014, 4:04 p.m., Sebastian Kügler wrote:
> > plasma/packagestructure.cpp, line 659
> > 
> >
> > This is going to be funny if you have multiple packages in the path 
> > you're specifying here, it will rely on directory listing ordering then -- 
> > perhaps not what you want. If it's random anyway, why parse all, and not 
> > just pick the first one?
> 
> Andrei Amuraritei wrote:
> Hi there, so this // Question: Would it be better to search just the 
> first level dirs for metadata.desktop file?  would be yes then ?
> Or do you mean just grab the first "Directory" found and search there ?

I've done something similar in Plasma Next, and there it tries to find a 
metadata.desktop file in the current directory, if that fails, it searches the 
subdirectories, but not recursively. In this case, we want to be a bit 
flexible, but not go overboard scanning whole file system structures just 
because a wrong path has been issued.


- Sebastian


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117174/#review54702
---


On March 30, 2014, 1:14 p.m., Andrei Amuraritei wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117174/
> ---
> 
> (Updated March 30, 2014, 1:14 p.m.)
> 
> 
> Review request for kdelibs, Albert Astals Cid, Aaron J. Seigo, David Faure, 
> and Ian Monroe.
> 
> 
> Bugs: 149479
> http://bugs.kde.org/show_bug.cgi?id=149479
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Even though the bug appears RESOLVED it is not.
> 
> Minor hack to packagestructure.cpp to search for the metadata.desktop file 
> recursively. This helps with installing desktop themes and removing them.
> I have tested this on kdelibs 4.13 compiled with kdesrc-build. When testing 
> themes ignore SoftSand for example, it's metadata.desktop is not properly 
> formatted. There are others too which are not formatted which I guess could 
> be fixed by setting a new format standard, maybe even a check package script 
> to check new uploads on kde-look.org.
> 
> 
> Diffs
> -
> 
>   plasma/packagestructure.cpp 71148e1 
> 
> Diff: https://git.reviewboard.kde.org/r/117174/diff/
> 
> 
> Testing
> ---
> 
> Compiled, run systemsettings, go to Desktop Themes, install / remove away. 
> Some themes are broken so they won't work (not install).
> 
> 
> Thanks,
> 
> Andrei Amuraritei
> 
>

Re: Review Request 117174: Fix installing and removing desktop plasma theme packages.

[Andrei Amuraritei]

> On March 31, 2014, 4:04 p.m., Sebastian Kügler wrote:
> > plasma/packagestructure.cpp, line 659
> > 
> >
> > This is going to be funny if you have multiple packages in the path 
> > you're specifying here, it will rely on directory listing ordering then -- 
> > perhaps not what you want. If it's random anyway, why parse all, and not 
> > just pick the first one?

Hi there, so this // Question: Would it be better to search just the first 
level dirs for metadata.desktop file?  would be yes then ?
Or do you mean just grab the first "Directory" found and search there ?


> On March 31, 2014, 4:04 p.m., Sebastian Kügler wrote:
> > plasma/packagestructure.cpp, line 653
> > 
> >
> > { go on the next line (here and elsewhere)

Sorry don't get what you mean here?


- Andrei


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117174/#review54702
---


On March 30, 2014, 1:14 p.m., Andrei Amuraritei wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117174/
> ---
> 
> (Updated March 30, 2014, 1:14 p.m.)
> 
> 
> Review request for kdelibs, Albert Astals Cid, Aaron J. Seigo, David Faure, 
> and Ian Monroe.
> 
> 
> Bugs: 149479
> http://bugs.kde.org/show_bug.cgi?id=149479
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Even though the bug appears RESOLVED it is not.
> 
> Minor hack to packagestructure.cpp to search for the metadata.desktop file 
> recursively. This helps with installing desktop themes and removing them.
> I have tested this on kdelibs 4.13 compiled with kdesrc-build. When testing 
> themes ignore SoftSand for example, it's metadata.desktop is not properly 
> formatted. There are others too which are not formatted which I guess could 
> be fixed by setting a new format standard, maybe even a check package script 
> to check new uploads on kde-look.org.
> 
> 
> Diffs
> -
> 
>   plasma/packagestructure.cpp 71148e1 
> 
> Diff: https://git.reviewboard.kde.org/r/117174/diff/
> 
> 
> Testing
> ---
> 
> Compiled, run systemsettings, go to Desktop Themes, install / remove away. 
> Some themes are broken so they won't work (not install).
> 
> 
> Thanks,
> 
> Andrei Amuraritei
> 
>

Re: Review Request 117174: Fix installing and removing desktop plasma theme packages.

[Sebastian Kügler]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117174/#review54702
---



plasma/packagestructure.cpp


{ go on the next line (here and elsewhere)



plasma/packagestructure.cpp


This is going to be funny if you have multiple packages in the path you're 
specifying here, it will rely on directory listing ordering then -- perhaps not 
what you want. If it's random anyway, why parse all, and not just pick the 
first one?


- Sebastian Kügler


On March 30, 2014, 1:14 p.m., Andrei Amuraritei wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117174/
> ---
> 
> (Updated March 30, 2014, 1:14 p.m.)
> 
> 
> Review request for kdelibs, Albert Astals Cid, Aaron J. Seigo, David Faure, 
> and Ian Monroe.
> 
> 
> Bugs: 149479
> http://bugs.kde.org/show_bug.cgi?id=149479
> 
> 
> Repository: kdelibs
> 
> 
> Description
> ---
> 
> Even though the bug appears RESOLVED it is not.
> 
> Minor hack to packagestructure.cpp to search for the metadata.desktop file 
> recursively. This helps with installing desktop themes and removing them.
> I have tested this on kdelibs 4.13 compiled with kdesrc-build. When testing 
> themes ignore SoftSand for example, it's metadata.desktop is not properly 
> formatted. There are others too which are not formatted which I guess could 
> be fixed by setting a new format standard, maybe even a check package script 
> to check new uploads on kde-look.org.
> 
> 
> Diffs
> -
> 
>   plasma/packagestructure.cpp 71148e1 
> 
> Diff: https://git.reviewboard.kde.org/r/117174/diff/
> 
> 
> Testing
> ---
> 
> Compiled, run systemsettings, go to Desktop Themes, install / remove away. 
> Some themes are broken so they won't work (not install).
> 
> 
> Thanks,
> 
> Andrei Amuraritei
> 
>

Re: KF5: Parsing times with timezone abbreviations

[David Jarvie]

On Mon, March 31, 2014 11:53 am, Kevin Kofler wrote:
> Thiago Macieira wrote:
>> Time zone abbreviations are useless, since they are not unique. Simply
>> strip them out of your string before passing to QDateTime.
>
> Nice theory, but there is no other way to know what time this actually is.
> Unless you can offer a mapping from latitude and longitude to timezone, or
> a way to automatically figure it out from "place name, country" (which is
> especially fun for those countries that span multiple time zones, because
> the place name can be a small town somewhere).
>
> I do see the problem, e.g. I get "EST" as the timezone for Sydney,
> Australia, which is obviously not the same as the US "EST". I suppose
> KDateTime will do the wrong thing for that. :-(
>
> Maybe we need a (timezone abbreviation, country) → timezone map (and
an API
> where I can just feed in the time including the abbreviation and the
country
> name and get a correct QDateTime; heck, for most countries, the
abbreviation
> could be ignored entirely, it only matters for huge countries such as the
> USA or Russia)?

They can't just be ignored for small countries, since they may contain a
daylight savings time indication.

-- 
David Jarvie.
KDE developer.
KAlarm author - http://www.astrojar.org.uk/kalarm

Re: Tests still failing in 4.13

[Vishesh Handa]

On Monday, March 31, 2014 01:42:01 AM Albert Astals Cid wrote:
> Hello people, at the moment we have various 4.13 projects failing.

Hello

> I am also open to be convinced that the test is right and that it's
> unfixable to run correctly on jenkins, but make sure you are really
> convincing if you resort to that ;-)
>
> nepomuk-core (1 failing test)
>  http://build.kde.org/view/KDE%20SC%20stable/job/nepomuk-core_stable/323/tes
> tReport/
>

The failing test is actually passing, but setting up the entire test 
environment and running the test doesn't always seem to work. Could we have an 
exception for this?

I'm not too inclined on trying to fix it considering that there have been no 
changes in Nepomuk for 4.13

-- 
Vishesh Handa

Re: KF5: Parsing times with timezone abbreviations

[Kevin Kofler]

Thiago Macieira wrote:
> Time zone abbreviations are useless, since they are not unique. Simply
> strip them out of your string before passing to QDateTime.

Nice theory, but there is no other way to know what time this actually is. 
Unless you can offer a mapping from latitude and longitude to timezone, or a 
way to automatically figure it out from "place name, country" (which is 
especially fun for those countries that span multiple time zones, because 
the place name can be a small town somewhere).

I do see the problem, e.g. I get "EST" as the timezone for Sydney, 
Australia, which is obviously not the same as the US "EST". I suppose 
KDateTime will do the wrong thing for that. :-(

Maybe we need a (timezone abbreviation, country) → timezone map (and an API 
where I can just feed in the time including the abbreviation and the country 
name and get a correct QDateTime; heck, for most countries, the abbreviation 
could be ignored entirely, it only matters for huge countries such as the 
USA or Russia)?

Kevin Kofler

Re: Review Request 112294: Implement multi-seat support in KDM

[Oswald Buddenhagen]

> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/dm.c, line 1397
> > 
> >
> > that seems questionable to me. why are you re-defining the display to 
> > be permanent? when the seat goes away, kdm won't know what to do with it.
> 
> Stefan Brüns wrote:
> When a seat goes away, rStopDisplay(d, DS_RESERVE) is called - maybe a 
> "d->displayType = dLocal | dReserve" is missing for these cases.
> But as long as the seat exists, you want it to behave like a static 
> display (restart server after session exit), so dPermanent is IMHO correct.
> 
> Oswald Buddenhagen wrote:
> i see what you mean ...
> still, it seems wrong to change the display type just so. this is 
> supposed to be a constant. this is of course a consequence of abusing reserve 
> displays in the first place - technically, there should be a dDynamic display 
> type. if having reserve displays on dynamic seats is technically possible at 
> all, this would also need some thought.
> 
> Stefan Brüns wrote:
> So, what approach to take here:
> 1. Its somewhat ugly, but it works, so use it.
> 2. Introduce a dDynamic display type, and make it behave like dPermanent.
>   a) add a DynamicServers option for this
>   b) create list of displays on the fly.
> 
> Reserve displays on dynamic seats is not possible currently, you cannot 
> change the VT. For this to work you need support for systemd-login, but this 
> is more serious surgery.

i would prefer 2.a. it shouldn't be much work.
i had shortly pondered 2.b myself, but then you'd need a new factory concept, 
and the configuration would be different, so i discarded it.


> On March 28, 2014, 10:59 a.m., Oswald Buddenhagen wrote:
> > kdm/backend/server.c, line 79
> > 
> >
> > why the redundant supply of the seat as a layout?
> 
> Stefan Brüns wrote:
> There are no config matches taking the "seat" into account, so the only 
> possibility to apply a specific config is to use "layout".
> 
> Oswald Buddenhagen wrote:
> something is wrong about this.
> what does the -seat option do in the first place? if it is sufficient to 
> actually launch the server correctly, then that's all that should be done - 
> the user can manually specifiy ServerArgs if he needs to configure the layout 
> for some reason. otherwise every user is forced to actually have a "seatN" 
> layout in his config.
> also, cannot the layout determine the seat? seems a bit stupid that these 
> are entirely orthogonal.
> 
> Stefan Brüns wrote:
> The seat option is used for matching devices from udev, i.e. input 
> devices, GPUs, ... It specifies *which* devices to use, but now *how to 
> configure* these.
> 
> Unfortunately the config files lack a "MatchSeat" option, so the only 
> possible options are either specifying a config file (or directory) or a 
> layout. There is a patch from Oleg Samarin floating around which adds 
> MatchSeat, but this is not upstream so currently no option.
> 
> If a layout is specified but not available, the default layout is chosen, 
> so no problem here.
> 
> ServerArgs is no alternative as it is specific to a display, but the 
> seats will use a more or less random display, dependent on when they become 
> available (much like Reserve servers, which take the first display available).

hrmpf. again somebody did half a job, and others need to make workarounds to 
make it usable.

it may be actually possible to integrate this nicely with kdm's super-complex 
config system.
it already supports a display-identifying mechanism, the so-called display 
class. you can have groups that look like [X-*:*_foos-Core]. you can even have 
StaticServers=:0_foos,:1_foos,:3_bars.

one way to make use of this would be simply replacing the display class with 
@seat1 or something like that. the actual display class is pretty much 
meaningless for local displays anyway (it was introduced to group remote 
(predominantly xdmcp) displays, which could have different physical properties 
for example).

another way would be making local displays look like @seat1:1. that makes a lot 
of sense - the seat kinda is the "host", even if of a different kind. this 
approach might be a bit more complex, though (it would need special handling of 
the host part, while just replacing the display class would be practically zero 
effort).

that way one could even have static secondary seats (something people had 
hacked a decade ago already). and it would have potential for supporting 
reserve displays on secondary seats 
(ReserveServers=:1,:2,:3,@seat1:5,@seat1:6). DynamicServers could use both 
static (@seat1:4,@seat2:7) and dynamic (:4,:7) seat to display assignment.
if the display class based approach was chosen, this would be :4_@seat1 and so 
on.

this seems quite complex

Re: Help splitting kde-workspace

[Martin Klapetek]

On Mon, Mar 31, 2014 at 6:50 AM, Aleix Pol  wrote:

>
> Hi,
> I've been looking into the issue with Nicolás (aka PovAddict) and we
> managed to figure out all repositories history except for plasma-workspace
> and plasma-desktop. The problem was that not only they were moved now, but
> they were moved back in the day from kde-base when kde-workspace was
> created. It seems like something possible to solve but it didn't look like
> something that would pay off, so I decided to graft those. If somebody has
> great problems, I'd suggest to use the occasion and suggest a sane solution
> before we all start working on it, otherwise I think that grafts are a good
> solution (you can use the time I'm sending this e-mail as a proof we tried
> otherwise).
>
> Other than that, the rest of repositories have been pushed with full
> history (AFAIK) and it seems to work well. If there's any left-over issue,
> there will "always" be the kde-workspace repository for reference.
>
> I just pushed as well changes in the dependency-data-kf5-qt5 file so that
> when one tries to compile "plasma-desktop" gets the correct dependencies;
> note that you shouldn't be building kde-workspace anymore. It worked on my
> system, I hope it will work on most systems.
>
> I hope this doesn't distort your workflow too much and that we can soon
> start to take advantage of the new, leaner, organization of the project.
>

Thanks for your hard work guys (you too Alex), well done.

Kudos & cookies

Cheers
-- 
Martin Klapetek | KDE Developer

Re: Re: Review Request 117157: Unlock session via DBus

[Martin Gräßlin]

On Sunday 30 March 2014 18:06:52 Thiago Macieira wrote:
> > Leaving access to an open shell is certainly bad enough - beyond question.
> > The question is whether gaining direct access to a running session and
> > random open clients (and leaving the stage untraced) is more valuable and
> > thus worth pretection.
> 
> We're assuming that the attacker already gained access to the session at
> this point. For example, if you've left a logged in shell in a virtual
> console. At that point, it's already game over.
> 
> Since that is so, let's stop trying to cover the sun with a sieve. Instead,
> let's make the life of developers and helpgivers easier: let there be an
> unlock command via D-Bus, without transiting the password again.

Personally I have to disagree. To me the graphical login is a an asset which 
needs to be protected in a stronger way. Access to a tty should not equal 
access to the graphical system. The fact that X is broken should not result in 
us adding further insecurities which need to be fixed up once we transit to 
Wayland. The opposite has to happen: all the small security issues we let in, 
because X was already broken need to get fixed. This thread turned into a nice 
TODO list :-)

Our default should be to be secure and not to allow to be insecure because 
developers need to have an easy way to fix their setup.

Btw. the greeter theme allows to be changed and the theme does not require 
authentication. It's up to the greeter theme to decide how to grant access. We 
even ship one theme for Plasma Active which does not provide any security. For 
use cases which require to allow quitting the locker through DBus this should 
be provided through the greeter theme, not through the lock process. If one 
wants to make the system less secure it should have to be explicitly changed 
and should require more privs.

Cheers
Martin

signature.asc
Description: This is a digitally signed message part.