SSO Shibboleth

Hi i tryed to use Shibboleth Idp with onelogin as a demo.
I am studyng your SAML Toolckit PHP downloaded from: https://github.com/onelogin/php-saml/archive/master.zip
I find your response in StackOverflow: https://stackoverflow.com/questions/32848457/onelogin-saml-php-toolkit-can-it-be-used-with-a-none-onelogin-saml-identity-pr but i don't understand how connect Shibboleth Idp, could you help me?
I have created an application on OneLogin As Type: SAML Test Connector (SP Shibboleth)
I selected this because i have to develop an application with IdP Shibboleth of my Customer.
I have configurated settings file as a guide from: https://developers.onelogin.com/saml/php
Atfter login i will be redirect to my demo application and i recive this error:
`
Warning: DOMDocument::schemaValidate(): Invalid Schema in \CIGS\DFS1_STAFF\WWW\WEBCIGS\WebMAIN\WebApp\lib\Saml2\Utils.php on line 147
1 | 0.0011 | 474840 | {main}( ) | ...\index.php:0
2 | 0.0457 | 1669464 | OneLogin_Saml2_Auth->processResponse( ) | ...\index.php:70
3 | 0.0459 | 1682064 | OneLogin_Saml2_Response->isValid( ) | ...\Auth.php:212
4 | 0.0462 | 1682520 | OneLogin_Saml2_Utils::validateXML( ) | ...\Response.php:146
5 | 0.0462 | 1682704 | schemaValidate ( ) | ...\Utils.php:147
`

My DOMDocument before $res = $dom->schemaValidate($schemaFile);:
DOMDocument Object ( [doctype] => [implementation] => (object value omitted) [documentElement] => (object value omitted) [actualEncoding] => [encoding] => [xmlEncoding] => [standalone] => 1 [xmlStandalone] => 1 [version] => 1.0 [xmlVersion] => 1.0 [strictErrorChecking] => 1 [documentURI] => my_path [config] => [formatOutput] => [validateOnParse] => [resolveExternals] => [preserveWhiteSpace] => 1 [recover] => [substituteEntities] => [nodeName] => #document [nodeValue] => [nodeType] => 9 [parentNode] => [childNodes] => (object value omitted) [firstChild] => (object value omitted) [lastChild] => (object value omitted) [previousSibling] => [nextSibling] => [attributes] => [ownerDocument] => [namespaceURI] => [prefix] => [localName] => [baseURI] => my_path/demo1/ [textContent] => https://app.onelogin.com/saml/metadata/9aeb4738-0155-4df8-a3e7-34559c20dba1l9V3SMP/2w8kt7I4UrN54oSto2s=KUrMPZie5XJWY3fufuUzmEb9cqzDLy0gsZnTCrBxeEFkMUhnrL7xFD7Ep9CM6xsbR4PuqM9lghLoFxpBaoJtqia0uTL36BYd0wfvPh2MM2XIuzLC3r4B24s9dUg0S+nQ09l04ntGuW9jB+YRLq7IX0ZP8lhVAWXe2zJV8AjBASbp+I1JWJ+RNuQCbtf/MzGHiw62F5ZZuC0xfjdKIoCEk1/sD+riEad17jKFH9LtH0+y6PTXnUi86hiS3aGu1BLsYRd5Pr+h2mKWD+4I1zGmX6scvgcyVEQ6wlpb3ztMQ1m5vlWObWnZTYg7x4P/4KQhNkP+jGPM5Qx5gLxl+GNmaQ==MIID2DCCAsCgAwIBAgIUQ8IBfXGbobc5QYVi+o0GDm9K/RUwDQYJKoZIhvcNAQEFBQAwRDEPMA0GA1UECgwGbXZiZGV2MRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxGjAYBgNVBAMMEU9uZUxvZ2luIEFjY291bnQgMB4XDTIxMDExMzExMDAwMloXDTI2MDExMzExMDAwMlowRDEPMA0GA1UECgwGbXZiZGV2MRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxGjAYBgNVBAMMEU9uZUxvZ2luIEFjY291bnQgMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA6W5LQupG1AUrGOCqNe829h5Mlb5q/Ag1NC2Xp+AKfT1+RDeKpUTljk0MXGbGIwbiXDSZYqI55Es+2ATg+SNghQF+fUfrMVzbj2beYVskVSHHXhgd5yKU86WYTpKj7W+Y0JQF3t051Ne3n0Inf/L0kFwK4ZeYhg2U3Sk34sOWiYsnCPB0vX6lNHMiH9xLrgve3ST37hC6It6/yDeYGMI6OB1Yrh0LM3NgYJ2F/cqp9wlCsMhI5JmdvopyC1UwovMS7C/9pTUksNEnQbYmsv/A8ytUxUvjtChlz22jw1HNI8BCsrqiwxxOXeTm7ipSiz12K1yumgZx7erth4PPqX8AUQIDAQABo4HBMIG+MAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFO80V94MzoRFI2TgMLOQpLn0U6nQMH8GA1UdIwR4MHaAFO80V94MzoRFI2TgMLOQpLn0U6nQoUikRjBEMQ8wDQYDVQQKDAZtdmJkZXYxFTATBgNVBAsMDE9uZUxvZ2luIElkUDEaMBgGA1UEAwwRT25lTG9naW4gQWNjb3VudCCCFEPCAX1xm6G3OUGFYvqNBg5vSv0VMA4GA1UdDwEB/wQEAwIHgDANBgkqhkiG9w0BAQUFAAOCAQEAWgORwQ+DqVCPDWMVehj84yTlwfEzX4as1lJwf54Z0pAPp+M0qB0rIYy3a03Hn0EIDKtfasE5eBnJp2kBC4SzP9QqQTh7AWLs374bcT8tjKnDDP2XG6gzs3iqjwdbPm7R8j7cQ7LupZ+EHKGrUHvaV3fPOgMv6b8gIcShv1d01k87T9rCYtZ9DqcMdtzSXoAn+8QIUHbY7qSD9IRtQNznJsIKdwgzgGzZ5JzkbBGPkns6fjwXhODQRLxD+Qxc8ja6Zi+OcAa3i6n4fNeCpEeg1vfLlMOCEPc/RF6F8PG6vSXOrvzDAiWdV1xlaiszkuqzF1uVBK8WY+r7X5a/j+tHfw==https://app.onelogin.com/saml/metadata/9aeb4738-0155-4df8-a3e7-34559c20dba1my_emailmy_path/metadata.phpurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportMichelemy_emailmy_surnamemy_email )
$schemaFile is in: my_path\lib\Saml2/schemas/saml-schema-protocol-2.0.xsd

My DOMDocument before $res = $dom->schemaValidate($schemaFile);:
DOMDocument Object ( [doctype] => [implementation] => (object value omitted) [documentElement] => (object value omitted) [actualEncoding] => [encoding] => [xmlEncoding] => [standalone] => 1 [xmlStandalone] => 1 [version] => 1.0 [xmlVersion] => 1.0 [strictErrorChecking] => 1 [documentURI] => my_path [config] => [formatOutput] => [validateOnParse] => [resolveExternals] => [preserveWhiteSpace] => 1 [recover] => [substituteEntities] => [nodeName] => #document [nodeValue] => [nodeType] => 9 [parentNode] => [childNodes] => (object value omitted) [firstChild] => (object value omitted) [lastChild] => (object value omitted) [previousSibling] => [nextSibling] => [attributes] => [ownerDocument] => [namespaceURI] => [prefix] => [localName] => [baseURI] => my_path/demo1/ [textContent] => https://app.onelogin.com/saml/metadata/9aeb4738-0155-4df8-a3e7-34559c20dba1l9V3SMP/2w8kt7I4UrN54oSto2s=KUrMPZie5XJWY3fufuUzmEb9cqzDLy0gsZnTCrBxeEFkMUhnrL7xFD7Ep9CM6xsbR4PuqM9lghLoFxpBaoJtqia0uTL36BYd0wfvPh2MM2XIuzLC3r4B24s9dUg0S+nQ09l04ntGuW9jB+YRLq7IX0ZP8lhVAWXe2zJV8AjBASbp+I1JWJ+RNuQCbtf/MzGHiw62F5ZZuC0xfjdKIoCEk1/sD+riEad17jKFH9LtH0+y6PTXnUi86hiS3aGu1BLsYRd5Pr+h2mKWD+4I1zGmX6scvgcyVEQ6wlpb3ztMQ1m5vlWObWnZTYg7x4P/4KQhNkP+jGPM5Qx5gLxl+GNmaQ==MIID2DCCAsCgAwIBAgIUQ8IBfXGbobc5QYVi+o0GDm9K/RUwDQYJKoZIhvcNAQEFBQAwRDEPMA0GA1UECgwGbXZiZGV2MRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxGjAYBgNVBAMMEU9uZUxvZ2luIEFjY291bnQgMB4XDTIxMDExMzExMDAwMloXDTI2MDExMzExMDAwMlowRDEPMA0GA1UECgwGbXZiZGV2MRUwEwYDVQQLDAxPbmVMb2dpbiBJZFAxGjAYBgNVBAMMEU9uZUxvZ2luIEFjY291bnQgMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA6W5LQupG1AUrGOCqNe829h5Mlb5q/Ag1NC2Xp+AKfT1+RDeKpUTljk0MXGbGIwbiXDSZYqI55Es+2ATg+SNghQF+fUfrMVzbj2beYVskVSHHXhgd5yKU86WYTpKj7W+Y0JQF3t051Ne3n0Inf/L0kFwK4ZeYhg2U3Sk34sOWiYsnCPB0vX6lNHMiH9xLrgve3ST37hC6It6/yDeYGMI6OB1Yrh0LM3NgYJ2F/cqp9wlCsMhI5JmdvopyC1UwovMS7C/9pTUksNEnQbYmsv/A8ytUxUvjtChlz22jw1HNI8BCsrqiwxxOXeTm7ipSiz12K1yumgZx7erth4PPqX8AUQIDAQABo4HBMIG+MAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFO80V94MzoRFI2TgMLOQpLn0U6nQMH8GA1UdIwR4MHaAFO80V94MzoRFI2TgMLOQpLn0U6nQoUikRjBEMQ8wDQYDVQQKDAZtdmJkZXYxFTATBgNVBAsMDE9uZUxvZ2luIElkUDEaMBgGA1UEAwwRT25lTG9naW4gQWNjb3VudCCCFEPCAX1xm6G3OUGFYvqNBg5vSv0VMA4GA1UdDwEB/wQEAwIHgDANBgkqhkiG9w0BAQUFAAOCAQEAWgORwQ+DqVCPDWMVehj84yTlwfEzX4as1lJwf54Z0pAPp+M0qB0rIYy3a03Hn0EIDKtfasE5eBnJp2kBC4SzP9QqQTh7AWLs374bcT8tjKnDDP2XG6gzs3iqjwdbPm7R8j7cQ7LupZ+EHKGrUHvaV3fPOgMv6b8gIcShv1d01k87T9rCYtZ9DqcMdtzSXoAn+8QIUHbY7qSD9IRtQNznJsIKdwgzgGzZ5JzkbBGPkns6fjwXhODQRLxD+Qxc8ja6Zi+OcAa3i6n4fNeCpEeg1vfLlMOCEPc/RF6F8PG6vSXOrvzDAiWdV1xlaiszkuqzF1uVBK8WY+r7X5a/j+tHfw==https://app.onelogin.com/saml/metadata/9aeb4738-0155-4df8-a3e7-34559c20dba1my_emailmy_path/metadata.phpurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportMichelemy_emailmy_surnamemy_email )
$schemaFile is in: my_path\lib\Saml2/schemas/saml-schema-protocol-2.0.xsd

Hi,
You get that error because you should not be providing a valid SAMLResponse to the ACS endpoint of the toolkit.
You are using Shibboleth as your IdP, so you don't need OneLogin to act as your IdP, so you don't need to configure any "SAML Test Connector (SP Shibboleth)" at OneLogin platform. That connector is used to connect a Shibolleth SP, but you mentioned that your goal is to connect Shibboleth IdP and the OneLogin PHP SAML Toolkit.
All you need to do is to publish the demo1 with Apache or Nginx. and configure the SAML Settings file and save it as settings.php
There you need to register the Shibboleth IdP metadata info.
Then access to the metadata.php file of the toolkit, and register this SP Metadata at Shibboleth IdP.
Review at Shibboleth IdP documentation how to obtain the IdP metadata and how register the SP metadata.

Hi,
You get that error because you should not be providing a valid SAMLResponse to the ACS endpoint of the toolkit.
You are using Shibboleth as your IdP, so you don't need OneLogin to act as your IdP, so you don't need to configure any "SAML Test Connector (SP Shibboleth)" at OneLogin platform. That connector is used to connect a Shibolleth SP, but you mentioned that your goal is to connect Shibboleth IdP and the OneLogin PHP SAML Toolkit.
All you need to do is to publish the demo1 with Apache or Nginx. and configure the SAML Settings file and save it as settings.php
There you need to register the Shibboleth IdP metadata info.
Then access to the metadata.php file of the toolkit, and register this SP Metadata at Shibboleth IdP.
Review at Shibboleth IdP documentation how to obtain the IdP metadata and how register the SP metadata.

Thank you so much!
Yesterday I tried to connect to Shibboleth OneLogin's IdP because the customer idp information i don't have yet.
So the only action that i have to do is setting settings.php file with the information about my IdP?
I tried to connect to OneLogin's IdP Shibboleth, configuring the configuration IdP serivice as https://developers.onelogin.com/saml/php point 4 section 10.
And registered in settings.php file as https://developers.onelogin.com/saml/php section 3.
In Configuration OneLogin IdP Setting i configured:

Login url: my_site/demo1/index.php
ACS (consumer) url: my_site/demo1/index.php?acs
SAML Recipient: my_site/demo1/index.php
SAML Single Logout Url: my_site/demo1/index.php?sls
ACS (consumer) url validator: .*
Audience: my_site/demo1/metadata.php

What change i have to do to work with Shibboleth OneLogin's IdP?
And when i change idP I have only to change the SAML Setting file, correct?

Thank you so much!
Yesterday I tried to connect to Shibboleth OneLogin's IdP because the customer idp information i don't have yet.
So the only action that i have to do is setting settings.php file with the information about my IdP?
I tried to connect to OneLogin's IdP Shibboleth, configuring the configuration IdP serivice as https://developers.onelogin.com/saml/php point 4 section 10.
And registered in settings.php file as https://developers.onelogin.com/saml/php section 3.
In Configuration OneLogin IdP Setting i configured:

Login url: my_site/demo1/index.php
ACS (consumer) url: my_site/demo1/index.php?acs
SAML Recipient: my_site/demo1/index.php
SAML Single Logout Url: my_site/demo1/index.php?sls
ACS (consumer) url validator: .*
Audience: my_site/demo1/metadata.php

What change i have to do to work with Shibboleth OneLogin's IdP?
And when i change idP I have only to change the SAML Setting file, correct?

If you are trying to connnect with OneLogin platform, use the "Advanced SAML Test Connector" instead of the "Shibboleth OneLogin's IdP".
If you registered the SP metadata in the connector, then you need to register the IdP metedata on the PHP SAML settings: settings.php file.

If you are trying to connnect with OneLogin platform, use the "Advanced SAML Test Connector" instead of the "Shibboleth OneLogin's IdP".
If you registered the SP metadata in the connector, then you need to register the IdP metedata on the PHP SAML settings: settings.php file.

Ok i try the advancedSamlTestConnector. I have a question about it: the SP is my web site. What i have to write in var entityId? And I don't have the x509cert and privateKey, can i don't assign any value?

Ok i try the advancedSamlTestConnector. I have a question about it: the SP is my web site. What i have to write in var entityId? And I don't have the x509cert and privateKey, can i don't assign any value?

The fact of assign a PrivateKey and Public cert to the SP is optional and you only need to assign them if you plan to sign the messages sent by the SP or you require the SAMLResponse Assertion to be encrypted. You can use self-signed certs, this tool creates it:
https://www.samltool.com/self_signed_certs.php
Please review the documentation of this toolkit, everything is described on it.

The fact of assign a PrivateKey and Public cert to the SP is optional and you only need to assign them if you plan to sign the messages sent by the SP or you require the SAMLResponse Assertion to be encrypted. You can use self-signed certs, this tool creates it:
https://www.samltool.com/self_signed_certs.php
Please review the documentation of this toolkit, everything is described on it.

I try the new settings.php file, i renamed settings in settingsInfo. But i have the same problem:
Warning: DOMDocument::schemaValidate(): Invalid Schema in \CIGS\DFS1_STAFF\WWW\WEBCIGS\WebMAIN\WebApp\lib\Saml2\Utils.php on line 151
This is my settings.php: https://www.codepile.net/pile/7AYNeWjy

I try the new settings.php file, i renamed settings in settingsInfo. But i have the same problem:
Warning: DOMDocument::schemaValidate(): Invalid Schema in \CIGS\DFS1_STAFF\WWW\WEBCIGS\WebMAIN\WebApp\lib\Saml2\Utils.php on line 151
This is my settings.php: https://www.codepile.net/pile/7AYNeWjy

Using OneLogin: SAML Test Connector (Advanced) it work correctly. I don't understand why it not work qui Shibboleth SAML Response

Using OneLogin: SAML Test Connector (Advanced) it work correctly. I don't understand why it not work qui Shibboleth SAML Response

The fact of assign a PrivateKey and Public cert to the SP is optional and you only need to assign them if you plan to sign the messages sent by the SP or you require the SAMLResponse Assertion to be encrypted. You can use self-signed certs, this tool creates it:
https://www.samltool.com/self_signed_certs.php
Please review the documentation of this toolkit, everything is described on it.

i find the problem, i think.
OneLogin return a Response XML with ds data, that it isn't in example on onelogin: https://www.samltool.com/generic_sso_res.php
In xml i have:
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
ds:SignedInfo
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
<ds:Reference URI="#pfx4a8b2268-73c7-a440-75aa-fe4f2aa782c5">
ds:Transforms
<ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
<ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
</ds:Transforms>
<ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
ds:DigestValueYx20g0pbvuMcH7uIl6ccwD308xA=</ds:DigestValue>
</ds:Reference>
</ds:SignedInfo>
ds:SignatureValuesignature</ds:SignatureValue>
ds:KeyInfo
ds:X509Data
ds:X509Certificatex509 cert</ds:X509Certificate>
</ds:X509Data>
</ds:KeyInfo>
</ds:Signature>

The fact of assign a PrivateKey and Public cert to the SP is optional and you only need to assign them if you plan to sign the messages sent by the SP or you require the SAMLResponse Assertion to be encrypted. You can use self-signed certs, this tool creates it:
https://www.samltool.com/self_signed_certs.php
Please review the documentation of this toolkit, everything is described on it.

i find the problem, i think.
OneLogin return a Response XML with ds data, that it isn't in example on onelogin: https://www.samltool.com/generic_sso_res.php
In xml i have:
<ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
ds:SignedInfo
<ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
<ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
<ds:Reference URI="#pfx4a8b2268-73c7-a440-75aa-fe4f2aa782c5">
ds:Transforms
<ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
<ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
</ds:Transforms>
<ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
ds:DigestValueYx20g0pbvuMcH7uIl6ccwD308xA=</ds:DigestValue>
</ds:Reference>
</ds:SignedInfo>
ds:SignatureValuesignature</ds:SignatureValue>
ds:KeyInfo
ds:X509Data
ds:X509Certificatex509 cert</ds:X509Certificate>
</ds:X509Data>
</ds:KeyInfo>
</ds:Signature>

here are my XML response, it is what i recive from onelogin: https://www.cigs.unimo.it/webapp/demo1/output.xml

here are my XML response, it is what i recive from onelogin: https://www.cigs.unimo.it/webapp/demo1/output.xml

And OneLogin Validator result:
The response was received at https://www.cigs.unimo.it/webapp/demo1/index.php?acs instead of https://www.cigs.unimo.it/webapp/demo1/index.php
A valid SubjectConfirmation was not found on this Response
Signature validation failed. Reference validation failed

And OneLogin Validator result:
The response was received at https://www.cigs.unimo.it/webapp/demo1/index.php?acs instead of https://www.cigs.unimo.it/webapp/demo1/index.php
A valid SubjectConfirmation was not found on this Response
Signature validation failed. Reference validation failed

You need to set as ACS URL:
https://www.cigs.unimo.it/webapp/demo1/index.php?acs

You need to set as ACS URL:
https://www.cigs.unimo.it/webapp/demo1/index.php?acs

this is my settings:
$spBaseUrl = 'https://www.cigs.unimo.it/webapp'; //or http://<your_domain>
$settingsInfo = array (
    'debug' => true,
    'sp' => array (
        'entityId' => $spBaseUrl.'/demo1/metadata.php',
        'assertionConsumerService' => array (
            'url' => $spBaseUrl.'/demo1/index.php?acs',
        ),
        'singleLogoutService' => array (
            'url' => $spBaseUrl.'/demo1/index.php?sls',
        ),
        'NameIDFormat' =><EMAIL_ADDRESS>    ),
    'idp' => array (
        'entityId' => 'urlentry/metadata',
        'singleSignOnService' => array (
            'url' => '/saml2/http-post/sso/',
        ),
        'singleLogoutService' => array (
            'url' => '/saml2/http-redirect/slo/',
        ),
        'x509cert' => 'cert',
    ),
);

I set acs, this is incorrect?

this is my settings:
$spBaseUrl = 'https://www.cigs.unimo.it/webapp'; //or http://<your_domain>
$settingsInfo = array (
    'debug' => true,
    'sp' => array (
        'entityId' => $spBaseUrl.'/demo1/metadata.php',
        'assertionConsumerService' => array (
            'url' => $spBaseUrl.'/demo1/index.php?acs',
        ),
        'singleLogoutService' => array (
            'url' => $spBaseUrl.'/demo1/index.php?sls',
        ),
        'NameIDFormat' =><EMAIL_ADDRESS>    ),
    'idp' => array (
        'entityId' => 'urlentry/metadata',
        'singleSignOnService' => array (
            'url' => '/saml2/http-post/sso/',
        ),
        'singleLogoutService' => array (
            'url' => '/saml2/http-redirect/slo/',
        ),
        'x509cert' => 'cert',
    ),
);

I set acs, this is incorrect?

What is this?On Jan 18, 2021 02:57, Michele Vitiello Bonaventura<EMAIL_ADDRESS>wrote:
this is my settings:
$spBaseUrl = 'https://www.cigs.unimo.it/webapp'; //or http://<your_domain>
$settingsInfo = array (
'debug' => true,
'sp' => array (
'entityId' => $spBaseUrl.'/demo1/metadata.php',
'assertionConsumerService' => array (
'url' => $spBaseUrl.'/demo1/index.php?acs',
),
'singleLogoutService' => array (
'url' => $spBaseUrl.'/demo1/index.php?sls',
),
'NameIDFormat' =><EMAIL_ADDRESS>),
'idp' => array (
'entityId' => 'urlentry/metadata',
'singleSignOnService' => array (
'url' => '/saml2/http-post/sso/',
),
'singleLogoutService' => array (
'url' => '/saml2/http-redirect/slo/',
),
'x509cert' => 'cert',
),
);
I set acs, this is incorrect?
—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or unsubscribe.

What is this?On Jan 18, 2021 02:57, Michele Vitiello Bonaventura<EMAIL_ADDRESS>wrote:
this is my settings:
$spBaseUrl = 'https://www.cigs.unimo.it/webapp'; //or http://<your_domain>
$settingsInfo = array (
'debug' => true,
'sp' => array (
'entityId' => $spBaseUrl.'/demo1/metadata.php',
'assertionConsumerService' => array (
'url' => $spBaseUrl.'/demo1/index.php?acs',
),
'singleLogoutService' => array (
'url' => $spBaseUrl.'/demo1/index.php?sls',
),
'NameIDFormat' =><EMAIL_ADDRESS>),
'idp' => array (
'entityId' => 'urlentry/metadata',
'singleSignOnService' => array (
'url' => '/saml2/http-post/sso/',
),
'singleLogoutService' => array (
'url' => '/saml2/http-redirect/slo/',
),
'x509cert' => 'cert',
),
);
I set acs, this is incorrect?
—You are receiving this because you are subscribed to this thread.Reply to this email directly, view it on GitHub, or unsubscribe.

That setting at the toolkit side is ok, you need to set the right value at the OneLogin connector

That setting at the toolkit side is ok, you need to set the right value at the OneLogin connector

That setting at the toolkit side is ok, you need to set the right value at the OneLogin connector

Hi i recived my customer inner-metadata.xml.
I am setting my settings file. In this file exist an entry for this domain, because is already used for login.
How work redirect?
When i try to login on shibboleth login page, i receive that the request is not supported,
The application that you connected is not registered for use trough this service.
i have to add and entry for my domain/folder also that the domain is already registered?
My new IDP:
'idp' => array (
'entityId' => 'http://www.cigs.unimo.it/shibboleth',
'singleSignOnService' => array (
'url' => 'https://idp.unimore.it/idp/profile/Shibboleth/SSO',
),
'singleLogoutService' => array (
'url' => 'http://www.cigs.unimo.it/Shibboleth.sso/SLO/POST',
),
'x509cert' => 'CERT',
),
the entityId i take from:
"
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="http://www.cigs.unimo.it/shibboleth">
<md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol urn:oasis:names:tc:SAML:1.1:protocol urn:oasis:names:tc:SAML:1.0:protocol">
"
For Logout Service i use:
"
<md:SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="http://www.cigs.unimo.it/Shibboleth.sso/SLO/POST"/>
"
And for Login:
"

"

That setting at the toolkit side is ok, you need to set the right value at the OneLogin connector

Hi i recived my customer inner-metadata.xml.
I am setting my settings file. In this file exist an entry for this domain, because is already used for login.
How work redirect?
When i try to login on shibboleth login page, i receive that the request is not supported,
The application that you connected is not registered for use trough this service.
i have to add and entry for my domain/folder also that the domain is already registered?
My new IDP:
'idp' => array (
'entityId' => 'http://www.cigs.unimo.it/shibboleth',
'singleSignOnService' => array (
'url' => 'https://idp.unimore.it/idp/profile/Shibboleth/SSO',
),
'singleLogoutService' => array (
'url' => 'http://www.cigs.unimo.it/Shibboleth.sso/SLO/POST',
),
'x509cert' => 'CERT',
),
the entityId i take from:
"
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="http://www.cigs.unimo.it/shibboleth">
<md:SPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol urn:oasis:names:tc:SAML:1.1:protocol urn:oasis:names:tc:SAML:1.0:protocol">
"
For Logout Service i use:
"
<md:SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" Location="http://www.cigs.unimo.it/Shibboleth.sso/SLO/POST"/>
"
And for Login:
"

"

*and for Login:
"

"

*and for Login:
"

"

My service when i do login it need in get: ?execution=e1s1 with number that change every try.
The library doing redirect with GET parameter: SAMLRequest=XXXXXX how could i change it?

My service when i do login it need in get: ?execution=e1s1 with number that change every try.
The library doing redirect with GET parameter: SAMLRequest=XXXXXX how could i change it?

You got
The response was received at https://www.cigs.unimo.it/webapp/demo1/index.php?acs instead of https://www.cigs.unimo.it/webapp/demo1/index.php

because at the IdP you registered wrong SP ACS endpoint, or the IdP removed the query parameters, if that is the case, you could always use demo2 which has the endpoints in different files, so no query parameter is required.

You got
The response was received at https://www.cigs.unimo.it/webapp/demo1/index.php?acs instead of https://www.cigs.unimo.it/webapp/demo1/index.php

because at the IdP you registered wrong SP ACS endpoint, or the IdP removed the query parameters, if that is the case, you could always use demo2 which has the endpoints in different files, so no query parameter is required.

Hi my customer registered my application in his Shibboleth login, but whene i try to login here, i recive an error in Customer IdP
What i have to do? https://app.cigs.unimore.it/demo1/

Hi my customer registered my application in his Shibboleth login, but whene i try to login here, i recive an error in Customer IdP
What i have to do? https://app.cigs.unimore.it/demo1/

Hi i have a correct configuration of shibboleth. I can login on shibboleth idp and it redirect me to my page after login.
But OneLogin now tell me this error:
"
failed to load external entity "/CIGS/DFS1_STAFF/WWW/WEBCIGS/WebMAIN/WebApp/lib/Saml2/schemas/xmldsig-core-schema.xsd" Element '{http://www.w3.org/2001/XMLSchema}import': Failed to locate a schema at location '/CIGS/DFS1_STAFF/WWW/WEBCIGS/WebMAIN/WebApp/lib/Saml2/schemas/xmldsig-core-schema.xsd'. Skipping the import. failed to load external entity "/CIGS/DFS1_STAFF/WWW/WEBCIGS/WebMAIN/WebApp/lib/Saml2/schemas/xenc-schema.xsd" Element '{http://www.w3.org/2001/XMLSchema}import': Failed to locate a schema at location '/CIGS/DFS1_STAFF/WWW/WEBCIGS/WebMAIN/WebApp/lib/Saml2/schemas/xenc-schema.xsd'. Skipping the import. Element '{http://www.w3.org/2001/XMLSchema}element', attribute 'ref': The QName value '{http://www.w3.org/2001/04/xmlenc#}EncryptedData' does not resolve to a(n) element declaration. Element '{http://www.w3.org/2001/XMLSchema}element', attribute 'ref': The QName value '{http://www.w3.org/2001/04/xmlenc#}EncryptedKey' does not resolve to a(n) element declaration. Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd
invalid_response
Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd
Not authenticated"
With SAML Traker i See correct authentication about my user with my data:
"
<saml2:AuthnStatement AuthnInstant="2021-02-04T08:52:10.281Z"
SessionIndex="_683d7dcca29XXXX"
>
<saml2:SubjectLocality Address="XXX.XXX.XXX.XXX" />
saml2:AuthnContext
saml2:AuthnContextClassRefurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml2:AuthnContextClassRef>
</saml2:AuthnContext>
</saml2:AuthnStatement>
saml2:AttributeStatement
<saml2:Attribute FriendlyName="givenName"
Name="urn:oid:<IP_ADDRESS>"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>XXXX</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="ou"
Name="urn:oid:<IP_ADDRESS>"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>people</saml2:AttributeValue>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>Esterni</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="sn"
Name="urn:oid:<IP_ADDRESS>"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>XXXX</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="cn"
Name="urn:oid:<IP_ADDRESS>"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>XXXX XXXX</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="uid"
Name="urn:oid:0.9.2342.192003<IP_ADDRESS>"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
saml2:AttributeValueXXXXXX</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="eduPersonScopedAffiliation"
Name="urn:oid:<IP_ADDRESS>.4.1.59<IP_ADDRESS>.9"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>XXX@XXXX.it</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="unimoreDiporg2"
Name="urn:oid:<IP_ADDRESS>.4.1.133<IP_ADDRESS>.5.1.12"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>{1}{020067}CENTRO INTERDIPARTIMENTALE GRANDI STRUMENTI</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="unimoreDiporg4"
Name="urn:oid:<IP_ADDRESS>.4.1.133<IP_ADDRESS>.5.1.14"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
<saml2:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:type="xsd:string"
>{1}{020067}CENTRO INTERDIPARTIMENTALE GRANDI STRUMENTI</saml2:AttributeValue>
</saml2:Attribute>
<saml2:Attribute FriendlyName="eduPersonPrincipalName"
Name="urn:oid:<IP_ADDRESS>.4.1.59<IP_ADDRESS>.6"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"
>
saml2:AttributeValueXXX@XXXX.it</saml2:AttributeValue>
</saml2:Attribute>
</saml2:AttributeStatement>
"

Failed to locate a schema at location '/CIGS/DFS1_STAFF/WWW/WEBCIGS/WebMAIN/WebApp/lib/Saml2/schemas/xmldsig-core-schema.xsd

The toolkit is not able to read that file, required in order to validate the SAMLResponse.

demo1 is in my swebapp floder that is loaded form IIS to serve the request from app.cigs.unimore.it.
web app is serve from app.cigs.unimore.it, to do accesso to demo1 i use app.cigs.unimore.it/demo1, the lib is in corrected location at webapp/lib/Saml2/schemas/files.
For now i have resolved doing comment from line 148 to 161 in Utils.php file in lib/Sam2 to ignore the validation of SAML2 Response. After i will extend your class, coping method content and removing the validation.
Now i see my data of user logged on index.php.
Now i am working to do logout with signed request.
Thank you for you support :)

i have addedd in settings.php in the configuration this array after baseurl:
'security' =>array(
// Indicates whether the samlp:logoutRequest messages sent by this SP
// will be signed.
'logoutRequestSigned' => true,
    // Indicates whether the <samlp:logoutResponse> messages sent by this SP
    // will be signed.
    'logoutResponseSigned' => true,
),

because i tried this withouth success:
The settings files described (settings.php and advanced_settings.php) are loaded by the toolkit if no other array with settings info is provided in the constructor of the toolkit. Let's see some examples.
// Initializes toolkit with settings.php & advanced_settings files.
$auth = new OneLogin_Saml2_Auth();
//or
$settings = new OneLogin_Saml2_Settings();
// Initializes toolkit with the array provided.
$auth = new OneLogin_Saml2_Auth($settingsInfo);
//or
$settings = new OneLogin_Saml2_Settings($settingsInfo);
You can declare the $settingsInfo in the file that contains the constructor execution or locate them in any file and load the file in order to get the array available as we see in the following example:
<?php

require_once 'custom_settings.php';  // The custom_settings.php contains a
                                     // $settingsInfo array.

$auth = new OneLogin_Saml2_Auth($settingsInfo);


[FEATURE]: Observability supports volume metrics for PowerStore

Describe the solution you'd like
Implement metrics service for PowerStore that support following functionality:

Gather and push volume metrics to OpenTelemetry
The k8s admin can configure the metrics collection frequency
The k8s admin has the option of enabling/disabling the collection of PowerStore metrics at deployment or runtime
Limit concurrent connections to the PowerStore storage system while collecting performance metrics
The implementation includes support for collecting metrics for multiple PowerStore arrays.
Update existing Karavi Observability helm chart and installer script with any relevant changes

Describe alternatives you've considered
N/A
Additional context
N/A

@tdawe: Thank you for submitting this issue!
The issue is currently awaiting triage. Please make sure you have given us as much context as possible.
If the maintainers determine this is a relevant issue, they will remove the needs-triage label and assign an appropriate priority label.

We want your feedback! If you have any questions or suggestions regarding our contributing process/workflow, please reach out to us at<EMAIL_ADDRESS>


Figure out how to do named import aliases

So I can import from @models instead of ../../src/blahblahblah

https://stackoverflow.com/questions/43281741/how-to-use-paths-in-tsconfig-json

Once again screwed by ESM. None of the tutorials out there actually use modules, and some of the top libraries for helping with this even have open issues saying it doesn't work with ESM.
 Typescript supports it but node and ts-node don't.

Found an extension that does this, at long last.

Kinda working but have to pick between runtime failures and IDE warnings :(


Task Template - closing modal with x

closing the modal with the x button prevents any other modals from being opened.
cancel button has correct functionality
(good job Tom)

As a (type of user)
I want (goal)
so that (reason/ why/ value)
Acceptance Criteria
Scenario 1:
GIVEN ... (you or your condition)
WHEN ... (what you do)
THEN ... (what you see)
Scenario 2:
xxxxxx
Validation Rules?
(If yes, list here)
Screenshot
(Link above will always be the most up-to-date version.)

@TomChapmanGov


Is there a problem with C99/C11?

What is the issue with the int in the for loop declaration? Gcc and Clang both support C99 and C11.

variable declared within for-loop is visible in the first following line since C11(C++ 11).
e.g.
for(int i=0; ;) {
}
i++;
BUT we are writing C.

C11 is C. The ability do declare and use variables in the for loop declaration simplifies code.

With gcc, -std=c99 must be declared. Which was not specified in Makefile.

OK. Changes will be added to dev branch.

Great.

:+1:


L7 redirection does not work on GKE COS images with direct routing

The underlying issue that caused https://github.com/cilium/cilium/issues/8982 no longer prevents Cilium from starting in GKE on COS, but it breaks L7 redirection:
kubectl -n cilium logs pod/cilium-mlhbg | grep warning | head -n 1
level=warning msg="xt_socket kernel module could not be loaded" error="could not load module xt_socket: exit status 1" subsys=iptables

If you SSH into the individual nodes in GKE and try to load it, it's clear that it's missing:
# modprobe xt_socket
modprobe: FATAL: Module xt_socket not found in directory /lib/modules/4.14.138+

Kernel:
$ uname -sr
Linux 4.14.138+

The datapath observation is that in cilium monitor, when L7 policy is enabled, packets are redirected to the proxy, but there is no response.

More recent evaluation of this suggests that if you deploy with GKE and tunnel mode (per the standard GSG documentation), this should work fine.
Any further discussion of this bug should be very explicit on:

Cilium helm options used to configure the cluster
What "does not work" means in terms of observed packet drops or traffic "disappearing"
Directionality of traffic affected
Policies in place (ingress, egress)

Recent patches were introduced to provide a socket fallback mode that remediates the underlying issue. Closing.


watch

Hello @matejsvajger,
Thanks for making this. Makes my configuration so much cleaner :)
not sure if you noticed/someone let you know before. Still wanted to let you know in case you didn't notice: the mix plugin doesn't seem to work with watch. I've tried to change some pug files and it didn't up. Maybe I've configured something wrong tho. Should it work?
Cheers,
Peter

This is working after I figured out that my output path didn't work due to the issue raised under https://github.com/matejsvajger/laravel-mix-pug/issues/7


Ability to launch a Tunnel Port Forwarding through a Dynamic Port Forwarding when available

Feature description
Now we can launch SSH and SFTP connections seamlessly through DPF tunnels but we can't launch Port Forwards over them.
So adding this ability will allow us to have a better WebSSH before having a full multi-jump feature.

Available inside the latest beta release WebSSH 19.7

Hi Arnaud, do you have any examples or documentation on how this works, how to set it up, and what benefits it gives please? Thanks.

@jarrah31 Yes documentation about DPF is here : https://webssh.net/documentation/help/networking/port-forwarding/#dynamic-port-forwarding
When you launch a "DPF" tunnel (with * wildcard char) WebSSH use it to launch any further SSH, SFTP or even TUNNEL connections through it.
So it "will act" as an "easy VPN" between your device and your trusted SSH server.
The next step is to implement "jump" : https://github.com/isontheline/pro.webssh.net/issues/577

When you launch a "DPF" tunnel (with * wildcard char) WebSSH use it to launch any further SSH, SFTP or even TUNNEL connections through it.

I'm a little confused because I've been using the DPF tunnel with a wildcard char for a while now, so I was wondering what this particular issue fixes (so that I can test it out)? Sorry if I've misunderstood something. :)

DPF has not been updated with this issue.
The problem was that TUNNEL connections launched after a DPF weren't using the DPF so I was unable to forward a port on a bastion.
This could be considered as a "hack" before true jump feature implemented.

Launching forwards through DPF let you to don't specify all port forwardings inside one TUNNEL but instead you can launch one or more forwards TUNNELs to do some things.
Perhaps this is only useful for me. I needed it last week under an IT incident.

Understood, thanks for the explanation Arnaud. :)


Remove all BUILD.gn files.

There is still one in updater/BUILD.gn, but I've
asked @mpcomplete if that is still needed.
@hansmuller

LGTM

@HansMuller beat me. :)

I removed updater/BUILD.gn along with the others. If that was a mistake, I'll restore it.


Monster Tamer Ep ?? - Saving & Loading - Part X

The scope of these videos is to cover the following items:
☐ add game menu with option to save game
☐ update menu scene with continue option to load game
☐ on new game, we should reset the player data (pos, monsters, etc)
☐ update data manager to persist monster data and current stats

part 1: create basic menu, with basic input, connect to world scene
part 2: add menu options and connect with player input

to show saving data is working, we can refresh the window to show data is being persisted for player position

part 3: fix minor bug in player movement, and add color support for menu based on options data

for the minor bug, we could likely fold this into the original two videos

part 4: added logic for new game and resetting data, enabling continue button, and updating the title scene


conversion to hap with FFmpeg

Hi,
I am having problems playing back DirectShow hap files (.avi) in either Cinder or openFrameworks DirectShow blocks/addons.
I've asked the developers of openFrameworks addon and they are having the same issue.
Would you perhaps have any suggestion on how to approach this problem?
Thanks!
MP

This is a problem with their implementation. I've left a comment on the linked issue for the OF addon, feel free to duplicate it to the Cinder block if you think it might apply there too.


