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

This should not try to configure 'everything' especially setting Cookie Authentication #809

Closed
Shazwazza opened this issue Dec 4, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request fixed
Milestone

Comments

@Shazwazza
Copy link
Contributor

Which version of Microsoft Identity Web are you using?
1.3.0

Where is the issue?

  • Other (please describe)

It's not possible to disable this library from adding cookie authentication which is problematic if you already have it. This library seems to be trying to do 'everything' which I understand can mean simplicity for some users but it reduces the flexibility. The work around for now is to pass in a "Fake" cookie scheme that doesn't exist, else you will get exceptions if you pass in an existing one since it already exists.

It would be nice to split this method up with additional public methods for more flexibility. See here

it's doing all sorts of stuff. Ideally there would just be a method to configure the OpenIdConnect options here

Is this a new or an existing app?
c. This is a new app or an experiment.

Expected behavior
Be able to add/configure the openidconnect options without configuring a bunch of unwanted or already existing services.

Actual behavior
There is not an option to do this

@Shazwazza
Copy link
Contributor Author

I'm happy to create a PR for this

@jmprieur
Copy link
Collaborator

jmprieur commented Dec 4, 2020

@Shazwazza
Thanks for the feedback. This is also to support samesite workarounds ... which are a bit of a pain to develop.

Yes, you could provide an experimental PR so that we understand your proposed design. We'd like to avoid breaking changes if possible, and also we want simples scenarios to remain simple, and complex ones to be possible.
Would that work for you?

On the other hand if you have very advanced needs, and know what you are doing, you might not even need to use AddMicrosoftIdentityWebApp. you could iinitialize OpenIdConnect yourself? (with builder.AddOpenIdConnect).

What is your scenario?
What part of Microsoft.Identity.Web do you want to leverage, and which don't you want?

@jmprieur jmprieur added enhancement New feature or request more info needed labels Dec 4, 2020
@jennyf19
Copy link
Collaborator

jennyf19 commented Dec 6, 2020

@Shazwazza do you mind trying this branch, which does offers the possibility of not defining the cookie scheme, if you do something like this:

services.AddAuthentication().AddMicrosoftIdentityWebApp(Configuration, cookieScheme: null);

cc: @jmprieur

@Shazwazza
Copy link
Contributor Author

Hi thanks for the replies.

@jennyf19 that branch you mention would be fine to have the possibility of passing in null instead of something fake, and then people can use post configure options to tweak any OpenIdConnect options created 👍

@jmprieur initializing OpenIdConnect manually for Azure AD (and b2c) is something I'd like to avoid where possible since it's quite a lot of code to maintain. It's something I previously had to do before netcore which ended up being a bit of a 'moving target'. I guess what might be ideal is to have public method that registers these OpenIdConnect options/code here https://github.com/AzureAD/microsoft-identity-web/blob/jennyf/cookies/src/Microsoft.Identity.Web/WebAppExtensions/MicrosoftIdentityWebAppAuthenticationBuilderExtensions.cs#L258-L366 which could then be tweaked if necessary in a post configure options callback. Though being able to pass in null for the cookie scheme is almost the same thing

What part of Microsoft.Identity.Web do you want to leverage, and which don't you want?

We're building the netcore version of the Umbraco CMS and just like in the versions previous to netcore we have the ability for people to install and configure external login providers for the back office. All of the auth is pre-configured in Umbraco so the external logins are just extensions to the auth that is already there.

@jmprieur
Copy link
Collaborator

jmprieur commented Dec 7, 2020

Thanks @Shazwazza

@jennyf19 that branch you mention would be fine to have the possibility of passing in null instead of something fake, and then people can use post configure options to tweak any OpenIdConnect options created 👍

passing null to which parameter, @Shazwazza ? In that branch you can pass null to the cookieScheme. For which other parameter would you like to pass-in null?

@Shazwazza
Copy link
Contributor Author

Sorry I should have been more clear, what @jennyf19 mentioned with this code in that branch:

services.AddAuthentication().AddMicrosoftIdentityWebApp(Configuration, cookieScheme: null);

Allowing passing null for cookieScheme is good and better than having to pass in a fake or unused cookie scheme.

@jennyf19
Copy link
Collaborator

jennyf19 commented Dec 7, 2020

thanks for the quick reply and confirmation @Shazwazza we'll have this fix out this week in 1.4.

@jennyf19 jennyf19 self-assigned this Dec 7, 2020
@jennyf19 jennyf19 added this to the 1.4.1 milestone Dec 7, 2020
@jennyf19 jennyf19 added the fixed label Dec 7, 2020
@jennyf19 jennyf19 modified the milestones: 1.4.1, 1.4.0 Dec 9, 2020
@jennyf19
Copy link
Collaborator

jennyf19 commented Dec 9, 2020

Included in 1.4 Release.

@jennyf19 jennyf19 closed this as completed Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed
Projects
None yet
Development

No branches or pull requests

3 participants