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

MBL-1213: Stop sending token as query param for V1, send the token as basic auth header #1958

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

Arkariang
Copy link
Contributor

@Arkariang Arkariang commented Feb 27, 2024

📲 What

  • Removed token as query paramenter for V1 endpoints
  • Added token as header for V1
  • Old V1 endpoint /xauth/access_token returned user and token as response, reason why CurrentUser login method required both parameters, with the new endpoint /v1/oauth/authorizations/exchange we first obtain the token and later on request for /v1/users/self.
  • Refactor CurrentUser , CurrentUserV2 and LoginUserCase to handle separately token and user

🤔 Why

  • Token as query param is a huge security risk

👀 See

| Before 🐛 |
before

|After 🦋 |
Screenshot 2024-02-27 at 2 37 31 PM

Screenshot 2024-02-27 at 2 37 43 PM

| | |

📋 QA

No user facing changes for this one, you should be able to navigate/ login out with ffon and ffoff as usual.
But you can take a look at the network inspector, check for V1 calls and see how there is no token as query param :)

Story 📖

MBL-1213

@Arkariang Arkariang changed the title Imartin/mbl 1213 MBL-1213: Stop sending token as query param for V1, send the token as basic auth header Feb 27, 2024
@Arkariang Arkariang self-assigned this Feb 27, 2024
@Arkariang Arkariang added the OAuth 2.0 Tied to epic https://kickstarter.atlassian.net/browse/MBL-1108 label Feb 27, 2024
@Arkariang Arkariang marked this pull request as ready for review February 27, 2024 15:50
@codecov-commenter
Copy link

codecov-commenter commented Feb 27, 2024

Codecov Report

Attention: Patch coverage is 68.62745% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 73.87%. Comparing base (f786861) to head (7c18329).

Files Patch % Lines
...kickstarter/libs/utils/ApplicationLifecycleUtil.kt 0.00% 12 Missing ⚠️
...ter/services/interceptors/ApiRequestInterceptor.kt 33.33% 0 Missing and 2 partials ⚠️
.../java/com/kickstarter/viewmodels/OAuthViewModel.kt 75.00% 1 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1958      +/-   ##
============================================
+ Coverage     73.85%   73.87%   +0.02%     
- Complexity     1975     1979       +4     
============================================
  Files           346      347       +1     
  Lines         19870    19881      +11     
  Branches       2758     2758              
============================================
+ Hits          14674    14688      +14     
+ Misses         3582     3578       -4     
- Partials       1614     1615       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ycheng-kickstarter
Copy link
Contributor

LGTM
Screenshot 2024-02-27 at 11 48 20 AM
Screenshot 2024-02-27 at 11 48 30 AM

@Arkariang Arkariang merged commit 925af00 into master Feb 28, 2024
3 checks passed
@Arkariang Arkariang deleted the imartin/mbl-1213 branch February 28, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OAuth 2.0 Tied to epic https://kickstarter.atlassian.net/browse/MBL-1108
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants