Rename Admin Role to SystemAdmin for Clarity #1054
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: The term "Admin" does not clearly convey its purpose as a system-wide administrator role, as discussed in [issue line#1048](line#1048). Renaming this role improves clarity for users and developers. Modifications: - Renamed all "Admin" to "SystemAdmin" across the codebase. - Updated `MetadataService.updateTokenLevel()` to avoid directly accessing the `admin` property. Result: - The role name "Admin" has been replaced with "SystemAdmin".
minwoox
commented
Nov 6, 2024
| @@ -86,7 +86,7 @@ public final class AuthConfig { | |||
| @JsonCreator | |||
| public AuthConfig( | |||
| @JsonProperty("factoryClassName") String factoryClassName, | |||
| @JsonProperty("administrators") @Nullable Set<String> administrators, | |||
| @JsonProperty("systemAdministrators") @Nullable Set<String> systemAdministrators, | |||
There was a problem hiding this comment.
I didn't implement this in a backward-compatible way because the configuration file raises an exception at startup. Please let me know if you have a different opinion on this. 🙇
jrhee17
approved these changes
Nov 8, 2024
| @@ -126,7 +126,7 @@ public final <T> CompletableFuture<T> execute(Command<T> command) { | |||
| if (!isStarted()) { | |||
| throw new ReadOnlyException("running in read-only mode. command: " + command); | |||
| } | |||
| if (!writable && !(command instanceof AdministrativeCommand)) { | |||
| if (!writable && !(command instanceof SystemAdministrativeCommand)) { | |||
There was a problem hiding this comment.
Double checked that the superclass [System]AdministrativeCommand isn't exposed in the JSON in any way
| */ | ||
| @JsonProperty | ||
| public boolean isAdmin() { | ||
| return isAdmin; | ||
| public boolean isSystemAdmin() { |
There was a problem hiding this comment.
Note:
- While releasing, if a user tries to access a token via UI served by old servers to a new server he/she may encounter errors.
- If a new token is registered while releasing, old servers may not be able to access it
- Regarding rollback plan: if a new token is registered we need to clean up the token before performing rollback. Having said this, I'm not sure if there is an easy way to detect this at the moment.
There was a problem hiding this comment.
While releasing, if a user tries to access a token via UI served by old servers to a new server he/she may encounter errors.
That's correct. 👍 We need to let them refresh the page after finishing the rolling restart.
If a new token is registered while releasing, old servers may not be able to access it
That's correct. 👍 I thought it should be okay, but it probably shouldn't be. 😓
Let me create a separate commit to solve this issue.
Regarding rollback plan:
I'm implementing a client that migrates admin to systemAdmin.
Let me also implement the client the vise-versa.
minwoox
added a commit
to minwoox/centraldogma
that referenced
this pull request
Nov 8, 2024
Motivation To ensure smooth compatibility with line#1054, the `Token` structure needs an update to support changes in `admin` property. Modifications: - Enhanced the Token deserializer to accept both `admin` and `systemAdmin` properties, ensuring backward compatibility. - Updated `MetadataService.updateTokenLevel()` to avoid direct access to the `admin` property. Result: - The `Token` structure now supports the upcoming changes, allowing seamless updates with line#1054.
minwoox
added a commit
that referenced
this pull request
Nov 8, 2024
Motivation To ensure smooth compatibility with #1054, the `Token` structure needs an update to support changes in `admin` property. Modifications: - Enhanced the Token deserializer to accept both `admin` and `systemAdmin` properties, ensuring backward compatibility. - Updated `MetadataService.updateTokenLevel()` to avoid direct access to the `admin` property. Result: - The `Token` structure now supports the upcoming changes, allowing seamless updates with #1054.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The term "Admin" does not clearly convey its purpose as a system-wide administrator role, as discussed in issue #1048. Renaming this role improves clarity for users and developers.
Modifications:
MetadataService.updateTokenLevel()to avoid directly accessing theadminproperty.Result:
authentication.administratorsin dogma.json is nowauthentication.systemAdministrators.systemAdminproperty instead ofadminwhen creating a token via REST API:{"appId": "foo", "isSystemAdmin": true, ...}SYSTEMADMINinstead ofadminwhen changing the token level:{"level":"SYSTEMADMIN"}