Set of fixes for better npmrc parameters support #12284

smnbbrv · 2018-09-15T12:25:56Z

Heavily related to a hot topic #10624. This issue is really annoying for lots of people.

Targets two points:

Support for per-project .npmrc file. This is done for the projects that include bundled .npmrc with readonly permissions, see https://docs.npmjs.com/files/npmrc
Support for private repositories like Nexus and Artifactory. I did not test with Artifactory because currently I have none, however Nexus works perfectly with the applied changes. Artifactory has a similar way of authentication, so it should also be covered.

Changes type:

alwaysAuth is changed to always-auth, which is a proper name, see https://docs.npmjs.com/misc/config#always-auth
added email parameter
_auth is normally a base64 encoded token:password string => added extracting those credentials from the token and left a fallback token assignment in case something else is stored in _auth

Thank you in advance.

hansl · 2018-09-15T18:38:31Z

packages/schematics/update/update/npm.ts

+              // attempt to parse username and password from base64 token
+              // to enable Artifactory / Nexus-like repositories support
+              const parsedToken = Buffer.from(token, 'base64').toString('ascii');
+              const [extractedUsername, extractedPassword] = parsedToken.split(':');


Looks like it cannot have : in the password. Is this expected? You might want to use this instead:

const delimiter = ':'; const [extractedUsername, ...passwordArr] = parsedToken.split(delimiter); const extractedPassword = passwordArr.join(delimiter);

Just slightly more ugly, but there's no native function that does it.

true, good point. Thanks for reviewing! I will apply the changes soon

hansl

Hey @smnbbrv ,

Great PR, love it. One nit, and also the validate test will pass if you squash the lint commit into its previous commit.

Once that's done I'll approve it. Thanks!

hansl

Sorry, I meant to request the changes above.

…ter support for private repositories

smnbbrv · 2018-09-15T20:01:58Z

@hansl updated the PR according to your comments and going to sleep :) If you have more comments / there is something more to fix, I'm glad to help

mrmaxsteel · 2018-09-17T12:31:11Z

FYI tested with Artifactory, did not work, see: #10624 (comment)

mrmaxsteel · 2018-09-17T13:16:22Z

@smnbbrv I found the issue, nothing to do with Artifactory. Your code to parse the username:password from the base64 _auth token doesn't work when there is whitespace between key and value in .npmrc.

For example, it works when .npmrc looks like:

_auth=<base64 user:pass>

But not when:

_auth = <base64 user:base>

smnbbrv · 2018-09-17T14:14:29Z

@hansl I added another fix that adds support of whitespaces around '='. Could you, please, revalidate?

@mrmaxsteel it's not really my code, I'm just a random stranger who wants the issue to be fixed :D Added fix to your findings. That's really cool that Artifactory works as well 👍

mrmaxsteel · 2018-09-17T14:41:48Z

Validated latest change, all works 👍

… account

smnbbrv · 2018-09-19T12:18:26Z

@hansl I can't really understand the ci failures... Does not seem to me like it is the issue connected to the changes I made

packages/schematics/update/update/npm.ts

aguacongas · 2018-09-21T20:57:12Z

packages/schematics/update/update/npm.ts

