[ENG-5769] Oauth 1.0a integration by opaduchak · Pull Request #78 · CenterForOpenScience/gravyvalet

opaduchak · 2024-07-05T11:02:30Z

Implementation of Oauth1.0a for GravyValet
ENG-5769

adlius

First round. Some questions.

addon_imps/citations/zotero_org.py

adlius · 2024-07-05T13:37:54Z

addon_service/common/credentials_formats.py

+            case CredentialsFormats.OAUTH1A:
+                return credentials.OAuth1TokenCredentials


For Oauth 1a, it should still be AccessTokenCredentials, right? I believe this affects how the header is constructed for the http requests.

There are oauth_token and oauth_token_secret, which in their nature and usage are drastically different from OAuth2 access and refresh tokens

addon_service/oauth1/models.py

adlius · 2024-07-05T13:59:59Z

addon_service/oauth1/models.py

+from addon_service.common.base_model import AddonsServiceBaseModel
+
+
+class OAuth1ClientConfig(AddonsServiceBaseModel):


Is there not an auth_callback_url?

auth_url is auth_callback_url, I'll change it if it's confusing

For the record: we're storing the auth_callback_url explicitly in case there are services that won't allow us to update the list of callbacks without invalidating our client and/or old credentials. That way, if necessary, we can encode this as https://[environment.]osf.io/oauth/callback/[provider] -- where we already have things set up to forward requests to GV if appropriate.

adlius · 2024-07-05T14:06:04Z

addon_toolkit/credentials.py



+@dataclasses.dataclass(frozen=True, slots=True)
+class OAuth1TokenCredentials:


I think for OAuth1 we should still use AccessTokenCredentials. The idea of the AccessTokenCredentials dataclass is to implement the iter_headers method so that GravyvaletHttpRequestors can automatically append the appropriate auth headers for the oauth requests.

Depends on if OAuth1 requests require the secret to be sent along with the token. If so, it's a different credentials shape. Otherwise, yes, we should keep using AccessTokenCredentials and create an OAuth1 equivalent to OAuth2TokenMetadata for tracking other details

Well, auth headers are different for OAuth1.0a and OAuth2, and even more: they are different for Zotero

adlius · 2024-07-05T14:07:52Z

addon_service/authorized_storage_account/models.py

+    is_oauth1_ready = models.BooleanField(
+        "addon_service.OAuth2TokenMetadata",
+        null=True,
+        blank=True,
+    )
+


I am not quite sure whether this boolean field is necessary.

It is necessary as it allows to discern temporary Request token credentials from final credentials

adlius · 2024-07-05T14:25:02Z

addon_service/authorized_storage_account/serializers.py

+            self.context["request"].session[
+                "oauth1a_account_id"
+            ] = authorized_account.pk


Do we have to store the authorized_account.pk in the session? Is there anything in the payload of the request to the callback url that can be used to identify which authorized account this is?

There isn't anything in payload which'd allow for is to identify it, I don't like this approach too, but I don't have any better ideas

the callback request includes the same temporary token received when initiating oauth1 -- could add a model to hold oauth1 temporary state (parallel OAuth2TokenMetadata) with indexed temporary_token field

(edit: nevermind -- i think that's true in specs but not for zotero, oh well)

jwalz

Thanks! This is a great start. I'll probably dig a bit more into the testing stuff to see if there's an easy way to consolidate strategies.

addon_service/common/known_imps.py

addon_service/credentials/serializers.py

addon_service/oauth1/models.py

jwalz · 2024-07-05T14:08:47Z

addon_service/tests/_factories.py

+class ExternalStorageOAuth2ServiceFactory(ExternalStorageServiceFactory):
+    oauth2_client_config = factory.SubFactory(OAuth2ClientConfigFactory)
+
+
+class ExternalStorageOAuth1ServiceFactory(ExternalStorageServiceFactory):
+    oauth1_client_config = factory.SubFactory(OAuth1ClientConfigFactory)
+
+    @classmethod
+    def _create(
+        cls,
+        model_class,
+        credentials_format=CredentialsFormats.OAUTH1A,
+        service_type=ServiceTypes.PUBLIC,
+        *args,
+        **kwargs,
+    ):
+        return super()._create(
+            model_class, credentials_format, service_type, *args, **kwargs
+        )


We never shoul had oauth_client_config as a sub-factory, considering it isnt' relevant and shouldn't be set for non-oauth(2) services.

Would recommend instead just updating the existing ExternalStorageServiceFactory._create function to

Accept oauth1_client_config and oauth2_client_config as parameters

Use the credentials_format to construct the appropriate form of client config using the appropriate factory if one wasn't passed

Ignore or error any irrelevant client_config that was passed.

Note, this is obviously simpler if we decide to collapse down to one OAuthConfig.

addon_service/oauth1/utils.py

addon_service/authorized_storage_account/models.py

addon_service/oauth1/utils.py

addon_service/oauth1/views.py

addon_service/tests/_helpers.py

Co-authored-by: Jon Walz <jon@cos.io>

aaxelb

looks good so far! had some suggestions -- most breakingly, adding a new model for the "temporary" oauth1 credentials

addon_service/common/known_imps.py

addon_service/authorized_storage_account/models.py

addon_service/oauth_utlis.py

addon_service/tests/_helpers.py

addon_service/oauth1/utils.py

addon_service/oauth1/models.py

fixed verbose comprehensions Co-authored-by: abram axel booth <aaxelb@users.noreply.github.com>

addon_service/authorized_storage_account/models.py

(avoid any logic in addon_service that explicitly branches by imp)

aaxelb · 2024-07-12T12:15:48Z

addon_toolkit/credentials.py

+        This is Zotero specific as other OAuth1.0a clients require request signing,
+        as per current architecture, we cannot it here.
+        """
+
+        yield "Authorization", f"Bearer {self.oauth_token_secret}"
+        # TODO: implement request signing for OAuth1.0a services that require it


gah, i missed that zotero uses the secret as bearer token instead of signing the request... feels wrong, but maybe it's true that https makes it safe, as argued in their example client code:

/** OAuth support for all api requests may be added in the future * but for now secure https provides similar benefits anyway */

(this speaks to the need for modular/swappable request-auth logic (instead of iter_headers) but sounds like that'll be part of s3 work -- this TODO seems fine for now)

We should be safe because https encypts entire http payload (including headers, query params, path, body, etc.)

aaxelb · 2024-07-12T12:27:58Z

addon_service/oauth1/models.py

+    access_token_url = models.URLField(null=False)
+
+    client_key = models.CharField(null=True)
+    client_secret = models.CharField(null=True)


hmm, the client secret should be stored encrypted -- but since OAuth2ClientConfig is the same, maybe worth splitting that into another ticket? (...and i'm starting to reconsider the EncryptedDataclassModel abstract base, now that we have three potential uses for it...)

I agree, storing any unencrypted credentials in the db, poses security risks

Yes, separate ticket. Need input from cloud eng/devops as to whether they want this in the database at all or if they'd prefer an approach like this one

opaduchak force-pushed the ENG-5769 branch from e42cc14 to a744b06 Compare July 5, 2024 11:04

opaduchak requested a review from aaxelb July 5, 2024 11:04

opaduchak self-assigned this Jul 5, 2024

opaduchak requested a review from adlius July 5, 2024 13:20

adlius reviewed Jul 5, 2024

View reviewed changes

jwalz reviewed Jul 5, 2024

View reviewed changes

opaduchak and others added 5 commits July 5, 2024 18:17

added oauth1 implementation comatioble with zotero

18f491f

added e2e test for oauth1

d84a2c1

cleaned up migrations

271cba0

fixed tests, made cleanup

f9ec901

Update addon_service/authorized_storage_account/models.py

8552be5

Co-authored-by: Jon Walz <jon@cos.io>

opaduchak force-pushed the ENG-5769 branch from edaab59 to 8552be5 Compare July 5, 2024 15:17

fixed simpler comments

7db1040

aaxelb requested changes Jul 5, 2024

View reviewed changes

opaduchak and others added 6 commits July 8, 2024 14:29

fixed nitpicks

5375b52

fixed more nitpicks

7c17719

Apply suggestions from code review

6d2200f

fixed verbose comprehensions Co-authored-by: abram axel booth <aaxelb@users.noreply.github.com>

fixed comments

1ebf5d4

fixed tests

da9b7e3

more fixes to tests

08f8d9a

aaxelb reviewed Jul 9, 2024

View reviewed changes

addon_service/authorized_storage_account/models.py Show resolved Hide resolved

fixed one more naming nit

05b1873

aaxelb mentioned this pull request Jul 11, 2024

wip/proposal opaduchak/gravyvalet#1

Closed

opaduchak and others added 2 commits July 12, 2024 12:37

fixed ExpiredTokenAccess for OAuth1.0a

63d76db

move zotero-specific logic to the zotero imp

2541c24

(avoid any logic in addon_service that explicitly branches by imp)

aaxelb reviewed Jul 12, 2024

View reviewed changes

opaduchak merged commit b7a43cc into CenterForOpenScience:develop Jul 15, 2024

		case CredentialsFormats.OAUTH1A:
		return credentials.OAuth1TokenCredentials

		from addon_service.common.base_model import AddonsServiceBaseModel


		class OAuth1ClientConfig(AddonsServiceBaseModel):



		@dataclasses.dataclass(frozen=True, slots=True)
		class OAuth1TokenCredentials:

Conversation

opaduchak commented Jul 5, 2024 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adlius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opaduchak Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaxelb Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwalz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaxelb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaxelb Jul 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

opaduchak commented Jul 5, 2024 •

edited by atlassian bot

Loading

opaduchak Jul 5, 2024 •

edited

Loading

aaxelb Jul 5, 2024 •

edited

Loading

aaxelb Jul 12, 2024 •

edited

Loading