Fix dependencies

Update tempfile version as it was failing for me locally:
error[E0554]: `#![feature]` may not be used on the stable release channel
  --> /Users/mxpv/.cargo/registry/src/github.com-1ecc6299db9ec823/rustix-0.36.8/src/lib.rs:99:26
   |
99 | #![cfg_attr(rustc_attrs, feature(rustc_attrs))]
   |                          ^^^^^^^^^^^^^^^^^^^^

and cleanup the old workaround for axum-core

@jsturtevant any thoughts on
---- synchronous::publisher::tests::test_connect stdout ----
thread 'synchronous::publisher::tests::test_connect' panicked at 'called `Result::unwrap()` on an `Err` value: Ttrpc(Windows(231))', crates\shim\src\synchronous\publisher.rs:159:50
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@jsturtevant any thoughts on
---- synchronous::publisher::tests::test_connect stdout ----
thread 'synchronous::publisher::tests::test_connect' panicked at 'called `Result::unwrap()` on an `Err` value: Ttrpc(Windows(231))', crates\shim\src\synchronous\publisher.rs:159:50
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

231 is all pipe instances are busy might be a flaky test. Has is shown up in other PR's?  I will create an issue and see if I can reproduce.

otherwise changes LGTM


Minimal integration with serde-reflection

Run serde-reflection to track serialization formats (and ideally add a unit test to detect changes)
Document how to generate stub code in other languages using serde-generate.
Consider switching from bincode to BCS and using BCS to implement the Digestible trait:
https://github.com/novifinancial/fastpay/blob/master/fastpay_core/src/messages.rs#L166
https://github.com/novifinancial/librabft_simulator/blob/master/bft-lib/src/smr_context.rs#L82

Sadly, switching to BCS requires some extra work on either BCS or FastPay because BCS doesn't have stream deserialization yet.


Screenshots and description do not match actual workflow

The screenshots seem not to depict the current state of outlook.
I use version "Microsoft Outlook for Microsoft 365 MSO (16.0.13328.20262) 32-Bit", which will open a web-page, when I click on the Add-In button - the webpage is: https://appsource.microsoft.com/de-de/marketplace/apps?product=office%3Boutlook&page=1&src=office&corrid=44f1107b-2cdc-4c76-9769-b288c00536c8&omexanonuid=c682f60f-85c5-44fd-b0e9-cb0cad42491b&referralurl= and sideloading seems not possible from there.
How do I sideload an add-in?

Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

ID: 3f364ae0-b7dd-edb3-b51b-8f6d0d99afbe
Version Independent ID: 7dd1ce8f-16e7-88f8-f291-4afbe76baba7
Content: Sideload Outlook add-ins for testing - Office Add-ins
Content Source: docs/outlook/sideload-outlook-add-ins-for-testing.md
Product: outlook
Technology: add-ins
GitHub Login: @o365devx
Microsoft Alias: o365devx

@glatzert Thanks for raising this issue. Can you access Outlook on the web and try the related instructions on that same page to sideload in that client?
Let us know if that helped or if we can help further.
Thanks.

Hi there,
my observations so far:

