Skip to content

Indicates that SaveTokens is not supported for WsFederation #10763

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

Merged
merged 2 commits into from
Jun 7, 2019

Conversation

Kahbazi
Copy link
Member

@Kahbazi Kahbazi commented Jun 3, 2019

@Tratcher
Copy link
Member

Tratcher commented Jun 3, 2019

Does this really prevent the base class intellisense from showing up?

The reference assemblies need to be updated because you added public API.

error : Generated code is not up to date in src/Security/Authentication/WsFederation/ref/Microsoft.AspNetCore.Authentication.WsFederation.netcoreapp3.0.cs. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md ...)

@analogrelay
Copy link
Contributor

Does this really prevent the base class intellisense from showing up?

I believe it will as long as the variable you are dereferencing is actually of the derived type (WsFederationOptions)

@Kahbazi could you verify this and post a quick screenshot?

@analogrelay analogrelay added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 3, 2019
@analogrelay analogrelay added this to the 3.0.0-preview7 milestone Jun 3, 2019
@Kahbazi
Copy link
Member Author

Kahbazi commented Jun 3, 2019

It's working!
image

@Tratcher
Copy link
Member

Tratcher commented Jun 3, 2019

Nice. And if you type in SaveTokens manually and hover over it does the right test come up?

@Kahbazi
Copy link
Member Author

Kahbazi commented Jun 3, 2019

yep
image

Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Very nice! Thanks @Kahbazi ! We should be able to take it from here.

@kevinchalet
Copy link
Contributor

Why not implementing proper SaveTokens support rather than hiding the config' property? 😄

@Tratcher
Copy link
Member

Tratcher commented Jun 4, 2019

@PinpointTownes What would you save? I was under the impression that the wsfed token wasn't useful beyond sign-in.

@analogrelay
Copy link
Contributor

@aspnet-hello
Copy link

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2620

@kevinchalet
Copy link
Contributor

@PinpointTownes What would you save?

The RequestedSecurityToken returned as part of the wresult parameter (aka the WS-Fed token 😄).

I was under the impression that the wsfed token wasn't useful beyond sign-in.

Keeping the WS-Fed token is definitely useful for delegation or impersonation scenarios (aka ActAs and OnBehalfOf), where the client will want to "exchange" the initial WS-Fed token with a token it can use itself to access protected resources (typically a WCF service or a REST API).

In the old world, this was achieved by setting saveBootstrapContext to true in the WIF options, so that you could access the token via ClaimsIdentity.BootstrapContext and use things like CreateChannelWithActAsToken or CreateChannelWithOnBehalfOfToken with WCF (or WSTrustChannelFactory manually if you were brave 🤣).

In the new world, it would make sense to store the WS-Fed token as an authentication property, exactly like what the OIDC/OAuth handlers do with access, identity and refresh tokens.

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2019

@PinpointTownes Reading through the prior thread (aspnet/Security#1716 (comment)) it ended because the tokens were too large and caused the cookie to hit header size limits.

I'm merging this change for now and we can revisit if there's enough interest in those tokens.

@Tratcher Tratcher merged commit 1b5db12 into dotnet:master Jun 7, 2019
@Kahbazi Kahbazi deleted the patch-4 branch June 7, 2019 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants