Skip to content

ui: keep the console loading when a role denies listLdapConfigurations - #14152

Open
stag7824 wants to merge 1 commit into
apache:4.22from
stag7824:fix-ui-ldap-config-denied-blocks-console
Open

ui: keep the console loading when a role denies listLdapConfigurations#14152
stag7824 wants to merge 1 commit into
apache:4.22from
stag7824:fix-ui-ldap-config-denied-blocks-console

Conversation

@stag7824

Copy link
Copy Markdown

Description

Fixes #13912

A role that denies listLdapConfigurations cannot use the web console at all: the UI never
renders and the user is redirected back to /user/login.

GetInfo in ui/src/store/modules/user.js runs about a dozen independent bootstrap calls
inside a single new Promise, and several of them are wired to that one shared reject.
listLdapConfigurations is one of them, and all it does with the answer is record a flag:

getAPI('listLdapConfigurations').then(response => {
  const ldapEnable = (response.ldapconfigurationresponse.count > 0)
  commit('SET_LDAP', ldapEnable)
}).catch(error => {
  reject(error)
})

When the role denies it, the 432 rejects the shared promise. That is a race against
listApis resolving it, and the tiny denied query almost always settles first, so GetInfo
rejects. The router guard in ui/src/permission.js treats a rejected GetInfo as a failed
session and logs the user out instead of building routes — so one denied flag read takes down
the whole console.

The tolerant pattern is already in the same function, a few lines away.
listNetworkServiceProviders and cloudianIsEnabled both swallow their own error and the
console loads fine without them. This change gives listLdapConfigurations the same
treatment, defaulting the flag rather than rejecting:

}).catch(ignored => {
  // A role is allowed to deny this read. It only records whether LDAP is configured, so
  // treat it as not configured rather than rejecting the promise the whole console waits on.
  commit('SET_LDAP', false)
})

Defaulting to false matches what the flag already means elsewhere — Logout resets it, and
a denied read is not evidence that LDAP is configured.

Scope

I have deliberately changed only the call named in the issue. listCapabilities, listZones
and the second listUsers are wired to the same shared reject and carry the same hazard,
but unlike this one they are not pure flag reads, so whether each is "essential" is a call for
you rather than for me. Happy to widen this PR if you'd like them handled together, or to
leave that to a follow-up.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • Major

How Has This Been Tested?

I could not add a unit test for this proportionately: user.js imports @/router, @/store
and @/vue-app, there are no existing tests for any store module to build on, and standing
that scaffolding up for a three-line change seemed like more maintenance burden than it is
worth. Happy to add one if you disagree.

What I did instead was reduce GetInfo's promise structure to a runnable model — a late
listApis that resolves, a tolerant denied call, and the denied listLdapConfigurations
and run it both ways:

without the fix  -> LOGGED OUT      (432 denied by role)
with the fix     -> console loads   (apis=1, ldap=false)

which reproduces the reported behaviour and the race described in the issue: the small denied
query settles before listApis can resolve the shared promise.

I also ran what CI runs, on Node 16 to match .github/workflows/ui.yml:

npm run lint       DONE  No lint errors found!
npm run test:unit  Test Suites: 9 passed, 9 total
                   Tests:       191 passed, 191 total

vue-cli-service lint autofixes by default, so I checked separately that it left the file
untouched rather than quietly reformatting the change into shape.

I do not have a deployment with custom roles to confirm the console rendering end to end, so
that part is unverified by me. The change itself is the same shape as the two tolerant calls
either side of it.

The same construction is present on 4.20 (it uses api( rather than getAPI( there). I have
targeted 4.22 since that is what the issue names, but happy to retarget.

GetInfo runs a dozen independent bootstrap calls inside a single Promise and
wires several of them to the same reject. listLdapConfigurations is one of
those, and all it does with the answer is record whether LDAP is configured.

When a role denies that API the 432 rejects the shared promise, and it wins the
race against listApis resolving it because it is a far smaller query. GetInfo
then rejects, and the router guard in permission.js treats that as a failed
session and logs the user out, so the console never renders at all.

Default the flag and let the console finish loading instead, matching
listNetworkServiceProviders and cloudianIsEnabled either side of it, which
already swallow their own errors.

Fixes apache#13912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant