Skip to content
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

Add the "http_token" authentication mechanism which provides 'Authentication: Token {TOKEN}' header #7551

Merged
merged 3 commits into from Feb 2, 2024

Conversation

yarikoptic
Copy link
Member

It is pretty much the "Bearer TOKEN" method but which uses different keyword "Token". It is e.g. the one provided by Django REST Framework. GitHub allows for both 'Bearer' and 'Token' keywords: https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api?apiVersion=2022-11-28

In our case of DANDI we ran into it only now that we decided to add handling of embargoed datasets, for which we need to authenticate to access the files. We are using Django REST Framework and insofar decision was made to retain current approach (and fix WWW-Authentication header response), instead of an alternative to allow for both 'Bearer' and 'Token' as was suggested in dandi/dandi-archive#1830

As it is ad-hoc (nothing AFAIK in W3C standard) Authentication response I do not think some automation would ever be created based on WWW-Authentication but we will be able to support such providers.

PR checklist

  • Provide an overview of the changes you're making and explain why you're proposing them.
  • Create a changelog snippet (add the CHANGELOG-missing label to this pull request in order to have a snippet generated from its title;
    or use scriv create locally and include the generated file in the pull request, see scriv).

It is pretty much the "Bearer TOKEN" method but which uses different keyword
"Token".  It is e.g. the one provided by Django REST Framework.
GitHub allows for both 'Bearer' and 'Token' keywords:
https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api?apiVersion=2022-11-28

In our case of DANDI we ran into it only now that we decided to add handling of
embargoed datasets, for which we need to authenticate to access the files.  We
are using Django REST Framework and insofar decision was made to retain current
approach (and fix WWW-Authentication header response), instead of an
alternative to allow for both  'Bearer' and 'Token' as was suggested in
dandi/dandi-archive#1830

As it is ad-hoc (nothing AFAIK in W3C standard) Authentication response I do
not think some automation would ever be created based on WWW-Authentication but
we will be able to support such providers.
@yarikoptic yarikoptic added semver-patch Increment the patch version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Jan 25, 2024
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jan 25, 2024
changelog.d/pr-7551.md Outdated Show resolved Hide resolved
@yarikoptic yarikoptic added the release Create a release when this pr is merged label Jan 25, 2024
@yarikoptic
Copy link
Member Author

fails unrelated (test_gh2043p1 on OSX and then ftp ones on windows appveyor) -- restarting

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (08315c1) 91.16% compared to head (532a5c6) 91.23%.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7551      +/-   ##
==========================================
+ Coverage   91.16%   91.23%   +0.07%     
==========================================
  Files         325      325              
  Lines       43440    43446       +6     
  Branches     5785     5785              
==========================================
+ Hits        39600    39640      +40     
+ Misses       3825     3791      -34     
  Partials       15       15              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jwodder added a commit to dandi/backups2datalad that referenced this pull request Jan 30, 2024
Revert this commit once the PR is merged.
@yarikoptic
Copy link
Member Author

ok, there were no objections and now we fixed all the rest and have datasets on github which need this. So I will merge and would be happy to address concerns later.

@yarikoptic yarikoptic merged commit 88dd9f0 into datalad:maint Feb 2, 2024
21 of 23 checks passed
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.19.6

@yarikoptic yarikoptic deleted the enh-token branch February 2, 2024 23:18
jwodder added a commit to dandi/backups2datalad that referenced this pull request Feb 4, 2024
Now that datalad/datalad#7551 has been merged & released
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Create a release when this pr is merged semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants