Feature-1816: Metacat Admin MN Privileges (auth.administrators)

[doulikecookiedough]

Summary of Changes:

• Added code to doAdminAuthorization method in D1Authhelper.java class to also check whether session subject is a Metacat admin user (part of the auth.administrators list) to determine MN Node access privileges.

Hi @taojing2002 - When you have a moment, can you please review my PR? @artntek If you have a moment, your feedback is welcome too.

[artntek]

LGTM. With some mocking, I think it might be possible to add some test cases in the existign class? ?

[doulikecookiedough]

After reviewing the D1AuthHelperTest test class, there exists a note regarding the doAdminAuthorization method that explains why it has not been implemented (requires client communication). Due to my lack of familiarity here and the likely associated time requirement to implement a working test - I am going to leave the test class as it is for now to move onto other tasks for the Metacat 3.0.0 release.

That being said - I have tested locally and my code change are working as expected with a valid token/auth.admin (manually adding my orcid as an auth.administrator to metacat.properties, retrieving the respective JWT token from the CN, and using it in a curl command) and non-matching subject from JWT token (subject does not match what is set in auth.admin).

[taojing2002]

@doulikecookiedough I agree with @artntek. It is good time to add a test for this method. We can use Mock or even without Mock to archive testing. This testing shouldn't be very hard and it can be a chance to get familiar with the code space.

[doulikecookiedough]

Understood @taojing2002! I will work on adding a test for this new change.

[taojing2002]

Thank you, Dou!

…

On Tue, Mar 5, 2024 at 2:36 PM Dou Mok ***@***.***> wrote: Understood @taojing2002 <https://github.com/taojing2002>! I will work on adding a test for this new change. — Reply to this email directly, view it on GitHub <#1819 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5QQDGDN2555HFLOLAS6O3YWZCIDAVCNFSM6AAAAABEGGPSBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZZG42TINRQGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[doulikecookiedough]

@taojing2002 I've added tests for the code change to the doAdminAuthorization method - can you please help review this PR again and let me know if you have any other feedback?

[artntek]

Network dependencies have been introduced in testDoAdminAuthorization_cnAdmin() and testDoAdminAuthorization_notAuthorized(). Can we mock these?

[doulikecookiedough]

Thank you @artntek for reviewing my changes. I've added a Mock which I believe has eliminated the network dependencies. I've also cleaned up the existing tests & added javadocs, implemented some missing tests & removed redundant ones and added a few new tests. Please let me know if you have any other feedback (and @taojing2002 as well).

Notes: I have not implemented the test for expandRightsHolder() at this time so I can prioritize testing the config changes for d1_portal (certificates).

• Update: There also exists a test in the test class D1NodeServiceTest which calls this method. I tested the relevant code locally and it appears to be working but it makes a network call to locate the CN, and then to get the subject list. I've attempted to re-work it with a mock but I'm unable to make progress at this time - so I've paused on it (and left a TODO statement).

[artntek]

This is one of the most beautiful tests I have ever seen. Thank you, and well done! 😄

a few linter errors for your consideration before merge:

[doulikecookiedough]

Thank you @artntek (and for the nudge to resolve my IDE issues)!