Add access control feature #133
Conversation
| * @param path the path of the file | ||
| * @param jsonPatchOperations the {@link JsonPatchOperation}s which would be applied | ||
| */ | ||
| static Change<JsonNode> ofJsonPatch(String path, JsonPatchOperation... jsonPatchOperations) { |
There was a problem hiding this comment.
JsonPatchOperation is an internal class and thus should not be exposed in our public API. How about moving this method under internal.jsonpatch?
| * | ||
| * LINE Corporation licenses this file to you under the Apache License, | ||
| * version 2.0 (the "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at: | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * http://www.apache.org/licenses/LICENSE-2.0 |
There was a problem hiding this comment.
https? Perhaps you need to update your copyright profile.
| } | ||
|
|
||
| final JsonNode found = node.at(path); | ||
| if (!found.isMissingNode()) { |
There was a problem hiding this comment.
How about returning earlier?
if (found.isMissingNode()) {
return node;
}
...| final String raw = path.last().getMatchingProperty(); | ||
| if (parentNode.isObject()) { | ||
| ((ObjectNode) parentNode).remove(raw); | ||
| } else { |
There was a problem hiding this comment.
How about:
else if (parentNode.isArray()) {
...
} else {
return node;
}| public final class TestAbsenceOperation extends JsonPatchOperation { | ||
| @JsonCreator | ||
| public TestAbsenceOperation(@JsonProperty("path") final JsonPointer path) { | ||
| super("testAbsent", path); |
| return serveMetaRepo(delegate, ctx, req, mds, user, projectName); | ||
| } | ||
|
|
||
| final CompletionStage<Collection<Permission>> f; |
There was a problem hiding this comment.
How about extracting the part after serveMetaRepo() to serveNonMetaRepo() so that it is clearer? e.g.
if (Project.REPO_META.equals(repoName)) {
return serveMetaRepo(delegate, ctx, req, mds, user, projectName);
} else {
return serveNonMetaRepo(delegate, ctx, req, mds, user, projectName);
}|
|
||
| static CompletableFuture<Revision> generateSampleFiles( | ||
| CommandExecutor executor, String projectName, String repositoryName) { | ||
|
|
||
| if (projectName.equals(INTERNAL_PROJECT_NAME)) { |
There was a problem hiding this comment.
How about adding a comment:
// Do not generate sample files for internal projects.| private static CompletableFuture<Revision> initializeMetadata(CommandExecutor executor, | ||
| String projectName, | ||
| Author author) { | ||
| if (projectName.equals(INTERNAL_PROJECT_NAME)) { |
| return CompletableFuture.completedFuture(Revision.INIT); | ||
| } | ||
|
|
||
| logger.info("Initializing metadata: {}/{}", projectName); |
There was a problem hiding this comment.
/{} seems unnecessary. (mismatching argument count)
| * under the License. | ||
| */ | ||
| /* | ||
| * Copyright (c) 2014, Francis Galiegue (fgaliegue@gmail.com) |
There was a problem hiding this comment.
You don't need this if you rewrote this class.
Codecov Report
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
+ Coverage 61.4% 63.12% +1.72%
==========================================
Files 261 272 +11
Lines 8837 9374 +537
Branches 1027 1059 +32
==========================================
+ Hits 5426 5917 +491
- Misses 2801 2834 +33
- Partials 610 623 +13
Continue to review full report at Codecov.
|
hyangtack
force-pushed
the
feature/add_access_control_rev2
branch
4 times, most recently
from
4255a84
to
679655d
Compare
| * <p>This operation only takes one pointer ({@code path}) as an argument. It | ||
| * is an error condition if no JSON value exists at that pointer.</p> | ||
| */ | ||
| public final class RemoveIfExistsOperation extends JsonPatchOperation { |
| import com.fasterxml.jackson.databind.SerializerProvider; | ||
| import com.fasterxml.jackson.databind.jsontype.TypeSerializer; | ||
|
|
||
| public final class TestAbsenceOperation extends JsonPatchOperation { |
| @@ -3,4 +3,4 @@ distributionPath=wrapper/dists | |||
| zipStoreBase=GRADLE_USER_HOME | |||
| zipStorePath=wrapper/dists | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-4.5.1-all.zip | |||
| distributionSha256Sum=ede2333b509ab67e4b6f51adb1419870fb34bbead0c8357dd803532b531508eb | |||
| distributionSha256Sum=ede2333b509ab67e4b6f51adb1419870fb34bbead0c8357dd803532b531508eb | |||
| * Configures the list of administrators. | ||
| */ | ||
| public CentralDogmaBuilder administrators(String... administrators) { | ||
| this.administrators = ImmutableSet.copyOf(administrators); |
There was a problem hiding this comment.
requireNonNull(administrators, "administrators")
| * Configures the list of administrators. | ||
| */ | ||
| public CentralDogmaBuilder administrators(Iterable<String> administrators) { | ||
| this.administrators = ImmutableSet.copyOf(administrators); |
There was a problem hiding this comment.
requireNonNull(administrators, "administrators")
| @@ -167,6 +168,10 @@ Core properties | |||
| this, Central Dogma will reject to mirror the Git repository. If ``null``, the default value of | |||
| '33554432 bytes' (32 MiB) is used. | |||
|
|
|||
| - ``administrators`` (set of string) | |||
|
|
|||
| - the set of administrator's ID. They are valid only if ``securityEnabled`` is ``true``. | |||
There was a problem hiding this comment.
the set of administrator's ID -> the user names (or ID) of administrators?
site/src/sphinx/auth.rst
Outdated
| } | ||
|
|
||
| Next, configure ``conf/shiro.ini`` based on your authentication system. A local database system of `Apache Shiro`_ | ||
| is used here to show you a simple example:: |
There was a problem hiding this comment.
How about mentioning that a user can also configure Shiro to enable authentication using LDAP or RDBMS as well?
There was a problem hiding this comment.
Added as a note
| Notification.error({ | ||
| message: message, | ||
| templateUrl: 'scripts/components/util/notification-template-with-login.html' | ||
| }); | ||
| } else if (arg.status === 403) { | ||
| // TODO(hyangtack) Refine the error message. | ||
| Notification.error('You have no permission'); |
There was a problem hiding this comment.
missing period? or Permission denied
| return defer.promise; | ||
| } | ||
| } | ||
| }); |
| <button class="btn btn-primary" | ||
| ng-click="modalOptions.ok();">{{ 'components.button_yes' | translate }} | ||
| </button> | ||
| </div> |
hyangtack
force-pushed
the
feature/add_access_control_rev2
branch
2 times, most recently
from
e57e13b
to
869e14e
Compare
|
@trustin I think I addressed every comment you mentioned. Please let me know if there is any comment I missed. Also, I fixed the followings:
|
| import com.linecorp.centraldogma.server.internal.metadata.Token; | ||
|
|
||
| /** | ||
| * A {@link User} which accesses the API with a secret. |
There was a problem hiding this comment.
secret is correct here because API requests will be sent with a secret and a token will be found by the secret.
| } | ||
|
|
||
| /** | ||
| * PUT /metadata/{projectName}/tokens |
| /** | ||
| * PUT /metadata/{projectName}/tokens | ||
| * | ||
| * <p>Updates the {@link PerRolePermissions} of the specified {@code projectName} and {@code repoName}. |
| /** | ||
| * POST /metadata/{projectName}/repos/{repoName}/perm/users | ||
| * | ||
| * <p>Adds {@link Permission}s of the specific users to the specified {@code projectName} and |
| } | ||
|
|
||
| /** | ||
| * PUT /metadata/{projectName}/repos/{repoName}/perm/users |
| } | ||
|
|
||
| /** | ||
| * PUT /metadata/{projectName}/repos/{repoName}/perm/tokens |
| * | ||
| * <p>Updates the {@link PerRolePermissions} of the specified {@code projectName} and {@code repoName}. | ||
| */ | ||
| @Put("/metadata/{projectName}/repos/{repoName}/perm/role") |
There was a problem hiding this comment.
Put method without the identifier at the end looks weird to me...
How about @Put("/metadata/{projectName}/rolePermission/repos/{repoName}")?
I think this is updating the rolePermission of the specific repository, so repoName should be at the last.
Also I think we do not have to define the path as same with the API updating perUserPermissions and perTokenPermissions since this has no identifier(Updating the whole permission).
WDYT?
Please, forget about the comment. How about just changing to POST?
There was a problem hiding this comment.
I agree with you. I also think POST is more reasonable. Thanks!
| public CompletionStage<ProjectInfo> removeToken(@Param("projectName") String projectName, | ||
| @Param("appId") String appId, | ||
| @Patch("/projects/{projectName}") | ||
| @Decorator(AdministratorsOnly.class) |
There was a problem hiding this comment.
If the owner of a project removes the project by mistake, does he/she have to ask the administrator to bring it back?
There was a problem hiding this comment.
That's a good question. I wonder if the project owner is allowed to unremove his or her project.
There was a problem hiding this comment.
As we discussed in LINE chat room, it'd be better to leave this as it is. A metadata is stored in a meta repository of each project so the metadata is not able to be accessed if a project is removed, by default. That means we have no chance to find a role of a user in our authentication decorator, for now. If we want to read a metadata from a removed project, we need to make some code more, but it's not an important specification now.
| @RequestObject Author author, | ||
| @RequestObject User loginUser) { | ||
| return getTokenOrRespondForbidden(appId, loginUser).thenCompose( | ||
| token -> mds.destroyToken(author, appId) |
There was a problem hiding this comment.
just wondering is there any reason that you used destroy instead of delete?
There was a problem hiding this comment.
removeToken() removes a token from a project. And destroyToken() removes a token from the system completely (removing it from every project and the system). So I want to make its meaning clear by using destroy instead of delete.
| if (isValidEmailAddress(emailAddr)) { | ||
| return emailAddr; | ||
| } | ||
| return emailAddr + "@localhost.localdomain"; |
There was a problem hiding this comment.
Need to raise an exception if the result is not a valid email address yet.
There was a problem hiding this comment.
I think I decided bad name for this method, so I changed it to toEmailAddress which means converting an argument emailAddr to an email address if it's not.
| return emailAddr + "@localhost.localdomain"; | ||
| } | ||
|
|
||
| public static String parseLogin(String emailAddr, String paramName) { |
There was a problem hiding this comment.
How about emailToUsername? It doesn't seem to parse but just extract in my opinion.
| @@ -66,6 +66,10 @@ | |||
| this.value = value.deepCopy(); | |||
| } | |||
|
|
|||
| public JsonNode value() { | |||
There was a problem hiding this comment.
It's used in the MetadataApiService like the following:
final ProjectRole role = ProjectRole.of(operation.value());
| /** | ||
| * POST /projects/{projectName}/members/{user} | ||
| * GET /projects/{projectName}/accessibility |
There was a problem hiding this comment.
The term 'accessibility' is often used for disabled people. Do we have a better term here?
There was a problem hiding this comment.
I will remove this API, then add checkPermissionOnly=true parameter to GET /projects/{projectName} API.
| public CompletionStage<ProjectInfo> removeToken(@Param("projectName") String projectName, | ||
| @Param("appId") String appId, | ||
| @Patch("/projects/{projectName}") | ||
| @Decorator(AdministratorsOnly.class) |
There was a problem hiding this comment.
That's a good question. I wonder if the project owner is allowed to unremove his or her project.
| @@ -75,6 +77,10 @@ public HttpResponse handleException(RequestContext ctx, HttpRequest req, Throwab | |||
| return HttpResponse.of(HttpStatus.BAD_REQUEST); | |||
| } | |||
|
|
|||
| if (cause instanceof IllegalAccessException) { | |||
| return HttpResponse.of(HttpStatus.FORBIDDEN); | |||
There was a problem hiding this comment.
This seems unnecessary now that we throw an HttpStatusException?
| */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| @JsonInclude(Include.NON_NULL) | ||
| public class Repo implements Identifiable { |
There was a problem hiding this comment.
Ok, it seems more consistent.
| return future; | ||
| } | ||
|
|
||
| private void push0(String projectName, String repoName, Author author, String commitSummary, |
| */ | ||
|
|
||
| /** | ||
| * Metadata management for projects in the Central Dogma. |
site/src/sphinx/auth.rst
Outdated
| Authentication and Access Control | ||
| ================================= | ||
|
|
||
| *Central Dogma* provides the features for authentication and access control. |
site/src/sphinx/auth.rst
Outdated
|
|
||
| .. note:: | ||
|
|
||
| `Apache Shiro`_ supports RDBMS or LDAP based security system as well. You can find the example configuration |
site/src/sphinx/auth.rst
Outdated
| Access Control | ||
| -------------- | ||
|
|
||
| One of the way of configuring the access control system for *Central Dogma* is setting a project with a web UI. |
There was a problem hiding this comment.
- One way to configure
- setting a project with a web UI -> to use the web UI?
site/src/sphinx/auth.rst
Outdated
| -------------- | ||
|
|
||
| One of the way of configuring the access control system for *Central Dogma* is setting a project with a web UI. | ||
| To open the web UI, you may access ``http://{your-central-dogma-domain-or-ip}:36462`` by your web browser. |
site/src/sphinx/auth.rst
Outdated
|
|
||
| One of the way of configuring the access control system for *Central Dogma* is setting a project with a web UI. | ||
| To open the web UI, you may access ``http://{your-central-dogma-domain-or-ip}:36462`` by your web browser. | ||
| You may configure a project with HTTP APIs, but we recommend the web UI because it is easier and simple. |
site/src/sphinx/auth.rst
Outdated
| Application Token | ||
| ^^^^^^^^^^^^^^^^^ | ||
|
|
||
| When a user uses the web UI, he or she should login first to get a token for the web session. But what should a user |
There was a problem hiding this comment.
the web session -> a new web session
site/src/sphinx/auth.rst
Outdated
| ^^^^^^^^^^^^^^^^^ | ||
|
|
||
| When a user uses the web UI, he or she should login first to get a token for the web session. But what should a user | ||
| do when he or she uses one of *Central Dogma* clients? The user may login *Central Dogma* server through HTTP login API |
There was a problem hiding this comment.
login... -> log in to a Central Dogma server via HTTP API
site/src/sphinx/auth.rst
Outdated
|
|
||
| ``Application Token`` is like a virtual user, so it can have any role in a project. Also, its permissions can be specified | ||
| in a repository configuration like a member. To get a new token, a user can use ``Application Tokens`` menu of the web UI. | ||
| ``Application ID`` is should be unique to identify where the request is from. |
There was a problem hiding this comment.
- is should be -> has to be
- the request is from -> a client request comes from
site/src/sphinx/auth.rst
Outdated
|
|
||
| .. image:: _images/auth_4.png | ||
|
|
||
| Anyone who is logged in the *Central Dogma* can create a new ``Application Token``, and the token is shared |
site/src/sphinx/auth.rst
Outdated
|
|
||
| Anyone who is logged in the *Central Dogma* can create a new ``Application Token``, and the token is shared | ||
| for everyone. So any owner of a project can add any token to their project. However only both the token creator | ||
| and the administrator are allowed to inactivate and/or remove the token. |
site/src/sphinx/auth.rst
Outdated
| `Apache Shiro`_ supports RDBMS or LDAP based security system as well. You can find the example | ||
| configuration files under the ``conf/`` directory in the distribution. | ||
|
|
||
| That's all. You are ready to use the security system of Central Dogma. |
site/src/sphinx/auth.rst
Outdated
| - the administrator of a project. A user who creates a project is to be an owner of the project by | ||
| default. Owners can add a user or a token as an owner or a member of the project, and can create | ||
| a new repository. Also, they can remove the repository or the project from the system and can | ||
| configure permissions for each role, a member and a token. |
There was a problem hiding this comment.
each role, member and token. (I think each can modifies the three nouns in the sentence.)
site/src/sphinx/auth.rst
Outdated
|
|
||
| - ``Guest`` of a project | ||
|
|
||
| - users who is logged in but is neither an owner nor a member of a project. A guest is not an administrator, |
There was a problem hiding this comment.
How about removing the sentence A guest is not an administrator?
| .. note:: | ||
|
|
||
| Do not forget to make a new ``Application Token`` before adding a token to a project. ``Add a token`` | ||
| button would be disabled if there is no token. The pencil icon on the right of the ``Tokens`` title |
There was a problem hiding this comment.
It's a title in a web UI.
site/src/sphinx/auth.rst
Outdated
|
|
||
| Do not forget to make a new ``Application Token`` before adding a token to a project. ``Add a token`` | ||
| button would be disabled if there is no token. The pencil icon on the right of the ``Tokens`` title | ||
| brings you the ``Application Token`` management page. |
|
|
||
| Permissions can be specified for a repository only. So a user can configure their repositories with different | ||
| access control levels. There are only two permission types currently, which are ``READ`` and ``WRITE``. | ||
| ``WRITE`` permission implies ``READ`` permission, so you cannot give only WRITE permission to a user, |
There was a problem hiding this comment.
How about just using contains instead of implies? I feel weird when I read the sentence.
There was a problem hiding this comment.
Left this as it is because we discussed about this last week.
site/src/sphinx/auth.rst
Outdated
|
|
||
| When a user uses the web UI, he or she should login first to get a token for a new web session. But what | ||
| should a user do when he or she uses one of Central Dogma clients? The user may log in to a Central Dogma | ||
| server via HTTP API and gets a session token. But it is inconvenient and the user may write more complicated |
There was a problem hiding this comment.
gets -> get? (because may is auxiliary verb here)
Modifications: - Do access control for every v1 APIs. - Update metadata when do a CRUD operation for a project. - Build metadata if no metadata exists. - Administrator IDs can be specified in the configuration. - Add web UI for metadata management. To-Dos: - Add more test cases and cleanup test cases. - Documentation (site)
hyangtack
force-pushed
the
feature/add_access_control_rev2
branch
from
1451a67
to
d8874e8
Compare
hyangtack
changed the title
|
Thanks, reviewers! |
Motivation: I found out `MigrationUtil` is not used anymore that is intruced token migration in line#133. Modifications: - Remove `MigrationUtil` - Move initialization logic to `ProjectInitializer` - Clean up some logs in `ZooKeeperCommandExecutor` Result: - Less code
* Remove MigrationUtil Motivation: I found out `MigrationUtil` is not used anymore which is introduced token migration in #133. Modifications: - Remove `MigrationUtil` - Move initialization logic to `ProjectInitializer` - Clean up some logs in `ZooKeeperCommandExecutor` Result: - Less code
Modifications:
To-Dos: