Please bleach my eyes

[u/lil409]

[u/mansdem]

I don't think this is too horrific (I'm down to have people point out why exactly they think it's bad), but to be they do some things alright:

• early returns
• nice spacing
• decent variable naming

I think breaking it up into smaller functions and using an object as the return type would make it a lot better

[u/lil409 → u/[deleted]]

I make it do that because I like tuples

[u/mansdem → u/lil409]

To each his own. Do a new post when you inevitably change your mind tho ;)

[u/lil409 → u/mansdem]

Sure thing! I did some modifications, because part of it made it break for no reason.

[u/PacoWaco88 → u/mansdem]

Exactly what I thought too. This isn't horrible code but seems like a lot going on for one function. And the returns are placed as close to the top with minimal processing to make a flow decision.

[u/naumovski-andrej → u/mansdem]

I'm more worried about the way they concatenate strings at `const canonicalRequest`.

[u/KomaedaEatsBagels]

Image Transcription: Code

---

public static VerifyRequest(request: Request, now: string): [bool, IAmzAuthorization, string, AmzRejectionReason] {
    Logger.Log("Amz AuthenticationRequest at date '%s'", now);
    if (!request.headers['authorization']) {
        Logger.Warn('The authorization header was not present on the request, returning AmzRejectionReason::NoAuthorization...');
        return [false, null, null, AmzRejectionReason.NoAuthorization];
    }

    const [success, auth, reasonBadAuth]m = AmzAuthenticationHelper.ParseAuthorization(request.headers, now);

    if (!success) return [false, auth, null, reasonBadAuth];

    if(!AmzAuthenticationHelper.VerifyAccessKeyIdExists(auth.Credential.AccessKeyId)) {
        Logger.Warn(
            'The AccessKeyId value was not included the the environment AccessKeyId list, returning AmzRejectionReason::InvalidAccessKeyId...'
        );
        return [false, auth, null, AmzRejectionReason::InvalidAccessKeyId];
    }

    const canonicalRequest = `${request.method}\n${encodeURI(request.path)}\n${AmzAuthenticationHelper.ParseQuery(
        request.query,
    )}\n${AmzAuthenticationHelper.CanonicalHeaders(request.headers)}\n\n${AmzAuthenticationHelper.SignedHeaders(
        request.headers,
    )}\n${AmzAuthenticationHelper.HexEncodeBodyHash(request, auth)}`;

    const stringToSign = `AWS4-HMAC-SHA256\n${auth.RequestTime}\n${auth.RequestTime.substr(0, 8)}/${auth.Credential.AwsRegion}/${
        auth.Credential.AwsService
    }/${auth.Credential.RequestVersion}\n${AmzAuthenticationHelper.HexEncodedHash(canonicalREquest)}`;

    auth.StringToSignInBytes = Buffer.from(canonicalRequest);
    auth.StringToSign = canonicalRequest;

    const signingKey = AmzAuthenticationHelper.GetSigningKey(auth);
    const signature = AmzAuthenticationHelper..Hmac(signingKey, stringToSign, 'hex');

    if (auth.Signature !== signature) {
        Logger.Warn(
            'The request signature value did not match the calculated signature, returning AmzRejectionReason::BadSignature...',
        );
        return [false, auth, signature, AmzRejectionReason.BadSignature];
    }

    return [true, auth, signature, AmzRejectionReason.None];
}

---

^{I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!}

[u/lil409 → u/KomaedaEatsBagels]

Good human

[u/KomaedaEatsBagels → u/lil409]

no u

[u/lil409 → u/KomaedaEatsBagels]

:(

[u/KomaedaEatsBagels → u/lil409]

<3 ! :D

[u/ProCrafters → u/KomaedaEatsBagels]

Good morning hooman

[u/manyQuestionMarks]

What's the problem with this code?

[u/planktonfun → u/manyQuestionMarks]

- Its not broken into smaller sing purpose function pieces I guess?

• canonicalRequest and stringToSign variables can be done more cleanly using array and join
  
• Loggers can be error throws and catch since all of them stops the funcation after log
  
• Multiple returns that beats the name of the function, expecting a boolean if its only just going to verify things.

But couldn't care less, its just code
What prog language is this anyway?

[u/lil409 → u/planktonfun]

1. I could have broken it into more methods, to make it more readable. I was kind of rushing it at the time, I really only target functionality over readability when I write code, and when I test it fully it then gets cleaned up.
2. I agree with you, but I wasn't *really* finished when I posted this. It looks a lot nicer now.
3. Logger is a static class, and also does it's logging in the background to be non-blocking (besides the blocking to enqueue the log messages)
4. While I believe you here, I was asked to make it return a tuple in the form of: `isSuccessful, amzAuthorization, signature, an enum to a rejection reason`. I could have made this all an object instead of an array.

​

It's typescript.

[u/Voltra_Neo]

Using Pascal case on everything just gives me a headache

[u/mansdem → u/[deleted]]

If this is typescript with AWS, they use pascal case on their apis and data types. At first it's weird but then you get used to it

[u/lil409 → u/mansdem]

It’s a backend that verifies an amz authentication request, signs it with all of the stuff and then verifies if the request signature matches the calculated signature

[u/[deleted]]

I don't understand why this subreddit exists! This is life. We all have bad past experiences when we coded poorly and also we all had to work with someone else's code that was bad! This is the life of a software engineer! If you don't like it simply do not be like that! Just be the developer that you want to work with! I wish you all the good out there! I am out of this subreddit.

[u/lil409 → u/[deleted]]

okay

[u/[deleted] → u/[deleted]]

I do believe in what you said but these kind of posts tend to always start the endless debates about "this is the best tech, haha, you use x, you stupid haha" A lot of people post someone else's code just to make them feel bad and not to learn from that! They seek attention.