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

Logging with organization token is successful and leads to side effects #658

Closed
merveenoyan opened this issue Feb 6, 2022 · 19 comments
Closed
Assignees

Comments

@merveenoyan
Copy link
Contributor

It was reported that users get KeyError when push_to_hub_keras() is used to push for organization.

Example:

from huggingface_hub import push_to_hub_keras
push_to_hub_keras(model=vqvae_trainer, repo_url='https://huggingface.co/keras-io/vq_vae', organization='keras-io')

When organization token is explicitly written, the problem goes away.
Example:

push_to_hub_keras(model=forest_model, .....
                  repo_path_or_name='.', repo_url = "https://huggingface.co/keras-io/deep-neural-decision-forests", 
                  use_auth_token=keras_io_hub_token)

Is this intended behavior? @nateraw
Also cc: @osanseviero

@merveenoyan merveenoyan self-assigned this Feb 6, 2022
@osanseviero
Copy link
Contributor

Is the issue reproducible?

@merveenoyan
Copy link
Contributor Author

merveenoyan commented Feb 7, 2022

@osanseviero yes, I was able to raise it.
Login with keras-io organization token with HF CLI, then do

from huggingface_hub import push_to_hub_keras
push_to_hub_keras(model=autoencoder, repo_url="https://huggingface.co/keras-io/conv-autoencoder", organization = "keras-io")

Weird enough, it doesn't happen at all when I login with my own token.

So either: login with keras-io token and define this explicitly in the pushing function, (which is cumbersome anyway)
or: login with personal account and don't define use_auth_token at all.

@osanseviero
Copy link
Contributor

@SBrandeis are people expected to be able to do login using an organization token? That sounds off no?

@SBrandeis
Copy link
Contributor

Hi, you need to provide a user access token (https://hf.co/settings/token) to be able to push to an organization

We should refrain from logging in with organization tokens and push users towards using their personal user access tokens when authentication is required 🙂

@merveenoyan
Copy link
Contributor Author

Maybe we could put a note in Hub since I didn't know that I wasn't supposed to do it @SBrandeis @osanseviero

@osanseviero
Copy link
Contributor

My question is why do we support login with organization tokens at all? What's the use case?

@osanseviero
Copy link
Contributor

cc @julien-c as well

@merveenoyan
Copy link
Contributor Author

@osanseviero +1, maybe we could explain that as well

@SBrandeis
Copy link
Contributor

My question is why do we support login with organization tokens at all? What's the use case?

Using an organization API token will work on most API routes that require only read access to repos, as well as with /resolve and /raw endpoints


This behavior holds mainly for backward compatibility and will probably be removed at some point in the future. We should not rely on it.

As stated before, the preferred way to authenticate programmatically with the hub are user access tokens

@osanseviero
Copy link
Contributor

Thanks for the context! In my opinion logging in with organization token should not be supported at all (cc @LysandreJik @muellerzr) since it looks like it's a successful login but can lead to a bunch of side effects. I would feel more comfortable if we error out in login when using organization token in huggingface_hub.

@osanseviero osanseviero changed the title KeyError: 'orgs' raised when pushing to organization when push_to_hub_keras() is used Logging with organization token is successful and leads to side effects Feb 7, 2022
@Pierrci
Copy link
Member

Pierrci commented Feb 7, 2022

Yes, the org API token isn't really thought to be used in the case of hfhub, its main use is rather the inference API (hence its name, different from the users' "access tokens"). Preventing from logging in with it makes sense to me (simple to detect since all org tokens start w/ api_org_)

@merveenoyan merveenoyan removed their assignment Feb 8, 2022
@julien-c
Copy link
Member

julien-c commented Feb 8, 2022

agreed on the proposed fix

@osanseviero
Copy link
Contributor

@merveenoyan or @muellerzr would any of you like to take this one?

@merveenoyan
Copy link
Contributor Author

I'll try 🤗

@merveenoyan merveenoyan self-assigned this Feb 11, 2022
@merveenoyan
Copy link
Contributor Author

merveenoyan commented Mar 17, 2022

Other than passing token as an argument, I tested for huggingface-cli. I wonder if we should put a warning along the lines of "If you're logging in to push models, make sure you are using your personal token" when users are logging in with organization token. When pushing I get following which is a bit confusing IMO.

HTTPError: 401 Client Error: Unauthorized for url: https://huggingface.co/api/repos/create - Unauthorized

@osanseviero
Copy link
Contributor

osanseviero commented Mar 17, 2022

Org/API tokens always begin with api_org_ as @Pierrci mentioned above, so we can easily detect them. The proposed solution is just to avoid allowing login with an org token, so this can simply be fixed by raising a value error if the api_org_ is at the beginning of the token in the login functions (such as https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/hf_api.py#L446).

@merveenoyan
Copy link
Contributor Author

@osanseviero I understood it and fixed for hub mixin and keras mixin, I'm trying to see where else this would fail atm so I'm testing pushing and pulling and if they fail. 👀 I'll write my findings here.

@osanseviero
Copy link
Contributor

This confused me

Other than passing token as an argument, I tested for huggingface-cli. I wonder if we should put a warning along the lines of "If you're logging in to push models, make sure you are using your personal token" when users are logging in with organization token.

We can simply raise an error here, no need to provide a warning.

I don't think you need to change the mixins code itself, though. By changing login you would be fixing this issue no?

@merveenoyan
Copy link
Contributor Author

We merged the PR that solves the issue, I'm closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants