PDJB-1275: Part 2: Fix other assumptions a landlord has a base user ID by samyou-softwire · Pull Request #1654 · communitiesuk/prsdb-webapp

samyou-softwire · 2026-07-28T09:35:40Z

Ticket number

Goal of change

remove all general assumptions from the codebase that a landlord has a single base user ID

Description of main change(s)

fixes many controllers & services to use the general methods for getting the current landlord, and uses the landlord to compare base user IDs

Anything you'd like to highlight to the reviewer?

this does not include any changes in function around org landlords. all of these changes should be stuff that would be expected to 'just work' with org landlords. any cases where we may want function to change (emails, page copy etc) are covered by specific tickets, referenced in TODO

most of the changes in this PR are uninteresting, removing the principal param from controllers etc. of particular note are the services which will now base their logic off of landlords directly

Checklist

Test suite has been run in full locally and is passing
Branch has been rebased onto main and run locally, with everything working as expected (both for your new feature
and any related functionality) not currently based on main, but does work locally right now
TODO comments referencing this JIRA ticket have been searched for and removed - if a future PR will address them,
mention that here
QA instructions have been added to the ticket (particularly if this is the last PR required to complete the ticket) I don't think there's much to QA here. I presume we'll do a healthy amount of mobbing before release
This feature is not behind a feature flag. I've checked that this is appropriate and we're happy with this code becoming live as soon as we release

samyou-softwire · 2026-07-28T09:46:54Z

 import org.springframework.data.jpa.repository.JpaRepository
 import uk.gov.communities.prsdb.webapp.database.entity.OrganisationLandlordUser

-// TODO: PDJB-1275: Move org landlord access checking to use this table


samyou-softwire · 2026-07-28T09:52:38Z

 ) {
    fun isAddressOwned(uprn: Long): Boolean = propertyOwnershipRepository.existsByIsActiveTrueAndAddress_Uprn(uprn)
-
-    fun isAddressOwnedByUser(


this was unused

…y ID

now it handles whether the current user can request as part of the service, checking landlord details and local council details

Travis-Softwire

What a monster! I've left some comments but nothing blocking, happy for it to be merged once they've been addressed

Travis-Softwire · 2026-07-31T15:41:20Z

            )
        }

