Part 5 - security: Manager still can update employees from other managers

[superyaooo]

I referred to #61 which solved the non-working update issue, but now the update works not-so-secure.

When logged in as "greg", "greg" can still update employees belonging to the other manager "oliver". For example, "greg" is able to update but not delete "Merry". After updating "Merry", her manager gets updated from "oliver" to "greg". Then "greg" is able to completely delete "Merry".

I do not believe it's supposed to work this way, but am not able to resolve it on my own. Please help :(

[jpizagno]

ok. I did not check the security with different managers. I will do so and get right back in a few days.

However I am not sure if this is a bug, or a new feature. It is not explicitly a requirement that managers cannot update employees from other managers. Where I work in the real world , managers are allowed to "update" any employee , even if not their own. :)

In any case @superyahoo , you bring up a great point for a new feature. Give me a few days to look into it.

[superyaooo]

@jpizagno I posted this after re-reading the docs for part 5. Under the Securing employees to their managers section, "In other words, any manager can log in and view the data, but only a given employee’s manager can make any changes." And in the tutorial here, there is an image at the end of the page, showing Access Denied when a manager tries to update an employee belonging to another manager. I tried rechecking my code and the code sample here, but couldn't find anything that I'm missing in my version.

Thank you in advance for checking it on your end. Please let me know how it goes on your version once you get a chance to look at it.

[jpizagno]

Hello @superyaooo . I am wrong, you are 100% correct. :>

So I found a solution to this problem, but am still testing it. However, I think my solution may only allow 1 manager to be be logged in and update the data at a given time.... that needs to be tested. Also this feels like a "frontend" solution, where the data security should be done on the backend.

Try this in the security subproject:

in src/main/resources/templates/index.html, change the span to:

Hello, <span id="managername" th:text="${#authentication.name}">user</span>.

at the end of src/main/js/app.js change the App call to:

ReactDOM.render(
    <App loggedInManager={document.getElementById('managername').innerHTML } />,
    document.getElementById('react')
)

in the begining (line 18 or so) of src/main/js/app.js change the constructor to:

this.state = {employees: [], attributes: [], page: 1, pageSize: 2, links: {}
   , loggedInManager: this.props.loggedInManager};

Note: at the end is a new State: loggedInManager: this.props.loggedInManager

around line 92 so of src/main/js/app.js is the onUpdate(employee, updatedEmployee). change onUpdate to:

onUpdate(employee, updatedEmployee) {

        if(employee.entity.manager.name == this.state.loggedInManager) {
            client({
                method: 'PUT',
                path: employee.entity._links.self.href,
                entity: updatedEmployee,
                headers: {
                    'Content-Type': 'application/json',
                    'If-Match': employee.headers.Etag
                }
            }).done(response => {
                /* Let the websocket handler update the state */
            }, response => {
                if (response.status.code === 403) {
                    alert('ACCESS DENIED: You are not authorized to update ' +
                        employee.entity._links.self.href);
                }
                if (response.status.code === 412) {
                    alert('DENIED: Unable to update ' + employee.entity._links.self.href +
                        '. Your copy is stale.');
                }
            });
        } else {
            alert("wrong manager");
        }
    }

[jpizagno]

Hello @gregturn Maybe you can please help us, and comment on this? I can see that my solution above is more of a "hack". The check for response.status.code === 403 is already there and should work, but does not seem to work. I would like to help, and I have a opened a pull request in Issue #61, but the code is not complete (as pointed out in this issue). If you give me a pointer, I would be happy to fix it properly and create a new pull request?

thanks, jpizagno

[superyaooo]

@jpizagno Your "hack" fixes the problem via the front-end! Thank you for the detailed explanation.

I also read #67 where the issuer mentions that when saving the update on the front-end, the manager property is not included (on the update panel there is only firstName, lastName and description) thus overwritten with null. I feel this is the root cause of the issue, because the following back-end code in EmployeeRepository would have worked to stop a manger from updating another manager's employee if the employee's original non-null manager info is retained: @PreAuthorize("#employee?.manager == null or #employee?.manager?.name == authentication?.name")

So if we could change the front-end code to make it pass in/retain manager info upon update, no back-end code change would be needed. For I think by applying the same @ HandleBeforeCreate code (which is used during creating) in SpringDataRestEventHandler to @ HandleBeforeSave, you're actually overwriting/setting the manager info of the employee with whoever is logged in via the back-end during updating.

Since I'm completely new to React (aka. can read, can't write), I don't know how to change the front-end code to fix this issue. Please let me know if my analysis makes sense. Also any additional help/suggestion would be highly appreciated as well!

Thank you again @jpizagno !

[jpizagno]

@superyaooo OK. I fixed the front-end code to make it pass in/retain the manager info. The new code can be found in my pull-request. I have upgraded my Pull Request here:

https://github.com/spring-guides/tut-react-and-spring-data-rest/pull/68

There is also pull request in my repo here: https://github.com/jpizagno/tut-react-and-spring-data-rest

If you can confirm that these changes are correct, then please star my repository and mention it here.

thanks

[superyaooo]

@jpizagno I've tested the changes and they work. I've also starred your repo. Thanks again for the help! Closing this ticket now.