Changes required to support JWT tokens (CADC-14107) #269
Conversation
| @@ -248,6 +248,25 @@ private static StringBuilder getContent(SignedToken token) { | |||
| return sb; | |||
| } | |||
|
|
|||
| /** | |||
| * Checks whether a token is a CADC access token or not | |||
There was a problem hiding this comment.
"a CADC access token" -> SignedToken
also, Base64.decodeString(String) exists and could be used instead of new String... it's the same code but intended for this use case
should be catch(IllegalArgumentException ignore)
Q. Why does this handle plain text and base64-encoded test? And why is the condition different in those two cases? Aren't all SignedToken values in headers base64 encoded?
| @@ -149,11 +149,14 @@ public static Subject validateTokens(Subject subject) throws NotAuthenticatedExc | |||
| log.debug("credentials: " + credentials); | |||
|
|
|||
| try { | |||
| if (!SignedToken.isSignedToken(credentials)) { | |||
There was a problem hiding this comment.
The logic in validateTokens could be simplified:
- remove the initial
if(AuthenticationUtil.TOKEN_TYPE_CADCbit entirely (and remove the deprecated constants in AuthenticationUtil and all use of them) - the
if (AuthenticationUtil.AUTHORIZATION_HEADERpart of where we look for SignedTokens, but now that we're only looking for one form, I would consolidate this with the subsequentiflike this:
if (AuthenticationUtil.AUTHORIZATION_HEADER.equals(p.getHeaderKey())) {
// split on space(s) and trim
String challenge = ...
String credentials = ...
if (SignedToken.isSignedToken(credentials) {
SignedToken validatedToken = SignedToken.parse(credentials);
...
} // else: some other kind of bearer token: leave AuthorizationTokenPrincipal for additional processing
}
| @@ -776,6 +776,9 @@ public static String getPrincipalType(Principal userID) { | |||
| if (userID instanceof X500Principal) { | |||
| return IdentityType.X500.getValue().toLowerCase(); | |||
| } | |||
| if (userID instanceof OpenIdPrincipal) { | |||
| return ((OpenIdPrincipal)userID).getIssuer().toExternalForm(); | |||
There was a problem hiding this comment.
I recall discussing the API to /ac/users/{value}?idType={type} and that we could do this for OpenID:
/ac/users/{sub}?iss={issuer}
So I don't see why we want to have this misleading/incorrect treatment of the issuer as a type???
the AC specific code (ACIdentityManager only?) that knows about this should treat OpenIDPrincipal explicitly and call getIssuer() itself to match the query param is is going to use... or is there some reason that you think that API should really use type as the param?
Used in opencadc/ac#179