Disable logging in with organization token #780

merveenoyan · 2022-03-18T09:42:35Z

Hello 👋 This PR solves this issue #658. I looked into code to see where the user might login and where tokens are checked. I've seen three cases:

When the token in the folder is retrieved (when user doesn't give it)
When user logs in through CLI or notebook (_login)
When user passes the token directly (which are checked inside repository related functions that are used in places like mixins) like so:

        if token is None:

            token = self._validate_or_retrieve_token()

       elif isinstance(token, str):

          if token.startswith("api_org"):
              raise ValueError("You must use your personal account token for login.")
          elif not self._is_valid_token(token):
              if self._is_valid_token(name):
                  warnings.warn(
                      "`create_repo` now takes `token` as an optional positional argument. "
                     ....

We also check inside validate_or_retrieve token in case the token pulled is organization token.
So I added checks there and wrote tests for those three cases too.
cc: @osanseviero

adrinjalali

Thanks for the fix @merveenoyan .

Now that you're touching these bits, I realize there's quite a few repetitions. Could you maybe refactor those bits by creating a single _get_token utility function which us used throughout the codebase instead of having the same logic multiple times?

I'm not attached to the _get_token function name though.

merveenoyan · 2022-03-18T11:37:38Z

@adrinjalali that's what I thought tbh when I saw them 😅 sure!

merveenoyan · 2022-03-18T15:54:54Z

I refactored & tested but will check if there's nits to do on Monday 🙂

osanseviero · 2022-03-21T09:04:06Z

src/huggingface_hub/hf_api.py

+                token, name = name, token
+        if isinstance(token, str):
+            if token.startswith("api_org"):
+                raise ValueError("You must use your personal account token for login.")


This is not just for login, since users might be passing the token as a param

osanseviero · 2022-03-21T09:05:25Z

src/huggingface_hub/hf_api.py

+        if name is not None:
+            return token, name
+        else:
+            return token


I think you can always return token, name. It would then return None for the second case for name which is fine and will make usage easier

osanseviero · 2022-03-21T09:07:45Z

src/huggingface_hub/hf_api.py

        if token is None:
-            token = self._validate_or_retrieve_token()
-        elif not self._is_valid_token(token):
-            if self._is_valid_token(name):
-                warnings.warn(
-                    "`create_repo` now takes `token` as an optional positional argument. "
-                    "Be sure to adapt your code!",
-                    FutureWarning,
-                )
-                token, name = name, token
-            else:
-                raise ValueError("Invalid token passed!")
+            token = self._validate_or_retrieve_token(function_name="create_repo")
+        elif isinstance(token, str):
+            token, name = self._validate_or_retrieve_token(
+                token, name, function_name="create_repo"
+            )


I think this can be simplified even further by just doing this:

token, name = self._validate_or_retrieve_token( token, name, function_name="create_repo" )

_validate_or_retrieve_token already handles case in which token is None or string, so no need to do the check here. Same applies for other functions

I think we can move _validate_or_retrieve_token outside this class, move it to a utils module and use it also in the other file.

@adrinjalali I recently pushed a new change but didn't do this yet, there's a repetition in user.py if we don't do this, I agree. Will push that soon.

@adrinjalali it doesn't make any sense to put _validate_or_retrieve_token under endpoint helpers, the function depends heavily on other classes and functions in hf_api.py, so instead I called it in user.py like hf_api._validate_or_retrieve_token.

osanseviero · 2022-03-21T09:08:45Z

src/huggingface_hub/hf_api.py

-                )
-            else:
-                raise ValueError("Invalid token passed!")
+            token = self._validate_or_retrieve_token(function_name="upload_file")


Should this be updated since the logic below is in validate_or_retrieve_token?

merveenoyan · 2022-03-24T11:40:16Z

I don't see anything else to refactor. I just have one question.

What is the reason behind this in upload_file? (below is initial version)

elif not self._is_valid_token(token):
 if self._is_valid_token(path_or_fileobj):

Do people pass tokens in paths or files? I refactored assuming that. (it didn't make much sense to me though) @osanseviero

adrinjalali · 2022-03-31T07:06:51Z

What is the reason behind this in upload_file? (below is initial version)

I believe this is there only for backward compatibility. At some point the order of args were changed, and therefore this makes sure the older order is still accepted.

adrinjalali

Otherwise LGTM

src/huggingface_hub/hf_api.py

Co-authored-by: Adrin Jalali <[email protected]>

HuggingFaceDocBuilderDev · 2022-03-31T10:59:57Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

adrinjalali · 2022-03-31T11:31:55Z

src/huggingface_hub/hf_api.py

+            name (``str``, `optional`):
+                Name of the repository.
+            function_name (``str``, `optional`):
+                If called from a function, name of that function for deprecation warning.


Missing Returns and Raises sections.

It would also be nice to explain why name and function_name are required and that they'd be removed in a future version (in v0.7 maybe?)

…e_hub into disable_org_token

adrinjalali

This ended up being quite a bit more than the original issue (it's probably my fault 🙈 ). But I really like the improvement here. Thanks @merveenoyan

merveenoyan · 2022-03-31T12:07:58Z

@adrinjalali what would you rather have, I really wonder 🙂

adrinjalali · 2022-03-31T12:09:22Z

One could argue that the refactoring could or should have been a separate PR. But I'm glad this is done now.

osanseviero · 2022-03-31T12:40:24Z

Note that this PR reverts docstrings changes needed for the new documentation (e.g. using *optional*). You can see it in the diff

merveenoyan added 3 commits March 17, 2022 19:13

disabled organization token

d0c18f7

checking under token _validate_or_retrieve_token()

32e8e55

added test for HfFolder token is organization token

87ee7bf

github-actions bot requested review from LysandreJik, muellerzr and osanseviero March 18, 2022 09:42

adrinjalali suggested changes Mar 18, 2022

View reviewed changes

refactored token retrieval & validation

9d9a4b3

osanseviero reviewed Mar 21, 2022

View reviewed changes

refactored _validate_or_retrieve_token

40553e9

LysandreJik mentioned this pull request Mar 24, 2022

Replace obsolete function argument name in docs for api.create_repo #795

Closed

refactored for repetitions

6d9fb56

merveenoyan requested a review from adrinjalali March 25, 2022 11:46

adrinjalali added this to the v0.4 milestone Mar 31, 2022

adrinjalali reviewed Mar 31, 2022

View reviewed changes

src/huggingface_hub/hf_api.py Show resolved Hide resolved

src/huggingface_hub/hf_api.py Show resolved Hide resolved

src/huggingface_hub/hf_api.py Show resolved Hide resolved

merveenoyan and others added 3 commits March 31, 2022 13:25

Update src/huggingface_hub/hf_api.py

de309f9

Co-authored-by: Adrin Jalali <[email protected]>

added docstring

ba927b5

Merge branch 'main' into disable_org_token

aa338cd

merveenoyan requested a review from adrinjalali March 31, 2022 11:01

adrinjalali reviewed Mar 31, 2022

View reviewed changes

merveenoyan added 2 commits March 31, 2022 14:41

added returns and raises

546f0bc

Merge branch 'disable_org_token' of github.com:huggingface/huggingfac…

71727dc

…e_hub into disable_org_token

adrinjalali approved these changes Mar 31, 2022

View reviewed changes

merveenoyan merged commit cc28c37 into main Mar 31, 2022

merveenoyan deleted the disable_org_token branch March 31, 2022 12:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable logging in with organization token #780

Disable logging in with organization token #780

merveenoyan commented Mar 18, 2022 •

edited

Loading

adrinjalali left a comment

merveenoyan commented Mar 18, 2022 •

edited

Loading

merveenoyan commented Mar 18, 2022

osanseviero Mar 21, 2022

osanseviero Mar 21, 2022

osanseviero Mar 21, 2022

adrinjalali Mar 22, 2022

merveenoyan Mar 23, 2022

merveenoyan Mar 24, 2022

osanseviero Mar 21, 2022

merveenoyan commented Mar 24, 2022

adrinjalali commented Mar 31, 2022

adrinjalali left a comment

HuggingFaceDocBuilderDev commented Mar 31, 2022

adrinjalali Mar 31, 2022

merveenoyan Mar 31, 2022

adrinjalali left a comment

merveenoyan commented Mar 31, 2022

adrinjalali commented Mar 31, 2022

osanseviero commented Mar 31, 2022

Disable logging in with organization token #780

Disable logging in with organization token #780

Conversation

merveenoyan commented Mar 18, 2022 • edited Loading

adrinjalali left a comment

Choose a reason for hiding this comment

merveenoyan commented Mar 18, 2022 • edited Loading

merveenoyan commented Mar 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan commented Mar 24, 2022

adrinjalali commented Mar 31, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Mar 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

merveenoyan commented Mar 31, 2022

adrinjalali commented Mar 31, 2022

osanseviero commented Mar 31, 2022

merveenoyan commented Mar 18, 2022 •

edited

Loading

merveenoyan commented Mar 18, 2022 •

edited

Loading