-
Notifications
You must be signed in to change notification settings - Fork 5
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
[ENG-6036] Implement DataverseStorageAddonImp #125
[ENG-6036] Implement DataverseStorageAddonImp #125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment on the "header transformation", but haven't dug into dataverse specifics
addon_imps/storage/dataverse.py
Outdated
@staticmethod | ||
def make_headers(raw_headers: dict): | ||
header_value = raw_headers.get("Authorization") | ||
return {"X-Dataverse-key": header_value.split(" ")[-1]} | ||
|
||
def __post_init__(self): | ||
self.network.add_header_transformation_callback(self.make_headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was the idea to keep request auth an entirely separate concern from addon imps (so multiple services could implement a common/standard api while using distinct auth methods) -- this is why ExternalService
has both credentials_format
and addon_imp
, and why auth headers were not exposed to imp code
...unfortunately, we did so imperfectly, as you found.
to support dataverse's custom auth header while maintaining that separation of concerns, could:
- remove
iter_headers
from addon_toolkit.credentials.Credentials and its subclasses - instead, generate headers in (or at least based on)
addon_service.common.credentials_formats
-- could be a method onCredentialsFormats
, so instead of self.account.credentials.iter_headers(), would do something like:
self.account.external_service.credentials_format.make_headers(self.account.credentials)
- add
CredentialsFormats.DATAVERSE_TOKEN
member that uses the existingAccessTokenCredentials
dataclass but generates aX-Dataverse-key: ...
header instead ofAuthorization: Bearer ...
...alternately, could destroy that separation of concerns altogether -- delete CredentialsFormats
and ExternalService.int_credentials_format
, make the addon-imp fully in charge of authenticating requests and whether to use oauth1/oauth2/etc...
i think there's value in maintaining the separation and keeping the imp oblivious to auth headers (this would allow for dataverse installations configured to use bearer tokens, for example), but i acknowledge this is a bit of a mess; apologies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed credentials handling, please look once more and tell me whether this is suffiecient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great!
async def get_external_account_id(self, _: dict[str, str]) -> str: | ||
return "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this should be unnecessary, it's already implemented by the base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the credentials changes seem a good improvement! i don't know the dataverse api well, but the code looks reasonable 👍
"mydata/retrieve", | ||
query=[ | ||
["page", page_cursor], | ||
*[("role_ids", role) for role in range(1, 9)], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dataverse docs say roles are configurable per instance... do we have a sense whether they actually vary in practice, or is it safe to hard-code this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are all of the standart roles which are present in all dataverses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a bit of dead code, otherwise seems good
def add_header_transformation_callback( | ||
self, callback: Callable[[dict], dict] | ||
) -> None: ... | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is no longer used, can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small change to make CI happy.
addon_service/common/known_imps.py
Outdated
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is causing problems with pre-commit; you should probably delete these two lines.
No description provided.