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

fix(plugin-npm-cli): fix login flow with the registry #1848

Closed
wants to merge 1 commit into from

Conversation

juanpicado
Copy link
Contributor

@juanpicado juanpicado commented Sep 20, 2020

What's the problem this PR addresses?

Add the missing logic the registry requires to login a user already registered.

Fixes #1044

How did you fix it?

Add missing request to perform a full login. The flow is like this:

  • User log in and the registry is supposed to return error 409 if exist already:
    • The request does not send the npmAuthToken on the first request
    • The registry will try to add a new user and if exist return 409 by design.
  • The CLI will request the logged user:
    • The CLI must send the npmAuthToken and registry will rely on that to return the logged user
    • If token is not being send then should throw an error
    • The response only includes a text, so we need to rely on the HTTP status and ok message.
  • If the previous step is success, a third request (PUT) to log in user:
    • The npmAuthToken must be send
    • If token is valid will successfully log in the user
    • If it fails, we throw an error.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I have verified that all automated PR checks pass.

@juanpicado
Copy link
Contributor Author

juanpicado commented Sep 20, 2020

I'd need some help to improve the code, not sure where to put things or create methods and so on and test updates/create are missing

@juanpicado juanpicado changed the title fix(plugin-npm-cli): fix login flow with registry fix(plugin-npm-cli): fix login flow with the registry Sep 20, 2020
@arcanis
Copy link
Member

arcanis commented Sep 20, 2020

Thanks for looking into it! Quick questions before discussing the implementation:

  • Do you see a way to split user creation out of user authentication? I don't like the idea of having a login command that does either. I realize npm doesn't make the distinction, but I think we should 🤔

  • Since the login works on the npm registry, do you think there is something that Verdaccio should do? Perhaps as an enhancement rather than a bugfix, so that we could do the new user registration on a different command, cf first point.

@juanpicado
Copy link
Contributor Author

Thanks for looking into it! Quick questions before discussing the implementation:

  • Do you see a way to split user creation out of user authentication? I don't like the idea of having a login command that does either. I realize npm doesn't make the distinction, but I think we should 🤔
  • Since the login works on the npm registry, do you think there is something that Verdaccio should do? Perhaps as an enhancement rather than a bugfix, so that we could do the new user registration on a different command, cf first point.

I need to look into those points then. For point one I'd like to do that, actually npm adduser is a useless command for most of the registries implementation, only verdaccio gives sense to that command allowing create a brand new user. I know there is a legacy npm/npm-profile@9710475 that's the unique hint I found by far.

It maybe fixed at Verdaccio, but not sure if would be a breaking change, we still support npm5 and I'm not sure side effects.

I'll back when I have more news.

@jounii
Copy link

jounii commented Oct 7, 2020

Any news on this? I faced these issues with yarn berry regards this and it makes Vercaddio impossible to use and basically yarn berry useless to publish packages.

I was hoping to get away from Yarn1 + Lerna + sematic-release by using yarn berry and couple of extra scripts or possibly make yarn berry plugin for sematic-release style publish, but seems that is impossible currently if using Verdaccio. I was going to use Vercaddio as testbed for development and Verdaccio is the internal enterprise registry, so it needs to work.

@juanpicado
Copy link
Contributor Author

juanpicado commented Oct 12, 2020

Any news on this?

Still checking, I'm swimming in the amazing world of registries and other pkg manager lately, no clear solution yet.

I faced these issues with yarn berry regards this and it makes Vercaddio impossible to use and basically yarn berry useless to publish packages.

