Skip to content
This repository has been archived by the owner on Dec 12, 2018. It is now read-only.

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

Merged
merged 2 commits into from
Sep 7, 2017
Merged

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

merged 2 commits into from
Sep 7, 2017

Conversation

bdemers
Copy link
Contributor

@bdemers bdemers commented Aug 24, 2017

No description provided.

@bdemers bdemers requested a review from dogeared August 24, 2017 21:12
@bdemers
Copy link
Contributor Author

bdemers commented Aug 24, 2017

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

Copy link
Member

@dogeared dogeared left a comment

Choose a reason for hiding this comment

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

👍 :shipit:

@george-hawkins-work
Copy link

george-hawkins-work commented Aug 29, 2017

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
Copy link
Contributor Author

bdemers commented Sep 6, 2017

@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
Copy link

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
Copy link
Contributor Author

bdemers commented Sep 7, 2017

@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!)

@bdemers bdemers merged commit 4a239cc into okta Sep 7, 2017
@bdemers bdemers deleted the custom-data-post branch September 7, 2017 20:23
@george-hawkins-work
Copy link

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants