Issue/628 by mhdirkse · Pull Request #681 · wearefrank/ladybug

mhdirkse · 2026-03-17T14:45:50Z

No description provided.

mhdirkse · 2026-03-23T13:41:03Z

See https://github.com/wearefrank/ladybug/actions/runs/23439488095/job/68186164956 for tests with FF!.

nielsm5 · 2026-03-23T16:29:54Z

+		} catch(HttpInternalServerErrorException e) {
+			return Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(e.getMessage()).build();


This is very strange? Why not use an ExceptionHandler ?
The whole point of the ExceptionHandlers is to uniformly handle all exceptions of a certain type.
If you're going to catch all exceptions, why throw them in the first place?

I intend to introduce exception handlers in a later PR. My aim with this PR is to pass a user role to the frontend and to make a start to make the UI role-dependent.

nielsm5 · 2026-03-23T16:35:43Z

+		String result = null;
+		// When authorization is disabled, isUserInRole() always returns true.
+		// Therefore we have to check for the most powerful rule first.
+		if (isUserInRoles(testerRoles)) {
+			result = TestToolApiImpl.TESTER;
+		} else if (isUserInRoles(dataAdminRoles)) {
+			result = TestToolApiImpl.DATA_ADMIN;
+		} else if(isUserInRoles(observerRoles)) {
+			result = TestToolApiImpl.OBSERVER;
+		} else {
+			result = TestToolApiImpl.NO_AUTHORIZATION;
+		}
+		log.info("Tell frontend that user is in role [{}]", result);
+		return result;


A user can have multiple roles, why not return all authorities? At least create a mechanism both implementations can use to map the roles...

The trouble is that with JAX-RS it is not so easy to get the user role. We have two tools:

We have a method isUserInRoles() that receives a list of roles and that returns true if the user is in a role from the list.

From Spring we get variables that hold cumulative lists of roles. testerRoles = IbisTester, dataAdminRoles = IbisDataAdmin, IbisAdmin, IbisTester and observerRoles = IbisObserver, IbisDataAdmin, IbisAdmin, IbisTester.

To get as close as possible to a single role, we must group the roles by distinguishing role. We can calculate that IbisTester < IbisTester, IbisDataAdmin, IbisAdmin < IbisTester, IbisDataAdmin, IbisAdmin, IbisObserver. Then we can use isUserInRole() to return either {IbisTester} or {IbisDataAdmin, IbisAdmin} or {IbisObserver}. Then we can have common logic to produce a result from one of IbisTester, {IbisTester}, IbisDataAdmin, IbisAdmin, {IbisDataAdmin, IbisAdmin}, IbisObserver and {IbisObserver}.

Complicated as this logic may be, it is possible to write unit tests for it. Although we produce more complex code than the code you reviewed, it is probably more trustworthy.

Niels, we discussed this orally. Now I understand. I think this PR does it the right way now.

nielsm5 · 2026-03-23T18:08:42Z

+			log.error("Cannot read properties from existing classpath resource test.properties");
+			return result;
+		}
+		String uiTestModeStr = props.getProperty("ladybug.ui.test.mode");


You could just use @Value("hier") instead of creating your own properties parser...

Then I need a class with annotation @propertysource to read my test.properties file. That file is optional - these are properties to make Ladybug behave in ways that are not relevant for real users. I am going to add properties here to let the backend throw fake exceptions. Within the @propertysource annotation I need to set ignoreResourceNotFound=true. But then I cannot inject values so easily with the @value annotation. When the property is not found, I get ${the.property} as value when the property name is the.property. I think the current approach is more appropriate. Less risk to introduce Spring configurations that may jeopardize other parts of Ladybug or the FF!.

nielsm5 · 2026-03-23T18:09:59Z

+	public static final class TestProperties {
+		private @Getter @Setter UI_TEST_MODE uiTestMode = UI_TEST_MODE.DEFAULT;
+	}


Why do you have a class with only 1 field? Why not just use the field directly?

I will add the logic for fake exceptions here instead of hard coding whether to throw fake exceptions or not. Then that logic is not temporary anymore.

nielsm5 · 2026-03-23T18:11:00Z

+		if (!StringUtils.isEmpty(transformation)) {
+			map.put(KEY_TRANSFORMATION, transformation);
+		}
+		map.put("uiTestMode", testProperties.getUiTestMode().toString().toLowerCase());


Please use enum.name()

It is in a bean now. Te enum value is put in the JSON automatically.