-      }
+
+    if (fs.existsSync(perProjectNpmrc)) {
+      npmrc = fs.readFileSync(perProjectNpmrc).toString('utf-8');
    } else {


You should parse both .npmrc files because the repository url can be in the per project file and credentials in the user file

True.

However if we are too picky here we enter the pain zone. First, then we need to support the global npmrc + userconfig flag / env variable as well. Second, we need to properly resolve the collisions between _authToken and _auth (because both could be present and we need to pick). Finally, the current approach is reading npmrc every time the package info is getting resolved which is not a standard npm behavior. Etc.

I'm not yet talking about having two different ways of getting NPM parameters.

The whole approach is not perfect and this PR does not pretend to be a fully working solution; at least it would be way more helpful than the current approach. Probably one needs to deeply rethink the concept.

I agree. However, IMHO, user file should be read first then because people having open source projects will propably share the project .npmrc without credentials

The private repos are nearly never used in the open source, hence no auth is required. Also if you change the order of resolving those files then you go in clear conflict with Npm itself

#10624 is regarding an issue with fontawesome pro, so I guess it's used in open source. and npm don't need authentication nor .npmrc.

https://fontawesome.com/pro - 60$ per license for 5 seats. What open source projects are you talking about?

angular-fontawesome for sample
Open source does not means free, anybody can share code on github but use a private repo with tokens they obvioulsly don't want to share.

OK, I have more arguments, but the discussion went away from the issue. What we discuss is only the cases when there is a project-specific .npmrc which is not containing _auth or _authToken, but there is one in some of the .npmrc above right?

Exactly, in that case, your PR doesn't fix the issue I guess

hansl · 2018-09-26T16:27:02Z

I'm going to let this in, but we should fix the local/global issue in RC.

angular-automatic-lock-bot · 2019-09-12T15:41:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

feat(@schematics/update): add per-project .npmrc file support

9e1f32a

googlebot added the cla: yes label Sep 15, 2018

hansl reviewed Sep 15, 2018

View reviewed changes

hansl approved these changes Sep 15, 2018

View reviewed changes

hansl suggested changes Sep 15, 2018

View reviewed changes

smnbbrv force-pushed the fix-10624 branch from 9d9bf71 to 9e1f32a Compare September 15, 2018 19:47

fix(@schematics/update): process always-auth and _auth correctly, bet…

7246555

…ter support for private repositories

hansl added the target: major This PR is targeted for the next major release label Sep 15, 2018

hansl previously approved these changes Sep 15, 2018

View reviewed changes

mrmaxsteel mentioned this pull request Sep 17, 2018

"ng update" doesn't support private repos such as FontAwesome 5 Pro #10624

Closed

smnbbrv dismissed hansl’s stale review via eb9460a September 17, 2018 14:12

smnbbrv force-pushed the fix-10624 branch from eb9460a to 7246555 Compare September 19, 2018 11:09

fix(@schematics/update): taking whitespaces around "=" in .npmrc into…

85ee603

… account

aguacongas reviewed Sep 21, 2018

View reviewed changes

packages/schematics/update/update/npm.ts Show resolved Hide resolved

aguacongas reviewed Sep 21, 2018

View reviewed changes

smnbbrv mentioned this pull request Sep 22, 2018

feat(@schematics/update): add full support of npmrc / npm config #12350

Closed

smnbbrv closed this Sep 22, 2018

smnbbrv reopened this Sep 24, 2018

hansl approved these changes Sep 26, 2018

View reviewed changes

hansl merged commit b19a348 into angular:master Sep 26, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set of fixes for better npmrc parameters support #12284

Set of fixes for better npmrc parameters support #12284

smnbbrv commented Sep 15, 2018 •

edited

Loading

hansl Sep 15, 2018

smnbbrv Sep 15, 2018

hansl left a comment

hansl left a comment

smnbbrv commented Sep 15, 2018

mrmaxsteel commented Sep 17, 2018

mrmaxsteel commented Sep 17, 2018 •

edited

Loading

smnbbrv commented Sep 17, 2018

mrmaxsteel commented Sep 17, 2018

smnbbrv commented Sep 19, 2018

aguacongas Sep 21, 2018 •

edited

Loading

smnbbrv Sep 22, 2018

aguacongas Sep 22, 2018 •

edited

Loading

smnbbrv Sep 22, 2018 •

edited

Loading

aguacongas Sep 22, 2018

smnbbrv Sep 22, 2018

aguacongas Sep 22, 2018

smnbbrv Sep 22, 2018

aguacongas Sep 22, 2018

hansl commented Sep 26, 2018

angular-automatic-lock-bot bot commented Sep 12, 2019

Set of fixes for better npmrc parameters support #12284

Set of fixes for better npmrc parameters support #12284

Conversation

smnbbrv commented Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansl left a comment

Choose a reason for hiding this comment

hansl left a comment

Choose a reason for hiding this comment

smnbbrv commented Sep 15, 2018

mrmaxsteel commented Sep 17, 2018

mrmaxsteel commented Sep 17, 2018 • edited Loading

smnbbrv commented Sep 17, 2018

mrmaxsteel commented Sep 17, 2018

smnbbrv commented Sep 19, 2018

aguacongas Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aguacongas Sep 22, 2018 • edited Loading

Choose a reason for hiding this comment

smnbbrv Sep 22, 2018 • edited Loading

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

hansl commented Sep 26, 2018

angular-automatic-lock-bot bot commented Sep 12, 2019

smnbbrv commented Sep 15, 2018 •

edited

Loading

mrmaxsteel commented Sep 17, 2018 •

edited

Loading

aguacongas Sep 21, 2018 •

edited

Loading

aguacongas Sep 22, 2018 •

edited

Loading

smnbbrv Sep 22, 2018 •

edited

Loading