GNOME Bugzilla – Bug 762155
Skip EnsureCredentials if the account is disabled
Last modified: 2016-12-16 16:39:07 UTC
If you call EnsureCredentials on a disabled Kerberos account, and then the call will fail and the account will be marked as "attention-needed". In other words, if you have flipped the network services switch, then change networks, your account will be marked as expired. This looks wrong. We should silently skip ensuring the credentials for such accounts. One option could be to check the interfaces implemented by the GoaObject and if it doesn't have any org.gnome.OnlineAccounts.* interface other than Account, then we can skip the call in the daemon itself. The other option is to let each provider implement it themselves. For the moment, I have implemented the second option in the Kerberos provider, because it was quick to hack it up. I wonder if there are better options here.
Created attachment 321426 [details] [review] kerberos: Do not use goa_object_peek_* in threaded code
Created attachment 321427 [details] [review] smtp-auth, utils: Avoid needless string copies
Created attachment 321428 [details] [review] kerberos: Only write to the optional (out) variable if it is not NULL
Created attachment 321429 [details] [review] kerberos: Skip EnsureCredentials if the account is disabled
Created attachment 321432 [details] [review] [option 2] kerberos: Skip EnsureCredentials if the account is disabled On second thoughts, we should really fail EnsureCredentials with an appropriate error code instead returning success with an unknown timestamp.
Comment on attachment 321427 [details] [review] smtp-auth, utils: Avoid needless string copies Pushed to master.
Comment on attachment 321428 [details] [review] kerberos: Only write to the optional (out) variable if it is not NULL Pushed to master.
Created attachment 327248 [details] [review] [option 1] provider: Skip EnsureCredentials if an account is disabled
(In reply to Debarshi Ray from comment #0) > One option could be to check the interfaces implemented by the GoaObject and > if it doesn't have any org.gnome.OnlineAccounts.* interface other than > Account, then we can skip the call in the daemon itself. It makes me cringe to have an array of these interface names: + org.gnome.OnlineAccounts.Account + org.gnome.OnlineAccounts.Manager + org.gnome.OnlineAccounts.OAuthBased + org.gnome.OnlineAccounts.OAuth2Based + org.gnome.OnlineAccounts.PasswordBased The array might get out of sync with the actual D-Bus interfaces defined in data/dbus-interfaces.xml. > The other option is to let each provider implement it themselves. > > For the moment, I have implemented the second option in the Kerberos > provider, because it was quick to hack it up. I wonder if there are better > options here. Minor advantage with the second option is that can be implemented without breaking (error) strings. It can be useful for stable branches.
So goaprovider.c already maintains a mapping between the property name associated with a disable-able feature and that feature (called show_account_items, but maybe it could be generalized?). One idea might be to call goa_provider_get_provider_features to find out what features are provided by a provider and then error out from ensure_crendentials if all of those specific features are disabled. You could also cheat and use g_dbus_interface_skeleton_get_properties on org.gnome.OnlineAccounts.Account and check if all properties that have disabled as their suffice are true, but that seems sloppier. Option 2 seems fine to me too, I guess. The only thing I'd say is, goa-daemon, and users of goa shouldn't be trying to call ensure_credentials in the first place on an account if they know the account is disabled. All these approaches above take the "fail with an error" approach, which is fine (and probably necessary to be race free), but I do think we shouldn't even try in the first place.
(In reply to Ray Strode [halfline] from comment #10) > So goaprovider.c already maintains a mapping between the property name > associated with a disable-able feature and that feature (called > show_account_items, but maybe it could be generalized?). One idea might be > to call goa_provider_get_provider_features to find out what features are > provided by a provider and then error out from ensure_crendentials if all of > those specific features are disabled. Good point. I totally forgot about the show_account_items array.
Created attachment 327395 [details] [review] provider: Skip EnsureCredentials if an account is disabled
(In reply to Ray Strode [halfline] from comment #10) > The only thing I'd say is, > goa-daemon, and users of goa shouldn't be trying to call ensure_credentials > in the first place on an account if they know the account is disabled. Most (maybe all) the applications that I am aware of shouldn't be doing that. Unless there is a bug in them of course. I am mainly concerned about goa-daemon calling EnsureCredentials on the accounts when the network changes. See comment 1. We can teach goa-daemon (in ensure_credentials_queue_check) to not call EnsureCredentials on a disabled account, but then I thought, why don't we just move it a bit further down inside goa_provider_ensure_credentials_sync. It will cover both internal calls from goa-daemon and those from outside. As far as I can see, we ignore errors from goa-daemon-initiated calls. So errors like these will get masked, but I can understand if you find that icky. Also related: https://bugzilla.gnome.org/show_bug.cgi?id=755209
Review of attachment 327395 [details] [review]: This seems fine to me. ::: src/goabackend/goaprovider.c @@ +806,3 @@ + features = goa_provider_get_provider_features (self); + + for (i = 0; show_account_items[i].property != NULL; i++) Since ensure_credentials_sync isn't related to showing the account, I'd probably do a prerequisite commit that renames show_account_items to something like provider_features_info or feature_property_map or something like that. Just for clarity sake.
(In reply to Debarshi Ray from comment #13) > Most (maybe all) the applications that I am aware of shouldn't be doing > that. Unless there is a bug in them of course. I am mainly concerned about > goa-daemon calling EnsureCredentials on the accounts when the network > changes. Right my point is, goa-daemon isn't special here, applications shouldn't be trying to ensure credentials on accounts that are disabled and neither should goa-daemon when the network changes. > We can teach goa-daemon (in ensure_credentials_queue_check) to not call > EnsureCredentials on a disabled account, but then I thought, why don't we > just move it a bit further down inside goa_provider_ensure_credentials_sync. > It will cover both internal calls from goa-daemon and those from outside. As > far as I can see, we ignore errors from goa-daemon-initiated calls. So > errors like these will get masked, but I can understand if you find that > icky. I realize practically speaking it doesn't matter, of course. I do think it's conceptually better to avoid it up front, but whatever.
(In reply to Ray Strode [halfline] from comment #14) > Since ensure_credentials_sync isn't related to showing the account, I'd > probably do a prerequisite commit that renames show_account_items to > something like provider_features_info or feature_property_map or something > like that. Just for clarity sake. I couldn't think of a suitable name, so I punted it. Thanks for the suggestions. :) (In reply to Ray Strode [halfline] from comment #15) > (In reply to Debarshi Ray from comment #13) > > We can teach goa-daemon (in ensure_credentials_queue_check) to not call > > EnsureCredentials on a disabled account, but then I thought, why don't we > > just move it a bit further down inside goa_provider_ensure_credentials_sync. > > It will cover both internal calls from goa-daemon and those from outside. As > > far as I can see, we ignore errors from goa-daemon-initiated calls. So > > errors like these will get masked, but I can understand if you find that > > icky. > I realize practically speaking it doesn't matter, of course. I do think it's > conceptually better to avoid it up front, but whatever. What if we introduce a separate error code in GoaError for such failures, and then special case it in goa-daemon? (The special handling would be nothing but an empty if branch with a comment.) Would that be a reasonable trade-off? In fact, a special error code might be useful for applications. They can add some assertion or something to catch programmer errors. I am feeling a bit lethargic to repeat the same logic in both places, but if you tell once more to just do it, then I can refactor the show_account_items-using code into separate functions in src/goabacked/goautils.c and use it.
Created attachment 327406 [details] [review] provider: Rename a variable
Created attachment 327407 [details] [review] provider: Skip EnsureCredentials if an account is disabled
Review of attachment 327406 [details] [review]: sounds good. maybe change the git summary from Rename a variable to "Rename show_account_items"
Review of attachment 327407 [details] [review]: +
(In reply to Debarshi Ray from comment #16) > What if we introduce a separate error code in GoaError for such failures, > and then special case it in goa-daemon? (The special handling would be > nothing but an empty if branch with a comment.) > > Would that be a reasonable trade-off? In fact, a special error code might be > useful for applications. They can add some assertion or something to catch > programmer errors. > > I am feeling a bit lethargic to repeat the same logic in both places, but if > you tell once more to just do it, then I can refactor the > show_account_items-using code into separate functions in > src/goabacked/goautils.c and use it. Honestly, I don't have super strong opinions here, so whatevers cool with me.
Comment on attachment 327406 [details] [review] provider: Rename a variable Thanks for the reviews, halfline. Pushed to master after tweaking the commit message as suggested.
Comment on attachment 327407 [details] [review] provider: Skip EnsureCredentials if an account is disabled Pushed to master
Review of attachment 321427 [details] [review]: No, this was wrong. Even though we already have our own reference on the GObject, the value of its properties can still change behind our backs if we are not on the thread where the object was constructed. I have reverted the commit.
Review of attachment 321432 [details] [review]: This patch is identical to attachment 319862 [details] [review] from bug 756498, which was committed.
Review of attachment 321432 [details] [review]: On second thoughts, it isn't. Anyway, attachment 327407 [details] [review] is clearly the better approach, and it was committed too.