Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IBX-5985: Added ability to check user against "X-Expected-User" header #56

Merged
merged 6 commits into from Jun 23, 2023

Conversation

Steveb-p
Copy link
Contributor

@Steveb-p Steveb-p commented Mar 24, 2023

Question Answer
JIRA issue IBX-5985
Type improvement
Target version v4.5 (can be backported?)
BC breaks no
Tests pass yes
Doc needed yes

This is PoC for handling of a special header to check that the user we are executing query with is the same as the REST client expects.

Handles cases where authentication has expired and we would otherwise receive a response for anonymous.

TODO:

  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@sonarcloud
Copy link

sonarcloud bot commented Mar 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Steveb-p Steveb-p changed the title Added ability to check user against "X-Expected-User" header RFC: Added ability to check user against "X-Expected-User" header Mar 29, 2023
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Steveb-p From my POV it would be better to have a Response containing header stating X-Authenticated-As: admin. I don't see much security concerns here vs the proposition you have, because both methods allow getting information about authenticated user. But maybe for mine we could have semantic config set by default to false if someone thinks that reveals some information.

@Steveb-p
Copy link
Contributor Author

Steveb-p commented Mar 29, 2023

@Steveb-p From my POV it would be better to have a Response containing header stating X-Authenticated-As: admin. I don't see much security concerns here vs the proposition you have, because both methods allow getting information about authenticated user. But maybe for mine we could have semantic config set by default to false if someone thinks that reveals some information.

It's not about security. It's about the fact that session might expire when performing REST request, and you would not realize that it happened. Querying for session is a solution, but performing a check each and every time you need to do something is highly innefficient at best.

Request listener has the advantage of stopping the application from performing any queries (database, search, so on) at the very moment there is a discrepancy between expected and actual user. Adding a response header helps for sure, but we can limit the resource usage.

EDIT: Also note this prevents POSTing data as a wrong user (anonymous).

@adamwojs
Copy link
Member

@ibexa/php-dev What is the consensus here?

@Steveb-p Steveb-p changed the base branch from main to 4.5 June 13, 2023 10:04
@bdunogier
Copy link

As far as I'm concerned, and I have spent a lot of time facing that particular problem (anonymous is a user and can use most resources without any error happening), I really, really like this solution. It will tremendously help with Ibexa Connect and overall any headless app.

@Steveb-p Steveb-p marked this pull request as ready for review June 15, 2023 16:53
@Steveb-p Steveb-p changed the title RFC: Added ability to check user against "X-Expected-User" header IBX-5985: Added ability to check user against "X-Expected-User" header Jun 16, 2023
@Steveb-p Steveb-p requested a review from a team June 20, 2023 07:24
@webhdx webhdx requested a review from a team June 20, 2023 07:29
@sonarcloud
Copy link

sonarcloud bot commented Jun 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tomaszszopinski
Copy link

A note form Paweł: this functionality could be expanded into email comparison (if theres an interest from client).

Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA approved on IbexaDXP 4.5 commerce.

@Steveb-p Steveb-p merged commit 5e7e8c8 into 4.5 Jun 23, 2023
12 checks passed
@Steveb-p Steveb-p deleted the user-check branch June 23, 2023 13:51
@Steveb-p Steveb-p added the Doc needed The changes require some documentation label Jun 23, 2023
@Steveb-p
Copy link
Contributor Author

Merged up into main in bb782c9.

@bdunogier
Copy link

Really good feature, thank you all. Now we just need to implement this in Ibexa Connect :)

@MagdalenaZuba MagdalenaZuba removed the Doc needed The changes require some documentation label Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants