TokenStorage (v2)

[aaschaer]

Shortcut: https://app.shortcut.com/globus/story/33154/sdk-storageadapterv2

Change Summary (all in experimental):

• Adds TokenStorage which expands upon StorageAdapter by adding namespace support and requires implementing classes to support remove_tokens_for_resource_server. It also allows passing token data as a dict in addition to a OAuthTokenResponse and removes on_refresh as that was only ever an alias for store
• Adds TokenData which is a data class for tokens and metadata, notably including identity_id which is needed by ValidatingTokenStorage
• Adds FileTokenStorage which is nearly much identical to FileAdapter beyond implementing StorageAdapterV2 instead of StorageAdapterV2
• Adds MemoryTokenStorage which remains fairly simple.
• Adds JSONTokenStorage which now stores data under namespaced keys and has logic for migrating storage from a SimpleJSONFileAdapter
• Adds SQLiteTokenStorage which is very similar SQLiteAdapter beyond implementing StorageAdapterV2. Both classes should even be able to use the same storage without causing issues
• Changes ValidatingStorageAdapater to ValidatingTokenStorage which now implements TokenStorage instead of StorageAdapter
• Removes IdentifiedOAuthTokenResponse as its functionality is now covered by TokenData

Notes:

• the default namespace is "DEFAULT" instead of None as originally spec'd this was to not require data migration for SQLiteTokenStorage to use storage previously used by SQLiteAdapter
• namespace is defined at class initialization rather than passed to each function as originally spec'd. This was to minimize needed changes for SQLiteAdapter and better conform with GlobusApp which will also have one namespace per instance.
• Its not clear to me that adding namespace support to MemoryTokenStorage changed anything since initializing another MemoryAdapter already partitioned the data in memory. Maybe that value should just be ignored?

---

📚 Documentation preview 📚: https://globus-sdk-python--980.org.readthedocs.build/en/980/

[derek-globus]

I know this name is coming from the current sqliteadapater but I wonder, could we actually just change this? It's way more verbose than the other method names.

• "store" doesn't mention tokens at all
• "get" mentions tokens but not that it's for a resource server
• "remove" mentions tokens and that it's for a resource server

I guess store isn't bounded to a particular resource server and get/remove are so maybe remove_token_data(resource_server: str) to mirror get_token_data(resource_server: str)?

[sirosen]

I would like us to reevaluate all of the names in play, for the record. That goes for the class name as well, per one of your other comments.

[aaschaer]

I went with

store_response
store_token_data_by_resource_server
get_token_data_by_resource_server
get_token_data
remove_token_data

Open to suggestions for changes, but it should at least be more uniform now

[derek-globus]

Why the difference in delimiter here? _ vs -

[aaschaer]

I think this is the only comment that didn't get addressed in my last commit/rebase.

Essentially I just wanted to keep the value used by SimpleJSONStorageAdapter so there was one less thing for users to worry about when migrating.

As for why it was like this originally 🤷

[derek-globus]

Haven't finished reviewing; posting comments now though for conversation.

[derek-globus]

I like the place this ended up.

Thanks again for iterating on it to refine it to this point!