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

MetadataLoader - load from stream #736

Closed
wants to merge 3 commits into from
Closed

Conversation

twehner
Copy link

@twehner twehner commented May 26, 2017

Adds overloads to the MetadataLoader to load from a Stream directly for cases where metadata is not stored on the file system and not available online. This is of particular use in multi-tenant systems where users are able to configure an IdP or SP via a web application. Additionally, this makes it easier to use a caching layer when dealing with very large metadata files such as the InCommon federation at http://md.incommon.org/InCommon/InCommon-metadata.xml (~43 MB).

@AndersAbel
Copy link
Member

Sounds like a good idea. What's the steps required for someone that wants to hand in a stream for metadata instead of specifying a location?

@twehner
Copy link
Author

twehner commented May 26, 2017

For instance,

An application my team is working on is an SP multi-tenant system where the user-administrators add the IdP configuration via the UI. That configuration data is stored in our database. This means that we cannot preconfigure any federations or IdP's in the app/web.config. So, once they have provided a metadata url or pasted in raw metadata xml, we have to manually build up our "Options" structure in code. With only url support we build it up as so:

private Options GetFederationOptions(string federationUrl, SPOptions spOptions)
{
    var options = new Options(spOptions);

    var federationMetadata = MetadataLoader.LoadFederation(federationUrl);
    foreach (ExtendedEntityDescriptor entity in federationMetadata.ChildEntities)
    {
        var idp = new Kentor.AuthServices.IdentityProvider(entity.EntityId, spOptions);
        idp.ReadMetadata(entity);
        options.IdentityProviders.Add(idp);
    }

    return options;
}

We would like to instead pull the metadata from our database or caching layer and pass it into that method:

private Options GetFederationOptions(string federationMetadataXml, SPOptions spOptions)
{
    var options = new Options(spOptions);

    var xmlBytes = Encoding.UTF8.GetBytes(federationMetadataXml);

    using (var metadataStream = new MemoryStream(xmlBytes))
    {
        var federationMetadata = MetadataLoader.LoadFederation(federationMetadataXml);
        foreach (ExtendedEntityDescriptor entity in federationMetadata.ChildEntities)
        {
            var idp = new Kentor.AuthServices.IdentityProvider(entity.EntityId, spOptions);
            idp.ReadMetadata(entity);
            options.IdentityProviders.Add(idp);
        }
    }

    return options;
}

@AndersAbel
Copy link
Member

Looks like a reasonable use case. What do you think about adding a notification (a callback hook) in the existing Federation class instead so that you don't have to mimic the behaviour of adding idps? The existing code also removes idps no longer present in the federation (which your example code doesn't). With a hook you would be able to just provide your own metadata stream and then let the existing logic handle the rest.

@twehner
Copy link
Author

twehner commented Jul 12, 2017

That's a good idea. I've updated this PR to get the test coverage fixed (and resolve conflicts) for the basic stream support in the loader. I will work on the other side of this (with your suggestion) in another PR.

@geekster42
Copy link

Any idea when these changes might make it into a release? This is a change that will make this library much more useful for multi-tenant, load balanced applications...and I could really use it :-)

@ejsmith
Copy link

ejsmith commented Aug 7, 2017

Yes, I really need this change to be accepted as well.

@mdesousa
Copy link

This is a great feature! Looking forward to it 👍

@robertgins
Copy link

I notice this did not make it into V1, I am stuck using reflection to solve the issue (as I described in #988) seems like such a simple thing to adapt.

@AndersAbel
Copy link
Member

@robertgins v1.0.0 was basically a release/versioning thing to do. Mostly because I'm now working on a 2.0.0 which builds on the .Net Standard compatible Microsoft.IdentityModel packages (already merged into master). I have not forgotten/ignored this, but right now getting to .Net Standard is a blocker for many more people.

@robertgins
Copy link

Just as a side note, the only thing necessary to fit both scenarios is to make the static MetadataLoader.Load with the XmlDictionaryReader param public. I currently call it via reflection with a XmlDictionaryReader created from a MemoryStream.

@AndersAbel
Copy link
Member

This is one of all the things I would like to address in making the library more flexible, with more extension points. The entire IdentityProvider dictionary will probably be made replaceable. And the metadata source will be configurable as well. I'm keeping this PR open for now, but marking it for the v3.0.0 release.

@AndersAbel AndersAbel added this to the v3.0.0 Flexibility milestone Sep 28, 2018
@samjetski
Copy link

Hey, I just ran into a need to use this functionality. I've updated it with the latest changes to resolve conflicts as this PR has been stale for a bit.

https://github.com/samjetski/Saml2/tree/736_MetadataLoaderFromStream

@AndersAbel would you like this as a new PR? Otherwise I'm guessing @twehner would need to merge the changes into this one. Is there anything else I can do to help get this merged sooner?

@brendonparker
Copy link

I have the same scenario (multi-tenant app where some tenants use SSO; have a UI for configuration). Some tenant's IdP's don't provide a metadata url, just the raw xml (i.e. Okta).

My approach was to write the metadata xml to a temp file, and then set the Idp MetadataLocation to the temp file, using the file:// format. That satisfied the current code which uses System.Net.WebClient. A bit tricky, but no reflection or code changes needed.

@samjetski
Copy link

@brendonparker Nice workaround. You might be interested to know that Okta does actually publish a URL, though I've found many sysadmins don't seem to be aware of this and just blindly click the link.

  1. Right-click the Identity Provider metadata link and select Copy Link Address. This metadata link contains the information that you need to configure SAML in your SAML SP application.

https://developer.okta.com/docs/guides/saml-application-setup/config-saml-in-app/

@dahlsailrunner
Copy link

We have a direct need for this functionality and will either be forking the repo and making the change (to allow metadata via stream) or would be interested if this PR can be resolved and merged. Any thoughts on direction here?

@AndersAbel
Copy link
Member

This is something that would be a good fit for the rework being done as part of the v3 release. @dahlsailrunner do you need this for Asp.Net Core? Or for any other framework?

@dahlsailrunner
Copy link

We will be using it for ASP.NET Core. In fact, we will likely be submitting a PR against the v2 branch later today that uses the approach of encoding and zipping the content for small storage in a database. Once that is submitted I'll post a note / ref here. If you have any pre-emptive comments let me know. and thanks! :)

@dahlsailrunner
Copy link

PR #1225 has been submitted as a possible solution for this. It's ready to merge (or so it appears) against branch v2.

@AlexOliinyk1
Copy link

any updates here ?

@AndersAbel AndersAbel closed this Apr 11, 2022
@AndersAbel AndersAbel removed this from the v3.0.0 MVP milestone Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants