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

Implement authorization on geocaching.su #7170

Merged
merged 16 commits into from Mar 18, 2019
Merged

Implement authorization on geocaching.su #7170

merged 16 commits into from Mar 18, 2019

Conversation

okainov
Copy link
Contributor

@okainov okainov commented Oct 27, 2018

Implements #5269

@cgeo-bot
Copy link

Can one of the admins verify this patch?

@cgeo cgeo deleted a comment Oct 27, 2018
@cgeo cgeo deleted a comment Oct 27, 2018
@UniQP
Copy link
Member

UniQP commented Oct 27, 2018

I deleted the comments

@okainov okainov changed the title WIP: migrate SU to new API [Do Not Merge] geocaching.su migrate SU to new API Oct 27, 2018
@UniQP
Copy link
Member

UniQP commented Oct 27, 2018

ok to test

@okainov okainov changed the title geocaching.su migrate SU to new API Implement authorization on geocaching.su Oct 27, 2018
@Lineflyer
Copy link
Member

ok to test

@cgeo-bot
Copy link

Build finished.

@Lineflyer
Copy link
Member

retest this please

@cgeo-bot
Copy link

Build finished.

@okainov
Copy link
Contributor Author

okainov commented Oct 29, 2018

@Lineflyer please add actual valid production Geocaching.su keys I sent before to keys.xml used in build

@Lineflyer
Copy link
Member

@okainov I am unable to to lack of admin account and knowledge. I guess either @rsudev or @kumy can help with that?!

@cgeo-bot
Copy link

Build finished.

1 similar comment
@cgeo-bot
Copy link

Build finished.

@cgeo-bot
Copy link

Build finished.

3 similar comments
@cgeo-bot
Copy link

Build finished.

@cgeo-bot
Copy link

Build finished.

@cgeo-bot
Copy link

Build finished.

@okainov okainov mentioned this pull request Oct 31, 2018
@rsudev
Copy link
Contributor

rsudev commented Oct 31, 2018

Regarding the keys, either @kumy or @mucek4 can do that, I was only tangentially involved in the build-machinery until now.

@rsudev
Copy link
Contributor

rsudev commented Oct 31, 2018

To give you a heads-up, I started to look at the PR. I will start putting out remarks tomorrow.
BTW: Is there a way to get keys for my developer build? (If you answered this already, please forgive me and kindly point me to the appropriate place)

@okainov
Copy link
Contributor Author

okainov commented Oct 31, 2018

@rsudev I just sent keys again to cgeo support mail. I was not able to quickly find way to message you directly, but feel free to provide a communication method to send you this directly

@okainov
Copy link
Contributor Author

okainov commented Mar 5, 2019

@Lineflyer @rsudev I improved OAuth part a bit to shorten the code and removed re-invented wheels, if there are more comments there let us continue improvements later there.

@Lineflyer
Copy link
Member

@rsudev If you do not object today, I will merge this new connector tomorrow to give it a test in our nightlies.

@Lineflyer
Copy link
Member

@okainov Can you rebase one more time? Recent commits produced a conflict.

Some OAuth servers implementation do not accept oauth_token
parameter when it's empty, so ther eis no need to send empty option
Methods getTokenPublicPrefKeyId() and getTokenSecretPrefKeyId()
are going to be reused in non-OC connectors, so let's create
interface for general OAuth-capable connector
Instead of using hardcoded OC-only keys, get them dynamically

Fixes: cgeo#7184
This class is being used outside of OC package, so need public
access to its constructor.
- Implement SU Authorization activity
- Implement SUParser and tests
- Use new API for searching by geocode and map
Proper OAuth requires to do not store user credentials and to use tokens
instead.

- Changed SUAuthorizationActivity from Credentials-capable
to OAuth-capable activity
- Removed SuLogin and use OAuthAuthorizationActivity instead
- Add User API call to get current user
Explicitly declare and catch possible exceptions during SuApi calls
Current behavior does not allow to have simple LoggingManager
without implementing LoaderManager.

See cgeo#7188
Caches loaded with old API didn't contain cache ID. However it's
the main identifier on SU, and it can be easily computed from geocode.
So, to avoid asking users to refresh all their caches it's easier
to add this small fix.
Also, do not put all images in description, but only cache photo
@okainov
Copy link
Contributor Author

okainov commented Mar 18, 2019

@Lineflyer done

@Lineflyer
Copy link
Member

This is a big one. Lets test.
I will merge as no one objected and this is the most meaningful point of time (most time before master might get to release)

@okainov
Thanks for your effort and for your patience with this one.
Hope you stay with it for maybe resulting bugfixes.
Regarding your open items (above) you might want to create issue as feature request documentation.

@Lineflyer Lineflyer merged commit 8c15c7e into cgeo:master Mar 18, 2019
@okainov
Copy link
Contributor Author

okainov commented Mar 18, 2019

@Lineflyer you've squashed all my pretty commit history that I was trying to keep as neat as possible :( Can I have label near my nick or my PRs "please don't squash"?

Nevertheless, finally, hoorray! Thanks!

@Lineflyer
Copy link
Member

Regarding the squashing:
I just followed the best practice we use here, that one PR is contained in one commit.

Regarding the final merge now:
Just want to give you the quick feeback, that the first impression for me as a tester is really positive. Authorization worked on first try. Speed and anticipated look and feel is matching :)

@Lineflyer
Copy link
Member

BTW:
Any cache on this platform I can use for testing log posting (e.g. your owned or some test cache)?

@okainov
Copy link
Contributor Author

okainov commented Mar 18, 2019

I just followed the best practice we use here, that one PR is contained in one commit.

I think this shouldn't be applied for so huge changesets and when author has neat commit history, I think we had similar discussion with @rsudev somewhere in my previous PR. I'd be glad to organize my commits if they're not neat enough for you, but in the future I'd prefer to have them merged without squashing, because I try to separate them to logically independent parts for ease of tracking.

Any cache on this platform I can use for testing log posting (e.g. your owned or some test cache)?

Feel free to spam in my caches:
Traditional https://geocaching.su/?pn=101&cid=21372
Virtual https://geocaching.su/?pn=101&cid=21360
(there should be no changes in app behavior at the moment but just in case)

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