Redesign the OAuth API

[hillairet]

One main point of friction of the current design is that it doesn't include the token classes out of the box, which is weird (according to @qw-in 😉 )

[qw-in]

A couple ideas:

1. Consider oauth2 library #37 - if possible we should lean on a battle tested oauth2 implementation
2. More integration with the rest of SpyLib
3. Provide library users more flexibility (eg access to an exchange_token fn)

[qw-in]

Note these issues:

• Bundle function into method #44
• Make handle optional #46
• Handle wrong store name on OAuth flow #49

[qw-in]

@hillairet just wanted to write down some of the issues and feature requests from the implementation of Together:

1. Mechanism to handle requesting more scopes. For example we released a new feature that required the read_themes. How do we automatically detect that old tokens need to be re-requested?
2. Better handling of online/offline token. If an offline token has already been issued, only an online token should be issued when a new staff member accesses the app
3. More control over the redirect after oauth. For example on first install we wanted to redirect to /welcome but that does not seem to be possible currently

[hillairet]

1. Seems like a whole discussion by itself! Do you mind starting a discussion for that specifically?
2. Looks like a bug. I don't think that requires discussion but instead needs a proper test to check that properly
3. Good point.

[lishanl]

1. We can use Access Scope to check the the existing merchant granted scope with the app required scopes and prompt the OAuth when needed for the merchant the accept the new scopes?

[hillairet]

Well in theory just doing offlinetoken.load(...) should give you the scopes too.
The challenge is the whole flow and how to put it in a library.

[qw-in]

Even more fun is handling associated_user scope. Which is any subset of the offline token scope...

Been thinking about it a lot and I really think the oauth is going to need to be able to read/write to some sort of database

For example, a session token is received and we have to answer the following questions:

1. Do we have an online token for this session?
2. Does the online token have the correct scope?
3. Has the online token been revoked? Probably don't always want to check this as it requires an api call...maybe there is a webhook?
4. Do we have an offline token for this store?
5. Does the offline token have the correct scope?

It would be really convenient if this could be handled in the library by providing it mechanisms to read/write tokens

[hillairet]

Some notes from the meeting @lishanl, @ponty33, @qw-in and I had:

• Let's focus on separating the framework agnostic code from the FastAPI code/router. That will allow us to properly design the framework agnostic code first then the FastAPI part.
• We have a bunch of clean up and refactoring to do so now is a good time.
• Still not clear how to make the init_oauth_router for FastAPI better ... to be discussed in this discussion thread.

[hillairet]

After starring quite a bit at FastAPI's routing code, I think I have a pretty neat proposal for the FastAPI interface for the OAuth.
What I like about this approach is:

• Total control of the processing of the endpoint left to the user
• No need to pass functions or classes, in particular OfflineToken and OnlineToken classes
• Rather elegant!

The following code works. OAuthRouter will inherit from FastAPI's APIRouter and we'll need to create a few classes inheriting from APIRoute as explained in the FastAPI docs

from spylib.oauth import OAuthRouter

router = OAuthRouter(
    public_domain: 'https://example.com',
    private_key: 'PRIVATEKEY',
    app_handle: 'supercoolapp',
    api_key: 'APIKEY',
    api_secret_key: 'APISECRETKEY',
    app_scopes: ['write_orders'],
    user_scopes: ['read_orders'],
)

@router.init('/shopify/auth'):
async def init():
    return

@router.install_callback('/callback')
async def install_callback():
    return


app = FastAPI()
app.include_router(router)

I managed to get the above code to work and I can do whatever I want before and after init() or install_callback() run! 😁

[qw-in]

Hmm, it is certainly a nice option. I do kind of feel like we should let fastapi do the routing and just hook into that?

from spylib.oauth.fastapi import OAuth

oauth = Oauth()

app = FastAPI()

@app.get('/callback')
oauth.handle_callback

Might not be valid syntax but you know what I mean.

We could then handle events the same way regardless of the framework

@oauth.on_offline_token_issued
async def...

---

I think this would also provide more customization options. Need more control over the actual http handler? No problem

@app.get('/callback')
async def cb(request):
    oauth.handlecb(request)
    # do stuff

[hillairet]

We are definitely in preference territory at this point. Your option can work for sure. I considered some decorators too.

The advantage of extending the router is that we'll have the knowledge of the callback URL when making the link for the redirect.
In my example the init() will know the callback URL automatically which is the advantage of being at the router level.

Using your example it'd something like that:

from spylib.oauth.fastapi import OAuth

oauth = Oauth()

app = FastAPI()

@app.get('/callback')
oauth.install(callback_url='/callback')

@app.get('/callback')
oauth.handle_callback

Not that bad but if we make a library to simplify things and take care of the details for us, then that should also be done automatically IMO.

[qw-in]

I'm mostly just a little worried about inheriting from the Router rather than using Depends for example. Router comes with a huge number of features that may/may not play nice with spylib now or in the future is my concern.

[qw-in]

Ok, I spent some time today and I think I have come up with a pattern/design that would work for together. I'll try and explain as best I can below.

Stage one - utility functions

This will provide a flexible base upon which everything else is built. It will also always mean there is an escape hatch in case someone has slightly different requirements than what the library currently provides. This includes but is not limited to:

# function names very much up for discussion

def create_authorization_url():
    """Returns a url that will initialise oauth"""
    pass
    
def verify_oauth_hmac():
    """Verify an hmac, for example during oauth callback"""
    pass
    
def verify_shop():
    """Verify a *.myshopify.com domain"""
    pass
    
async def exchange_code_for_token():
    """Exchange temporary code for access token"""
    pass
    
# Session tokens are very much part of oauth imo - they are directly tied to online tokens
def validate_session_token():
    """Ensure a session token is valid"""
    pass

Stage two - configuration/convenience class

Add an App class that provides convenience methods to call the functions from stage one. Its job is just to hold configuration at this stage.

app = App(
    api_key='api-key',
    api_secret_key='api-secret-key',
   ...
)

app.verify_oauth_hmac() # <- no need to pass secret

Stage three - larger flows

Identify common flows that the app can automate. For example:

1. first install -> valid online & offline token
2. normal login -> valid online token
3. session token header -> valid online token

The following code would in theory be all that is needed to support ^ from a user. In this case for fastapi but you can see how it would be trivial to handle in any (async) framework

from fastapi import FastAPI
from spylib.fastapi.app import App

f = FastAPI()

app = App(
    api_key='api-key',
    api_secret_key='api-secret-key',
    offline_scopes=['read_themes', 'write_products'],
    online_scopes=['read_products'],
    callback_url="https://example.com/callback"
)

@app.save_installation
async def save_installation(install: Installation):
    """My custom code to persist an installation (includes offline token, shop, scopes, etc.)"""
    pass
    
@app.load_installation
async def load_installation(shop: str) -> Union[Installation, None]:
    """My custom code to load an installation"""
    pass
    
@app.save_login
async def save_login(login: Login):
    """My custom code to persist a login (includes online token, shop, scopes, user_id, user scopes, expiration, etc.)"""
    pass
    
@app.load_login
async def load_login(shop: str, user_id: str) -> Union[Login, None]:
    """My custom code to load a login"""
    pass

@f.get('/login')
async def login(shop: str):
    # Redirect URL will be one the following based on calling `load_login` & `load_installation`
    # - offline token request
    # - online token request
    # - direct redirect to embedded app
    redirect_url = await app.initiate_auth(shop)
    return RedirectResponse(url=redirect_url) # If forget the exact syntax

# Could also be written like this
@f.get('/login')
async def login(redirect_url: str = Depends(app.initiate_auth)):
    return RedirectResponse(url=redirect_url) # If forget the exact syntax
    
@f.get('/callback')
async def callback(redirect_url: str = Depends(app.initiate_auth)):

    # Will verify the whole callback
    # Will exchange the token
    # Will store the offline/online token
    # Will redirect to get online token if needed
    # Will finally redirect back to app (or let you do so)

    return RedirectResponse(url=redirect_url) # If forget the exact syntax

