Incorrect authz policy order

[kiootic]

Currently, some API requires the request contains either a valid session, or master key. This is represented as:

policy.AnyOf(
	authz.PolicyFunc(policy.RequireMasterKey),
	policy.AllOf(
		authz.PolicyFunc(policy.DenyNoAccessKey),
		policy.RequireValidUser,
	),
)

However, due to ordering of the policies, if the session is expired, the produced error reason is AccessKeyNotAccepted, instead of the expected reason NotAuthenticated. Client SDK would not refresh the session automatically and breaks the session.

To fix it, the order should be changed so that NotAuthenticated would be produced.

[louischan-oursky]

I still do not understand why this happened. If we treat the anyOf and allOf having short circuit behavior.

The boolean expression of the above policy is MasterKey || (API Key && ValidSession).

If it is Master Key, then it passes.
Otherwise if it is not API Key, then it fails with invalid API Key.
Otherwise if it is valid session, then it passes.
Otherwise if it is invalid session, then it fails with invalid session.

[kiootic]

The problem is that it's not a simple true/false checks; it returns an error.

SDK refresh session only if error has reason NotAuthenticated, which is produced by RequireValidUser policy. In expired session case, the compound policy got both AccessKeyNotAccepted and NotAuthenticated errors, and return first error (i.e. AccessKeyNotAccepted). This breaks the SDK refreshing behavior.

[louischan-oursky]

I guess the AccessKeyNotAccepted error is produced by RequireMasterKey, am I correct?

In JavaScript, false || 0 evaluates to 0. Following the short circuit behavior, MasterKey || (API Key && ValidSession) should return the error of ValidSession

[kiootic]

I guess the AccessKeyNotAccepted error is produced by RequireMasterKey, am I correct?

Yes.

In JavaScript, false || 0 evaluates to 0. Following the short circuit behavior, MasterKey || (API Key && ValidSession) should return the error of ValidSession

Suppose errors are falsy values. In case of expired session, MasterKey() returns error AccessKeyNotAccepted, APIKey() returns ok (i.e. truthy value), and ValidSession() returns error NotAuthenticated. Then, MasterKey() || (APIKey() && ValidSession()) returns NotAuthenticated. Therefore, we should change AnyOf to return last error, instead of first error. Do you mean this?

[louischan-oursky]

Yes. AnyOf should return the last error.

[louischan-oursky]

Since the behavior of AnyOf is changed, please check if any existing code is relying on the old behavior (returning the first error)

[kiootic]

The only API that uses AnyOf is this case, so there is no problem.