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

Adding a new filter for the auth token #169

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ryanwelcher
Copy link
Contributor

Being able to filter the auth token provides the ability to authenticate against a different AAD application.

@psignoret
Copy link
Owner

I'd like to better understand how this would be used. Can you give me an example scenario where this would be useful?

@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented Feb 14, 2018

@psignoret absolutely.

In our use-case, we have a separate AAD instance that allows us to log in as Vendors. To make this work, we had to unhook the built-in authenticate method and add our own that did the same but if there was no access_token received, we then authenticated against our AAD instance.

Clearly, this is not a scalable solution as we need to copy/paste any changes in the plugin into our custom authenticate method.

With this filter in place, we can simply do the following without the overhead of maintaining the custom version of the authenticate method:

function filter_add_sso_auth_token( $token, $code, $aad_sso_settings ) {
	if ( ! isset( $token->access_token ) ) {
		$aadsso_options = get_option( 'aadsso_settings' );

		if ( isset( $aadsso_options['client_id_vendor'] ) && isset( $aadsso_options['client_secret_vendor'] ) ) {
			$aad_sso_settings->client_id     = $aadsso_options['client_id_vendor'];
			$aad_sso_settings->client_secret = $aadsso_options['client_secret_vendor'];

			$token = \AADSSO_AuthorizationHelper::get_access_token( sanitize_text_field( $code ), $aad_sso_settings );
		}
	}
	return $token;
}

** Note that we have added *_vendor option to the admin **

@psignoret
Copy link
Owner

So, taking a step back from the implementation, your goal is for users from more than one organization to be able to sign in to the block, right? Perhaps two specific organizations, rather than any organization?

I'm not a fan of this approach (which relies on failed token requests), and would prefer to address this scenario intentionally.

@ryanwelcher
Copy link
Contributor Author

Perhaps we can filter the stored credentials instead?

@psignoret
Copy link
Owner

Just filtering the credentials shouldn't be enough, unless you have a rather strange app setup. Can you share how you've set up the app in the different Azure AD tenants?

Normally, this would be addressed in one of two ways:

  1. As a single-tenant app using the tenant-specific endpoint, with guest users. The app registration in Azure AD is set to "single-tenant" (the default), and the plugin is configured to use the tenant-specific endpoint (https://login.microsoftonline.com/{domain-name-or-tenant-id}/...). Users from other organizations who need to be given access to the site/blog are invited into the "host" Azure AD tenant as B2B guest users. This is generally the most appropriate approach for a group of vendors needing access.
  2. As a multi-tenant app using the common endpoint. User sign in from their home tenant, in the context of their home tenant, consent to the app and sign in. The app registration would be set to "multi-tenant", and the plugin is configured to use the common endpoint (https://login.microsoftonline.com/common/...).

To me it sounds like your scenario is probably best suited for the first case (single-tenant app with B2B guest users). Though it technically works, I would say this plugin is not currently well-suited for a multi-tenant approach (e.g. there would need to be tenant whitelisting, per-tenant access groups, an easy path for admin consent, etc.).

Since I suspect this may get into details you may not want to share publicly, can you please send me an email to philippe.signoret@outlook.com, and we can discuss this further there? (We'll report back to this thread once we agree on the best approach.)

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.

None yet

2 participants