Outlook Desktop goes to that Store Page mentioned above, with no way of sideloading
outlook.com does not work for the user I tried (it's a corporate login and I seem to have no outlook license for outlook online)
mail.domain.com is an on-prem OWA, which has some settings regarding Add-Ins (did not have time to see, if I can load one or not)
outlook.com with a personal user will show the add-in button below the "new Mail"-Window (which is really an odd placement, but that's probably another UX issue).

I'll try the latter one and report back to you. Nevertheless for Outlook Desktop the Screenshots do not add up.

@glatzert Thanks for sharing this info.
@exextoc Can you take a look?
Thanks.

If I understand your description correctly, your Exchange account is on-premise. Is that right?

In desktop Outlook, do you see a "Manage Add-ins" option in 'File', under the 'Info' tab? The "Manage Add-ins" button should open "Get-Addins" interface in OWA and the experience should be similar to "Sideload an add-in in classic Outlook on the web" in https://docs.microsoft.com/en-us/office/dev/add-ins/outlook/sideload-outlook-add-ins-for-testing#sideload-an-add-in-in-classic-outlook-on-the-web

@exextoc Thanks for your response.
@glatzert Can you try this out and let us know if it helped?
Thanks.

@ElizabethSamuel-MSFT and @exextoc thanks - I could sideload it via the OWA of my company.
I think the docs could be improved by mentioning, the "File" > "Manage Add-Ins" for my Outlook Version

@glatzert Thanks for letting us know the outcome. We discuss this option in the Outlook 2013 section of the article. I'll update the 'Outlook 2016 and later' section to indicate the 2013 instructions as an alternative. PR will follow soon.


Cannot register multiple runners for the same org.

I was trying to create multiple github runnner for an Org, using indididual repo keys,
- name: Install Runner1 on github-runners-ubuntu
  hosts: github-runners-ubuntu

  roles:
    - role: ansible-github-actions-runner
      vars:
        gh_runner_config_labels:
          - ubuntu22
          - linux
          - self-hosted
        gh_runner_config_url: https://github.com/acme.net
        gh_runner_config_token: APERFECTLYFINETOKEN
        gh_runner_config_name: ubuntu-runner1
        gh_runner_allow_runasroot: "Refuse"

    - role: ansible-github-actions-runner
      vars:
        gh_runner_config_labels:
          - ubuntu22
          - linux
          - self-hosted
        gh_runner_config_url: https://github.com/acme.net
        gh_runner_config_token: ANOTHERPERFECTLYFINETOKEN
        gh_runner_config_name: ubuntu-runner2
        gh_runner_allow_runasroot: "Refuse"

I expected this to create two runners for on the host and that they both would be online.
Instead it replaced the first runner with the second one.

Ive been looking at the code to understand why its replacing the existing one instead of generating a separate one, but Im not able to figure it out as of yet so figured Id put in an Issue.

It seems that If i keep the runners in separate folders by specifying the path for each runner with:
gh_runner_installation_path
This seems to solve the situation for me.

It seems that If i keep the runners in separate folders by specifying the path for each runner with: gh_runner_installation_path
This seems to solve the situation for me.

Adding to this, I had to also set 'gh_runner_workspace_path' to be unique for every runner, otherwise i sometime got error for missing files in '/var/cache/github-actions-runner' folder.


Android: Add preference for full screen reading

This came up on the forum but there should be a way to mark a story unread half-way down while reading it without losing scroll position. Maybe I'm overthinking it but should we instead have a full-screen preference with the title section being sticky at the top so the menu button is always visible?

Commenting here - I'm the person who left the comment on the forum.
I'm not concerned about losing scroll position - most of the time, if I'm marking a story unread, I'll probably get back to it on the desktop. I mostly want a convenient way of doing so.
Adding a preference option would be great for me.


Cache directive

I think a kind of @cache(maxAge: '12h') directive makes a lot of sense for agogos as an API gateway, it could also respect HTTP Cache-Control headers for example.
There's some thinking to be done about how to cache it exactly, if it's allowed on fields or whole entities, etc.

Apollo has done a lot of prior work on this, should probably not re-invent the wheel completely here.
https://www.apollographql.com/docs/apollo-server/performance/caching/


CDF is broken sometimes

I'm not sure why this happens on occasion. Maybe the shape isn't rendering correctly, and is filling in the wrong section for some reason.

I believe I fixed this with the recent commits.


UI set away status

Adds a Profile tab and allows setting user's away status and status text.

Ready.

Thanks @borisyankov !

Add OwnAvatar component

This comment still applies: https://github.com/zulip/zulip-mobile/pull/3267#discussion_r255269225

Add AwayStatusSwitch component

+type Props = {|
+  ...PropsFromState,
+  dispatch: Dispatch,
+|};

On another PR we discussed this name PropsFromState, which should be something else.

I'm also curious what the intended use and meaning of the type is exactly. The dispatch prop is also provided by connect... so if this gets called something like PropsFromConnect, then it should probably go there. Also if the idea is that the props mentioned explicitly in Props will correspond to props the caller can, or should, pass in, then dispatch doesn't belong in that list.

Separately: this in the commit message is good information that I think would be best captured in jsdoc on the component:

     * retrieves the current user's `user status` data and presents it
     * allows by switching it to control the `away` status

Add ProfileCard component

Similarly, this would be good to capture as jsdoc on the component:
    This is very similar to `AccountDetails` but used to show the
    current users account.

Rework AccountDetails to be more generic
Move "Send private message" out of AccountDetails.
This allows us to reuse AccountDetails in ProfileCard but not
show the "Send private message" button that is not appropriate there.

Hmm interesting. Shouldn't this go before we create ProfileCard, then? Or at least before we wire it up in the navigation.

Also this looks in the code like it will change the layout -- the button was previously inside a ComponentList. Haven't yet compared visually.

AccountDetails: show user status text if one is set

+    // backgtoundColor: 'red',

You'll want to take this out, presumably.
(Also it's misspelled.)

+    const statusText = userStatus[user.user_id] && userStatus[user.user_id].status_text;
[...]
+          {statusText !== undefined && (
+            <RawLabel style={[styles.largerText, componentStyles.statusText]} text={statusText} />
+          )}

This logic feels to me like it's calling for a (very simple) selector. Best if the component doesn't have to deal with the nuances of what it means for the whole object to be missing from the map, vs. the object to be present but have undefined for status_text.
E.g. getUserStatusText: (state: GlobalState, userId: number) => string | void, making use of the kind of signature discussed in #3015.

Add UserStatusScreen

    Values are imported from a separate file as Prettier does not
    'like' unicode and messes up the formatting.

Huh indeed!
Less euphemistically: Prettier has a bug in handling Unicode. :stuck_out_tongue:
I suspect that it's specifically about non-BMP code points, which includes most emoji -- those are the code points that in JavaScript (and Java) if you're not careful you end up treating as if they're two code points. Like the RN bug that underlay #2787.
Searching their tracker, this looks a lot like prettier/prettier#770. ... But that was supposedly fixed in Prettier 1.3.0, and we're on 1.12.1. Hm.
Anyway:

At a minimum, there should be a comment inside that tiny file saying why it's there -- it's a peculiar way for the code to be, and so it should provide an answer to the reader's natural question.

Better, let's be good citizens and do something about the issue :wink: . Looks like there are newer releases of Prettier -- see if the issue goes away on upgrade? If not, file a bug with Prettier upstream.

    Possible presets are given for a one-click entry.

Good idea!
+  getInitialStatusText = () => {
+    const { selfUserDetail, userStatus } = this.props;
+    const selfUserStatus = userStatus[selfUserDetail.user_id];
+    return (selfUserStatus && selfUserStatus.status_text) || '';
+  };

Another use case for that selector :slightly_smiling_face:
+  handleStatusTextUpdate = () => {
+    const { dispatch } = this.props;
+    const { statusText } = this.state;
+    dispatch(updateUserStatusText(statusText));
+    dispatch(navigateBack());
+  };
+
+  handleStatusTextClear = () => {
+    const { dispatch } = this.props;
+    dispatch(updateUserStatusText(''));
+    dispatch(navigateBack());
+  };

Hmm -- perhaps the "clear" case should change the text to '', and then call the "update" case?
+  handleStatusTextChangeText = (statusText: string) => {
+    this.setState({
+      statusText,
+    });
+  };

This is a bit confusing -- it's basically named after one call site, the onChangeText of the input, but then we use it in another place too where onChangeText is definitely not what just happened.
Perhaps it'd be clearer to name it after what it does? E.g. setStatusTextState. In particular, if it weren't for the unfortunate issue of identity across re-renders, I think we wouldn't have a name for this at all and would just write e.g. statusText => this.setState({ statusText }).
... Hmm, in fact it looks like you are already giving up identity across re-render!
            onPress={() => {
              this.handleStatusTextChangeText(statusSuggestion);
            }}

If you can fix that reasonably, that'd be good.
If you can't... might as well at least make it clearer to read, by writing the setState directly.
+export default [
+  '📅 In a meeting',
+  '🚌 Commuting',
+  '🤒 Out sick',
+  '🌴 Vacationing',
+  '🏠 Working remotely',
+];

These should probably get translated. See LanguagePicker (since #3231) for an example of how to do that cleanly.
Hmm, more precisely: I think in your current version, OptionButton will try to translate them for the labels, but the actual value entered will be untranslated. (And no translation will happen anyway because they're not in the messages list.) Anyway, we should translate them.

Separately: some of these feel like they ought to go with an "away" state. But this UI is no more awkward than the webapp seems to be for this, so shrug.

Add Set a status button to ProfileCard

Backticks mean code :slightly_smiling_face: . You want double-quotes, I think.
Also there's an unclosed backtick in the commit-message body. Small but it sticks out at me.

Add getAvatarFromUser and getAvatarFromMessage

Good idea!

What about in Avatar? Very similar logic there too.

-            avatarUrl={message.avatar_url || getGravatarFromEmail(message.sender_
email)}
+            avatarUrl={getAvatarFromMessage(message)}

This code differs slightly from old to new: in the old version, an avatar_url of '' would fail over to getGravatarFromEmail. Was that case impossible? Or, is the new behavior the right thing in that case anyway? Deserves a few words in the commit message.

Also, that would have been impossible in the first place if that file had @flow strict-local. A good followup would be to go and make it that way now. Looks like there's just one obstacle to that, and it looks like an easy one.

On another PR we discussed this name PropsFromState, which should be something else.

That was written at the time when I wrote the other commit. We have agreed on PropsFromConnect but also it does not matter for this commit so I reverted it to the usual Props.

Hmm -- perhaps the "clear" case should change the text to '', and then call the "update" case?

We close the screen after the button press, so that step is not needed.

About this: https://github.com/zulip/zulip-mobile/pull/3267#discussion_r255269225
I did simplify the OwnAvatar even further, removing the shape as prop.
After refactoring the avatar/gravatar logic in a later commit it is almost as simple as just using Avatar (maybe 1 line of code difference). We need the component to get the data out of the user anyway.
Also, currently we don't have a way to not show the PresenceStatusIndicator in Avatar but we don't want it in OwnAvatar.
These are the reasons that justify this component's existence for me.
As for whether Avatar is needed, I am not sure, maybe we can refactor it later indeed.

Perhaps that TextAvatar path in Avatar never gets used at all?

It is currently used in TitleGroup. It makes sense to make the usage in that file and in GroupPmConversationItem the same. There is some (minor) value in abstracting away how an avatar is rendered by just using Avatar in both places.

As for whether Avatar is needed, I am not sure, maybe we can refactor it later indeed.

Perhaps that TextAvatar path in Avatar never gets used at all?

It is currently used in TitleGroup. It makes sense to make the usage in that file and in GroupPmConversationItem the same. There is some (minor) value in abstracting away how an avatar is rendered by just using Avatar in both places.

Searching their tracker, this looks a lot like prettier/prettier#770. ... But that was supposedly fixed in Prettier 1.3.0, and we're on 1.12.1. Hm.

I did update to the latest one 1.16.4 and tried it, the same result.

Hmm interesting. Shouldn't this go before we create ProfileCard, then? Or at least before we wire it up in the navigation.

I did move it before the creation of ProfileCard and moved the changes to this file to its creation.

Also this looks in the code like it will change the layout -- the button was previously inside a ComponentList. Haven't yet compared visually.

It doesn't change visually.

I did create several user status selectors (with docs) and did rework the other commits to use them.

After several smaller changes according to comments and few visual improvements, this should be ready now.

About this: #3267 (comment)
I did simplify the OwnAvatar even further, removing the shape as prop.
After refactoring the avatar/gravatar logic in a later commit it is almost as simple as just using Avatar (maybe 1 line of code difference). We need the component to get the data out of the user anyway.
Also, currently we don't have a way to not show the PresenceStatusIndicator in Avatar but we don't want it in OwnAvatar.
These are the reasons that justify this component's existence for me.

Cool, thanks for this reply!
One of my concerns in the linked comment was that part of the commit message is mistaken, namely this bit:

does not render TextAvatar as it is only for group conversations

That part is still there. Regardless of what we do with the code, we need to make the commit message accurate.

After refactoring the avatar/gravatar logic in a later commit it is almost as simple as just using Avatar (maybe 1 line of code difference).

This sounds to me like further reason why we shouldn't have this duplication. The "1 line of code difference" describes the extra cost of using the common Avatar vs. using OwnAvatar. But the cost of having OwnAvatar is much greater than that: for a start, this file is 34 lines long.
But:

We need the component to get the data out of the user anyway.

Hmm, given the context in which we invoke this component (from MainTabs, rather than from an ordinary component), I agree that it'd be inconvenient to pass this data in from the caller. Shrug, then.

That still leaves

the commit message needs to be accurate
the jsdoc mentions three optional parameters -- only one of which is a parameter (the others come from connect), and none of which are optional.

The first of those two issues, I mentioned in my original review, which I linked to again last time. It's frustrating to have to keep repeating it.

In general:
Thanks @borisyankov for these revisions! I appreciate the factoring out of those selectors (complete with jsdoc! :heart: :writing_hand:), and the moving some details into jsdoc and reordering of some commits.
The first 7 of these commits (up through f5dd46993 "Remove 'SwitchandLogoutbuttons fromSettings") all look good, with the exception of my comments above about "Add OwnAvatar` component".
In the remaining 4 commits:
0d602b340 AccountDetails: show user status text if one is set
9bbc2ad65 Add `UserStatusScreen`
2a800e6dc Add 'Set a status' button to `ProfileCard`
a44282c7d Add `getAvatarFromUser` and `getAvatarFromMessage`

I'm a bit frustrated because each one of them has things I asked for in my previous review above that you've neither changed nor replied about. Would you please look back at those?

Hmm -- perhaps the "clear" case should change the text to '', and then call the "update" case?

We close the screen after the button press, so that step is not needed.

We do, but the field is still visible for a couple hundred ms during the animation.
I just played with this -- enter some text, then hit "Clear". It is disconcerting as the text stays there unchanged, looking like my request to clear it has been ignored. I then tried making the change I suggested above, and the immediate feedback of seeing the text get cleared is a definite improvement.
It also reduces duplication in the code.

I just tried playing with the UI from this branch.
The new ProfileCard and UserStatusScreen have a problem with vertical overflow -- they can be taller than the screen, and then they don't seem to scroll. Here's ProfileCard after I've set a status:

That's on an emulated Pixel 2.
... And actually now I can't even get to UserStatusScreen, because the button has been scrolled completely off the bottom. (Before setting a status, the button is visible but partly cut off.) But it was bad too -- the buttons get covered up by the keyboard, which makes the UI extremely confusing unless you already know (or guess) that they're hiding there and try closing the keyboard.
Should also check what it's like for AccountDetailsScreen when the user has set an away status.

Perhaps that TextAvatar path in Avatar never gets used at all?

It is currently used in TitleGroup.

Is it? How's that?
I just looked again at the relevant logic. I'm now pretty confident the truth is that the TextAvatar codepath is unreachable.
Here's that logic:
    const fullAvatarUrl =
      typeof avatarUrl === 'string' ? getFullUrl(avatarUrl, realm) : getGravatarFromEmail(email);
    const AvatarComponent = fullAvatarUrl ? ImageAvatar : TextAvatar;

So the question is if fullAvatarUrl can ever be falsy.
Here's getGravatarFromEmail:
export const getGravatarFromEmail = (email: string = '', size: number = 80): string =>
  `https://secure.gravatar.com/avatar/${md5(email.toLowerCase())}?d=identicon&s=${size}`;

Nope, never falsy.
Here's getFullurl:
export const getFullUrl = (url: string = '', realm: string): string =>
  !url.startsWith('http') ? `${realm}${url.startsWith('/') ? '' : '/'}${url}` : url;

Rather a mess in a number of ways, but setting that aside -- this will never be falsy either!

If url is a nonempty string, neither branch will be falsy.
If url is '', we get the first branch... which will be a nonempty string. (Even if realm is something bogus, we'll get /.)
Even if a null sneaks in here, that will just blow up at startsWith. And undefined would do the same except it gets replaced with '' before that.

Perhaps I'm missing something here! If so I will be glad for you to show it to me.
Even then, though, I'm disappointed that you didn't make the effort to set it out clearly in the previous comment. It's rather a lot more work in general to demonstrate that a given code path isn't ever taken than it is to demonstrate that it is -- the former has to cover all possibilities, while the latter need only cover one -- but here I am doing that work on the harder side, and you haven't done the courtesy of sharing your thinking on the easier side.

Perhaps I'm missing something here! If so I will be glad for you to show it to me.

I was referring to Avatar being used there... assuming that we were using the TextAvatar for a group icon in the title bar. After your comments and reading through code/history I realized I was misremembering that.
Apparently, originally the intention was not to decide between User / Group for TextAvatar but user vs user without an avatar. Currently, indeed, we do not have this case. Not sure if that ever was true, but if so, was a very long time ago.
I have started refactoring Avatar, removing the AvatarComponent, fixing some of the types, for example https://github.com/zulip/zulip-mobile/commit/ef209dbb8c9399a9250ea51f496faefa9905aed6#diff-253b256c35ad836307b741a080fd2b1a fixed several issues but made the === 'string' check incorrect in Avatar.
I am also making it take a user prop instead of the other params.

Another refactor I think makes sense after that is to rename:
Avatar => AvatarWithPresence
'ImageAvatar=>UserAvatar TextAvatar=>GroupAvatar`

Apparently, originally the intention was not to decide between User / Group for TextAvatar but user vs user without an avatar. Currently, indeed, we do not have this case. Not sure if that ever was true, but if so, was a very long time ago.

Yeah, that story makes sense.

I have started refactoring Avatar, removing the AvatarComponent, fixing some of the types [...] I am also making it take a user prop instead of the other params.
Another refactor I think makes sense after that is to rename:

Avatar => AvatarWithPresence
ImageAvatar => UserAvatar
TextAvatar => GroupAvatar

These all sound good!

I think we should merge this independently and should not delay this PR.

Sure. The things currently standing between this PR and a merge are

the items after the line in this comment, on OwnAvatar
the still-outstanding items from the previous review, referred to in this comment
the small UI issue in this comment (could merge with this, but I'd definitely want it addressed in followup)
the major UI issues in this comment

A summary of what id the 'diff' with the latest reviewed version:

the items after the line in this comment, on OwnAvatar

Major rework of how the getGravatar* functions work.
Fixes inconsistencies, potential bugs and size issues.

the still-outstanding items from the previous review, referred to in this comment

For OwnAvatar i did fix the commit message, the jsdoc was fixed too

the small UI issue in this comment (could merge with this, but I'd definitely want it addressed in followup)

I did fix it so the 'Clear' action does show in the UI. Could have added even a bit of delay, since I had to use setTimeout anyway, but not sure it was a good idea, so I didn't.

the major UI issues in this comment

Major rework of the component. Now everything fits in small screens too.

I am not sure if I am missing anything as the comments started hiding and links stopped working...

Thanks @borisyankov for these revisions!

Re comments and links to them: as we discussed in audio chat today, what I'd like to be standard procedure -- and this is quite important to me -- is that when resubmitting a branch for review, you go back through every review comment, item by item, and make sure you've addressed each one. (Meaning: either done as suggested, or mentioned that you aren't doing it and why.)
When you do that, it won't matter if some links to comments don't work -- because you're going through every comment anyway.
This might require explicitly expanding some comments in the UI, because GitHub is being GitHub. A bit annoying of a UI, but it works fine.

I'm going to merge these commits in a bit:

bccde1d37 userStatusSelectors: Create three user-status selectors
88d7ff6f2 Add `OwnAvatar` component
304fab86b Add `AwayStatusSwitch` component
d79bf1bbd Rework `AccountDetails` to be more generic
e2cc51ebf Avatar: Do not use `TextAvatar` and simplify
8d4941e65 Rename `ImageAvatar` to `UserAvatar`
4cc9bed38 Rename `Avatar` to `UserAvatarWithPresence`

I quite like the direction of the changes to the avatar code! Specific comments below on the commits I'm not yet merging, but in general I think this makes very good progress to making that code more understandable and robust.

I'm still not fully satisfied with the various avatar components, but this is now good enough that I'm willing to merge OwnAvatar. I know you have some further refactors coming on that, which I'm looking forward to; and there are some specific things I'm interested in, which I can describe later.

Small bit of commit-message style, in "e2cc51ebf Avatar: Do not use TextAvatar and simplify":
For mentioning a commit, instead of a long link like https://github.com/zulip/zulip-mobile/commit/ef209dbb8c9399a9250ea51f496faefa9905aed6, let's write just the commit ID (the "sha1"). And just a short form -- 7 hex digits is Git's default, and I use 9, which will last even for a giant repo.
This makes it much more readable in prose. It also makes it much more convenient for use at the command line, and transparent for other tools like gitk -- every tool gets to linkify it internally in its own UI, rather than making you go to a browser and wait for GitHub to load.

"b567f28be Add getAvatarFromUser and getAvatarFromMessage"
This is one with some unaddressed earlier comments.

"4c6b2601e Rename TextAvatar to GroupAvatar"
This looks good! Except that a couple of these lines should raise mental question marks :stuck_out_tongue: -- most of all this one:

+ * @prop name - User's names to extract initials from.

What is this name prop anyway? It's a name, but also several names?
Whatever the real answer is, the text and likely the name of the prop should be adjusted to all agree on it.

"82dcdc3cd Add getAvatarFromParams function"

Huh, "from params" is kind of a funny thing to say in a function's name. :slightly_smiling_face: What is that intended to mean in this case? Naturally it is using the parameters it's passed... what is this name intended to say that isn't contained in a name like getAvatar?

Perhaps you were aiming to distinguish it from a selector? I think the "FromFoo" doesn't totally accomplish that anyway; but computeAvatar would be one way to do it. Also "...AvatarUrl" would be more accurate.

avatarUrl: ?string -- this type is too vague, permitting null when that's not what we mean. Let's say string | void instead.

This combines a nontrivial refactor with a change in behavior for getAvatarFromMessage. Both changes are good, but combining them in one commit makes them both confusing for the reader to unpack -- better to split them into two commits.

"2dd1179e5 Make all avatar-genrating functions take a 'size' parameter"
This makes a lot of sense, thanks! As a small bit of style: I'm not a fan of duplicating the choice of default value (80) to four different places. Most of these don't actually care at all what the default is, only that it's optional -- how about just writing size?: number there? Then when it's absent, the single default at the very inside will take effect.

"947637ccb Use getMediumAvatar if larger avatar needed"
Makes sense. Let's move that explanation (especially about the two sizes default and medium and where the number 100 comes from) into a comment inside the function -- it's something that's important for understanding the new code as it is, not only the diff between old and new.
Also in general: no need to duplicate information between a commit message and a comment. When you move information like that into a comment, go ahead and take out the details in the commit message that just repeat it.

"ee4f669bb Redesign AccountDetails to fit screens better"
Cool, I like this UI change!
The commit is a bit confusing because it silently makes a mostly-unrelated (though also good) change: it cuts where we'd been showing the presence indicator overlaid on the avatar.
That is certainly a good change! It's basically one of the payoffs of the previous refactors to the avatar components. Best to make it a separate commit, so its commit message can properly celebrate. :smiley:
(If we were determined to keep it one commit, it would in any case absolutely need to be mentioned in the commit message. But by far the clearest way to do that is to make it its own commit.)

The next three commits look good (as they did before); leaving unmerged because they depend on others above.

The next three each have unaddressed comments from before:

86944d053 AccountDetails: show user status text if one is set
bc55720e9 Add `UserStatusScreen`
dc90c8ff1 Add 'Set a status' button to `ProfileCard`

So please read through my comments above and take care of those.

More on "bc55720e9 Add UserStatusScreen":
Thanks for the change to the "clear" case here!
I'm curious about the setTimeout. Do you believe it should be needed in principle; or did you find that in a certain environment it was empirically needed? When I tried this change the other day, it WFM without that; but I'm not sure whether it should be needed in principle.

Pushed those 7 commits as 70d406709..f37856207. Thanks again!

(Quick thing I neglected to mention on Friday as I was rushing out the door:

in general: no need to duplicate information between a commit message and a comment. When you move information like that into a comment, go ahead and take out the details in the commit message that just repeat it.

I went ahead and did this on a few of the commits I merged, before merging them. You had kindly put information there into comments in response to a previous round of review (thanks!), and I hadn't yet mentioned this finer point. As you revise the remaining commits, please make this change for them too.)

I did push an update. Changes from the last version:
Added a commit 'ComponentList: Fix a layout issue adding extra space at bottom'
It fixes a bug that adds an extra space at the bottom of the list of components if the last component is null. While that seems it would occur rarely, if ever, this happens in AccountDetails if the timezone prop is not set (we render a null).

Small bit of commit-message style, in "e2cc51ebf Avatar: Do not use TextAvatar and simplify":
For mentioning a commit, instead of a long link like https://github.com/zulip/zulip-mobile/commit/ef209dbb8c9399a9250ea51f496faefa9905aed6, let's write just the commit ID (the "sha1"). And just a short form -- 7 hex digits is Git's default, and I use 9, which will last even for a giant repo.

Makes sense. I will use 7 digits from now on.

"b567f28be Add getAvatarFromUser and getAvatarFromMessage"
This is one with some unaddressed earlier comments.

I am not sure what this is referring to. If it is the comments about the Avatar component that is now called UserAvatarWithPresence - the comments are referring to an old implementation. Currently we reuse the same code used everywhere else via getAvatarFromParams.

"4c6b2601e Rename TextAvatar to GroupAvatar"
This looks good! Except that a couple of these lines should raise mental question marks 😛 -- most of all this one:

+ * @prop name - User's names to extract initials from.

What is this name prop anyway? It's a name, but also several names?
Whatever the real answer is, the text and likely the name of the prop should be adjusted to all agree on it.

I did several changes that resulted in a new commit 'GroupAvatar: rename prop name, clarify usage'

"82dcdc3cd Add getAvatarFromParams function"

Huh, "from params" is kind of a funny thing to say in a function's name. 🙂 What is that intended to mean in this case? Naturally it is using the parameters it's passed... what is this name intended to say that isn't contained in a name like getAvatar?

Perhaps you were aiming to distinguish it from a selector? I think the "FromFoo" doesn't totally accomplish that anyway; but computeAvatar would be one way to do it. Also "...AvatarUrl" would be more accurate.

The weird naming was selected to correspond to the getAvatarFromUser and getAvatarFromMessage functions.
If it wasn't that clear it is the similar (the same signature) the drawback of that name is not justified.
Now I renamed it to getAvatarUrl.

avatarUrl: ?string -- this type is too vague, permitting null when that's not what we mean. Let's say string | void instead.

I left this as-is as there are 7-8 other places that refer to that same type.
I added another commit called 'flow: Specify avatar_url more precisely' that addresses this.

This combines a nontrivial refactor with a change in behavior for getAvatarFromMessage. Both changes are good, but combining them in one commit makes them both confusing for the reader to unpack -- better to split them into two commits.

I did extract the change that made getAvatarFromMessage support a realm parameter into a separete commit:
'getAvatarFromMessage: Take into account current realm'
The refactor itself (and only it) remains in the previous commit.

"2dd1179e5 Make all avatar-genrating functions take a 'size' parameter"
This makes a lot of sense, thanks! As a small bit of style: I'm not a fan of duplicating the choice of default value (80) to four different places. Most of these don't actually care at all what the default is, only that it's optional -- how about just writing size?: number there? Then when it's absent, the single default at the very inside will take effect.

Good find. I did remove the defaults. Added details in the commit message.
Also added a change to getGravatarFromEmail in another commit right after it:
'getGravatarFromEmail: Remove default value for size'

"947637ccb Use getMediumAvatar if larger avatar needed"
Makes sense. Let's move that explanation (especially about the two sizes default and medium and where the number 100 comes from) into a comment inside the function -- it's something that's important for understanding the new code as it is, not only the diff between old and new.
Also in general: no need to duplicate information between a commit message and a comment. When you move information like that into a comment, go ahead and take out the details in the commit message that just repeat it.

I did add a JSDoc with that information to getMediumAvatar and removed the duplications from the commit message.

"ee4f669bb Redesign AccountDetails to fit screens better"
Cool, I like this UI change!
The commit is a bit confusing because it silently makes a mostly-unrelated (though also good) change: it cuts where we'd been showing the presence indicator overlaid on the avatar.
That is certainly a good change! It's basically one of the payoffs of the previous refactors to the avatar components. Best to make it a separate commit, so its commit message can properly celebrate. 😃
(If we were determined to keep it one commit, it would in any case absolutely need to be mentioned in the commit message. But by far the clearest way to do that is to make it its own commit.)

I did split this into two commits.
The new one is called:
'AccountDetails: Use getAvatarFromUser and realm within'

This logic feels to me like it's calling for a (very simple) selector. Best if the component doesn't have to deal with the nuances of what it means for the whole object to be missing from the map, vs. the object to be present but have undefined for status_text.

I did rework the commit and extracted the logic as a selector.

At a minimum, there should be a comment inside that tiny file saying why it's there -- it's a peculiar way for the code to be, and so it should provide an answer to the reader's natural question.

I did add comment in userStatusTextSuggestions.js

Backticks mean code 🙂 . You want double-quotes, I think.
Also there's an unclosed backtick in the commit-message body. Small but it sticks out at me.

Fixed.

More on "bc55720e9 Add UserStatusScreen":
Thanks for the change to the "clear" case here!
I'm curious about the setTimeout. Do you believe it should be needed in principle; or did you find that in a certain environment it was empirically needed? When I tried this change the other day, it WFM without that; but I'm not sure whether it should be needed in principle.)

I tested this and apparently I was wrong we needed it for the state to be reflected in the UI.
What is funny, we still needed it to have the state be mutated before the updateStatusText call.
The state mutations are batched so that could have been expected.
I wasn't sure setTimeout guarantees us the state would be mutated, though I think it does.
I also experimented with setState and a second parameter being a call to updateStatusText - that is now guaranteed to be after the mutation.
Instead of keeping this, I reworked the code a bit to be more straight-forward and not require that deep knowledge of how state mutations work.

Thanks @borisyankov for all these revisions! I especially appreciate that you went through previous comments and addressed most of them.
This is quite close -- if it were earlier in the day I'd be going and merging the bulk of it.

One key thing that's still open is the UserStatusScreen portion of this UI issue:

The new ProfileCard and UserStatusScreen have a problem with vertical overflow -- they can be taller than the screen, and then they don't seem to scroll. [... in UserStatusScreen] the buttons get covered up by the keyboard, which makes the UI extremely confusing unless you already know (or guess) that they're hiding there and try closing the keyboard.

Replying to one question of yours above:

I am not sure what this is referring to. If it is the comments about the Avatar component that is now called UserAvatarWithPresence [...]

I was thinking of this comment, which is about code in Lightbox (quoted above the comment):

This code differs slightly from old to new: in the old version, an avatar_url of '' would fail over to getGravatarFromEmail. Was that case impossible? Or, is the new behavior the right thing in that case anyway? Deserves a few words in the commit message.

Also this comment which suggests a followup:

Also, that would have been impossible in the first place if that file had @flow strict-local. A good followup would be to go and make it that way now. Looks like there's just one obstacle to that, and it looks like an easy one.

Other items that potentially we can leave for followups, but would be good to take care of:

In the pre-supplied values for UserStatusScreen:

These should probably get translated. See LanguagePicker (since #3231) for an example of how to do that cleanly.
Hmm, more precisely: I think in your current version, OptionButton will try to translate them for the labels, but the actual value entered will be untranslated. (And no translation will happen anyway because they're not in the messages list.) Anyway, we should translate them.

In "Redesign AccountDetails to fit screens better": it'd be good to write "200" just once, as it's an arbitrary size that it's important to keep matching between the two places we mention it. Just a local variable like const avatarSize = 200 would do nicely, I think.

About the Prettier issue in UserStatusScreen:

Better, let's be good citizens and do something about the issue :wink: . Looks like there are newer releases of Prettier -- see if the issue goes away on upgrade? If not, file a bug with Prettier upstream.

It's the end of my day now, but when I get to my desk tomorrow I plan to go about merging this.
If by that point you've resolved the "covered up by the keyboard" issue, then I expect to merge the whole thing.
If you can also take care of the other items in my previous comment, that would be great. If not, that will just mean a few more followup items. Thanks again!

One key thing that's still open is the UserStatusScreen portion of this UI issue:

The new ProfileCard and UserStatusScreen have a problem with vertical overflow -- they can be taller than the screen, and then they don't seem to scroll. [... in UserStatusScreen] the buttons get covered up by the keyboard, which makes the UI extremely confusing unless you already know (or guess) that they're hiding there and try closing the keyboard.

I did rework the UserStatusScreen - now it uses a FlatList that only takes as much of the screen as there is space. Also, the buttons are now always on screen. That seems to work quite well on smaller screens.
For ProfileCard I replaced the View with a ScrollView and that is enough to fix the now rare case that we need to scroll this screen (after making sure in previous changes to not add extra space)

I was thinking of this comment, which is about code in Lightbox (quoted above the comment):
This code differs slightly from old to new: in the old version, an avatar_url of '' would fail over to getGravatarFromEmail. Was that case impossible? Or, is the new behavior the right thing in that case anyway? Deserves a few words in the commit message.

Supposedly impossible. We either have an actual avatar url or null.

Also, that would have been impossible in the first place if that file had @flow strict-local. A good followup would be to go and make it that way now. Looks like there's just one obstacle to that, and it looks like an easy one.

I think we have @flow strict-local now or we are talking about different files?

Other items that potentially we can leave for followups, but would be good to take care of:

In the pre-supplied values for UserStatusScreen:

These should probably get translated. See LanguagePicker (since #3231) for an example of how to do that cleanly.
Hmm, more precisely: I think in your current version, OptionButton will try to translate them for the labels, but the actual value entered will be untranslated. (And no translation will happen anyway because they're not in the messages list.) Anyway, we should translate them.

We should ship as-is but will do this in subsequent commit.

In "Redesign AccountDetails to fit screens better": it'd be good to write "200" just once, as it's an arbitrary size that it's important to keep matching between the two places we mention it. Just a local variable like const avatarSize = 200 would do nicely, I think.

Done.

About the Prettier issue in UserStatusScreen:
Better, let's be good citizens and do something about the issue 😉 . Looks like there are newer releases of Prettier -- see if the issue goes away on upgrade? If not, file a bug with Prettier upstream.

Tested with the new version and it wasn't fixed.
But I did try many variations in order to reproduce a minimal verrsion for a bug report.
A breakthrough! It seems the default eslint/prettier settings do not cause the issue.
I'll continue testing to see what is the exact thing that causes it.

Thanks again @borisyankov for these revisions!
On UserStatusScreen, I'm still seeing the buttons obscured by the keyboard, and no scroll. This is
with commit b32103e34 (the one you pushed a few minutes ago), with a FlatList there. Screenshot:

Huh, the plot thickens! It's not 100% of the time -- here's another screenshot, where the buttons are nicely visible:

And the suggestions list scrolls.
That's what I saw immediately after switching apps back to Zulip, from the system settings where I was browsing around for some details.
I'll poke a bit more. Perhaps you also have ideas?

Hmm, now it mostly doesn't reproduce, in either of two emulators. I'm not sure what the pattern is.
Some observations and tentative patterns:

I observed it several times after reloading in the dev popup menu, and after re-running react-native run-android to reinstall a current build of the APK. So that rules out some ways it could have been just stale code from a different version of the branch.

But, at some point the Metro bundler (react-native start) crashed and I restarted it... and I think I haven't seen it since then. So possibly the bundler was serving stale code? ... alternately with serving good code? Grr, if so.

I think I only ever saw it when navigating to UserStatusScreen was the very first time the keyboard showed up since starting the app. That could be a clue.
The keyboard often behaved funny, as it sometimes does in debug builds in the emulator.

E.g., I'd hit r r to reload... and that would bring up the keyboard, and it'd stay up when the app loaded and showed the main nav screen, even though the keyboard makes no sense there.
So it's possible whatever bug this was only applies in the emulator and/or in a debug build.

Cool, I'm going to go ahead and merge -- :crossed_fingers: that that puzzling symptom won't recur in the wild. If it does, we'll debug it then.

Thanks again for the revisions!
I think you have several followup items, some of them quick -- and you mentioned some in your last comment.
To the question in your last comment:

I think we have @flow strict-local now or we are talking about different files?

I meant Lightbox.js. Which doesn't yet, so this is one open followup item.

I meant Lightbox.js. Which doesn't yet, so this is one open followup item.

Oh yeah, sure, will do that next.
Working on a few follow-up commits, but seeing the agreed changes have not made it to the web yet, we would be fine for that release without them (different icon for 'unavailable' etc.)

And merged! Thanks again for all your work on this.
I went and split up a couple of the commits that needed splitting. In one case, one of the needed commits was basically one of the commits in @jainkuniya 's #3362... so I cherry-picked that commit here :stuck_out_tongue_closed_eyes: .
There are various follow-up items mentioned in the thread above, and there are some further cleanups to the avatar logic that I have in some terse notes locally but I don't think I've described. I'll try to collect all those followups in a comment after this one, so they don't fall through the cracks. (I know you're already working on some of them.)
And then I'll make a new release with this change!

Follow-up items, with details in various comments above (Ctrl+F is effective after you tell GitHub to expand the middle of the thread; that widget in turn can be found by Ctrl+F for "hidden item"):

Lightbox strict-local
Translations for pre-supplied values for UserStatusScreen
File a bug with Prettier upstream for that Unicode (perhaps specifically non-BMP) issue

Other follow-up items to clean up the *Avatar* components:

A number of props are redundantly described across several components out of UserAvatar, UserAvatarWithPresence, and OwnAvatar; in types, jsdoc, and defaults. I think this is the root cause of a number of specifics mentioned below.

Fortunately, this is a solvable problem! OwnAvatar and UserAvatarWithPresence can defer to UserAvatar for these details, getting things DRY.
For examples, see the various *Input* components that wrap Input. In particular InputWithClearButton, PasswordInput, and commits a15c446d2 and 31523e10b on the late MultilineInput.

Absolute vs. potentially-relative URLs:

UserAvatar takes absolute only, while UserAvatarWithPresence takes potentially-relative. They should be made consistent, one way or the other -- pushing the call to getAvatarUrl either inward to UserAvatar or outward to callers.
Inward makes OwnAvatar still simpler, and it better aligns UserAvatar's interface with its name. So I think that's probably preferable.
Also the jsdoc on UserAvatar falsely says it takes potentially-relative.

Defaults for shape:

Redundant defaults to rounded at several layers -- should be at just one, similar to size in this thread.
Divergent defaults, to rounded and circle. There's no reason showing the self-user's own avatar should inherently be associated with a different shape from showing another user's -- so the choice of circle is better put at the caller.

(With the *Input* technique mentioned above for typing a wrapper component, it becomes basically zero-boilerplate for OwnAvatar to offer the shape prop, which helps here.)

Type for shape: it's written as string on the inner component, but really has a more specific type.
Defaults of bogus empty-string values, in UserAvatarWithPresence for avatarUrl, email, and realm.

Other follow-up items:

The logic in getMediumAvatar is fairly messy. For one thing -- why is there a //g option on that regexp? That seems like if it had any effect it would be a bug.
realm_message_retention_days: ?number in InitialDataRealm can't be quite right; see below.
I plan to make a pass over the various jsdoc involved in this branch for some small style adjustments.
I'll do a tx push and pull; that will conveniently sync the new strings into all the languages' message catalogs, as well as present them to our kind translators.

Other comments:

In one of today's revisions I noticed the commit that narrowed the avatar_url types changed from saying string | void to saying string | null -- because it turns out the server sends null. That is good information that I'm glad we learned! That's the kind of detail that I hope to learn and record when making the effort to write down good specific types. :slightly_smiling_face:

Hmm, in fact that reminds me of a general point: there is no undefined in JSON. There's null, and there's a property just not existing. So any time we have a ?foo or a foo | void in our types for data we get from the server, that's almost certainly mistaken.
At a quick grep, I see just one remaining example of that: realm_message_retention_days: ?number in InitialDataRealm. So that'd be good to clear up too.

Thanks for this description:

What is funny, we still needed it to have the state be mutated before the updateStatusText call.
The state mutations are batched so that could have been expected.
I wasn't sure setTimeout guarantees us the state would be mutated, though I think it does.
I also experimented with setState and a second parameter being a call to updateStatusText - that is now guaranteed to be after the mutation.
Instead of keeping this, I reworked the code a bit to be more straight-forward and not require that deep knowledge of how state mutations work.

This all makes sense! And I like the resulting code.

Thanks for this change in AccountDetails! I appreciated the additional cleanup:

I did split this into two commits.
The new one is called:
'AccountDetails: Use getAvatarFromUser and realm within'

If you look back I think you'll see that that the split I was asking for was something different and independent:

The commit is a bit confusing because it silently makes a mostly-unrelated (though also good) change: it cuts where we'd been showing the presence indicator overlaid on the avatar.

So I took care of that split before I merged today. That was the commit I took from #3362 . :wink:

Oh, one other small cleanup that this is likely a good time for, which I just happened to see in the codebase:
$ rg -iA1 fixme.*realmbot
src/title/TitlePrivate.js
21:    // $FlowFixMe: sort out RealmBot
22-    const avatarUrl: string | null = user.avatar_url;

src/pm-conversations/PmConversationList.js
53:            // $FlowFixMe: sort out RealmBot
54-            const avatarUrl: string | null = user.avatar_url;

In reality I think cross-realm bots have avatars? But we're always falling back to gravatars for them. Seems the data is getting misplaced somewhere.


Adding a new TLS secret for ZK.

@bleporini , thank you for submitting the PR, what's the reason for it? is it just to split the broker and zookeeper certs?
I think you would also need to change the appendix cert name if you renaming all of the certs.

Hi @bleporini - in this scenario we walk through how to have separate certs per component: https://github.com/confluentinc/confluent-kubernetes-examples/tree/master/security/userprovided-tls_mtls_confluent-rbac
That scenario uses these scripts to create server certificates per component: https://github.com/confluentinc/confluent-kubernetes-examples/tree/master/assets/certs/component-certs
With this other scenario in place, is there a benefit to this Zookeeper separation here?

Sorry I forgot the description. In fact the way certificates are generated has changed: initially ZK and Kafka used the same certificate server.pem, but at some point it became kafka-server.pem and zookeeper-server.pem and there was no server.pem  generated. So the procedure detailed in https://github.com/confluentinc/confluent-kubernetes-examples/tree/master/security/production-secure-deploy failed and replacing server-key.pem with kafka-server-key.pem makes ZK cluster failing because of hostname verification during TLS handshake:
[ERROR] 2021-08-23 14:31:24,008 [QuorumConnectionThread-[myid=0]-1] org.apache.zookeeper.common.ZKTrustManager performHostVerification - Failed to verify hostname: zookeeper-1.zookeeper.confluent.svc.cluster.local
 970   │ javax.net.ssl.SSLPeerUnverifiedException: Certificate for <zookeeper-1.zookeeper.confluent.svc.cluster.local> doesn't match any of the subject alternative names: [kafka, *.my.domain, kafka.confluent.svc.cluster.local, *.kafka.confluent.svc.cluster.local]

The only purpose for this PR is to fix production-secure-deploy/ confluent-platform-production.yaml.

@bleporini you're still however missing zookeeper-server.pem , this is never generated


[RemoteMirror][swift-inspect] Decode locks in priority-escalation concurrency runtime.

When possible, decode the DrainLock/ExecutionLock fields of tasks and actors in concurrency runtimes built with priority escalation, and show the corresponding thread info in swift-inspect output.
We weren't properly decoding actor flags previously, so fix that up as well and have Remote Mirror split them out into separate fields so clients don't have to. We were missing the Job Storage field from the definition of DefaultActorImpl in RuntimeInternals.h, fix that so we actually read the right data.
rdar://88598003

I extracted the actor flags constants into a common header, and switched all the booleans to actual bools.

@swift-ci please test