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

Username prompt when using conan upload does not accept email addresses #3444

Closed
3 tasks done
joechrisellis opened this issue Aug 30, 2018 · 8 comments
Closed
3 tasks done
Milestone

Comments

@joechrisellis
Copy link
Contributor

joechrisellis commented Aug 30, 2018

When attempting to upload to a repository you aren't yet authenticated with, conan asks you for your username and password. In some contexts, the username might be an email address -- in my specific case, using Artifactory with SAML SSO.

However, conan says that the username is invalid, which I think is because it contains the character '@'. It appears that usernames are validated using a regex in conans/model/ref.py, which is ^[a-zA-Z0-9_][a-zA-Z0-9_\+\.-]{min_chars,max_chars}$. This won't match usernames with an '@', therefore disallowing email addresses.

It is possible to specify an email address as your username for a repository with the conan user command, like this:

conan user -r your_repo your@email.com

Upon which, conan upload asks only for your password, and works as expected.

To reproduce, create a new remote, and immediately try to upload a package.

$ conan remote add test-remote ...
$ conan upload -r test-remote MyPackage/1.0@user/channel --all 
Uploading MyPackage/1.0@user/channel to remote 'test-remote'
Please log in to "test-remote" to perform this action. Execute "conan user" command.
Remote 'test-repo' username: my@email.com
ERROR: my@email.com is not a valid username
Remote 'test-remote' username: ...

This should be an easy fix, simply add '@' to the regex. :P

  • I've read the CONTRIBUTING guide.
  • I've specified the Conan version, operating system version and any tool that can be relevant.
  • I've explained the steps to reproduce the error or the motivation/use case of the question/suggestion.
@memsharded
Copy link
Member

Yes, this looks like a bug.

We need to decouple the Username validation from the user field in conan package references. Right now:

class Username(str):
    def __new__(cls, name):
        ConanName.validate_user(name)

@joechrisellis
Copy link
Contributor Author

I can sort this tonight 👍

@jgsogo
Copy link
Contributor

jgsogo commented Aug 30, 2018

That function, validate_user, is only used to validate that username, it may have a completely different set of checks (now it uses the same pattern and constraints as every item of a Conan reference string), and maybe we should move that function closer to the Username class.

Just as a reference, in order to document a regex for this, I'm trying different usernames at bintray.com and I'm able to get two different errors:

  • Username must start with a letter or a digit, end with a letter or a digit, and can only contain letters, digits and dashes.
  • Username must be between 3 to 30 chars long.

Which conan-server are you using, @joechrisellis?

@joechrisellis
Copy link
Contributor Author

Hi @jgsogo, I'm using whichever conan server is provided with Artifactory. I don't know exactly what this is under the hood -- I'd assume it's just the standard conan-server, perhaps with some modifications to integrate it with Artifactory.

@memsharded
Copy link
Member

Hi,

No, Artifactory has its own implementation of the conan protocol, it is not the python conan_server embedded in Artifactory (which is written in Java). Artifactory users, are not specific to conan, so they might have their own restrictions.

I have checked the usage of Username, and it seems it is only used for client side validation, but not used at all in the client code. So my suggestion would be to remove the checks completely, and let the servers decide what is valid and what not. WDYT @lasote?

@lasote
Copy link
Contributor

lasote commented Aug 31, 2018

Yes, we have to decouple the "username" from the "user in a reference". No validations for login username makes sense anymore since it is decoupled in the server technologies.

@jgsogo
Copy link
Contributor

jgsogo commented Aug 31, 2018

Right now the validation is only used there, I agree to remove it all, and afterwards, take care of what @lasote says (but that's a different issue not related to this one).

@joechrisellis, shall we wait for your PR? 😊

@joechrisellis
Copy link
Contributor Author

@jgsogo yep, I can work on this ASAP, thanks :)

memsharded added a commit that referenced this issue Sep 4, 2018
Removed client-side username checks (#3444)
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
Removed dead code for client-side user checks and added tests
grisumbras pushed a commit to grisumbras/conan that referenced this issue Dec 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants