-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Authorization with OAuth 2 #5383
Conversation
OSM's OAuth2 implementation guarantees that all permissions are granted
app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorization.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorization.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorization.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
The final shape this PR shall take depends on what decision is being made on openstreetmap/openstreetmap-website#4360 If the checkboxes will return for the OAuth 2.0 implementation, will need to read and handle the |
…lete into oauth2 # Conflicts: # app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorization.kt
app/src/main/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorization.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Flo Edelmann <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me -- I don't see anything that jumps out as obviously wrong, anyway.
app/src/test/java/de/westnordost/streetcomplete/data/user/oauth/OAuthAuthorizationTest.kt
Show resolved
Hide resolved
const val OAUTH2_TOKEN_URL = "https://www.openstreetmap.org/oauth2/token" | ||
const val OAUTH2_AUTHORIZATION_URL = "https://www.openstreetmap.org/oauth2/authorize" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't present when I wrote the initial implementation for JOSM, but the OSM website now supports rfc8414 (see https://www.openstreetmap.org/.well-known/oauth-authorization-server ).
If I were writing an implementation today, I would not hardcode the OSM OAuth URLs. And I'm kind of planning on removing the hardcoded OAuth URLs in JOSM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why would you not hardcode them? I get that when writing a generic OAuth client, it would be very wise to support rfc8414 but since this only speaks with OSM, it's basically one hardcoded URL (https://www.openstreetmap.org) vs two (auth url + token url).
(Also, at the time of implementation, the testing instance didn't have the .well-known file. Not sure if it changed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, at the time of implementation, the testing instance didn't have the .well-known file. Not sure if it changed.
The testing instance does have a .well-known file now ( https://api06.dev.openstreetmap.org/.well-known/oauth-authorization-server ).
But why would you not hardcode them?
As a general principle, I try to avoid hardcoding things that can be obtained through standardized means (assuming the standard is reasonable). As an example, website might decide that they want to move the /oauth2
endpoints to /oauth
when they remove OAuth 1.0 support. I don't think OSM will do that, so it probably isn't a "big" issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks!
By the way, openstreetmap/openstreetmap-website#4360 may affect JOSM implementation, too:
According to the specification, a scope
parameter is returned from the redirect URI which contains the actually granted scopes by the user. Meaning, according to the specification, a user may be allowed to not grant all permissions of what has been requested by the client. The app can/should handle that somehow, either rejecting the access token if it does not have all required scopes granted and/or dealing with the missing permission in the UI.
(That is, if OSM website maintainers decide that they will consider making use of this in the future. But I understand you like future-proofing JOSM for possible RFC compliant changes to the OAuth 2 API on OSM)
# Conflicts: # app/src/main/java/de/westnordost/streetcomplete/StreetCompleteApplication.kt
By the way, thanks to @dirkmueller from SUSE for sponsoring this, I also mentioned you in the changelog for the upcoming version. |
Implement authorization with OAuth 2. Fixes #5322
Blocked by decision to be made in openstreetmap/openstreetmap-website#4360
Maybe @tsmock would review this (OAuthAuthorization.kt)?