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

feat: support authorization #259

Merged
merged 2 commits into from
Apr 28, 2019
Merged

feat: support authorization #259

merged 2 commits into from
Apr 28, 2019

Conversation

hyj1991
Copy link
Contributor

@hyj1991 hyj1991 commented Mar 14, 2018

This change is Reviewable

@hyj1991 hyj1991 force-pushed the master branch 2 times, most recently from 6642eaa to fdccd89 Compare March 14, 2018 14:35
@fengmk2
Copy link
Member

fengmk2 commented Aug 10, 2018

能否 rebase 一下 master 分支?然后补充一下单元测试?

@hyj1991
Copy link
Contributor Author

hyj1991 commented Aug 10, 2018

好古老的 pr...因为当时合并到 bnpm 去了,这里就没管了...有需求的话我来弄下

@codecov-io
Copy link

codecov-io commented Aug 10, 2018

Codecov Report

Merging #259 into master will increase coverage by 0.01%.
The diff coverage is 96.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
+ Coverage   90.04%   90.06%   +0.01%     
==========================================
  Files          27       28       +1     
  Lines        1849     1882      +33     
  Branches      310      321      +11     
==========================================
+ Hits         1665     1695      +30     
- Misses        184      187       +3
Impacted Files Coverage Δ
lib/get.js 98.27% <100%> (+0.27%) ⬆️
lib/cnpm_config.js 96% <96%> (ø)
lib/install.js 97.39% <0%> (-1.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65ad3e...ab5390c. Read the comment docs.

@hyj1991 hyj1991 force-pushed the master branch 2 times, most recently from 99fd9d1 to d5a987f Compare August 10, 2018 06:17
@hyj1991 hyj1991 force-pushed the master branch 11 times, most recently from 4884159 to d0bd1f2 Compare August 13, 2018 04:18
@fengmk2
Copy link
Member

fengmk2 commented Aug 13, 2018

@hyj1991 readme 增加一下使用方式。

@@ -0,0 +1,3 @@
registry=https://registry-mock.org/
//registry-mock.org/:always-auth=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

没有 :username

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mock 数据少写了个 username,其实这里只要看下 auth 的时候 header 里面有没有 basic auth 信息就行了,我补充下

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已经在 mock 数据中补充了 username 信息


}

createConfigs();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createConfigs 做成 lazy load,在第一次 get 的时候才调用。然后单元测试里面就容易 mock 了。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里要测试下 .cnpmrc 的实际解析,感觉 mock 掉 process.env.Home 指向预设的内容,再调用下 get 方法看下 basic auth 有没有设置到 header 里面去比较好

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里会导致不需要这个功能的用户强制生成了 .cnpmrc 文件。

@hyj1991
Copy link
Contributor Author

hyj1991 commented Aug 13, 2018

使用方式其实就是 cnpm login 后,这个 pr 会给 cnpm install 操作会带上用户账号信息,这样 cnpm server 上如果开启了 alwaysAuth 配置,那么未登录的账号就无法再安装模块了

@fengmk2 fengmk2 merged commit 4464362 into cnpm:master Apr 28, 2019
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.

None yet

3 participants