Skip to content
This repository has been archived by the owner on Jul 6, 2022. It is now read-only.

Move authorization flow to external browser #87

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JaCzekanski
Copy link

Problem

Original flow of authentication using embedded Webview doesn't work for Google accounts that are secured using U2F key like Yubikey.
Screenshot 2019-08-22 at 14 00 25
comment: WebKit does not support U2F api

Solution

Move authorization flow to default browser which will allow the user to sign to U2F secured accounts. Noti will receive access token using a deep link to schema noti://redirect.
To use different redirect_uri separate client_id for Pushbullet had to be generated.

Screenshots

Screenshot 2019-08-22 at 13 57 17
Screenshot 2019-08-22 at 13 58 17
Screenshot 2019-08-22 at 14 06 22

access token is received using deep link to application
@JaCzekanski JaCzekanski changed the title Feature/auth in browser Move authorization flow to external browser Aug 22, 2019
@jariz
Copy link
Owner

jariz commented Aug 22, 2019

Very cool! Thanks a lot.

A minor security comment though: please include a nonce in the callback url so noti can verify it generated the URL itself rather than a different application.
I just tested this and even though the redirect_uri is set in the app configuration, pushbullet allows you to pass additional query parameters like so:

https://www.pushbullet.com/authorize?client_id=....&redirect_uri=noti%3A%2F%2Fredirect%3Fnonce%3D6345242353&response_type=token&scope=everything
                                                                 ^

@JaCzekanski
Copy link
Author

I added secure nonce generation which will prevent reusing callback URIs.
It's my first time writing Swift so if there are any comments regarding code style or bad practices - don't hesitate to point them.

@jariz
Copy link
Owner

jariz commented Aug 22, 2019

Just FYI, any (unsandboxed) app can write to an other app's defaults.

defaults write io.jari.Noti nonce 1337
open "noti://redirect?nonce=1337#access_token=evil"

I was more thinking about just keeping it in memory.
But, your fix already greatly decreases the attack surface, just thought I'd ensure you're aware of this.
I'm fine with merging this.

About the codestyle, it's fine really. This is not exactly a great example on how to do swift development correctly as it was my first swift project as well 😛

@JaCzekanski
Copy link
Author

You're right - storing nonce in memory would suffice.
Apart from that I can't think of any real security threat with current solution. Worst case scenario: some one opens Noti using deep link by accident and is relogged into another account with access token, but nonce will prevent it right now.

I'm happy with current solution - I'm able to log in and notification mirroring is working again for my phone.
If you have any ideas for improvements I'll be happy to include them in this PR.

@JaCzekanski JaCzekanski force-pushed the feature/auth-in-browser branch from ca4c6d7 to 534980d Compare September 29, 2019 11:12
@JaCzekanski
Copy link
Author

Sorry for the lack of activity - I completely forgot about this PR.
I made a small change which and now nonce is stored in AppDelegate during the authentication process.

@JaCzekanski JaCzekanski force-pushed the feature/auth-in-browser branch from 58a6d99 to 0fb8a33 Compare September 29, 2019 12:23
@IanVS
Copy link

IanVS commented Dec 3, 2019

Where's this stand now? Seems like a great change to me!

@agucova
Copy link

agucova commented May 19, 2020

@jariz Is the project no longer maintained? I can fork and merge the pull request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants