-
Notifications
You must be signed in to change notification settings - Fork 122
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
Prepare Token Updates for #1054 Compatibility #1056
Conversation
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.
throw new IllegalArgumentException( | ||
"The token is already " + | ||
(toBeAdmin ? "admin" : "user")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question) The serialized format of tokens.json
seems the same. Did you change to validate this? I am asking because this could override the previous state when multiple updates are performed. For example, one deprecates a token, and another makes this token admin. This may not happen in the real world as the permission is only granted to admins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serialized format of tokens.json seems the same.
That is true except the token now deserializes from both: {"appId": "..", "admin": true, ...}
and {"appId": "..", "systemAdmin": true, ...}
. This is the preliminary work for #1054.
Let's imagine that we rolling-update with #1054 without this PR. When a new structured token (with systemAdmin
) is committed, the old servers will fail to read the token because it cannot recognize systemAdmin
property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that the JSON pointer to /appIds/{appId}/admin
should be /appIds/{appId}/systemAdmin
when #1054 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR gets merged, the JSON pointer does not point /appIds/{appId}/admin
anymore. It replaces the whole Token
at /appIds/{appId}
. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍
Motivation
To ensure smooth compatibility with #1054, the
Token
structure needs an update to support changes inadmin
property.Modifications:
admin
andsystemAdmin
properties, ensuring backward compatibility.MetadataService.updateTokenLevel()
to avoid direct access to theadmin
property.Result:
Token
structure now supports the upcoming changes, allowing seamless updates with Rename Admin Role to SystemAdmin for Clarity #1054.