@f.post('/api/resource')
async def create_resource(login: Login = Depends(app.authenticate(scope=['read_products'])):
    # login now has valid online token
    # optionally also verifies this user has the needed shopify scopes for this endpoint
    # could return a token that can call shopify directly here instead - but that is out of the oauth scope
    # could return a status/code to inform the frontend an oauth login is required

# This pattern continues, webhooks for example

app = App(
    webhooks: ['uninstall', ...],
    webhook_url: 'https://example.com/webhooks',
)

@app.on_uninstall
async def on_uninstall(webhook):
    pass

@f.post('/webhook')
async def webhook(success = Depends(app.handle_webhook)):
    return ...

[hillairet]

Ok so taking into account your concerns for the router inheritance, although I don't think that would cause any issue, we could maybe find a compromise with decorators rather than dependencies.

One advantage of the decorator over the dependency is that we can handle the inputs and output.

I don't get why you need save/load_installation/login when we already have OfflineTokenABC and OnlineTokenABC. This part doesn't need redesign and doesn't need duplication.
If the name is an issue, when using the abstract class, one can rename:

class Installation(OfflineTokenABC):

And because of the inheritance of the abstract class, one can add fields if/when needed.

Also with your design there are two places for flexibility when only one is needed!

How about that much simpler approach:

from fastapi import FastAPI
from spylib.oauth.fastapi import OAuth

f = FastAPI()

oauth = OAuth(
    api_key='api-key',
    api_secret_key='api-secret-key',
    offline_scopes=['read_themes', 'write_products'],
    online_scopes=['read_products'],
    callback_url="https://example.com/callback"
)

@f.get('/callback')
@oauth.callback
async def callback(offlinetokenresponse: OfflineTokenResponse):
    #  In another file: Class OfflineToken(OfflineTokenABC)
    offlinetoken = OfflineToken.create_from_response(offlinetokenresponse)

    await offlinetoken.save()

The decorator takes care of returning the right redirect but if the user returns something in the callback function other than None, then the decorator uses that returned value instead.
Flexible and minimal code for the user.

[qw-in]

I don't get why you need save/load_installation/login when we already have OfflineTokenABC and OnlineTokenABC.

Yeah, the choice was really between abc & decorators. The decorators seem to offer more flexibility (can support sync or async, can omit if app does not need online token for example)

A couple concerns using the existing Token classes:

1. How is the oauth/app instance able to call the token's methods? That is not clear from your example - I see where the offline token is saved but that is a one off
2. To my knowledge the Token classes don't store/load enough data. For example the scope, associated_user_scope, 'expiry', 'associated_user` are all needed

we could maybe find a compromise with decorators rather than dependencies

I like the decorators, good idea! Dependencies do offer the ability to protect an entire router which can be nice. Probably a minimal change to support one or the other - I don't have a strong preference

[qw-in]

Github won't let me edit, so it looks like I was incorrect about point 2. Just ignore that one. Although I'm gonna look at the code just to verify - can see the return type hints in the docs

[hillairet]

Reply to your point 1.
Well how many times and places does the oauth need to save the offline token??? Isn't it every time when we process the /callback that we do the API call to get the token?

Ok so seems like we have converged on the decorators applied to the endpoints. ✔️
Now we just need to align on the Token class usage. I'm pretty sure we have enough in the Token class and if we don't we should just add it. No need for an additional layer of abstraction IMO.

[qw-in]

Isn't it every time when we process the /callback that we do the API call to get the token?

@hillairet I don't think we are quite on the same page. For example when I say the session token -> online token flow it looks something like this:

graph TD
    AA(Session token) --> A
    A(Is it valid?) -->|Valid token| B
    A -->|Invalid token| C(Unauthorized)
    B(Is associated shop installed?) -->|Yes| D
    B -->|No| E(Go to install flow)
    D(Are scopes up to date?) -->|Yes| F
    D -->|No| E
    F(Is there a valid online token<br>associated with this session?) -->|Yes| G
    F -->|No| H(Go to login flow)
    G(Return valid online token)

Loading

So, just adding a decorator to a route required loading both the offline & online token. Along with scopes etc.

[hillairet]

I thought we were redesigning the OAuth ... I am confused 😕

[qw-in]

We are putting this discussion on hold while completing #138

[hillairet]

I like the decorators, good idea! Dependencies do offer the ability to protect an entire router which can be nice. Probably a minimal change to support one or the other - I don't have a strong preference

Coming back to this.
I don't think it has to be all decorators or all dependencies. It depends!
For the oauth callback, it definitely make sense to have decorator to me so that we can control the input and output of the function.
Also we should be able to allow the decorated function to return a redirect and overwrite the SPyLib standard redirect!

For authentication of endpoints, you are right that dependencies fit well in particular to be applied to a whole router. So let's use dependencies.

[hillairet]

Now back to my previous comment about being confused:
This discussion is about the OAuth process itself, not what we do with the tokens from the OAuth process. However expanding to the usage of the tokens was interesting nonetheless.
Let let's focus on the OAuth. I think that has mostly converged.

[hillairet]

We should implement this happy path (in blue) and then implement each check and each non-happy path outcome one by one. The session token valid/invalid is a basic check so happy path.

graph TD
    AA(Session token):::happypath --> A
    classDef happypath fill:#00D,color:#fff
    A(Is it valid?):::happypath -->|Valid token| B
    A -->|Invalid token| C(Unauthorized):::happypath
    B(Is associated shop installed?):::happypath -->|Yes| D
    B -->|No| E(Go to install flow)
    D(Are scopes up to date?):::happypath -->|Yes| F
    D -->|No| E
    F(Is there a valid online token<br>associated with this session?):::happypath -->|Yes| G
    F -->|No| H(Go to login flow)
    G(Return valid online token):::happypath

Loading

[lishanl]

wow it's hard to see in the light mode.

[hillairet]

Ah yes thanks for letting me know. I fixed it.