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

Return 401 for requests with expired session #15637

Merged
merged 15 commits into from
May 8, 2020

Conversation

amiedes
Copy link
Contributor

@amiedes amiedes commented Apr 30, 2020

Closes part of https://github.com/CartoDB/cartodb-platform/issues/6539

Problem

Some controllers relying on the api_authorization_required and any_api_authorization_required before filters where returning valid responses for requests including a session cookie from a previous (closed) session.

Solution

  • I've changed the code so controller actions relying in session authentication raise a Carto::ExpiredSessionError error.

  • I wrote a controller spec for this specific issue in spec/requests/carto/api/organization_users_controller_spec.rb, and a more generic one within spec/lib/carto/authentication_manager_spec.rb, since it's an issue affecting multiple controllers (and though this was better than writing a spec for every specific controller).

@amiedes
Copy link
Contributor Author

amiedes commented May 4, 2020

This is fixed in staging.

Request:

curl 'https://ayuntamiento-banastas.carto-staging.com/u/amiedes/api/v1/organization/ca79293a-98bf-47df-b99c-538053c3c3cb/users?per_page=50&order=username&page=1' \
  -H 'Connection: keep-alive' \
  -H 'Pragma: no-cache' \
  -H 'Cache-Control: no-cache' \
  -H 'Accept: application/json, text/javascript, */*; q=0.01' \
  -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.129 Safari/537.36' \
  -H 'X-Requested-With: XMLHttpRequest' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'Sec-Fetch-Mode: cors' \
  -H 'Sec-Fetch-Dest: empty' \
  -H 'Referer: https://ayuntamiento-banastas.carto-staging.com/u/amiedes/organization' \
  -H 'Accept-Language: es-ES,es;q=0.9,en;q=0.8' \
  -H 'Cookie: _ga=GA1.2.806941613.1584373139; _fbp=fb.1.1584715612062.629557369; hubspotutk=ea6055c2557555ab0d785d1a22d48bf2; __hstc=237193439.ea6055c2557555ab0d785d1a22d48bf2.1586254323211.1586254323211.1586330268635.2; _gid=GA1.2.1648849578.1588581493; _gat_UA-20934186-25=1; _gat_UA-20934186-31=1; _cartodb_base_url=https%3A%2F%2Fayuntamiento-banastas.carto-staging.com%2Fu%2Famiedes; _cartodb_session=WHFMZGpBRGxoY3Awc29ZeFpkcFZlNnVNY3BGcFhrcUs2bUlNZFd6TWM2UzVESWJVS2dyenJDajBFazhpSk1EWHB2MVBoVnZMWmNpdC9Ta08zcVJZZHl4eEh2VWI5Vnp3WEVUY0FVTEkvYy9hazI0ekNZN0Z6NkhUTWIrN2YxM2swMUdUS28xT0VlN0I5WFk2NG9jemFycHg3OFRaaDlOa3ZQZERTaDNUcE9jRkpDUDNTd1MwR2RoUC9jWG4rbGI4RW1sKyszZU5JYlBxZUE5TFJ3SW9Zd3pEVVNRZ1VJQzQ1QUU1S2lBQm12cW1FVnc2a3ZxSlU5ZGJ6clorNmUxaUthOVNkOHkrVzNvUmdPNENIZCthcHNSeXpyY2hNMTZ1UW55UjVveS9yWExUdlI3WllOcTZWU3FGWjBJeHIzODJkeEcra1JtMG1KNmVPMFlHWUtIM1ZGT25ldHVBajBHWHp3VlNIanVDMi8zZGRMcFhEWUo2WnIrdXo2YjdlSDRlTmlvQ3JHU2JPc2t4NExGKzVneHVPNUI3ZDE3ZjljNGx5QkJ5YWl5bCtKOStwdFVkKzZSVUZTZFBISjlyVkJEZ290L3ZuRmdDeW4vQXNZYlNORlF3SkhRc0kyRC94dWlaTGhaUDVycU03U1lBMG9zUWhTZm81Q2lYWEZ6RHZBc2MvK2M2cWhyOGZPTjdMdnNnOVdsNllzRjZqblY2eWtZOGI3RVdxYnZPc2hwaEZ1ZU5OM2dXSzlCNHJQakkzQkl5NzY5NXhLbWNXaGtZendhZElFN0FWWGcxYUtzdXVSaXk2Z0pnYkdpVU0xTVBRczl4QWpISStEU3NKQ3pEMkVmWUU1a3k3VjNWTzFsVlpseG5HMlJmYkNIODFoazhXNXh5SEZGOWNtb0lHT0xvVU1PYzNKTTdKWnpZdWFqZnY3RDVsWXJMWUlFY3ZIcWJSMlJXSlR1aWhJWENUTzFQamkvL3dnNWQ1aHN4ZGxVWVlpQWRRUnN4NitrR3JNeTVnakhLL0tFWDNUd1FNaUJ3WDN5UnYrR2p2Nitnb2g5S3NLMk15S3RTRDAwOFBpVlQyU0ZTc3QwRXIwNjJqSUZWZmxEdExrMjIzTXEwSXlKLzl1VWhzbTljclFZc2lHdTVpOUJiRmJiRW9sUHY3bmIzK2FNVS9GVkxZMklaOXNDM1hhdm5DaUdMaTdsdEoyaENCT3g0ajBzVHZXcDFwVEhQNE1PTFBMMFhyU1U2TUVzc04zYVhKTzZWU3Y5T1VGUEtISFB6RkhtUHZYZ0trdDJ2TlFtZTBzai9hT0tMVEhQSHJ4VUFtbEt3NmZRa2sxNEJKb3RwV0hsNjJ4SXFOM3ZFd0JlSUxtNkp3L1ZjQTcvQm1uNVN3OXM3L1prbVhiOWtqVlF5SW5ZdnVIVnEvU0U9LS0wYmdDRFFSZVEyS3ByS2REL1RMdjl3PT0%3D--2a9403b80d41afb31a04b08de8115aba57821b1c' \
  --compressed -v

