eclipse / kapua

Eclipse Public License 2.0
225 stars 160 forks source link

FIX - wrong Cors filtering error upon unauthorized API request #3954

Closed Agnul97 closed 9 months ago

Agnul97 commented 9 months ago

Brief description of the PR If you tried to call the API using a wrong auth token (for example, empty or expired) Kapua was returning a wrong error message in the HTTP body regarding the CORS filtering error, even though the "allowed origins" contained the origin used to perform the call. The right response you would expect in this case is a 401 error code and nothing else.

Description of the solution adopted This problem was caused by a regression I introduced in a past work on the CORS filtering logic. I didn't realize that the KapuaTokenAuthenticationFilter class, upon wrong authentication, was continuing the filtering chain to the CORS filter, and this one had to perform the filtering before returning the 401 error code to the client. This is the reason behind (see also https://github.com/eclipse/kapua/pull/3324): the browser, having made an API request, will check first of all if the header of the response contains the origin headers. If not, it will display a network problem/CORS problem without showing the content of the response, even when it could contain, for example, a 401 status in the header. This means that to correctly display the 401 error code on the client side, the back-end side has to perform the cors filtering even if authentication fails.

Tests done to verify the change To test the change, I've performed API calls with swagger using both same-origin and cross-origin as values in the SEC_FETCH_SITE field of the header. Also, I tried using wrong, empty or expired token as auth. fields. In this way, all possible scenarios were covered

codecov[bot] commented 9 months ago

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (474caaf) 20.74% compared to head (9b6c8ed) 20.73%. Report is 25 commits behind head on develop.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/eclipse/kapua/pull/3954/graphs/tree.svg?width=650&height=150&src=pr&token=1P4N4CApH8&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse)](https://app.codecov.io/gh/eclipse/kapua/pull/3954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse) ```diff @@ Coverage Diff @@ ## develop #3954 +/- ## ============================================= - Coverage 20.74% 20.73% -0.01% Complexity 6 6 ============================================= Files 1951 1951 Lines 41767 41766 -1 Branches 3956 3955 -1 ============================================= - Hits 8664 8661 -3 - Misses 32700 32702 +2 Partials 403 403 ``` | [Files](https://app.codecov.io/gh/eclipse/kapua/pull/3954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse) | Coverage Δ | | |---|---|---| | [.../api/core/auth/KapuaTokenAuthenticationFilter.java](https://app.codecov.io/gh/eclipse/kapua/pull/3954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-cmVzdC1hcGkvY29yZS9zcmMvbWFpbi9qYXZhL29yZy9lY2xpcHNlL2thcHVhL2FwcC9hcGkvY29yZS9hdXRoL0thcHVhVG9rZW5BdXRoZW50aWNhdGlvbkZpbHRlci5qYXZh) | `25.00% <ø> (ø)` | | | [...kapua/commons/rest/filters/CORSResponseFilter.java](https://app.codecov.io/gh/eclipse/kapua/pull/3954?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse#diff-Y29tbW9ucy1yZXN0L2ZpbHRlcnMvc3JjL21haW4vamF2YS9vcmcvZWNsaXBzZS9rYXB1YS9jb21tb25zL3Jlc3QvZmlsdGVycy9DT1JTUmVzcG9uc2VGaWx0ZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/eclipse/kapua/pull/3954/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=eclipse)