Implements the multi-state validators for OneDrive

[mereghost]

Related Work Packages: OP#62749, OP#62750, OP#62751

Gives OneDrive the same treatment we gave Nextcloud

• Base Group
• Authentication Group
• AMPF Group
• Main Validator
• Hook into the Registry

[NobodysNightmare]

🟡 I was a bit surprised when I saw this check. Probably because I was wondering whether any of the other cases that are covered in later checks would be considered query_result.result == :error.

However, the more I think about it, the more I believe that maybe we are overusing the UI that we built here to the point that we are misusing it.

So I believe that diagnostic_request was strategically put as one of the first checks, so that if it fails, we don't claim that anything else has succeeded. For example just reordering the tests to check client_id first, would make it look successful if another error was to shadow it.

How sure can we be about the check order here? E.g. tenant_id is now checked before check_client_id. But if Microsoft decided to authorize requests first before telling you whether a given tenant exists, we'd errorneously show the tenant id check as successful, if the client id was wrong.

Long story short: Shouldn't we strip many of these checks down to :diagnostic_request and then make that test pass if the request was successful and if the request was not successful we should select the error message depending on the error we can see?

Then we'd always show an accurate error message and we would never indicate something as healthy, that might just look healthy because another error is shadowing it. After all: All checks that are based on query_result are different perspectives on the same, single check that was performed.

[Kharonus]

The current order is the order we get from login.microsoft.com. I.e., if tenant and client id is wrong, tenant failure has precedence before client id. This can change, of course, but we should try to be very specific about WHY the request failed.

this leads me to point 2:
stripping down everything to diagnostic_request and pass error message was a very early approach. And then we got the request to please, please interpret that error. This is, what Marcello here did - parsing the error message to to specific checks.

Also having two checks "Client ID" and "Client Secret", where one failed, is much easier to parse for a human, then having to read that detail from the error description below.

The shadowing "should" not be a problem, as long as the microsoft api is stable in their error behaviour.

[mereghost]

any of the other cases that are covered in later checks would be considered query_result.result == :error.

No, they won't. :error will only happen on cases where we don't return a more specific error, usually meaning we don't understand the response from the server.

If this fails, there's no point in continuing (same goes for Nextcloud capabilties request).

Shouldn't we strip many of these checks down to :diagnostic_request

More granular steps is the whole purpose of this, so if we get an :unauthorized response, we check the response and return what is wrong with it.

The order here is, in fact, tied to the order that the MS Graph API returns errors for those cases. (Tenant -> Secret -> Id)

In a way, what we are doing is exactly what you want, we are just explicit of the checks we are running that are based on a single request. (again, same as NC)

[NobodysNightmare]

stripping down everything to diagnostic_request and pass error message was a very early approach. And then we got the request to please, please interpret that error.

Just to be very clear: I still suggest to interpret the error/error message. I am just suggesting to boil it down to one check with different messages (that we control).

The shadowing "should" not be a problem, as long as the microsoft api is stable in their error behaviour.

That "should" is exactly the concern that I wanted to express here. It's a brittle assumption and one that you can't even properly put into a VCR casette. Our customers will be first to notice once our assumption breaks.

If we want to keep it like that, I'd probably wish for a comment that highlights how this depends on M$ APIs to evaluate errors in the exact same order that we assume here.

[Kharonus]

I do not yet see any blocker.

Well, except to the cleanup of the "old" validators.

[mereghost]

I do not yet see any blocker.

Well, except to the cleanup of the "old" validators.

We should remove then on the PR that replaces the UI, otherwise tests will explode. =/