SAP / cloud-security-services-integration-library

Integration libraries and samples for authenticating users and clients bound to XSUAA authentication and authorization service or Identity authentication service.
Apache License 2.0
151 stars 136 forks source link

Allow to remove more than one prefix from global scope names #304

Closed palianytsia closed 4 years ago

palianytsia commented 4 years ago

The premise is as following:

It would simplify the development of authorization checks if we could program them against the local scopes independent of where they are coming from, so ideally such setup of ResourceServerTokenServices

@Bean
public ResourceServerTokenServices sapResourceServerTokenServices() {
    OAuth2ServiceConfiguration applicationXsuaaConfig = 
        CFEnvironment.getInstance().loadForServicePlan(Service.XSUAA, CFConstants.Plan.APPLICATION);
    OAuth2ServiceConfiguration brokerXsuaaConfig =
        CFEnvironment.getInstance().loadForServicePlan(Service.XSUAA, CFConstants.Plan.BROKER);
    return new SAPOfflineTokenServicesCloud(applicationXsuaaConfig).withAnotherServiceConfiguration(
        brokerXsuaaConfig).setLocalScopeAsAuthorities(true);
}

would strip the application instance xsappname from scopes if token is coming from its clients and broker instance xsappname if tokens are retrieved via broker clones. (Right now such configuration will drop all the scopes from a token coming from the broker instance).

Alternatively I'd ask you to simply expose a setter for xsuaaScopeConverter, so we don't have to code something as fragile as:

String appId = Environments.getCurrent().getXsuaaConfiguration().getProperty(APP_ID);
String relaxedAppId = appId.replaceAll("!.", "![tb]");
Field field = ReflectionUtils.findField(SAPOfflineTokenServicesCloud.class, "xsuaaScopeConverter");
ReflectionUtils.makeAccessible(field);
ReflectionUtils.setField(field, sapOfflineTokenServicesCloud, new XsuaaScopeConverter(relaxedAppId));

Thanks, Ivan

nenaraab commented 4 years ago

hi @palianytsia

thanks for your request.

In regard to your idea. I had similar ideas in context of the spring-xsuaa client lib. I thought about a generic solution to allow the configuration of another appId in case the app is bound to multiple xsuaa instances. BUT: what if the same scope "DISPLAY" is exposed by multiple applications like in your sample? And what if this is not desired / its not semantically the same? I even can not check that (because sometimes not all scopes are provided via the token, because it was filtered by the application during token exchange or because the business user has only display permissions in context of one of the xsappname!t101.DISPLAY BUT NOT xsappname!b101.DISPLAY).

If you're not live with customers you may like to get rid of the Xsuaa of plan application and solely use the (unified) xsuaa instance of plan broker.

In regard to your second idea... if you like, you can contribute here... to set the ScopeConverter (https://github.com/SAP/cloud-security-xsuaa-integration/blob/master/java-security/src/main/java/com/sap/cloud/security/token/XsuaaScopeConverter.java) in context of the SAPOfflineTokenServicesCloud. Then it is in the responsibility of the application. The default should be XsuaaScopeConverter.

Best regards, Nena

palianytsia commented 4 years ago

Hi Nena,

thanks for looking into this issue.

To me it sounds like a responsibility of an application developer to apply the correct configuration, so if anyone has setLocalScopeAsAuthorities(true) and attaches another xsuua configuration via withAnotherServiceConfiguration() they affirm that the xsappname of both (default and another) xsuaa instances should be striped from incoming scopes. This is not to be confused with bound instances as the default behaviour of SAPOfflineTokenServicesCloud is to load a single xsuaa config even if there are more instances bound to the application.

So either one is willing to distinguish where the scopes are coming from (and therefore shouldn't set localScopeAsAuthorities to true) or not. I can hardly imagine the case when auth checks will be coded against local scopes from one xsuaa instance and global scopes from another one.

That said, I can also understand your concerns that this may be a breaking change for some applications. Feel free to close this issue if you think the risk is too high and I will follow with a PR for custom ScopeConverter setter.

Unfortunately (or fortunately) we already have productive customers so switching to a unified broker plan is not possible.

Regards, Ivan

nenaraab commented 4 years ago

Hi @palianytsia maybe we should raise an error if setLocalScopeAsAuthorities is called together with withAnotherServiceConfiguration.

Best regards, Nena

nenaraab commented 4 years ago

Closing this issue because of 2 weeks inactivity. Please let us know, whether this should be reopened.