nielsm5 · 2026-03-23T18:13:08Z

        @if (activeTab === GLOBAL) {
          <div class="form-group">
-            <label>Report generator</label>
+            <label>Report generator{{ optionalNotAuthorized() }}</label>


Why don't you just disable the field with a tooltip that the user doesn't have the required roles?

…ress test

mhdirkse · 2026-04-02T16:59:25Z

I am busy processing review comments. I need to push to save my work. As it is now it is not ready for merging.

mhdirkse · 2026-04-02T17:05:50Z

I am busy extending the PR with extra Cypress tests for authorization. I will enable authorization with and without using the test mode. Without the test mode we login as observer and check that the control about the report generator is disabled - no option anymore to enable / disable the report generator except back to default. With the test mode and as observer, we try to disable the report generator and check that we get a toast originating from a backend error.

…end errors

mhdirkse · 2026-04-03T13:59:15Z

See https://github.com/wearefrank/ladybug/actions/runs/23948757436 for test with FF!.

mhdirkse · 2026-04-03T17:21:22Z

See https://github.com/wearefrank/ladybug/actions/runs/23955165018 for the tests with the FF!. I may put this to ready for review before the tests will have finished.

nielsm5

Je gebruikt de testmode nu niet meer maar het zit er wel in. Zelf denk ik dat het handiger is zaken uit de codebase te houden wanneer deze niets toevoegen. (Behalve als je andere plannen hebt. Maar dan zou het eigenlijk in een los pr moeten 😉.)

mhdirkse added 23 commits March 16, 2026 17:27

Throw fake errors

c83836c

Merge branch 'master' into issue/628

618ac93

Keep logic for fake backend errors without using it now

fddc902

First try to accomodate settings dialog to user role

a9be057

Merge branch 'master' into issue/628

a79ea90

Replace not-allowed fields by text message

98cb53b

Show edit permission by labels and disablement of controls

02a5ddc

Add fetching the user role with Spring MVC

1b26fc6

Do not show success toast after failure

ad41b53

Fix API call for user role for Spring MVC

6dd6227

Do not consider other authorities than ROLEs

bbe97bf

Fix previous commit

8fa2900

Remove duplicate logic to remove ROLE_ prefix

67acd4f

Fix previous commit

ffc1c35

Remove unnecessary @Autowired

c4d4ea0

Reviewed my own code

1517e7f

Have reset to factory defaults in one HTTP request

effe5d8

Remove unused method

8ba8c37

Remove HTTP method to get transformation from frontend

243c2c6

Support tweaking the UI with file test.properties

014e45b

Extend explanation of test.properties

598a9ed

Do not show confusing success toasts for debug settings dialog

7549e39

Prevent in UI that report transformation is cleared

f08adb8

mhdirkse marked this pull request as ready for review March 23, 2026 13:18

Fix Cypress tests

8deed72

mhdirkse requested a review from nielsm5 March 23, 2026 15:01

jacodg approved these changes Mar 23, 2026

View reviewed changes

nielsm5 requested changes Mar 23, 2026

View reviewed changes

Merge branch 'master' into issue/628

f3e1fe3

mhdirkse added 2 commits April 2, 2026 17:18

Prepare web.xml for manipulation in CI/CD

901f482

Work in progress - adding authorization tests to ladybug-frontend Cyp…

d3815f9

…ress test

mhdirkse marked this pull request as draft April 2, 2026 16:58

mhdirkse added 10 commits April 3, 2026 14:18

Have Cypress tests for the role in the frontend and for catching back…

a965390

…end errors

Merge branch 'master' into issue/628

64d12f2

Fix previous commit

1e8ddcb

Fix CICD and have common code again for handling an error

972e879

Fix CICD

ee61ade

Fix CICD

3a2b454

Give waitForUrl.sh execute permission

adf5747

Fix CICD

79c7b3d

Fix CICD

a0276df

Extend e2e.ts with an extra exception handler

320ff7e

mhdirkse added 8 commits April 3, 2026 16:07

Complete removing unnecessary logic introduced for testing

9a0afe0

Do not delegate logging to methods because line numers are then lost

1923dde

More common code for handling errors from HTTP requests

1d53a2a

Cleanup code

df3d6aa

Fix previous commit

f2c8396

Fix modal dialogs and error handling

a7ead97

Add unit tests for preventing that the XSLT is cleared

1aa0f17

One more point about error handling

e726d0b

mhdirkse marked this pull request as ready for review April 3, 2026 17:26

mhdirkse requested a review from nielsm5 April 3, 2026 17:26

nielsm5 approved these changes Apr 9, 2026

View reviewed changes

mhdirkse merged commit 7163921 into master Apr 10, 2026
12 of 15 checks passed

mhdirkse deleted the issue/628 branch April 10, 2026 08:40

		} catch(HttpInternalServerErrorException e) {
		return Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(e.getMessage()).build();

Conversation

mhdirkse commented Mar 17, 2026

Uh oh!

mhdirkse commented Mar 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdirkse commented Apr 2, 2026

Uh oh!

mhdirkse commented Apr 2, 2026

Uh oh!

mhdirkse commented Apr 3, 2026

Uh oh!

mhdirkse commented Apr 3, 2026

Uh oh!

nielsm5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants