Skip to content
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

Addition of form_post OIDC response mode #2818

Merged
merged 7 commits into from May 12, 2022
Merged

Addition of form_post OIDC response mode #2818

merged 7 commits into from May 12, 2022

Conversation

scheibling
Copy link
Contributor

Details

Resolves #2748

Changes

New Features

  • Adds support for form_post response mode, required for Azure-compatibility

Breaking Changes

None

Additional Details

@scheibling
Copy link
Contributor Author

scheibling commented May 6, 2022

@BeryJu Right, it's working now at least!

I added a separate class for the post response, and I've imported the AutosubmitChallenge and AutoSubmitChallengeResponse classes from providers.saml.views.flows for now, I'm going to do some testing with this setup and see whether I can find any shortcomings.

@@ -238,6 +256,131 @@ def create_code(self, request: HttpRequest) -> AuthorizationCode:

return code

class OAuthPostFulfillmentStage(ChallengeStageView):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this extra stage view shouldn't be required, imo it makes more sense to hook in here https://github.com/goauthentik/authentik/blob/master/authentik/providers/oauth2/views/authorize.py#L248, rename redirect to get_response and have it check the parameters to either return an Autosubmit Challenge or a redirect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree, this was just a way of testing the concept on my part for now. I'll have a look at integrating it a bit better later today.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, had some more free time this afternoon. I've removed the separate class and moved the FORM_POST handling to create_response_uri() and redirect() respectively

I'll have to spend some more time going through a couple of more scenarios where form_post might want a non-implicit response, I've only been testing with an application of ours that has a very specific capability for OIDC (Azure) so far.

https://github.com/scheibling/authentik/blob/b7fd2b38935ed63a2bd1214f6c41d6ac2e6485b9/authentik/providers/oauth2/views/authorize.py#L265-L282

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at it with a little more sleep behind me, I'm going to work on the create_response_uri-part a bit more since the current code doesn't really take any of the response_type parameters into account.

Form_post in particular can be requested with just the code and id_token parts by themselves, I'm gonna read up a bit on that part of the protocol when I get into work tomorrow.

The original version was written as if fragment always just creates an implicit response with just an id_token and query only provides a code, couldn't there be other combinations on those as well?

I'll be back with an update once I'm up to speed with the latest!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, apparently the code that's already in create_implicit_response works fine for form_post as well!

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #2818 (df2ebed) into master (f00657f) will decrease coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #2818      +/-   ##
==========================================
- Coverage   91.73%   91.72%   -0.01%     
==========================================
  Files         455      455              
  Lines       20039    20049      +10     
==========================================
+ Hits        18381    18388       +7     
- Misses       1658     1661       +3     
Impacted Files Coverage Δ
authentik/providers/oauth2/views/authorize.py 80.90% <57.90%> (-2.04%) ⬇️
authentik/providers/oauth2/models.py 93.73% <100.00%> (+0.03%) ⬆️
authentik/events/models.py 82.45% <0.00%> (-0.38%) ⬇️
authentik/admin/tasks.py 100.00% <0.00%> (+9.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f00657f...df2ebed. Read the comment docs.

scheibling and others added 3 commits May 6, 2022 15:48
Added handling for FORM_POST in create_response_uri
Added handling for FORM_POST in return class
@scheibling scheibling marked this pull request as ready for review May 8, 2022 08:46
@BeryJu BeryJu merged commit d4abf56 into goauthentik:master May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Support for response_mode: form_post
2 participants