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

Migration from OAuth1 to OAuth2 for OSM API #7333

Merged
merged 43 commits into from
Apr 4, 2024
Merged

Conversation

strump
Copy link
Sponsor Contributor

@strump strump commented Feb 8, 2024

This PR doesn't make any UI changes. If user was logged in to OSM account then he would see new alert asking to re-login. Old authentication secrets are removed. New OAuth2 token is requested.

Old OAuth1 New OAuth2
OM requires login + password to use it OM still requires login + password, but could be changed to browser redirects
OM stores string token and string secret to authenticate each API call OM stores only string oauthToken to authenticate API calls
Each API call requires additional query parameters: oauth_consumer_key, oauth_token, oauth_signature_method , oauth_signature, etc. See specfication Each API call requires additional header Authorization: Bearer {oauthToken}

Questions

Q1. Users should re-login once after installing new OM version

Should we notifiy users somehow? Or they'll get notification why try to edit some POI.

A1: User would see alert asking to relogin.

Q2. What is time-to-live for OAuth2 token?

I couldn't find this info in OSM documentation. Installed OM on my Android to check when API call will fail.

Q3. Unused Facebook and Google authentications

Found some unsed code to login using Facebook or Google (probably with WebView). Should we keep it? Did it ever worked?

Q4. How to manage secrets properly?

I used my personal OSM account s.trump@gmail.com to register OAuth2 applications for prod and dev environments.

Someone should replace constants in private_default.h and editor/osm_auth.cpp with official ones and prepare secret values for prod env. Already handled by @rtsisyk

Update 2024-03-10

If user was previously logged in then after installing new version he/she will see a dialog. This dialog informs him/her that old authentication is not working and he/she needs to re-login.

re-login-dialog

Update 2024-03-20

Screenshot 2024-03-20 at 09 54 48

Note: text on the alert has "here" link. But it's not a link. It's just a text with blue color. When user taps on this UITextView a listener navigates to URL. To make part of text look blue I use two localizable strings instead of one:

Android iOS
{alert_reauth_message} {alert_reauth_message_ios} + ' ' + {alert_reauth_link_text_ios}
Please login to OpenStreetMap to automatically upload all your map edits. Learn more <a href="https://github.com/organicmaps/organicmaps/issues/6144">here</a>. Please login to OpenStreetMap to automatically upload all your map edits. Learn more + space + here
Link is highlighted automatically. Pressing on a link works automatically too. Need to add blue text style to {alert_reauth_link_text_ios} string manually. Handle tap event explicitly

Comment on lines 37 to 73
@interface RedirectDelegate : NSObject<NSURLSessionDataDelegate>

@property(nonatomic) BOOL handleRedirects;

- (instancetype)init:(BOOL)handleRedirects;

- (void) URLSession:(NSURLSession *)session
task:(NSURLSessionTask *)task
willPerformHTTPRedirection:(NSHTTPURLResponse *)response
newRequest:(NSURLRequest *)newRequest
completionHandler:(void (^)(NSURLRequest *))completionHandler;
@end

@implementation RedirectDelegate
- (instancetype)init:(BOOL)handleRedirects
{
self = [super init];
if (self)
{
_handleRedirects = handleRedirects;
}

return self;
}

- (void) URLSession:(NSURLSession *)session
task:(NSURLSessionTask *)task
willPerformHTTPRedirection:(NSHTTPURLResponse *)response
newRequest:(NSURLRequest *)newRequest
completionHandler:(void (^)(NSURLRequest *))completionHandler
{
if (_handleRedirects && response.statusCode >= 300 && response.statusCode < 400) {
completionHandler(nil);
}
else {
completionHandler(newRequest);
}
}
@end
Copy link
Sponsor Contributor Author

@strump strump Feb 8, 2024

Choose a reason for hiding this comment

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

Current implementation of bool HttpClient::RunHttpRequest() doesn't support handling of redirects.

Actual behaviour:

  1. Make HTTP request to osm.org/oauth2/authorize
  2. Server responses with HTTP Status 302 and redirects to om://oauth2/osm/callback?code=xQpf58...
  3. iOS follows redirect and tries to open URL om://oauth2/osm/callback?code=xQpf58...
  4. iOS gives error: Unknown protocol

Expected behaviour:

  1. Make HTTP request to osm.org/oauth2/authorize
  2. Server responses with HTTP Status 302 and redirects to om://oauth2/osm/callback?code=xQpf58...
  3. If HttpClient::handle_redirects field is true then do not follow redirect and give back response from server.

To handle redirect I had to implement RedirectDelegate. Is it made correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current HttpClient/Downloader is one big mess, unfortunately. m_handleRedirects solution looks good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Since when did increasing the entropy become a better solution? ;)

Copy link
Member

Choose a reason for hiding this comment

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

handleRedirects is unclear. Does it mean that HttpClient handles redirects automatically? Or the user should handle it manually? followRedirects is more clear.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Replaced handleRedirects with followRedirects all over the code.

dispatch_async(dispatch_get_main_queue(), ^{
dispatch_async(dispatch_get_current_queue(), ^{
Copy link
Sponsor Contributor Author

@strump strump Feb 9, 2024

Choose a reason for hiding this comment

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

For some reason tests on MacOS hangs with dispatch_get_main_queue() function. Replaced with dispatch_get_current_queue(). Looks like main queue != current queue. It doesn't break iOS.

But dispatch_get_current_queue() function is deprecated. What is propper fix here?

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Because of my change tests are failing at assertion:

ASSERT_EQUAL(id, threads::GetCurrentThreadID(), ("OnWrite called from different threads"));

How to fix it on MacOS? Remove assert?

Copy link
Member

Choose a reason for hiding this comment

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

HTTP code is thread-safe only if it's called on the same thread. Instead of removing the assert, the callbacks logic should be checked, why different thread is used in different OnWrite calls.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Looks like there is no such thing as main thread in our test apps.
If you go to downloader_test.cpp you can see calls to QCoreApplication::exec().

I think, main queue is created only after QCoreApplication::exec() call. But it doesn't work for osm_auth_tests.cpp. I'm getting error:

QApplication::exec: Please instantiate the QApplication object first

Copy link
Sponsor Contributor Author

@strump strump Feb 9, 2024

Choose a reason for hiding this comment

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

This is how good stack trace looks like when running platform_tests:
good-stack-trace

dispatch_async(dispatch_get_main_queue(), ...) really dispatches call in main queue. Need to do simillar for osm_auth_tests.

Copy link
Sponsor Contributor Author

@strump strump Feb 15, 2024

Choose a reason for hiding this comment

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

@biodranik I found a workaround for MacOS: see commit #6c8171f8.

For MacOS each test should be wrapped in lambda and executed in a separate thread while main thread is used for main queue.

Do you have any ideas how to improve this solution? Introduce macro for this case?

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look like a good solution. A better approach is needed. Maybe hide relevant logic in the test's main, like it was already done for some tests, or check if the code can be corrected/rewritten properly.

Copy link
Sponsor Contributor Author

@strump strump Feb 27, 2024

Choose a reason for hiding this comment

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

@biodranik I changed HttpSessionManager behaviour in commit #ec7ece7e
Instead of using main queue for all callbacks it now uses custom queue.

Map download functionality works on iOS simulator. Tests oam_auth_tests does not fail.

Need to fix platform_tests

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

👆 Update: that solution doesn't actually work.

We can't replace main queue with custom queue because there is no guaranty that the same thread will be used. When we post task to a queue with dispatch_async(...) it could be handled from different threads. See https://stackoverflow.com/q/52389851

So we should either keep main queue and use some workarounds for unit-tests or get rid of queues and use threads in HttpSessionManager class.

@rtsisyk
Copy link
Contributor

rtsisyk commented Feb 11, 2024

Q1. Users should login again after installing new OM version
Should we notifiy users somehow? Or they'll get notification why try to edit some POI.

My proposal is to keep the existing OAuth1 code for some time while we test this new OAuth2 implementation on Android first. Would it be possible to keep two versions?

Q2. What is time-to-live for OAuth2 token?
I couldn't find this info in OSM documentation. Installed OM on my Android to check when API call will fail.

Please use refresh tokens: https://oauth.net/2/refresh-tokens/

Q3. Unused Facebook and Google authentications

Found some unsed code to login using Facebook or Google (probably with WebView). Should we keep it? Did it ever worked?

Let's remove it via a separate PR.

Q4. How to manage secrets properly?

I used my personal OSM account s.trump@gmail.com to register OAuth2 applications for prod and dev environments.
Someone should replace constants in private_default.h and editor/osm_auth.cpp with official ones and prepare secret values for prod env.

Yes, please use private_default.h for now. I will take care of it later.

@rtsisyk rtsisyk self-requested a review February 11, 2024 08:32
@mmd-osm
Copy link

mmd-osm commented Feb 11, 2024

Q2. What is time-to-live for OAuth2 token?
I couldn't find this info in OSM documentation. Installed OM on my Android to check when API call will fail.

Please use refresh tokens: https://oauth.net/2/refresh-tokens/

OAuth2 bearer tokens on osm.org don't expire at the moment, hence there's also no refresh token available. However, for the sake of being future proof, it's a good idea to consider refresh tokens. Maybe they will be opt-in at some point in the future.

--> https://github.com/openstreetmap/openstreetmap-website/blob/master/config/initializers/doorkeeper.rb#L213

@biodranik
Copy link
Member

Q1. Users should re-login once after installing new OM version

Notification is definitely needed, maybe in the case of not uploaded edits only.

Q2. What is time-to-live for OAuth2 token?

It's not reset now, but it would be great to properly detect the case when it's reset, and at least offer to relogin again. Ideally use what Roman mentioned.

Q3. Unused Facebook and Google authentications

Don't remove it. We want to use it for faster user registration in OSM in the future, when we reuse OSM login instead of implementing OM auth.

Q4. How to manage secrets properly?

In the same way as they're managed now. We'll set them properly later.

@rtsisyk
Copy link
Contributor

rtsisyk commented Feb 13, 2024

Q3. Unused Facebook and Google authentications

Don't remove it. We want to use it for faster user registration in OSM in the future, when we reuse OSM login instead of implementing OM auth.

What is the point of keeping died code? It will be broken during edits because it can't be run. The old code can always be restored from git when it is needed.

@biodranik
Copy link
Member

biodranik commented Feb 13, 2024

Nobody will look for some older code in git history without knowing that some code existed before. Commented code is very explicit and acts as a TODO comment.

@westnordost
Copy link

Q2. What is time-to-live for OAuth2 token?

I couldn't find this info in OSM documentation. Installed OM on my Android to check when API call will fail.

Not only is there currently no refresh token because OAuth2 tokens on OSM are valid forever, according to the OSM website maintainers are of the opinion that expiring OAuth2 tokens would not really serve a useful purpose in the case of OSM because they can already be revoked by the user (source). So, it is very unlikely that OSM OAuth2 tokens will ever expire and that they will ever be refresh tokens.

That however does still mean that the case that the OAuth2 token suddenly is not accepted by OSM anymore needs to be handled gracefully. As before with OAuth 1.0a, I figure, because it is also possible for the user to revoke these tokens.

In StreetComplete, I blanket handle such cases by logging out the user (i.e. clearing the token) when any authorization-related HTTP error code is returned on any call to the OSM API (HTTP 401 or HTTP 403).

@rtsisyk
Copy link
Contributor

rtsisyk commented Feb 18, 2024

I've heard today that OAuth1 in osm.org will be disabled on June 1 this year. I'm withdrawing my request for support for both versions. Please go ahead 🚀

@strump strump marked this pull request as ready for review February 27, 2024 09:35
@rtsisyk
Copy link
Contributor

rtsisyk commented Mar 3, 2024

Is this patch ready for beta?

@Jean-BaptisteC
Copy link
Member

I have done firsts tests on Android, and it works properly;
✅ Connection to OSM
✅ Update POI (I can't test to create a new POI, I use the developer OSM website)

@strump
Copy link
Sponsor Contributor Author

strump commented Mar 4, 2024

@rtsisyk feature is ready to merge. Tests are failing. Maybe PR #7405 will fix MacOS tests

I also made a change to osm_auth_tests. Asked @biodranik if my workaround is OK: comment

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Is there any UX migration for existing users? How do they know that they should login again?

What happens with Google Play users who were logged in but where OSM login dialog is disabled?

namespace osm
{
using platform::HttpClient;
using std::string;

constexpr char const * kApiVersion = "/api/0.6";
constexpr char const * kFacebookCallbackPart = "/auth/facebook_access_token/callback?access_token=";
Copy link
Member

Choose a reason for hiding this comment

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

Are these oauth2? Can we reuse them later for faster user registration, or should it be rewritten from scratch?

dispatch_async(dispatch_get_main_queue(), ^{
dispatch_async(dispatch_get_current_queue(), ^{
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't look like a good solution. A better approach is needed. Maybe hide relevant logic in the test's main, like it was already done for some tests, or check if the code can be corrected/rewritten properly.

@@ -0,0 +1,226 @@
import http.client
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this!!!

rtsisyk added a commit that referenced this pull request Mar 10, 2024
See #7333

Signed-off-by: Roman Tsisyk <roman@tsisyk.com>
Copy link
Contributor

@rtsisyk rtsisyk left a comment

Choose a reason for hiding this comment

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

Thank you for this substantial contribution. I've tested it on both Android and iPhone, and it functions perfectly. Excellent job!

I agree with the comment that it would be nice to notify the user about the need for re-authentication. However, I don't see it as a blocker for this PR. The situation that the authentication token expires, and the user needs to authenticate is quite common. The app didn't handle such case before this PR and doesn't handle with this PR. Therefore, the existing state of affairs is preserved. My recommendation is to file a separate ticket for this issue and address it somewhere in the future, based on priorities.

@rtsisyk
Copy link
Contributor

rtsisyk commented Mar 10, 2024

Credentials have been added to master.

@rtsisyk rtsisyk requested a review from biodranik March 10, 2024 10:29
@biodranik
Copy link
Member

The situation that the authentication token expires, and the user needs to authenticate is quite common. The app didn't handle such case before this PR and doesn't handle with this PR.

It is not a common situation with OM and mapsme. The token has NEVER expired before. That is the reason why the app didn't handle such case.

It is crucially important to support a proper UX migration.

@strump did you test what happens in the following scenarios? Are there any notifications on iOS and Android?

  1. Make sure you're logged in qith oauth1

  2. Edit something on the current master but don't upload it to OpenStreetMap.org.

  3. Update the app and run it with new oauth2

  4. Any notifications about not uploaded features/missing login?

  5. Edit something.

  6. Any notifications?

  7. Make sure that you're logged in with oauth1 and there are no edits.

  8. Update the app

  9. Any notifications?

  10. Edit something

  11. Any notifications?

OsmOAuth auth = OsmOAuth::DevServerAuth();
bool result;
TEST_NO_THROW(result = auth.ResetPassword(kForgotPasswordEmail), ());
TEST_EQUAL(result, true, ("Correct email"));
TEST_NO_THROW(result = auth.ResetPassword("not@registered.email"), ());
TEST_EQUAL(result, false, ("Incorrect email"));
TEST_EQUAL(result, true, ("Server responses OK even if email is incorrect"));
Copy link
Member

Choose a reason for hiding this comment

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

This change should be reverted, it is wrong.

@@ -20,6 +20,10 @@ using namespace pugi;
extern char const * kValidOsmUser;
extern char const * kValidOsmPassword;

#if defined(OMIM_OS_MAC)
extern void runMainQueue(void func());
Copy link
Member

Choose a reason for hiding this comment

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

This test workaround is very ugly, are there any better options?

@@ -11,6 +11,7 @@
NSString * const kOSMRequestToken = @"OSMRequestToken";
Copy link
Member

Choose a reason for hiding this comment

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

Are all these constants still used?

Copy link
Member

Choose a reason for hiding this comment

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

Aren't these controllers needed for Google/FB OSM auth?

newRequest:(NSURLRequest *)newRequest
completionHandler:(void (^)(NSURLRequest *))completionHandler
{
if (_handleRedirects && response.statusCode >= 300 && response.statusCode < 400) {
Copy link
Member

Choose a reason for hiding this comment

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

Please check indents and code style (e.g. {}) everywhere. Maybe they are wrongly set in XCode.

Comment on lines 37 to 73
@interface RedirectDelegate : NSObject<NSURLSessionDataDelegate>

@property(nonatomic) BOOL handleRedirects;

- (instancetype)init:(BOOL)handleRedirects;

- (void) URLSession:(NSURLSession *)session
task:(NSURLSessionTask *)task
willPerformHTTPRedirection:(NSHTTPURLResponse *)response
newRequest:(NSURLRequest *)newRequest
completionHandler:(void (^)(NSURLRequest *))completionHandler;
@end

@implementation RedirectDelegate
- (instancetype)init:(BOOL)handleRedirects
{
self = [super init];
if (self)
{
_handleRedirects = handleRedirects;
}

return self;
}

- (void) URLSession:(NSURLSession *)session
task:(NSURLSessionTask *)task
willPerformHTTPRedirection:(NSHTTPURLResponse *)response
newRequest:(NSURLRequest *)newRequest
completionHandler:(void (^)(NSURLRequest *))completionHandler
{
if (_handleRedirects && response.statusCode >= 300 && response.statusCode < 400) {
completionHandler(nil);
}
else {
completionHandler(newRequest);
}
}
@end
Copy link
Member

Choose a reason for hiding this comment

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

handleRedirects is unclear. Does it mean that HttpClient handles redirects automatically? Or the user should handle it manually? followRedirects is more clear.

@@ -20,12 +20,12 @@ namespace qt
char const * kTokenKeySetting = "OsmTokenKey";
Copy link
Member

Choose a reason for hiding this comment

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

Are these vars still used?

Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

Was maps downloading tested on the Simulator and the iOS device?

platform/http_client_apple.mm Outdated Show resolved Hide resolved

[[[HttpSessionManager sharedManager]
dataTaskWithRequest:request
delegate:nil
Copy link
Member

Choose a reason for hiding this comment

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

Can self implement the delegate in a simpler way?

Copy link
Member

Choose a reason for hiding this comment

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

Meaning: will it be simpler if instead of passing an external delegate, it will be implemented in this class, and delegate:self will be passed to the function?

m_urlReceived = response.URL.absoluteString.UTF8String;

NSString * redirectUri = [response.allHeaderFields objectForKey:@"Location"];
if (redirectUri)
Copy link
Member

Choose a reason for hiding this comment

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

The Location response header indicates the URL to redirect a page to. It only provides a meaning when served with a 3xx (redirection) or 201 (created) status response.

Should status code be checked too?

platform/http_client_curl.cpp Outdated Show resolved Hide resolved
@biodranik biodranik force-pushed the osm-oauth2-migration branch 3 times, most recently from 9a1a5be to 1469c03 Compare March 10, 2024 15:34
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Made updates all over the code

Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Removed EditorHostFragment.clearNoobAlertFlag() method

Signed-off-by: S. Kozyr <s.trump@gmail.com>
… code

Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
Signed-off-by: S. Kozyr <s.trump@gmail.com>
@rtsisyk
Copy link
Contributor

rtsisyk commented Mar 26, 2024

Please postpone merging of this PR until April's release is released. No need to rush. We will merge it in Apr (~1 week from now) to have more time for beta-testing.

Copy link
Contributor

@kirylkaveryn kirylkaveryn left a comment

Choose a reason for hiding this comment

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

I have tested it on an iPhone11Pro (device) and it works well. Thank you!

…ev.openstreetmap.org

Have to create new one and update Debug credentials

Signed-off-by: S. Kozyr <s.trump@gmail.com>
@rtsisyk
Copy link
Contributor

rtsisyk commented Apr 4, 2024

It's time to merge. Let's address all leftovers and pending comments via separate PRs.

@rtsisyk rtsisyk merged commit 921cdaa into master Apr 4, 2024
16 checks passed
@rtsisyk rtsisyk deleted the osm-oauth2-migration branch April 4, 2024 05:45
@biodranik
Copy link
Member

@strump what's left? Can you please create a separate issue for it, if necessary?

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.

None yet

7 participants