-
Notifications
You must be signed in to change notification settings - Fork 27
OIDC-243: API Access using Accesstoken #86
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
base: master
Are you sure you want to change the base?
Conversation
oidc-authenticator/src/main/java/org/xwiki/contrib/oidc/auth/OIDCAuthServiceImpl.java
Outdated
Show resolved
Hide resolved
|
Thanks for the contribution @ruediste! Would be better to:
(see https://dev.xwiki.org/xwiki/bin/view/Community/DevelopmentPractices fore more detail about all dev practices) |
...uthenticator/src/main/java/org/xwiki/contrib/oidc/auth/internal/OIDCClientConfiguration.java
Outdated
Show resolved
Hide resolved
| jwtClaimsSet = jwtProcessor.process(authorizationHeaderValue.substring("Bearer ".length()), null); | ||
|
|
||
| ClaimsSet claimsSet = new ClaimsSet(); | ||
| claimsSet.putAll(jwtClaimsSet.toJSONObject()); | ||
|
|
||
| UserInfo userInfo = new UserInfo(jwtClaimsSet); |
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 don't really understand your process here. You don't seem to ever validate the token on the provider and seems to expect it to be a complete (signed) user info, which is not really what an access token is supposed to be in OIDC AFAIK.
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.
As far as I understood the JWT Processor performs the validation against the JWKS set specified in the constructor.
The issue access token vs id token vs user info bothered me during implementation as well. I'd like not to request the full user info from the provider for each request. However, some caching could help here.
The UserInfo is required for the group checking. This could be dropped altogether, as an existing user should also have the required groups. Unless the groups are beeing changed after the user has been created.
The UserInfo is also required to determine he XWiki User ID from the token (via the substitutor). This part can obviously not be skipped.
What do you think, what is the best option?
- Keep as-is
- Drop the Group Check
- Use a cached User Info
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.
My worry is that your solution seems a bit limited and not very standard. Another problem is that since it's not really a token, and it's never validated, it's not possible to drop it, so if someone get a hand on this signed user info it will keep working more or less forever (unless you reset the provider's keys).
It feels much more standard to accept real tokens and validate them on the provider. On the performance side of things, I agree with you that re-doing the token validation on the provider and user authorization/membership sync with every HTTP request does not sound like the best idea. But having some caching, and then some background thread in charge of regularly re-validating the cached tokens and re-sync info (in case it's been invalidated, or some authorization related info changed on provider side) should do the job.
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 looked at the code again. To get full compatibility with the existing login logic, I propose the following approach:
- Send both an id and access token using
X-Id-TokenandX-Access-Tokenheaders - Validate the signature and audience of the Id token (like I did in this PR for the access token)
- Use the access token to retrieve the user info
What do you think?
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 implemented the mechanism above. Please review.
...uthenticator/src/main/java/org/xwiki/contrib/oidc/auth/internal/OIDCClientConfiguration.java
Show resolved
Hide resolved
| if (configuration.isAllowAccessToken()) { | ||
| HttpServletRequest request = context.getRequest().getHttpServletRequest(); | ||
| String idTokenHeader = request.getHeader("X-Id-Token"); | ||
| String accessTokenHeader = request.getHeader("X-Access-Token"); |
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.
Wouldn't it make more sense to receive the access token as a Bearer Authorization header ?
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.
That could certainly be done this way. But for the Authorization header I would expect that single header to be sufficient for authorization, thou we would have to combine the two tokens. And sending the id token is non-standard anyways. I would prefer to leave it as-is.
| String idTokenHeader = request.getHeader("X-Id-Token"); | ||
| String accessTokenHeader = request.getHeader("X-Access-Token"); | ||
|
|
||
| if (idTokenHeader != null && accessTokenHeader != null) { |
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.
Seems to me the ID token should be optional (I'm not even sure it's needed at all actually as there is probably a way to request an ID token from the provider, with the access token).
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 looked into this and have not found a way to get one.
oidc-authenticator/src/main/java/org/xwiki/contrib/oidc/auth/internal/OIDCUserManager.java
Outdated
Show resolved
Hide resolved
| { | ||
|
|
||
| @Override | ||
| public DocumentReference load(Pair<String, String> key) throws Exception |
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 unfortunately have no experience with this use case (and not much time to research it), which makes it hard to help you. That being said, my understanding is that this is supposed to be covered with the capacity of the /token endpoint to take in input a JWT Bearer token instead of the authorization code (that you get in a code flow). That way you end up with the ID token and a new access token that you can then use to request the UserInfo exactly like in the usual authentication (i.e. you can just reuse the same code that already exist after the response of the token endpoint).
See the following references:
- https://connect2id.com/products/server/docs/api/token#token-endpoint (JWT bearer assertion section)
- https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth2-on-behalf-of-flow which descibe this use case specifically for MS Entra ID (but based on the same enpoint)
- https://datatracker.ietf.org/doc/html/rfc7523#section-2.1 for the reference specification
Of course, if it becomes too time-consuming for you to try to find the right standard in that OIDC/OAuth2 jungle, an easier alternative for now would be to create your own custom authenticator extending OIDCAuthServiceImpl and adding that layer of token support. That way, you can take shortcuts that feat exactly your need and specific setup without the constraint of being generic enough for the standard OIDC Authenticator.
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 have studied the documentation and found the following: In the JWT Bearer Flow you obtain a special JWT that identifies you as a user. This special JWT is then sent to the idP to get an access token and id token.
For that to work the idP has to be configured accordingly, which is not done by default. Obviously, it has to be configured which JWTs the idP accepts. I tested with a keycloak, using an access token generated with the same keycloak instance, and it did not work out of the box.
Therefore I think this route is a dead end.
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.
Therefore I think this route is a dead end.
Well I would not call that a dead end in general since it does sounds like a good standard for that use case. But indeed if it's not properly supported by the provider you are relying on, it's not going to help you.
3c3e052 to
903114c
Compare
Add the ability to authenticate using an
Authorization: Bearer <access token>HTTP header.