stormpath / stormpath-sdk-java

Official Java SDK for the Stormpath User Management REST API
222 stars 155 forks source link

Use post for /api/v1/users updates #1356

Closed bdemers closed 7 years ago

bdemers commented 7 years ago

re: #1354 @george-hawkins-aa give try this out

george-hawkins-work commented 7 years ago

OK - as remarked in my comment on #1354 I can confirm that the change in this pull request to DefaultDataStore resolves the issue of losing custom data on doing a password reset.

I would point out that the original cause of the problem was the change in how custom data is handled now vs pre-Okta.

Previously custom data got stored in an instance of a concrete implementation of the CustomData interface. As part of the switchover it ended up being stored in a plain old LinkedHashMap.

As a result the special case handling based around checking for CustomData no longer works (see my earlier comment on #1354).

So the change by @bdemers means that missing fields, i.e. custom data, won't get deleted now the user profile is POSTed rather than PUT. However the fields wouldn't be missing in the first place if the special case handling around CustomData still kicked in.


Anyway - I think custom data should either be stored as a CustomData instance or if you've decided that LinkedHashMap is the right way to go then CustomData and the related checks etc. should be gotten rid. At the moment they just confuse things if they're just hanging around as remnants of an earlier age.


Whatever is decided can you push out a fix for this soon on Maven Central? Losing all custom data related to a user on password resets seems quite a big issue (I wonder how it wasn't picked up earlier by us or another customer that migrated from Stormpath). Sorry it took me 5 days to give feedback - we have password resetting disabled currently for our users and I've been racing around on other issues.


It's always good having the developers on a project, that you depend on, be on your side. So at the risk of @bdemers marking me as a jerk ("he's the guy who suggested there should be more unit tests!") I'd just like to comment a bit (more).

I do note that this pull request comes without a unit test.

Back in the pre-Okta days I'd often look at the changes commited by @dogeared and others in response to issues I'd logged and they generally came with a reassuring amount of tests etc.

And they often came with comments from people like @lhazlewood that implied that everything really was getting a proper code review. Often I might think some of the comments from @lhazlewood were a bit nitpicky but nitpicky is good when it comes to a security related framework.

I understand that you've probably been under extreme time pressure to handle the switch over to Okta, so keeping this discipline while meeting the August shutdown deadline may have been impossible.

But I hope things will calm down for you guys and that for future development you'll be able to devote the same level of time to code review, unit tests and such like as seen earlier.

bdemers commented 7 years ago

@george-hawkins-aa thanks for the feedback! We won't be changing this API significantly, just maintenance releases. The new Okta SDK models User Profile data (i.e. custom data) slightly differently (as it is not an independent endpoint, like you mentioned)

richard-hulm commented 7 years ago

Hi @bdemers is this likely to be merged and released soon?

This is a major issue with the reset password functionality for our application - so if not will have to fork and build from there

Thanks,

bdemers commented 7 years ago

@richard-hulm @george-hawkins-aa working on it as I type We ran into a few CI caused by a few Travis updates. (things are green again!)

george-hawkins-work commented 7 years ago

Hello @bdemers - thanks for releasing this fix in 2.0.2-okta. I know this isn't the right place to ask but there isn't an obvious better forum. You said above:

We won't be changing this API significantly, just maintenance releases.

Are the Stormpath frameworks for Spring Boot etc. essentially end-of-life then? I notice that the documentation for them doesn't seem to have resurfaced on the Okta site following the shuttering of the old Stormpath site - so I guess no new users are being encouraged to take up these frameworks?

It would be nice to get some clarity from e.g. @lhazlewood - it seems a little unfair to have encouraged us to migrate to Okta if the framework we were using are now essentially at a dead end - we'd have more carefully evaluated alternatives, e.g. Auth0, rather than migrating if we'd been aware that no real future development was planned for these frameworks 😞

If we are sticking with Okta is there a clear migration path (another one!) to an Okta blessed framework for Spring Boot that will be seeing more than just "maintenance releases"?


Also we're a little worried by #1358 - maybe the Stormpath code I reference isn't involved in what Okta call "self-validation" in which case this is a non-issue. But otherwise we'd obviously be a bit upset if things suddenly stop working on October 6th - the date Okta have announced for switching off this capability.