Service Accounts - HLRC #72431
Service Accounts - HLRC #72431
Conversation
ywangd
added
>enhancement
:Security/Client
|
Pinging @elastic/es-security (Team:Security) |
|
@elasticmachine update branch |
| final AuthenticateResponse response = new AuthenticateResponse( | ||
| new User((String) a[0], ((List<String>) a[1]), (Map<String, Object>) a[2], | ||
| (String) a[3], (String) a[4]), (Boolean) a[5], (RealmInfo) a[6], (RealmInfo) a[7], (String) a[8], | ||
| Map.of(TOKEN_NAME_FIELD, ((Map<String, Object>) a[9]).get(NAME_FIELD))); |
There was a problem hiding this comment.
I don't think this is right. The JSON response exposes token as a field, so the Java response object should too.
There was a problem hiding this comment.
It's certainly weird. But the AuthenticateResponse class is pretty weird already. It's internal structure (fields) mimics the server side Authenticate, i.e. both have:
- User object
- Auth realm
- Lookup realm
- Auth type
But the issue is that server side Authenticate has a quite different serialisation (JSON) form than its internal structure. This means, the client side AuthenticateResponse needs to bridge this difference by declaring a parser that has the shape of the JSON form, but a constructor to restore the internal structure.
I was trying to fit my changes with a similar style. I agree it's awkward and will change it to be a top level token field. But we might want to revamp AuthenticateResponse so that all other fields are also aligned with the JSON response from the server side, instead of the backing class's internal strcuture.
...est-high-level/src/main/java/org/elasticsearch/client/security/support/ServiceTokenInfo.java
Show resolved
Hide resolved
...t-high-level/src/main/java/org/elasticsearch/client/security/support/ServiceAccountInfo.java
Show resolved
Hide resolved
...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java
Outdated
Show resolved
Hide resolved
|
Took me a while to get back to this. It's now ready for another look. Thanks! |
| @@ -114,24 +124,35 @@ public String getAuthenticationType() { | |||
| return authenticationType; | |||
| } | |||
|
|
|||
| public Map<String, Object> getToken() { | |||
There was a problem hiding this comment.
Should this be some new (nested?) class rather than a Map ?
I'm of 2 minds
- on one hand, it's hardly ever going to be used, so simple seems better
- but, really it should be a type like (almost) everything else in the HLRC.
There was a problem hiding this comment.
I just made the change to replace the Map with a nested TokenInfo class. But now I still think it might be better for it to be just a Map because this field is more of the metadata for the authentication type field. Also its own fields may not all exist or make sense for different token types if we decide to expand it to cover other types. For example, name does not apply to OAuth Tokens. So it needs to be declared as StringOrNull. Overall I think its simpler for it to be just a Map.
There was a problem hiding this comment.
In case it is not clear from the last comment: I am keeping the token field as a Map. The change is in another branch that I decided to not merge.
There was a problem hiding this comment.
Sorry to drag on this minor issue, but I decided to make one small change: When the response does not have a token field, instead of an empty Map, the code now represents it just as a null value. I think a null value is more accurate semantically. Also this would be the case if we replace the Map with a named class. So overall I feel null is better suited.
|
@elasticmachine run elasticsearch-ci/part-2 |
This PR adds corresponding components in High Level Rest Client for the new APIs related to the service accounts feature.
This PR adds corresponding components in High Level Rest Client for the new APIs related to the service accounts feature.