Migration from OAuth1 to OAuth2 for OSM API

strump commented

2024-02-08 16:59:46 +00:00

Member

~~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

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.

Update 2024-03-20

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://git.omaps.dev/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

~~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](https://oauth.net/core/1.0/#anchor13) | 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. <img alt="re-login-dialog" src="/uploads/42ea869c531b6dc3cf2c543d7f443415/548e01a0-b245-438e-a327-fc52519d8aa1" width="300px"/> ## Update 2024-03-20 <img width="300px" alt="Screenshot 2024-03-20 at 09 54 48" src="/uploads/ba10763b00597823337e89c56126c216/b9bdf8c3-a01c-417e-b006-6d7f35bb3edb"> 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://git.omaps.dev/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 |

strump commented

2024-02-08 17:05:52 +00:00

Author

Member

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

Actual behaviour:

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

Expected behaviour:

Make HTTP request to osm.org/oauth2/authorize
Server responses with HTTP Status 302 and redirects to om://oauth2/osm/callback?code=xQpf58...
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?

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?

strump commented

2024-02-09 09:18:30 +00:00

Author

Member

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?

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?

strump commented

2024-02-09 11:29:29 +00:00

Author

Member

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?

Because of my change tests are failing at [assertion](https://github.com/organicmaps/organicmaps/blob/master/platform/http_request.cpp#L185): ``` ASSERT_EQUAL(id, threads::GetCurrentThreadID(), ("OnWrite called from different threads")); ``` How to fix it on MacOS? Remove assert?

biodranik commented

2024-02-09 12:53:55 +00:00

(Migrated from github.com)

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.

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.

strump commented

2024-02-09 15:44:57 +00:00

Author

Member

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

Looks like there is no such thing as main thread in our test apps. If you go to [downloader_test.cpp](https://github.com/organicmaps/organicmaps/blob/master/platform/platform_tests/downloader_tests/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 ```

strump commented

2024-02-09 15:54:36 +00:00

Author

Member

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

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

This is how good stack trace looks like when running `platform_tests`: ![good-stack-trace](/uploads/49b145013786047aa8006d615a6fb705/48bdf08b-118e-4866-b5b1-08814aa44da8) `dispatch_async(dispatch_get_main_queue(), ...)` really dispatches call in main queue. Need to do simillar for `osm_auth_tests`.

biodranik commented

2024-02-09 22:54:05 +00:00

(Migrated from github.com)

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

rtsisyk commented

2024-02-11 08:31:55 +00:00

Owner

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.

> 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 commented

2024-02-11 08:32:08 +00:00

Owner

requested review from @rtsisyk

requested review from `@rtsisyk`

mmdosm commented

2024-02-11 15:55:03 +00:00

(Migrated from github.com)

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

> > 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 commented

2024-02-11 19:55:38 +00:00

(Migrated from github.com)

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.

> 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 commented

2024-02-13 08:47:22 +00:00

Owner

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.

> > 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 commented

2024-02-13 20:49:16 +00:00

(Migrated from github.com)

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.

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.

strump commented

2024-02-15 07:57:28 +00:00

Author

Member

@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?

`@biodranik` I found a workaround for MacOS: see commit [#6c8171f8](https://git.omaps.dev/organicmaps/organicmaps/pulls/7333/commits/6c8171f83786773ebf19905dde2d4e468435ec25). 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?

westnordost commented

2024-02-15 16:36:03 +00:00

(Migrated from github.com)

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).

> 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](https://community.openstreetmap.org/t/oauth-1-0a-and-http-basic-auth-shutdown/108490/3)). 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).

biodranik commented

2024-02-16 11:25:36 +00:00

(Migrated from github.com)

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.

rtsisyk commented

2024-02-18 12:20:40 +00:00

Owner

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 🚀

rtsisyk commented

2024-02-18 12:36:29 +00:00

Owner

mentioned in issue #7422

strump commented

2024-02-27 09:37:49 +00:00

Author

Member

@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

`@biodranik` I changed `HttpSessionManager` behaviour in commit [#ec7ece7e](https://git.omaps.dev/organicmaps/organicmaps/pulls/7333/commits/ec7ece7eccb70f0ca4bf5c6ea3199eaa5aaa9ed1) 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`

strump commented

2024-02-28 08:14:40 +00:00

Author

Member

👆 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.

👆 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 commented

2024-03-03 17:41:21 +00:00

Owner

Is this patch ready for beta?

Jean-BaptisteC commented

2024-03-03 18:05:52 +00:00

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)

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 commented

2024-03-04 08:01:09 +00:00

Author

Member

@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

`@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](https://git.omaps.dev/organicmaps/organicmaps/pulls/7333#discussion_r1490560279)

biodranik commented

2024-03-04 19:01:39 +00:00

(Migrated from github.com)

Review: Changes requested

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?

**Review:** Changes requested 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?

rtsisyk commented

2024-03-10 08:58:32 +00:00

Owner

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

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

rtsisyk commented

2024-03-10 09:00:50 +00:00

Owner

Thanks for adding this!!!

rtsisyk commented

2024-03-10 10:28:38 +00:00

Owner

Review: Approved

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.

**Review:** Approved 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 commented

2024-03-10 10:28:38 +00:00

Owner

approved this merge request

rtsisyk commented

2024-03-10 10:28:55 +00:00

Owner

Credentials have been added to master.

rtsisyk commented

2024-03-10 10:29:09 +00:00

Owner

requested review from @biodranik

requested review from `@biodranik`

biodranik commented

2024-03-10 10:44:43 +00:00

(Migrated from github.com)

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?

Make sure you're logged in qith oauth1
Edit something on the current master but don't upload it to OpenStreetMap.org.
Update the app and run it with new oauth2
Any notifications about not uploaded features/missing login?
Edit something.
Any notifications?
Make sure that you're logged in with oauth1 and there are no edits.
Update the app
Any notifications?
Edit something
Any notifications?

> 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? 0. Make sure you're logged in qith oauth1 1. Edit something on the current master but don't upload it to OpenStreetMap.org. 2. Update the app and run it with new oauth2 3. Any notifications about not uploaded features/missing login? 4. Edit something. 5. Any notifications? 1. Make sure that you're logged in with oauth1 and there are no edits. 2. Update the app 3. Any notifications? 4. Edit something 5. Any notifications?

biodranik commented

2024-03-10 11:43:50 +00:00

(Migrated from github.com)

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

biodranik commented

2024-03-10 11:52:00 +00:00

(Migrated from github.com)

This change should be reverted, it is wrong.

biodranik commented

2024-03-10 11:54:42 +00:00

(Migrated from github.com)

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

biodranik commented

2024-03-10 11:55:31 +00:00

(Migrated from github.com)

Are all these constants still used?

biodranik commented

2024-03-10 11:56:34 +00:00

(Migrated from github.com)

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

biodranik commented

2024-03-10 11:57:42 +00:00

(Migrated from github.com)

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

biodranik commented

2024-03-10 11:59:36 +00:00

(Migrated from github.com)

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

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

biodranik commented

2024-03-10 12:03:21 +00:00

(Migrated from github.com)

Are these vars still used?

biodranik commented

2024-03-10 12:23:00 +00:00

(Migrated from github.com)

  if (self = [super init])
    _handleRedirects = handleRedirects;

  return self;

```suggestion:-0+0 if (self = [super init]) _handleRedirects = handleRedirects; return self; ```

biodranik commented

2024-03-10 12:24:45 +00:00

(Migrated from github.com)

Can self implement the delegate in a simpler way?

Can `self` implement the delegate in a simpler way?

biodranik commented

2024-03-10 12:25:55 +00:00

(Migrated from github.com)

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?

> 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?

biodranik commented

2024-03-10 12:26:11 +00:00

(Migrated from github.com)

  else if (!m_handleRedirects)

```suggestion:-0+0 else if (!m_handleRedirects) ```

biodranik commented

2024-03-10 12:27:08 +00:00

(Migrated from github.com)

Review: Changes requested

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

**Review:** Changes requested Was maps downloading tested on the Simulator and the iOS device?

rtsisyk commented

2024-03-12 08:17:20 +00:00

Owner

Please generate translations using ./tools/python/translate.py

strump commented

2024-03-21 19:58:21 +00:00

Author

Member

translate.py requires DeepL API key. DeepL requires card number to prove you are a human. But Ukrainian cards are not accepted.

Could someone translate alert_reauth_message string? Or I can do it manually through web UI

`translate.py` requires DeepL API key. DeepL requires card number to prove you are a human. But Ukrainian cards are not accepted. Could someone translate `alert_reauth_message` string? Or I can do it manually through web UI

strump commented

2024-03-21 20:00:46 +00:00

Author

Member

Replaced handleRedirects with followRedirects all over the code.

Replaced `handleRedirects` with `followRedirects` all over the code.

biodranik commented

2024-03-22 23:54:55 +00:00

(Migrated from github.com)

If someone was already logged in, then he is not a noob anymore ) Maybe remove it?

biodranik commented

2024-03-22 23:55:45 +00:00

(Migrated from github.com)

Which other dialog can be displayed at that time?

biodranik commented

2024-03-23 00:03:57 +00:00

(Migrated from github.com)

@kirylkaveryn is it possible to reuse the translation above, with the embedded HTTP URL, without manual strings splitting? NSMutableAttributedString should support clickable links with "Selectable" text field, right?

@strump strings may not split fine in different languages. Is it possible to dynamically find and cut the HTML tag on iOS?

`@kirylkaveryn` is it possible to reuse the translation above, with the embedded HTTP URL, without manual strings splitting? NSMutableAttributedString should support clickable links with "Selectable" text field, right? ` @strump` strings may not split fine in different languages. Is it possible to dynamically find and cut the HTML tag on iOS?

biodranik commented

2024-03-23 00:05:29 +00:00

(Migrated from github.com)

Can this 3party library be completely removed?

biodranik commented

2024-03-23 00:06:06 +00:00

(Migrated from github.com)

  if (!oauth2code || oauth2code->empty())

```suggestion:-0+0 if (!oauth2code || oauth2code->empty()) ```

biodranik commented

2024-03-23 00:06:27 +00:00

(Migrated from github.com)

    if (json_is_string(token_node))

```suggestion:-0+0 if (json_is_string(token_node)) ```

biodranik commented

2024-03-23 00:07:33 +00:00

(Migrated from github.com)

m_authenticityToken

biodranik commented

2024-03-23 00:12:18 +00:00

(Migrated from github.com)

  if (request.WasRedirected())
  {

```suggestion:-0+0 if (request.WasRedirected()) { ```

biodranik commented

2024-03-23 00:12:31 +00:00

(Migrated from github.com)

  else
  {

```suggestion:-0+0 else { ```

biodranik commented

2024-03-23 00:15:36 +00:00

(Migrated from github.com)

No need to split these lines in XCode. Longer lines are easier to read in some cases.

  MWMOsmReauthAlert * alert = [NSBundle.mainBundle loadNibNamed:[self className] owner:nil options:nil].firstObject;

No need to split these lines in XCode. Longer lines are easier to read in some cases. ```suggestion:-0+0 MWMOsmReauthAlert * alert = [NSBundle.mainBundle loadNibNamed:[self className] owner:nil options:nil].firstObject; ```

biodranik commented

2024-03-23 00:16:17 +00:00

(Migrated from github.com)

Doesn't it look ugly? :)

    [self.alertController.ownerViewController performSegueWithIdentifier:kMap2OsmLoginSegue sender:nil];

Doesn't it look ugly? :) ```suggestion:-0+0 [self.alertController.ownerViewController performSegueWithIdentifier:kMap2OsmLoginSegue sender:nil]; ```

biodranik commented

2024-03-23 00:17:28 +00:00

(Migrated from github.com)

HttpClient & HttpClient::SetFollowRedirects(bool followRedirects)

```suggestion:-0+0 HttpClient & HttpClient::SetFollowRedirects(bool followRedirects) ```

biodranik commented

2024-03-23 00:18:15 +00:00

(Migrated from github.com)

Won't changing the default value break some other existing requests?

biodranik commented

2024-03-23 00:18:58 +00:00

(Migrated from github.com)

  if (self = [super init])
    _followRedirects = followRedirects;

```suggestion:-0+0 if (self = [super init]) _followRedirects = followRedirects; ```

biodranik commented

2024-03-23 00:19:17 +00:00

(Migrated from github.com)

Wrong indent

biodranik commented

2024-03-23 00:20:36 +00:00

(Migrated from github.com)

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?

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?

biodranik commented

2024-03-23 00:20:52 +00:00

(Migrated from github.com)

  else if (m_followRedirects)

```suggestion:-0+0 else if (m_followRedirects) ```

biodranik commented

2024-03-23 00:21:22 +00:00

(Migrated from github.com)

nit

    settings::Set(kOauthTokenSetting, {});

nit ```suggestion:-0+0 settings::Set(kOauthTokenSetting, {}); ```

biodranik commented

2024-03-23 00:21:48 +00:00

(Migrated from github.com)

Should the PR be rebased?

biodranik commented

2024-03-23 00:22:46 +00:00

(Migrated from github.com)

Review: Commented

Thanks! We're almost there.

@kirylkaveryn can you please review and test the iOS code?

Any volunteers to test Android?

**Review:** Commented Thanks! We're almost there. @kirylkaveryn can you please review and test the iOS code? Any volunteers to test Android?

kirylkaveryn commented

2024-03-25 07:59:25 +00:00

Member

This property seems private so It's better to place it inside the m file. But be careful while moving this line from one file to another with the outlets! Sometimes it can duplicate and produce crashes (just deleting the property or renaming may produce the same error). So it's better to move the line, clean-up the outlet in the InterfaceBuilder, and re-create the outlet.
retain should not be used because this property attribute is from the MRC (Manual Reference Counting) world.
For IBOutlets @property (nonatomic, weak) IBOutlet should be used in most cases.

1. This property seems private so It's better to place it inside the `m` file. But be careful while moving this line from one file to another with the outlets! Sometimes it can duplicate and produce crashes (just deleting the property or renaming may produce the same error). So it's better to move the line, clean-up the outlet in the InterfaceBuilder, and re-create the outlet. 2. `retain` should not be used because this property attribute is from the MRC (Manual Reference Counting) world. For IBOutlets `@property (nonatomic, weak) IBOutlet ` should be used in most cases.

kirylkaveryn commented

2024-03-25 08:09:26 +00:00

Member

Usually for links NSLinkAttributeName is used with the URL as value to make a tappable link inside the text fully handled by the system.
So the link for the only here can be added without explicit link opening.

Usually for links `NSLinkAttributeName` is used with the URL as `value` to make a tappable link inside the text fully handled by the system. So the link for the only `here` can be added without explicit link opening.

kirylkaveryn commented

2024-03-25 08:12:29 +00:00

Member

This method does nothing and should not be used.
https://developer.apple.com/documentation/foundation/userdefaults/1414005-synchronize

This method does nothing and should not be used. https://developer.apple.com/documentation/foundation/userdefaults/1414005-synchronize

kirylkaveryn commented

2024-03-25 08:13:20 +00:00

Member

Can be removed

strump commented

2024-03-25 08:50:24 +00:00

Author

Member

Replaced !m_handleRedirects with m_followRedirects.
Fixed formatting

Replaced `!m_handleRedirects` with `m_followRedirects`. Fixed formatting

strump commented

2024-03-25 09:03:01 +00:00

Author

Member

Fixed argument name

strump commented

2024-03-25 09:03:51 +00:00

Author

Member

Renamedm_authenticity_token to m_authenticityToken

Renamed`m_authenticity_token` to `m_authenticityToken`

strump commented

2024-03-25 09:04:02 +00:00

Author

Member

Fixed formatting

strump commented

2024-03-25 09:04:10 +00:00

Author

Member

Fixed

strump commented

2024-03-25 09:04:23 +00:00

Author

Member

Ok, removed.

strump commented

2024-03-25 09:07:34 +00:00

Author

Member

In http_client_apple.mm I replaced

if (_handleRedirects && response.statusCode >= 300 && response.statusCode < 400)

with

if (!_followRedirects && response.statusCode >= 300 && response.statusCode < 400)

In `http_client_apple.mm` I replaced ``` if (_handleRedirects && response.statusCode >= 300 && response.statusCode < 400) ``` with ``` if (!_followRedirects && response.statusCode >= 300 && response.statusCode < 400) ```

strump commented

2024-03-25 09:13:24 +00:00

Author

Member

For Android it's tricky. In android/app/src/main/cpp/app/organicmaps/util/HttpClient.cpp there was a line:

SetBoolean(env, httpParamsObject.get(), ids.GetId("followRedirects"), m_handleRedirects);

Meaning that m_handleRedirects field is used as followRedirects field which is wrong. But HTTP Client in Java works in such way that it doesn't follow redirect to unknown protocols.
So if followRedirects = true and HTTP Response redirect to om://oauth2/osm/callback?code=P7dyHlhtepaYT then Java doesn't follow this redirect and everything works fine. So I changed code to:

SetBoolean(env, httpParamsObject.get(), ids.GetId("followRedirects"), m_followRedirects);

It should work correctly now.

For Android it's tricky. In `android/app/src/main/cpp/app/organicmaps/util/HttpClient.cpp` there was a line: ``` SetBoolean(env, httpParamsObject.get(), ids.GetId("followRedirects"), m_handleRedirects); ``` Meaning that `m_handleRedirects` field is used as `followRedirects` field which is wrong. But HTTP Client in Java works in such way that it doesn't follow redirect to unknown protocols. So if `followRedirects = true` and HTTP Response redirect to `om://oauth2/osm/callback?code=P7dyHlhtepaYT` then Java doesn't follow this redirect and everything works fine. So I changed code to: ``` SetBoolean(env, httpParamsObject.get(), ids.GetId("followRedirects"), m_followRedirects); ``` It should work correctly now.

strump commented

2024-03-25 09:15:48 +00:00

Author

Member

Tests are green, so this flag is handled correctly.

strump commented

2024-03-25 09:15:56 +00:00

Author

Member

Fixed formatting

strump commented

2024-03-25 09:16:06 +00:00

Author

Member

Fixed

strump commented

2024-03-25 09:16:16 +00:00

Author

Member

Rebased to master

strump commented

2024-03-25 09:16:36 +00:00

Author

Member

Fixed formatting

strump commented

2024-03-25 09:17:06 +00:00

Author

Member

Fixed according to suggestion

strump commented

2024-03-25 09:18:48 +00:00

Author

Member

We don't use liboauthcpp lib for OAuth2. In OM it's used to calculate SHA1.
I think we can remove it in PR #7658

We don't use `liboauthcpp` lib for OAuth2. In OM it's used to calculate SHA1. I think we can remove it in PR #7658

strump commented

2024-03-25 09:21:10 +00:00

Author

Member

I'm not very good at string processing with NSString. I created strings alert_reauth_message_ios and alert_reauth_link_text_ios by splittinh alert_reauth_message. Maybe we can do the same from Objective-C code.

I'm not very good at string processing with `NSString`. I created strings `alert_reauth_message_ios` and `alert_reauth_link_text_ios` by splittinh `alert_reauth_message`. Maybe we can do the same from Objective-C code.

strump commented

2024-03-25 09:30:00 +00:00

Author

Member

Update: suggested change gives compilation error:

/home/runner/work/organicmaps/organicmaps/qt/osm_auth_dialog.cpp:126:5: error: no matching function for call to 'Set'
    settings::Set(kOauthTokenSetting, {});
    ^~~~~~~~~~~~~
/home/runner/work/organicmaps/organicmaps/platform/settings.hpp:49:6: note: candidate template ignored: couldn't infer template argument 'Value'
void Set(std::string const & key, Value const & value)

Reverting ...

Update: suggested change gives compilation error: ``` /home/runner/work/organicmaps/organicmaps/qt/osm_auth_dialog.cpp:126:5: error: no matching function for call to 'Set' settings::Set(kOauthTokenSetting, {}); ^~~~~~~~~~~~~ /home/runner/work/organicmaps/organicmaps/platform/settings.hpp:49:6: note: candidate template ignored: couldn't infer template argument 'Value' void Set(std::string const & key, Value const & value) ``` Reverting ...

strump commented

2024-03-25 09:31:56 +00:00

Author

Member

Removed calls to NSUserDefaults.synchronize method all over the code

Removed calls to `NSUserDefaults.synchronize` method all over the code

strump commented

2024-03-25 09:32:09 +00:00

Author

Member

Removed calls to NSUserDefaults.synchronize method all over the code

Removed calls to `NSUserDefaults.synchronize` method all over the code

kirylkaveryn commented

2024-03-25 10:20:43 +00:00

Member

@biodranik Yes it's possible. But to set different attributes for different texts we should retrieve the link's range from the base string and update attrs for this range. So it's easier to have 2 strings rather than retrieve one string from another, update attrs, and combine the back.

`@biodranik` Yes it's possible. But to set different attributes for different texts we should retrieve the link's range from the base string and update attrs for this range. So it's easier to have 2 strings rather than retrieve one string from another, update attrs, and combine the back.

strump commented

2024-03-25 10:34:59 +00:00

Author

Member

Changed link handling with NSLinkAttributeName

Changed link handling with `NSLinkAttributeName`

rtsisyk commented

2024-03-26 17:02:34 +00:00

Owner

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.