-
Notifications
You must be signed in to change notification settings - Fork 61
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
Split saml.ts - MR #1 #19
Conversation
Note: I always make sure that my individually commits makes sense on their own, compiles and pass the tests. I think it would be better not to squash them (I see that it's usually the case in my other PRs) as it makes future review harder for sure big PRs especially. |
I agree it would improve the code quality to have functions that only receive the pieces of data they need with more specific types then a big object. The approach seems sounds to me. |
@forty It looks like this PR is up to bat :) Would you like to fix the conflicts and we'll get it reviewed? As for not squashing commits, if you keep them nice and clean such that they stand alone, I'm OK with that. However, if they stand alone, why not make them separate PRs so that they are easier to review? |
@forty , I'm trying to land these PRs in order. Are you in a position to fix these merge conflicts so that I can review the code? |
4c1af42
to
e1b498b
Compare
@cjbarth Sorry for the late response, the idea of rebasing this got me a bit lazy 😓 It was a bit tough, but I think I managed 🎉 hopefully I have not reverted too many things. At least I had The good news is that the number of conflict I got on saml.ts alone is itself a good reason to split that file :D |
(And yes, 100% agree this should be split in several PRs, but the idea of this PR was to show the direction I wanted to go. I'll make smaller PRs for next ones) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I like the direction this is going, however, it seems there may have been some refactors that either break or complicate things in unexpected ways. I look forward to this landing though, huge improvements, not just from the refactor, but also a clarification and simplification of style.
This will be the next PR to land (baring a major security issue), so there should be no more tedius merge conflict fixing. Sorry about that, and thanks for being a good sport.
test/tests.spec.ts
Outdated
@@ -2312,12 +2312,6 @@ describe("node-saml /", function () { | |||
afterEach(async function () { | |||
await samlObj.cacheProvider.removeAsync("_79db1e7ad12ca1d63e5b"); | |||
}); | |||
it("errors if bad xml", async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea... probably a mistake, in a rebase or before. I have pushed a commit to put it back.
return await promiseWithNameID(decryptedIds[0]); | ||
} | ||
throw new Error("Missing SAML NameID"); | ||
return await processValidlySignedPostRequest(doc, dom, this.options.decryptionPvk ?? null); | ||
} | ||
|
||
generateServiceProviderMetadata( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears that this logic is now split. Here we check to make sure we have all the correct parameters, and in metadata.ts
we actually generate the metadata. It seems wiser to include these checks in metadata.ts
because the code that generates the metadata is also in the best position to tell if it has everything it needs to do so. It also keeps the logic all in one place.
Is there something I'm missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic I followed is that I let the argument validation/checking in saml.ts, and then in the smaller files, put stricter types in the signature.
The idea in that in /saml/ you would have lower level saml implementation, with rigid parameters and not very convenient to use (but simple to review and clear), and saml.ts would be a wrapper around that which is convenient to use, with optional and default parameters.
Here for example, there is no need to know about the decryptionPvk to generate SAML metadata (the the low level saml implemenation would not need to know about it). However, in the context of the API of saml.ts, it does not make sense to generate metadata without a decryption cert if decryptionPvk was provided in the option, so it normal that this function would fail if used incorrectly.
Let me know if it makes sense. If you'd rather have everything moved over to metadata.ts, I happy to do the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to get @markstos and possibly some other thoughts on this. I'm in favor of keeping it all together for the above-mentioned reasons. I see no value in a function that does work that would potentially produce an invalid or useless output. The node-saml
library is the lower-level library for passport-saml
; I can't think of a valid use-case where it itself would need to yet again be split, even if just internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I side with @cjbarth's viewpoint here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I merged both function 👍
src/saml/logout.ts
Outdated
} | ||
}; | ||
|
||
export const processValidlySignedSamlLogout = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing the Async
suffix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I think I must have thought the -Async suffix were only because we were transitioning to cb to async on the public interface, so I did not include it in any of my internal functions I think, as I feel it's a bit redundant with the function signature (in TS at least).
Should I follow a rule of suffixing -Async to all function that return a Promise? If that's the rule, I'll look into adding a lint rule for that in a next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have gone both ways on this one. Initially I was against the Async
suffix, but it was needed for refactor reasons. Then I read some stuff about how having that suffix can make code review easier (very obvious if someone forgot an await
). I also try to remember that others will be using this library, and there is a strong convention in APIs to use the async
suffix.
Having said that, anyone using TypeScript wouldn't have this problem. Then again, not all those using this library would be using TypeScript. I think I'm 51% in favor of the suffix since it is very common to have a mix of callback and async functions in JavaScript code, and this makes the difference very obvious to any consumer or reviewer, no matter the IDE.
I think though that we should use the same convention over at passport-saml
no matter what though. @markstos , do you have any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll but the -Async back for now, so we can have this discussion separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, when I was reviewing the code more thoroughly, it seems that we sometimes use the Async suffix for functions that return Promises and other times don't, so we're internally inconsistent. Thus, no matter what decision we make, we'll have to have some code changes to unify our convention internally.
|
||
const deflateRawAsync = util.promisify(zlib.deflateRaw); | ||
|
||
export const getAdditionalParams = (params: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that the signature of this function is now a little more difficult to use since it will no longer determine which additional params are needed based on the operation? Basically, this is now just a glorified object merging utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, this is now just a glorified object merging utility
That was also my though. On the other hand, if you look at what is left in _getAdditionalParams, the only thing it does is a ternary on the operationAdditionalParams.
My plan was to eventually get rid of _getAdditionalParams completely (as mentioned in another comment, I initially planed to do it in another PR to let the test untouched for this big code change, but let me know if I should do it here) and instead call directly getAdditionalParams
I have the same thoughts for _requestToUrlAsync. I think I'm not fond of passing this "operation" parameter to select the behavior of the function.
If we really want to avoid duplicating the logic of selection the right additional params in the function, think I would rather have a function that does just that, and directly use getAdditionalParams.
For example replace
this._getAdditionalParams(RelayState, operation, overrideParams)
by
return getAdditionalParams({
relayState,
globalAdditionalParams: this.options.additionalParams,
operationAdditionalParams: getAddtionalParamsForOperation(operation)
overrideParams,
});
With that said, getAdditionalParams is still a glorified object merging utility. I'd say that's fine, because I would find duplicating that code a bit too much, but that could be fine too. Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I realize that this would makes some things a bit more verbose, but personally I think that verbose is not always a bad thing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually going to comment similarly, verbosity isn't always bad. I really favor obvious code, especially with so many people coming in and out of this code all the time. We want to promote successful and bug-free code writing wherever possible. I personally would be comfortable seeing your suggesting in "by".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I do that in another PR? This change forces to update the tests which are testing _getAdditionalParams directly, so if we agree on not touching the tests in that PR, it's better to wait the next one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that we'd do the entire refactor and then leave a stub for _getAdditionalParams
.
|
||
const inflateRawAsync = util.promisify(zlib.inflateRaw); | ||
const deflateRawAsync = util.promisify(zlib.deflateRaw); | ||
|
||
interface NameID { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a type that passport-saml
depends on. Did you also try loading this in as a dependency of passport-saml
and seeing if passport-saml
still passes all of its tests?
If you need help with that LMK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll check that, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compilation fail, but only due to an unrelated issue: the call to getLogoutResponseUrl
is missing the success
parameter that was added recently.
Otherwise things seem fine, tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like it shouldn't happen. I've just landed some new code in to passport-saml
, so maybe that was the source of the mismatch? When I get some time, I'll pull this branch and verify myself for a second set of eyes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I might have not rebased on very latest commit indeed. I'll retry on the split PRs
@cjbarth I answered to all your comments, I'll wait for your answers before pushing more changes. I plan to push updates at fixup commits which are ready for autosquashing so that you can easily look at the new things only (and I'll rebase --autosquash when we are all good). if you prefer that I autosquash as I go, let me know. |
0d0d01f
to
f23f6aa
Compare
I pushed some updates, and asked a few more questions. Sorry, I promise to make smaller PR next time :) |
} | ||
return metadataXml; | ||
return generateServiceProviderMetadata({ | ||
...this.options, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how this is convenient, and probably not worth changing in this PR, but I'm in favor of being explicit about what we send; it makes refactoring and code analysis tools easier to use.
@cjbarth I split the PR. Only issue is that I have no idea how to target my other branch in this repository for the next PR. Not a big deal, I ordered the PRs, let's do them one by one, and I'll rebase whenever one is merged. |
@forty , do you think that you'll be able to address these conflicts as we push to a goal of having all the outstanding PRs done by April for when Node 12 is deprecated? |
# Conflicts: # src/saml.ts # src/saml/common.ts
@forty I really wanted to help land this, but it seems that your changes have broken a test. I recognize that this wasn't that way before, however, updating dependencies, which didn't break any tests, resulted in this new behavior for your changes. |
@forty Are you still interested in following through on these, or should we just close them? |
See #147. |
As discussed in node-saml/passport-saml#629, I would like to refactor the code of saml.ts to make it easier to review and to work with.
My idea is to split it by feature. So for example we would have:
In those files, I would like to avoid having SAML class (ie
this
orself
) or SamlOption as parameter, as I feel it makes it hard to understand the input of the function. Instead, each function would take as input exactly what it needs to operate. I think this makes review much easier. It also often allow to have stricter types.Since there is a lot of code to move, in order to bring more confidence that I'm not breaking anything, my strategy is to change the tests as little as possible (so I still leave some legacy functions which are tested to make sure tests are not broken - I will clean later once the split is done)
This is a first MR, to make sure you like this approach. I would rather like to do several smaller MRs rather than a big large one with the split, to avoid conflicting too much with other PRs if that works for you.
The only test I'm really changing (removing) is a test that called a function with a
string
parameter withnull
, which is not possible with out TS config. I know it's possible in JS, but I feel we should not start testing anything that TS can guarantee us, as not having to do this is the main benefit of TS IMO.Thanks for letting me know what you think (@markstos and @cjbarth especially)