-        if (landlordService.retrieveLandlordByBaseUserId(principal.name) != null) {


It looks like we've just removed this check altogether? Should we not instead check if the current user is associated with a landlord? This still feels like a failure mode worth checking

Travis-Softwire · 2026-07-31T16:12:59Z

 @RequestMapping(LANDLORD_BASE_URL, "/")
 class LandlordController(
-    private val landlordService: LandlordService,
+    private val userToLandlordService: UserToLandlordService,


Not for this PR, but the heavy use of this has got me thinking about whether we should cache the landlord ID in session on login, which would not only save at least one DB roundtrip per call for most LL endpoints, but would also potentially make our system operator functionality MUCH simpler to implement when we get there.

Travis-Softwire · 2026-07-31T16:24:54Z

            throw ResponseStatusException(
                HttpStatus.NOT_FOUND,
-                "User $baseUserId is not authorized to update property ownership $propertyOwnershipId",
+                "Current user is not authorized to update property ownership $propertyOwnershipId",


This is a loss of fidelity in the logs that I don't think is necessary - we can still grab the base user Id to tell us who was trying to access something they shouldn't have been

Travis-Softwire · 2026-07-31T16:48:30Z

    private val updateConfirmationSender: EmailNotificationService<LandlordUpdateConfirmation>,
    private val absoluteUrlProvider: AbsoluteUrlProvider,
 ) {
-    fun retrieveLandlordByRegNum(regNum: RegistrationNumberDataModel): Landlord? {


Were we not using this? I thought we let LCs search by LRN?

Travis-Softwire · 2026-07-31T16:57:43Z

+    fun getPropertyOwnershipIfCurrentUserAuthorized(propertyOwnershipId: Long): PropertyOwnership {
        val propertyOwnership = getPropertyOwnership(propertyOwnershipId)
+        val baseUserId = SecurityContextHolder.getContext().authentication.name
+        val landlord = userToLandlordService.getLandlordForBaseUserIdOrNull(baseUserId)


Why this vs the getCurrentLandlordForUser you've used everywhere else?

Travis-Softwire · 2026-07-31T17:30:30Z

@@ -89,8 +85,9 @@ class LandlordController(
        @RequestParam(value = "page", required = false) @Min(1) page: Int = 1,


Are we still using the principal param?

Travis-Softwire · 2026-07-31T17:31:12Z

@@ -80,11 +80,9 @@ class InviteJointLandlordController(
        @PathVariable invitationId: Long,


Still using the principal param?

Travis-Softwire · 2026-07-31T17:31:19Z

@@ -96,22 +94,19 @@ class InviteJointLandlordController(
        principal: Principal,


Still using the principal param?

Travis-Softwire · 2026-07-31T17:36:00Z

        whenever(mockPropertyOwnershipService.getPropertyOwnership(propertyId)).thenReturn(propertyOwnership)
-        setMockPrincipal(baseUserId)
-        whenever(mockLandlordService.retrieveLandlordByBaseUserId(baseUserId)).thenReturn(null)
+        whenever(mockUserToLandlordService.getCurrentLandlordForUser()).thenThrow(PrsdbWebException("Landlord not found"))


Is this test actually testing anything now or just asserting on the mock directly?

Travis-Softwire · 2026-07-31T17:38:01Z

            2,
-            3,
-            "USER_ID",
            1,


Careful here that we're using the correct arg in the correct place - might be worth using named arguments just in case these args change again in the future

samyou-softwire self-assigned this Jul 28, 2026

samyou-softwire changed the base branch from main to feat/PDJB-1275-add-service-methods-and-fix-site-globally-for-OL July 28, 2026 09:43

samyou-softwire commented Jul 28, 2026

View reviewed changes

samyou-softwire force-pushed the feat/PDJB-1275-add-service-methods-and-fix-site-globally-for-OL branch from ffe9a28 to 5db182d Compare July 28, 2026 15:55

samyou-softwire force-pushed the feat/PDJB-1275-fix-landlord-permission-checks branch from 0163c8e to 8469ec7 Compare July 28, 2026 16:28

samyou-softwire force-pushed the feat/PDJB-1275-add-service-methods-and-fix-site-globally-for-OL branch from 5db182d to 0b68d23 Compare July 29, 2026 08:27

Base automatically changed from feat/PDJB-1275-add-service-methods-and-fix-site-globally-for-OL to main July 29, 2026 10:29

samyou-softwire force-pushed the feat/PDJB-1275-fix-landlord-permission-checks branch 8 times, most recently from 58de050 to d4d0ffa Compare July 30, 2026 15:38

samyou-softwire added 14 commits July 30, 2026 16:55

PDJB-1275: Replace LandlordService references to getting a landlord b…

5886e9a

…y ID

PDJB-1275: Update tests

9b97f9a

PDJB-1275: Make get landlord by ID method generic

fd2d66a

PDJB-1275: Fix landlord update to not be ID specific

21d2153

PDJB-1275: Fix leave property service

b21ca91

PDJB-1275: Fix requesting property/compliance by 1L ID

6219b01

PDJB-1275: Fix some further usages

1249244

PDJB-1275: Remove final TODOs for ILR

3c35f6d

PDJB-1275: Fix PropertyOwnershipRepository

5c82922

PDJB-1275: Fix PropertyOwnershipService

c35aa0b

now it handles whether the current user can request as part of the service, checking landlord details and local council details

PDJB-1275: Fix JointLandlordInvitationService

2479795

PDJB-1275: Fix property details landlord cards

c669fbd

PDJB-1275: Fix PropertyComplianceService

a4e8418

PDJB-1275: Lint

d0aa514

PDJB-1275: Fix remaining broken tests

537dbf6

samyou-softwire force-pushed the feat/PDJB-1275-fix-landlord-permission-checks branch from d4d0ffa to 537dbf6 Compare July 30, 2026 15:55

samyou-softwire marked this pull request as ready for review July 31, 2026 08:23

samyou-softwire mentioned this pull request Jul 31, 2026

PDJB-1276: Org landlords on property details landlords tab #1667

Open

8 tasks

Travis-Softwire approved these changes Jul 31, 2026

View reviewed changes

		@@ -89,8 +85,9 @@ class LandlordController(
		@RequestParam(value = "page", required = false) @Min(1) page: Int = 1,

		@@ -80,11 +80,9 @@ class InviteJointLandlordController(
		@PathVariable invitationId: Long,

		@@ -96,22 +94,19 @@ class InviteJointLandlordController(
		principal: Principal,

Uh oh!

Conversation

samyou-softwire commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket number

Goal of change

Description of main change(s)

Anything you'd like to highlight to the reviewer?

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Travis-Softwire left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

samyou-softwire commented Jul 28, 2026 •

edited

Loading