Fix undefined access error in empty access policies

[dennis531]

Changes:

• Add check if fetched acls contain an ace property
• Hopefully improve code readability

Close #334

[github-actions]

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-855

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-855

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

[github-actions]

This pull request is deployed at test.admin-interface.opencast.org/855/2024-08-29_11-08-20/ .
It might take a few minutes for it to become available.

[lkiesow]

We know allow is a boolean, don't we?

Suggested change

	} else if (policy.allow === true) { //|| policy.allow === "true") {
	} else if (policy.allow) {

[dennis531]

I prefer a strict type check to prevent false truthy values, for example "false", see https://developer.mozilla.org/en-US/docs/Glossary/Truthy.

[lkiesow]

Isn't the whole idea of Typescript that we have the strict type check? And allow is defined as a boolean, isn't it? So, this can never have the value "false" can it?

[dennis531]

The policy variable has the type Ace which defines allow as boolean:

opencast-admin-interface/src/slices/aclDetailsSlice.ts

Lines 10 to 14 in ace958b

	export type Ace = {
	allow: boolean,
	role: string,
	action: string
	}

But yes, it can, because Typescript only does a type check at compile time, see https://learning-notes.mistermicheels.com/javascript/typescript/runtime-type-checking/. A solution could be a validation library for external data.

[Arnei]

Although if it can be string, we should adjust our type definition to correctly reflect that (allow: boolean | string), or fix our backend to always give us a boolean. Since in this case we have full control over the external data (aka the admin ui backend), introducing a validation library is arguably overkill.

[lkiesow]

Interesting, I thought TypeScript would include some runtime type checks.

[…] or fix our backend to always give us a boolean […]

I think that's already what the backend does. So I think we can still assume this to be a boolean.

But let's assume we aren't sure about the type here, that would mean the code would convert a value like null, undefined, "true" or "True" to false. This is different from how Opencast's core would handle this: A non-existing allow value is assumed to be true. A string would cause an error.

In other words, this isn't really a type check, but just setting a default value of false for everything but true. That means, what this actually does is just to re-define which values are truthy.

If we want an actual type check, we could e.g. add a typeof type guard before line 877. Something like:

if (typeof policy.allow !== "boolean") {
  console.error("Invalid value of allow in acl");
  continue:
}

If we want a behavior identical to Opencast's, we could even do

if (policy.allow === undefined) {
  policy.allow = true
}
if (typeof policy.allow !== "boolean") {
  console.error("Invalid value of allow in acl");
  continue:
}

Or we could just assume that the backend provides correct data :D

In the end, I think this is an interesting thing to argue about, but it has little real word relevance. If you ant to modify this, that's good. If you don't, that's fine as well, and we can merge it like this.

What do you think?

[dennis531]

I think the type checking is not really necessary if we know that the backend provides the correct data. Nevertheless, we should unify the behavior of unset parameters.

[github-actions]

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.