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

Removed client-side username checks (#3444) #3464

Merged
merged 1 commit into from Sep 4, 2018

Conversation

Projects
None yet
2 participants
@joechrisellis
Copy link
Contributor

commented Sep 3, 2018

Fairly simple fix I think, just removing the client side tests when authenticating with the server. An error will still be raised iff the server determines the username to be invalid, but client side checks are no longer in place. This means that it's completely at the discretion of the server to decide whether or not a username is valid or invalid.

As I mentioned in #3444, the client side checks made it impossible to authenticate with an Artifactory instance using SAML SSO using the prompts in conan upload. The removal of these checks should fix this. I haven't yet been able to test whether or not Artifactory will deem email addresses to be OK -- I can check this Wednesday. But I think now the onus is on Artifactory to exhibit the correct behaviour rather than any bug in conan.

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

Changelog: Fix: Removed login username syntax checks that were no longer necessary.

@ghost ghost added the contributor pr label Sep 3, 2018

if self._interactive:
self.out.write("Remote '%s' username: " % remote_name)
user_input = self.get_username(remote_name)
username = Username(user_input)

This comment has been minimized.

Copy link
@memsharded

memsharded Sep 3, 2018

Contributor

I am not sure about this. The implementation is:

class Username(str):

    def __new__(cls, name):
        """Simple name creation.

        @param name:        string containing the desired name
        """
        ConanName.validate_user(name)
        return str.__new__(cls, name)

So it will still raise if validate_user() pattern is not fullfilled. Sounds a test would help to make sure it is working as expected.

This comment has been minimized.

Copy link
@joechrisellis

joechrisellis Sep 3, 2018

Author Contributor

You're right -- I got a different err msg after this change and assumed it was sent over from the server. On further inspection it's just displaying the exception message from models/ref.py.

@joechrisellis joechrisellis force-pushed the joechrisellis:develop branch from f1967ac to 6f5a08c Sep 3, 2018

@memsharded memsharded added this to the 1.8 milestone Sep 3, 2018

@memsharded
Copy link
Contributor

left a comment

The implementation now seems correct. However, to make it a complete change, it would be necessary:

  • To remove the Username class from the code. It is not used anywhere else, it is dead code now
  • To remove the username_test, that is testing such unused class.
  • A new test that proves that characters not valid in the name field of a conan_ref, like @ are indeed allowed now.

The first two are easy, just remove them. The last one might be a bit more complicated, but the TestClient allows in the constructor the user field which accepts user-password to be used in auth, that would be enough to test it. Ask for help if needed, thanks :)

@joechrisellis joechrisellis force-pushed the joechrisellis:develop branch from 6f5a08c to e7f66f2 Sep 4, 2018

@joechrisellis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

Added tests and rm the dead code, let me know if this is ok :)

Removed client-side username checks (#3444)
Removed dead code for client-side user checks and added tests

@joechrisellis joechrisellis force-pushed the joechrisellis:develop branch from e7f66f2 to f8f5ba3 Sep 4, 2018

@memsharded memsharded merged commit c9dd840 into conan-io:develop Sep 4, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the contributor pr label Sep 4, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

Excellent, thank you!

jgsogo added a commit to jgsogo/conan that referenced this pull request Sep 4, 2018

lasote added a commit that referenced this pull request Sep 20, 2018

Add more meaninful error when parsing conanfile reference (closes #3204
…) (#3410)

* check that parameters are string before applying regex match

* add tests related to ConanName

* typo

* update docs related to function

* use six to check for string types

* use a custom message to validate each field

* refactor tests to validate custom messages

* fix tests: message is more explicit

* given #3464 this is dead code

* format error message only if we are actually going to use it

* minor changes: address @lasote review

danimtb added a commit that referenced this pull request Sep 27, 2018

Validate ConanFileReference only if requested (#3623)
* check that parameters are string before applying regex match

* add tests related to ConanName

* typo

* update docs related to function

* use six to check for string types

* use a custom message to validate each field

* refactor tests to validate custom messages

* fix tests: message is more explicit

* given #3464 this is dead code

* format error message only if we are actually going to use it

* minor changes: address @lasote review

* optional validation of conanfile reference

@jgsogo jgsogo referenced this pull request Oct 24, 2018

Merged

Do not ask for username if it is already given #3839

2 of 2 tasks complete

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Add more meaninful error when parsing conanfile reference (closes con…
…an-io#3204)  (conan-io#3410)

* check that parameters are string before applying regex match

* add tests related to ConanName

* typo

* update docs related to function

* use six to check for string types

* use a custom message to validate each field

* refactor tests to validate custom messages

* fix tests: message is more explicit

* given conan-io#3464 this is dead code

* format error message only if we are actually going to use it

* minor changes: address @lasote review

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Validate ConanFileReference only if requested (conan-io#3623)
* check that parameters are string before applying regex match

* add tests related to ConanName

* typo

* update docs related to function

* use six to check for string types

* use a custom message to validate each field

* refactor tests to validate custom messages

* fix tests: message is more explicit

* given conan-io#3464 this is dead code

* format error message only if we are actually going to use it

* minor changes: address @lasote review

* optional validation of conanfile reference
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.