Why? This PR is about yarn login which I doubt you might use via CI`.

I was hoping to get away from Yarn1 + Lerna + sematic-release by using yarn berry and couple of extra scripts or possibly make yarn berry plugin for sematic-release style publish, but seems that is impossible currently if using Verdaccio. I was going to use Vercaddio as testbed for development and Verdaccio is the internal enterprise registry, so it needs to work.

If you enable npmAuthToken and npmAlwaysAuth yarn berry will login you perfectly. I mean, you would still need to create the token via npm login/adduser but, once you have the token you should be not blocked to use yarn berry.

Unless I missing something from your side, let me know.

// .yarnrc.yml
npmRegistryServer: "http://localhost:4873"

unsafeHttpWhitelist:
  - localhost

npmScopes:
  testscope:
    npmPublishRegistry: "http://localhost:4873"
    npmRegistryServer: "http://localhost:4873"
    npmAlwaysAuth: true

npmAuthToken: "xxxxxxHJGSw=="
npmAlwaysAuth: true

ref https://github.com/juanpicado/yarnv2_test

@jounii
Copy link

jounii commented Oct 14, 2020

If you enable npmAuthToken and npmAlwaysAuth yarn berry will login you perfectly. I mean, you would still need to create the token via npm login/adduser but, once you have the token you should be not blocked to use yarn berry.

Unless I missing something from your side, let me know.

Thank you for this.

I was able to get the publish work with verdaccio, but it does differ slightly in yarn2 from npm, which I used as a comparison to get better logging. I could not figure out how yarn2 would give more verbose logging.

Following caveats which I had to solve:

  • Yarn2 does not pick global npmAuthToken setting from ~/.yarnrc.yml, at least with npmScopes:. NPM does work fine with this (with it's .npmrc of course). Workaround is to copy the npmAuthToken to the repository local .yarnrc.yml file. Maybe there's some configuration syntax which I did not see
  • Yarn2 login works only once with Verdaccio, so if you lose/delete your npmAuthToken value, only way to get it back is to remove the user from Verdaccio and login again
  • Yarn2 does not respect the npmScopes: values with npm publish for packages which are scoped in their package.json. Either you have to configure the registry to be global before running publish or add publishConfig.registry value to the package.json to force certain registry

Probably just need to add some additional CI steps for this as the "normal" configuration is not enough.

EDIT:

Just to clarify why this configuration. The company internal registry (Verdaccio) is accessed through scoping (and packages are only published to that one) and any other packages are accessed directly from their own registries. Verdaccio is not used as proxy to public registries.

@juanpicado
Copy link
Contributor Author

I could not figure out how yarn2 would give more verbose logging.

yeah, me neither :( maybe @arcanis @merceyz have an idea. Actually is really helpful for debugging.

@jounii
Copy link

jounii commented Oct 14, 2020

Update regards publishing to scoped registry server.

The documentation says that npmPublishRegistry is not needed if npmRegistryServer is given, but this is incorrect.. You have to define npmPublishRegistry separately, otherwise, the global value is used and not the npmScope npmRegistryServer value. I was trusting documentation on this one and didn't remember there was a separate value for publish registry.

@stavalfi

This comment has been minimized.

demurgos added a commit to demurgos/berry that referenced this pull request Nov 19, 2023
This commit fixes `yarn npm login` when the remote registry is Verdaccio.

When a user already exists, the registry replies with `409 Conflict`. The official npm client then retrieves the latest user state and inserts a revision, using HTTP basic authentication. This step was missing, and this commits adds it.

The change was tested to work with a private Verdaccio registry. It should now be as reliable as the official npm client.

- Closes yarnpkg#1044
- Closes yarnpkg#1848
- Closes verdaccio/verdaccio#1737
demurgos added a commit to demurgos/berry that referenced this pull request Nov 19, 2023
This commit fixes `yarn npm login` when the remote registry is Verdaccio.

When a user already exists, the registry replies with `409 Conflict`. The official npm client then retrieves the latest user state and inserts a revision, using HTTP basic authentication. This step was missing, and this commits adds it.

The change was tested to work with a private Verdaccio registry. It should now be as reliable as the official npm client.

- Closes yarnpkg#1044
- Closes yarnpkg#1848
- Closes verdaccio/verdaccio#1737
demurgos added a commit to demurgos/berry that referenced this pull request Nov 19, 2023
This commit fixes `yarn npm login` when the remote registry is Verdaccio.

When a user already exists, the registry replies with `409 Conflict`. The official npm client then retrieves the latest user state and inserts a revision, using HTTP basic authentication. This step was missing, and this commits adds it.

The change was tested to work with a private Verdaccio registry. It should now be as reliable as the official npm client.

- Closes yarnpkg#1044
- Closes yarnpkg#1848
- Closes verdaccio/verdaccio#1737
demurgos added a commit to demurgos/berry that referenced this pull request Nov 19, 2023
This commit fixes `yarn npm login` when the remote registry is Verdaccio.

When a user already exists, the registry replies with `409 Conflict`. The official npm client then retrieves the latest user state and inserts a revision, using HTTP basic authentication. This step was missing, and this commits adds it.

The change was tested to work with a private Verdaccio registry. It should now be as reliable as the official npm client.

- Closes yarnpkg#1044
- Closes yarnpkg#1848
- Closes verdaccio/verdaccio#1737
arcanis pushed a commit that referenced this pull request Nov 28, 2023
**What's the problem this PR addresses?**

This commit fixes `yarn npm login` when the remote registry is
Verdaccio.

- Closes #1044
- Closes #1848
- Closes verdaccio/verdaccio#1737

...

**How did you fix it?**

When a user already exists, the registry replies with `409 Conflict`.
The official npm client then retrieves the latest user state and inserts
a revision, using HTTP basic authentication. This step was missing, and
this commits adds it.

The change was tested to work with a private Verdaccio registry. It
should now be as reliable as the official npm client.

...

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.
merceyz pushed a commit that referenced this pull request Jan 30, 2024
**What's the problem this PR addresses?**

This commit fixes `yarn npm login` when the remote registry is
Verdaccio.

- Closes #1044
- Closes #1848
- Closes verdaccio/verdaccio#1737

...

**How did you fix it?**

When a user already exists, the registry replies with `409 Conflict`.
The official npm client then retrieves the latest user state and inserts
a revision, using HTTP basic authentication. This step was missing, and
this commits adds it.

The change was tested to work with a private Verdaccio registry. It
should now be as reliable as the official npm client.

...

**Checklist**
<!--- Don't worry if you miss something, chores are automatically
tested. -->
<!--- This checklist exists to help you remember doing the chores when
you submit a PR. -->
<!--- Put an `x` in all the boxes that apply. -->
- [x] I have read the [Contributing
Guide](https://yarnpkg.com/advanced/contributing).

<!-- See
https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released
for more details. -->
<!-- Check with `yarn version check` and fix with `yarn version check
-i` -->
- [x] I have set the packages that need to be released for my changes to
be effective.

<!-- The "Testing chores" workflow validates that your PR follows our
guidelines. -->
<!-- If it doesn't pass, click on it to see details as to what your PR
might be missing. -->
- [x] I will check that all automated PR checks pass before the PR gets
reviewed.

(cherry picked from commit db6210f)
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

Successfully merging this pull request may close these issues.

yarn npm login is not compatible with verdaccio
4 participants