Site icon indicating copy to clipboard operation
Site copied to clipboard

Migrate to OAuth2

Open HarelM opened this issue 1 year ago • 7 comments

Since OAuth 1 will be deprecated soon in OSM, remove it from our code and only use OAuth 2. The code base already handles OAuth2 well, so I removed all the places where OAuth 1 is used. I also removed the need to authenticate from routes that are not "guarded". This will affect users who did not clean the app / did a logout-login / installed the app recently (In the last 1.5 years since #1798 was published).

Closes #1968 Resolves #1985

I have tried to migrate the code to use the following webtokenhandler but the documentation is lacking and I think that it will end up very close to what I did anyway, so this closes this issue without implementing it.

  • #1968

FYI: @zstadler

HarelM avatar Mar 30 '24 11:03 HarelM

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Mar 30 '24 11:03 CLAassistant

What is the planned user experience for those who still use OAuth1? The server shos about 2 such requests per minute

zstadler avatar Mar 30 '24 12:03 zstadler

The client will get a 401, current client code doesn't do anything with it, besides showing generic error code according to the relevant operation. I can change the client code to logout, which is the usual behavior for such a case, but this won't affect clients that are not up to date.

HarelM avatar Mar 30 '24 13:03 HarelM

Also the rate will be dramatically reduced, as most operations do not require a logged in user.

HarelM avatar Mar 30 '24 14:03 HarelM

Codecov Report

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

Project coverage is 91.02%. Comparing base (bae2247) to head (81ab25c).

Files Patch % Lines
IsraelHiking.API/Controllers/OsmController.cs 66.66% 1 Missing :warning:
...elHiking.API/Services/Osm/OsmAuthFactoryWrapper.cs 50.00% 0 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1982      +/-   ##
==========================================
+ Coverage   91.01%   91.02%   +0.01%     
==========================================
  Files          93       93              
  Lines        6821     6808      -13     
  Branches      982      981       -1     
==========================================
- Hits         6208     6197      -11     
+ Misses        411      410       -1     
+ Partials      202      201       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 30 '24 14:03 codecov[bot]

How difficult would it be to add user migration code to the client, as you did during the migration to pmtiles? Any simple mechanism would do, even a "Please logout and login" popup, as long as the reliance on OAuth1 credentials is not silently ignored.

zstadler avatar Mar 31 '24 06:03 zstadler

I can check the current token and logout the user in case of oauth1. The current code will force the next login to be of oauth2 type. This is the easiest solution I believe as I only need to delete the user info from the state.

HarelM avatar Mar 31 '24 09:03 HarelM

Codecov Report

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

Project coverage is 90.77%. Comparing base (153f0de) to head (2258547).

Files Patch % Lines
IsraelHiking.API/Controllers/OsmController.cs 66.66% 1 Missing :warning:
...elHiking.API/Services/Osm/OsmAuthFactoryWrapper.cs 50.00% 0 Missing and 1 partial :warning:

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1982      +/-   ##
==========================================
+ Coverage   90.76%   90.77%   +0.01%     
==========================================
  Files          94       94              
  Lines        8193     8177      -16     
  Branches      992      991       -1     
==========================================
- Hits         7436     7423      -13     
+ Misses        531      529       -2     
+ Partials      226      225       -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar May 29 '24 07:05 codecov-commenter