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

Feature access control #2196

Merged
merged 13 commits into from
Jan 10, 2020
Merged

Conversation

nkorange
Copy link
Collaborator

@nkorange nkorange commented Dec 18, 2019

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

Introduce access control

Brief changelog

Issue: #1105

Design document: https://nacos.io/zh-cn/blog/access%20control%20design.html

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
  • Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

nacos.core.auth.default.token.secret.key:SecretKey012345678901234567890123456789012345678901234567890123456789
感觉后面一个串比较特殊,如果有也建议从配置文件中读取,配置里面标记用户可修改。 不要代码写死敏感信息了

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

AccessException code这里的code都是401或者403,还是变化的呢?如果是不变的可以这里直接返回固定码。如果有变化,可以把常用的异常码定义到这个异常中

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

public static final String DEFAULT_NAMESPACE_ID = "public";
默认命名空间是空串,不是public;

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

private static final String AUTH_CONFIG_PREFIX = "config/";

格式跟方案不一致; 如果资源中有的时候考虑一下匹配效率。尽量避免的设计。可以参考一下ACM的资源设计
https://help.aliyun.com/document_detail/85118.html?spm=a2c4g.11186623.6.566.72b13733Td5GYG

if (permissionAction.contains(permission.getAction()) &&
Pattern.matches(permissionResource, permission.getResource())) {
return true;
}

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

@DeleteMapping(params = "delType=ids")
@Secured(action = ActionTypes.WRITE, parser = ConfigResourceParser.class)
public RestResult<Boolean> deleteConfigs(HttpServletRequest request, HttpServletResponse response,
                                         @RequestParam(value = "ids") List<Long> ids) {

这个不能使用注解鉴权,因为需要根据id查到三元组变成resource才行。 不然解析的都是空

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

@Secured(action = ActionTypes.READ, parser = ConfigResourceParser.class)
public void listener(HttpServletRequest request, HttpServletResponse response)
    throws ServletException, IOException {

一样的问题统一改一下吧,批量接口都没法通过入口解析出三元组,需要接口级别鉴权去做;没法在入口统一搞掉。除非定义各种不同的解析器,但这样会带来重复解析的成本

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

@Value("${nacos.core.auth.enabled:false}")
private boolean authEnabled;

关鉴权必须重启吗? 紧急关鉴权怎么动态关呢。 cluster.conf机制,还是动态配置机制,还是管控api机制,如果是管控api机制,管控api的权限怎么控制,这块需要设计一下

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

鉴权核心类的ut和it需要补充一下。

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

// 这块何时不为空呢? 感觉每次请求这块都是空。
String resource = secured.resource();

            if (StringUtils.isBlank(resource)) {
                ResourceParser parser = secured.parser().newInstance();
                resource = parser.parseName(req);
            }

            if (StringUtils.isBlank(resource)) {
                // deny if we don't find any resource:
                throw new AccessException("resource name invalid!");
            }

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

需要补充核心鉴权类关系,核心权限数据扭转关系(权限数据怎么加载到内存,修改变更怎么更新到每个server的)

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

namespaceAdminRoles 无用代码统一清理一下

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

权限数据需要做好缓存,不然每次都重新从数据库查询,代价比较大。 server数据可以跟数据库多好定期对账。

Copy link
Collaborator

@yanlinly yanlinly left a comment

Choose a reason for hiding this comment

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

密码传输过程中需要加密传输,看一下是否有好的加密算法可以解决这个问题

Copy link
Collaborator

@wfnuser wfnuser left a comment

Choose a reason for hiding this comment

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

Cool job! But the commits to review is toooooo much...

public Page<PermissionInfo> getPermissions(String role, int pageNo, int pageSize) {
PaginationHelper<PermissionInfo> helper = new PaginationHelper<>();

String sqlCountRows = "select count(*) from permissions where ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess use string builder is better?
And I think inserting 'where' in the front of String variable where instead of append 'where' to the end of string sqlFetchRows is better.

@nkorange nkorange merged commit 8273198 into alibaba:develop Jan 10, 2020
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.

3 participants