Differential D21795 Diff 76071 src/backends/polkit-1/Polkit1Backend.cpp

Changeset View

Standalone View

View Options

src/backends/polkit-1/Polkit1Backend.cpp

1/* 1​/*
2* Copyright (C) 2008 Nicola Gigante <nicola.gigante@gmail.com> 2​* Copyright (C) 2008 Nicola Gigante <nicola.gigante@gmail.com>
3* Copyright (C) 2009 Radek Novacek <rnovacek@redhat.com> 3​* Copyright (C) 2009 Radek Novacek <rnovacek@redhat.com>
4* Copyright (C) 2009-2010 Dario Freddi <drf@kde.org> 4​* Copyright (C) 2009-2010 Dario Freddi <drf@kde.org>
5* Copyright (C) 2020 David Edmundson <davidedmundson@kde.org> 5​* Copyright (C) 2020 David Edmundson <davidedmundson@kde.org>
sitterUnsubmitted
Not Done

According to the diff of the diff you seem to have lost David's copyright

sitter: According to the diff of the diff you seem to have lost David's copyright
6* 6​*
7* This program is free software; you can redistribute it and/or modify 7​* This program is free software; you can redistribute it and/or modify
8* it under the terms of the GNU Lesser General Public License as published by 8​* it under the terms of the GNU Lesser General Public License as published by
9* the Free Software Foundation; either version 2.1 of the License, or 9​* the Free Software Foundation; either version 2.1 of the License, or
10* (at your option) any later version. 10​* (at your option) any later version.
11* 11​*
12* This program is distributed in the hope that it will be useful, 12​* This program is distributed in the hope that it will be useful,
13* but WITHOUT ANY WARRANTY; without even the implied warranty of 13​* but WITHOUT ANY WARRANTY; without even the implied warranty of
Show All 15 Lines
29#include <QApplication> 29​#include <QApplication>
30#include <QWidget> 30​#include <QWidget>
31 31​
32#include <QDBusConnection> 32​#include <QDBusConnection>
33#include <QDBusConnectionInterface> 33​#include <QDBusConnectionInterface>
34 34​
35#include <PolkitQt1/Authority> 35​#include <PolkitQt1/Authority>
36#include <PolkitQt1/Subject> 36​#include <PolkitQt1/Subject>
37​#include <polkitqt1-version.h>
37 38​
38#include "kauthdebug.h" 39​#include "kauthdebug.h"
39 40​
40namespace KAuth 41​namespace KAuth
41{ 42​{
42 43​
43PolkitResultEventLoop::PolkitResultEventLoop(QObject *parent) 44​PolkitResultEventLoop::PolkitResultEventLoop(QObject *parent)
44 : QEventLoop(parent) 45​ : QEventLoop(parent)
Show All 19 Lines64Polkit1Backend::Polkit1Backend()
64 : AuthBackend() 65​ : AuthBackend()
65{ 66​{
66 setCapabilities(AuthorizeFromHelperCapability | CheckActionExistenceCapability | PreAuthActionCapability); 67​ setCapabilities(AuthorizeFromHelperCapability | CheckActionExistenceCapability | PreAuthActionCapability);
67 68​
68 // Setup useful signals 69​ // Setup useful signals
69 connect(PolkitQt1::Authority::instance(), SIGNAL(configChanged()), 70​ connect(PolkitQt1::Authority::instance(), SIGNAL(configChanged()),
70 this, SLOT(checkForResultChanged())); 71​ this, SLOT(checkForResultChanged()));
71 connect(PolkitQt1::Authority::instance(), SIGNAL(consoleKitDBChanged()), 72​ connect(PolkitQt1::Authority::instance(), SIGNAL(consoleKitDBChanged()),
72 this, SLOT(checkForResultChanged())); 73​ this, SLOT(checkForResultChanged()));
73} 74​}
sitterUnsubmitted
Not Done

Is there a reason you use stringy connection syntax instead of &member syntax?

sitter: Is there a reason you use stringy connection syntax instead of &member syntax?
feverfewAuthorUnsubmitted
Not Done

This line wasn't changed?

feverfew: This line wasn't changed?
74 75​
75Polkit1Backend::~Polkit1Backend() 76​Polkit1Backend::~Polkit1Backend()
76{ 77​{
77 78​
78} 79​}
79 80​
80void Polkit1Backend::preAuthAction(const QString &action, QWidget *parent) 81​void Polkit1Backend::preAuthAction(const QString &action, QWidget *parent)
81{ 82​{
▲ Show 20 Lines • Show All 73 Lines • ▼ Show 20 Line(s)155{
155 return QDBusConnection::systemBus().baseService().toUtf8(); 156​ return QDBusConnection::systemBus().baseService().toUtf8();
156} 157​}
157 158​
158AuthBackend::ExtraCallerIDVerificationMethod Polkit1Backend::extraCallerIDVerificationMethod() const 159​AuthBackend::ExtraCallerIDVerificationMethod Polkit1Backend::extraCallerIDVerificationMethod() const
159{ 160​{
160 return VerifyAgainstDBusServiceName; 161​ return VerifyAgainstDBusServiceName;
161} 162​}
162 163​
163bool Polkit1Backend::isCallerAuthorized(const QString &action, QByteArray callerID) 164​bool Polkit1Backend::isCallerAuthorized(const QString &action, const QByteArray &callerID, const QVariantMap &details)
sitterUnsubmitted
Not Done

action has & on the wrong size of the space, same for details ;)

sitter: action has & on the wrong size of the space, same for details ;)
164{ 165​{
165 PolkitQt1::SystemBusNameSubject subject(QString::fromUtf8(callerID)); 166​ PolkitQt1::SystemBusNameSubject subject(QString::fromUtf8(callerID));
166 PolkitQt1::Authority *authority = PolkitQt1::Authority::instance(); 167​ PolkitQt1::Authority *authority = PolkitQt1::Authority::instance();
167 168​ QMap<QString, QString> polkit1Details;
169​ for (auto it = details.cbegin(); it != details.cend(); ++it) {
sitterUnsubmitted
Done

you want constBegin/End here

sitter: you want constBegin/End here
170​ polkit1Details.insert(it.key(), it.value().toString());
171​ }
168 PolkitResultEventLoop e; 172​ PolkitResultEventLoop e;
169 connect(authority, SIGNAL(checkAuthorizationFinished(PolkitQt1::Authority::Result)), 173​ connect(authority, SIGNAL(checkAuthorizationFinished(PolkitQt1::Authority::Result)),
170 &e, SLOT(requestQuit(PolkitQt1::Authority::Result))); 174​ &e, SLOT(requestQuit(PolkitQt1::Authority::Result)));
175​#if POLKITQT1_IS_VERSION(0, 113, 0)
176​ authority->checkAuthorizationWithDetails(action, subject, PolkitQt1::Authority::AllowUserInteraction, polkit1Details);
177​#else
171 authority->checkAuthorization(action, subject, PolkitQt1::Authority::AllowUserInteraction); 178​ authority->checkAuthorization(action, subject, PolkitQt1::Authority::AllowUserInteraction);
179​#endif
sitterUnsubmitted
Not Done

looks to me this was only added 4 months ago. so this would probably require a version bump. seeing as we are fairly conservative with frameworks' requirements it may be better to if version>=whatevs the call.

sitter: looks to me this was only added 4 months ago. so this would probably require a version bump.
chinmoyrUnsubmitted
Not Done

I am not sure what to do here. The commit didn't really change the version and it is not of KF5 either.

chinmoyr: I am not sure what to do here. The commit didn't really change the version and it is not of KF5…
sitterUnsubmitted
Done

What I mean is PolkitQt1::checkAuthorizationWithDetails was only introduced some months ago, so you can't just assume it's available.
You'll want to guard it with something like

#if POLKITQT1_IS_VERSION(0, 113, 0)
    authority->checkAuthorizationWithDetails(...
#else
    authority->checkAuthorization(...
#endif
sitter: What I mean is `PolkitQt1::checkAuthorizationWithDetails` was only introduced some months ago…
172 e.exec(); 180​ e.exec();
173 181​
174 if (authority->hasError()) { 182​ if (authority->hasError()) {
175 qCDebug(KAUTH) << "Encountered error while checking authorization, error code:" << authority->lastError() << authority->errorDetails(); 183​ qCDebug(KAUTH) << "Encountered error while checking authorization, error code:" << authority->lastError() << authority->errorDetails();
176 authority->clearError(); 184​ authority->clearError();
177 } 185​ }
178 186​
179 switch (e.result()) { 187​ switch (e.result()) {
Show All 11 Lines197 for (auto it = m_cachedResults.begin(); it != m_cachedResults.end(); ++it) {
191 if (it.value() != actionStatus(action)) { 199​ if (it.value() != actionStatus(action)) {
192 *it = actionStatus(action); 200​ *it = actionStatus(action);
193 emit actionStatusChanged(action, *it); 201​ emit actionStatusChanged(action, *it);
194 } 202​ }
195 } 203​ }
196} 204​}
197 205​
198bool Polkit1Backend::actionExists(const QString &action) 206​bool Polkit1Backend::actionExists(const QString &action)
199{ 207​{
200 return m_cachedResults.value(action) != Action::InvalidStatus; 208​ return m_cachedResults.value(action) != Action::InvalidStatus;
sitterUnsubmitted
Not Done

Mhhhh. Mhhhhhhhh. I don't really have a suggestion here, but this is an incredibly dangerous change. Nested event loops can cause all sorts of negative effects. That's why the isValid had a note in the documentation, to tell the frontend dev to be careful. By adding a new loop in existing api we add a pit to fall into. I really don't know what can be done about it though. Could we get by with 1 second maybe?

sitter: Mhhhh. Mhhhhhhhh. I don't really have a suggestion here, but this is an incredibly dangerous…
dfaureUnsubmitted
Not Done

This code is FULL of nested event loops.

Does it run in a GUI process, or in some sort of separate backend process?

dfaure: This code is FULL of nested event loops. Does it run in a GUI process, or in some sort of…
201} 209​}
202 210​
211​QVariantMap Polkit1Backend::backendDetails(const DetailsMap &details)
212​{
213​ QVariantMap backendDetails;
214​ for (auto it = details.cbegin(); it != details.cend(); ++it) {
215​ switch (it.key()) {
216​ case Action::DetailMessage:
217​ backendDetails.insert(QStringLiteral("polkit.message"), it.value());
218​ break;
219​ case Action::DetailOther:
220​ default:
221​ backendDetails.insert(QStringLiteral("other_details"), it.value());
222​ break;
223​ }
224​ }
225​ return backendDetails;
226​}
227​
203} // namespace Auth 228​} // namespace Auth
204 229​