Change conditional to point to username field in client config

[Mierdin]

#3219 changed the logic of when a token is retrieved from the API by looking for the 'credentials' field of the configuration.

However, when this section is missing (or when the config is omitted entirely, like it is in CI), this field is set to an empty dictionary, and then because both username and password default to None and then are added to this dict, the statement if credentials: will never evaluate to False, because this dict will never be empty.

In CI, this causes problems We want to skip this step when the config is missing, because in CI this is expected and other API methods will work just fine because auth is disabled. This change proposes looking at just specifically the username field, because even though st2 login omits the password field by default, it still writes the username field so can be relied upon.

[codecov-io]

Codecov Report

Merging #3226 into master will decrease coverage by -0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3226      +/-   ##
==========================================
- Coverage   77.86%   77.81%   -0.06%     
==========================================
  Files         433      433              
  Lines       22421    22421              
==========================================
- Hits        17458    17445      -13     
- Misses       4963     4976      +13

Impacted Files	Coverage Δ
st2client/st2client/base.py	80.3% <100%> (-2.02%)	❌
st2api/st2api/controllers/v1/triggers.py	71.43% <ø> (-4.93%)	❌
st2common/st2common/bootstrap/rulesregistrar.py	74.34% <ø> (-4.42%)	❌
st2common/st2common/transport/consumers.py	86.32% <ø> (-3.16%)	❌
st2common/st2common/models/db/init.py	94.15% <ø> (-0.98%)	❌
st2api/st2api/controllers/resource.py	86.78% <ø> (-0.83%)	❌
st2common/st2common/rbac/resolvers.py	88.62% <ø> (-0.79%)	❌
st2api/st2api/controllers/v1/packs.py	91.36% <ø> (-0.62%)	❌
st2actions/st2actions/container/base.py	90.59% <ø> (-0.59%)	❌
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bb86ec...83af8a4. Read the comment docs.

[bigmstone]

👍

[arm4b]

Considering all efforts and discussions around this bug, do you think it's possible to include a test case for this particular scenario?

[Kami]

Thanks, good catch.

And yes, I agree, we should add a test case for that (so it doesn't try to authenticate when: 1. no credentials are set, 2. only password is set but not the username).

[Kami]

Now that I think more about it - this will probably work for v2.2.0, but we need to add some more robustness and logic to handle more scenarios.

Good example of scenario which is not handled correctly right now is a case when only a username is provided in the config and there is no cached token. In such scenario we don't want to perform an authentication with the auth API since password is mandatory (but that's what happens right now).

[lakshmi-kannan]

This entire piece of code is a mess. We've added several layers of "patch" and we should refactor this post release.

[Kami]

Agreed - and we should start with tests and making sure we handle all the edge cases.

[Kami]

In short, if we are "in a rush" with v2.2.0, I would be fine with merging this and then work on additional robustness + test cases mentioned in https://github.com/StackStorm/st2/pull/3226/files#r101514299 in the very near future (and we can release that as v2.2.1).

(I looked into more "quick fixes" + tests myself, but it requires more time to do it correctly and not something which should be rushed so we either ignore that for now or make it a blocker for v2.2.0 and delay the release)

[Mierdin]

Thanks all for the discussion. In the interest of moving 2.2 forward, I'm going to merge this, but I created #3232 to ensure we address additional testing and corner cases in 2.2.1