Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add: GUID Transformation #18

Conversation

JakubDolba
Copy link
Contributor

@JakubDolba JakubDolba commented Jun 12, 2019

this fixes #17

There is use-case for normalizing GUID format (upper vs. lower case) before saving GUID to Member

this functionality is adding simple callable option to change GUID format

  • any callable is acceptable so developer can apply custom functions

@JakubDolba JakubDolba force-pushed the feature/guid-transformation-callback branch from 293a588 to 6c05344 Compare June 12, 2019 22:30
@@ -147,6 +147,11 @@ public function acs()
$guid = $auth->getNameId();
}

$guidTransformation = Config::inst()->get(SAMLConfiguration::class, 'guid_transformation_callable');
if ($guidTransformation !== null) {
$guid = $guidTransformation($guid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice idea. How would you feel about adding an extension point instead? E.g. $this->updateGuid($guid) then extensions can call strtoupper() if they want to.

Copy link
Contributor Author

@JakubDolba JakubDolba Jun 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was thinking about that but this means you have to actually create extension and do something in code
typical use-case for this is simple upper/lower case so I thought that simple callable as string is much more simpler for that (ie. you don't need tuch code by using simple callable string)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. The difference is where you place the burden of maintenance - either on user code or in this module. Generally we place that burden on project or module developers which keeps our support footprint lighter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if extension is prefered way how to do this...

added a fixup commit; will rebase if new commit will be ok, also I will change title and descriptions (will remove "callable")

@JakubDolba JakubDolba changed the title Add: GUID Transformation Callable Add: GUID Transformation Jun 12, 2019
@JakubDolba JakubDolba force-pushed the feature/guid-transformation-callback branch 3 times, most recently from 5782b08 to a72a947 Compare June 20, 2019 03:56
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested docs change, assuming you've updated to use $this->extend()

@@ -149,6 +150,11 @@ SilverStripe\SAML\Extensions\SAMLMemberExtension:
- 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name': 'Email'
```

### GUID Transformation

If you prefer received GUID in lower-case or upper-case format you can extend and overload method:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you prefer received GUID in lower-case or upper-case format you can extend and overload method:
If you prefer to receive the GUID in lower-case or upper-case format you can use the `updateGuid()` extension point on `\SilverStripe\SAML\Control\SAMLController`.

…bility to store guid only as lowercase for example)
@JakubDolba JakubDolba force-pushed the feature/guid-transformation-callback branch from a72a947 to 7a0810c Compare June 24, 2019 21:47
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@robbieaverill robbieaverill merged commit e8c6a98 into silverstripe:master Jun 24, 2019
@JakubDolba JakubDolba deleted the feature/guid-transformation-callback branch June 25, 2019 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert guid to uppercase
2 participants