Response:

< HTTP/1.1 401 Unauthorized
...
{"errors":"Your session has expired.","errors_cause":null}* Closing connection 0

@amiedes amiedes marked this pull request as ready for review May 4, 2020 08:45
@amiedes
Copy link
Contributor Author

amiedes commented May 4, 2020

Update: this same error is not happening in Central (I also checked there just in case).

@gonzaloriestra gonzaloriestra requested review from alrocar and removed request for gonzaloriestra May 4, 2020 09:35
@alrocar alrocar self-assigned this May 5, 2020
Copy link
Contributor

@alrocar alrocar left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, feel free to ignore them. Moving to acceptance.

lib/carto/authentication_manager.rb Outdated Show resolved Hide resolved
lib/carto/authentication_manager.rb Outdated Show resolved Hide resolved
lib/carto/authentication_manager.rb Show resolved Hide resolved
@alrocar alrocar removed their assignment May 5, 2020
@amiedes
Copy link
Contributor Author

amiedes commented May 5, 2020

I just spotted a bug testing another PR based on this. Previously, when accessing certain pages which authenticated via session and we detected it was invalid, we were redirecting to the login page:

image

Now that login page fails with a 401:

image

Maybe the session is not resetting correctly? I'm moving this back to development

@alrocar
Copy link
Contributor

alrocar commented May 6, 2020

I didn't mention in my previous review but:

  • Let's update the NEWS file. Short description of the issue + link to the issue in Github
  • pg12 tests don't work because we are upgrading many components in CI, but pg10, should work. Let's make sure they are green before delegating the PR to anyone else.

Thanks!

@amiedes
Copy link
Contributor Author

amiedes commented May 7, 2020

The login issue is fixed: I've tested it in staging with a user/pass account and Google login with my personal email. I've also tested login with MFA.

Google login with my CARTO email gets stuck, but probably it's a matter of my staging account - please test with yours to verify.

A pending improvement is that the /me endpoint is returning a 500 instead of a 401. For some reason Rails rescue_from is not detecting this error. But given that there is a specific issue for that endpoint, I'd leave it out of this PR for the moment.

I'm moving this back to acceptance.

@simon-contreras-deel simon-contreras-deel self-assigned this May 7, 2020
Copy link
Contributor

@simon-contreras-deel simon-contreras-deel left a comment

Choose a reason for hiding this comment

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

Just a comment, but it looks nice!

app/controllers/application_controller.rb Outdated Show resolved Hide resolved
@simon-contreras-deel
Copy link
Contributor

simon-contreras-deel commented May 7, 2020

Acceptance: 🍏

From my side I see it is fine. I have been checking calls from v1 and v3 after logout and everything works fine. I have checked it with different users and orgs. And of course, the org users one is ok.

@simon-contreras-deel simon-contreras-deel merged commit 2f8e9c0 into master May 8, 2020
@simon-contreras-deel simon-contreras-deel deleted the amiedes-fix-api-auth branch May 8, 2020 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants